Extension: fix credential-push 405, guard the publish path, add a unit suite #234

Merged
bvandeusen merged 4 commits from dev into main 2026-08-03 08:29:09 -04:00
9 changed files with 379 additions and 10 deletions
Showing only changes of commit f9111c06a7 - Show all commits
+8 -3
View File
@@ -123,13 +123,18 @@ jobs:
fi fi
# Exclusions mirror --ignore-files in extension/package.json's web-ext # Exclusions mirror --ignore-files in extension/package.json's web-ext
# scripts: these files are not packaged into the XPI, so touching them # scripts: these files are not packaged into the XPI, so touching them
# (e.g. Renovate bumping the web-ext devDep) changes nothing shipped # (e.g. Renovate bumping the web-ext devDep, or editing a spec)
# and must not demand a version bump. # changes nothing shipped and must not demand a version bump.
# KEEP IN SYNC with --ignore-files — a file packaged into the XPI but
# excluded here is exactly the silent-stale-ship this job exists to
# prevent. test/version.spec.js pins the two lists' shared intent.
CHANGED=$(git diff --name-only "$BASE" HEAD -- extension/ \ CHANGED=$(git diff --name-only "$BASE" HEAD -- extension/ \
':(exclude)extension/package.json' \ ':(exclude)extension/package.json' \
':(exclude)extension/package-lock.json' \ ':(exclude)extension/package-lock.json' \
':(exclude)extension/README.md' \ ':(exclude)extension/README.md' \
':(exclude)extension/.gitignore') ':(exclude)extension/.gitignore' \
':(exclude)extension/vitest.config.js' \
':(exclude)extension/test/**')
if [ -z "$CHANGED" ]; then if [ -z "$CHANGED" ]; then
echo "No packaged extension files changed since $BASE — nothing to guard." echo "No packaged extension files changed since $BASE — nothing to guard."
echo "OK: extension version $PKG" echo "OK: extension version $PKG"
+14 -3
View File
@@ -1,5 +1,5 @@
name: extension name: extension
# Lint-only workflow. The sign-and-publish dance moved into build.yml's # Lint + unit tests. The sign-and-publish dance moved into build.yml's
# `sign-extension` job (2026-05-25) — `:latest` now always bundles the XPI # `sign-extension` job (2026-05-25) — `:latest` now always bundles the XPI
# because sign-extension runs as a build-web dependency in the SAME workflow, # because sign-extension runs as a build-web dependency in the SAME workflow,
# eliminating the prior race between build.yml and a separate extension.yml. # eliminating the prior race between build.yml and a separate extension.yml.
@@ -10,10 +10,15 @@ on:
paths: paths:
- 'extension/**' - 'extension/**'
- '.forgejo/workflows/extension.yml' - '.forgejo/workflows/extension.yml'
# test/version.spec.js asserts ci.yml's extension-version guard never
# ignores a file web-ext actually packages, so a ci.yml-only edit can
# break this suite and must trigger it.
- '.forgejo/workflows/ci.yml'
pull_request: pull_request:
branches: [main] branches: [main]
paths: paths:
- 'extension/**' - 'extension/**'
- '.forgejo/workflows/ci.yml'
workflow_dispatch: workflow_dispatch:
jobs: jobs:
@@ -23,7 +28,13 @@ jobs:
image: node:24-bookworm-slim image: node:24-bookworm-slim
steps: steps:
- uses: actions/checkout@v4 - uses: actions/checkout@v4
- name: Install web-ext # Not --no-save: vitest and web-ext are both real devDependencies now,
run: cd extension && npm install --no-save --no-audit --no-fund # and the suite needs vitest resolvable from node_modules.
- name: Install dev dependencies
run: cd extension && npm install --no-audit --no-fund
- name: Lint - name: Lint
run: cd extension && npm run lint run: cd extension && npm run lint
# Pure-logic specs over lib/url.js and lib/platforms.js plus manifest /
# package version-consistency checks. No browser, no network.
- name: Unit tests
run: cd extension && npm run test:unit
+18
View File
@@ -13,10 +13,20 @@ git.fabledsword.com/bvandeusen/ci-python:3.14
- node (frontend job: `npm install` + vitest + vite build) - node (frontend job: `npm install` + vitest + vite build)
- docker CLI + buildx (`.forgejo/workflows/build.yml`: build-web, build-ml — Fabled-Git registry push) - docker CLI + buildx (`.forgejo/workflows/build.yml`: build-web, build-ml — Fabled-Git registry push)
## Secondary runtime image
node:24-bookworm-slim — `.forgejo/workflows/extension.yml` only.
The extension lane is the one job that does NOT run on `ci-python:3.14`: it
needs a current Node for `web-ext` and vitest and nothing Python at all. Kept
on the upstream slim image rather than adding a Node toolchain to `ci-python`,
per `docs/process.md`'s "add deps to the image when used by >1 project".
## Per-job tool installs ## Per-job tool installs
- `pip install -r requirements.txt pytest pytest-asyncio` — in `backend-lint-and-test` and `integration` jobs - `pip install -r requirements.txt pytest pytest-asyncio` — in `backend-lint-and-test` and `integration` jobs
- `npm install --no-audit --no-fund` — in `frontend-build` job - `npm install --no-audit --no-fund` — in `frontend-build` job
- `npm install --no-audit --no-fund` — in `extension.yml`'s `lint` job (web-ext + vitest)
## Notes ## Notes
@@ -35,3 +45,11 @@ git.fabledsword.com/bvandeusen/ci-python:3.14
memory bans `npm install` locally). Using `npm install` rather than memory bans `npm install` locally). Using `npm install` rather than
`npm ci` until a lockfile lands. `npm ci` until a lockfile lands.
- No `imagemagick` / `pandoc` per-job installs needed. - No `imagemagick` / `pandoc` per-job installs needed.
- `extension/`'s vitest specs load `lib/*.js` by evaluating the real file as a
classic script (`test/helpers/loadLib.js`) rather than adding `module.exports`
shims to production code — the libs ship as `background.scripts`, not ES
modules, so the specs exercise exactly the bytes packaged into the XPI.
- Extension test files are excluded from the XPI via `--ignore-files` in
`extension/package.json`, and the same paths are excluded from `ci.yml`'s
`extension-version` guard. Those two lists must agree — `test/version.spec.js`
asserts the guard never ignores a file web-ext actually packages.
+6 -4
View File
@@ -4,12 +4,14 @@
"private": true, "private": true,
"description": "Firefox extension for FabledCurator", "description": "Firefox extension for FabledCurator",
"scripts": { "scripts": {
"lint": "web-ext lint --source-dir=. --no-config-discovery --ignore-files package.json package-lock.json web-ext-artifacts node_modules README.md .gitignore", "lint": "web-ext lint --source-dir=. --no-config-discovery --ignore-files package.json package-lock.json web-ext-artifacts node_modules README.md .gitignore vitest.config.js \"test/**\"",
"start": "web-ext run --source-dir=. --no-config-discovery --ignore-files package.json package-lock.json web-ext-artifacts node_modules README.md .gitignore --firefox=firefox", "start": "web-ext run --source-dir=. --no-config-discovery --ignore-files package.json package-lock.json web-ext-artifacts node_modules README.md .gitignore vitest.config.js \"test/**\" --firefox=firefox",
"build": "web-ext build --source-dir=. --no-config-discovery --ignore-files package.json package-lock.json web-ext-artifacts node_modules README.md .gitignore --overwrite-dest", "build": "web-ext build --source-dir=. --no-config-discovery --ignore-files package.json package-lock.json web-ext-artifacts node_modules README.md .gitignore vitest.config.js \"test/**\" --overwrite-dest",
"sign": "web-ext sign --source-dir=. --no-config-discovery --ignore-files package.json package-lock.json web-ext-artifacts node_modules README.md .gitignore --channel=unlisted --api-key=$WEB_EXT_API_KEY --api-secret=$WEB_EXT_API_SECRET" "sign": "web-ext sign --source-dir=. --no-config-discovery --ignore-files package.json package-lock.json web-ext-artifacts node_modules README.md .gitignore vitest.config.js \"test/**\" --channel=unlisted --api-key=$WEB_EXT_API_KEY --api-secret=$WEB_EXT_API_SECRET",
"test:unit": "vitest run"
}, },
"devDependencies": { "devDependencies": {
"vitest": "^4.0.0",
"web-ext": "^10.0.0" "web-ext": "^10.0.0"
} }
} }
+29
View File
@@ -0,0 +1,29 @@
import { readFileSync } from 'node:fs'
import { fileURLToPath } from 'node:url'
import path from 'node:path'
const LIB_DIR = path.join(path.dirname(fileURLToPath(import.meta.url)), '..', '..', 'lib')
/**
* Load an extension lib and hand back the globals it declares.
*
* The files under lib/ are CLASSIC scripts, not ES modules: manifest.json
* lists them in `background.scripts` and options.html pulls them in with a
* plain <script> tag, so they declare bare functions into a shared scope and
* export nothing. Rather than bolt a `module.exports` shim onto production
* code that would never run in the browser, evaluate the real file the same
* way the browser does — as a script body — and pick the declarations back out.
*
* This means the specs exercise the exact bytes that get packaged into the
* XPI. Only usable for libs that touch no browser APIs at load time
* (url.js, platforms.js); cookies.js and api.js reference `browser.*` and
* would need stubbing, which is why they aren't loaded this way.
*
* @param {string} filename e.g. 'url.js'
* @param {string[]} names declarations to return, e.g. ['normalizeApiUrl']
*/
export function loadLib(filename, names) {
const source = readFileSync(path.join(LIB_DIR, filename), 'utf8')
const factory = new Function(`${source}\nreturn { ${names.join(', ')} }`)
return factory()
}
+127
View File
@@ -0,0 +1,127 @@
import { describe, it, expect } from 'vitest'
import { loadLib } from './helpers/loadLib.js'
const { getPlatformFromUrl, isArtistPage, PLATFORMS, PLATFORM_ARTIST_PATTERNS } = loadLib(
'platforms.js',
['getPlatformFromUrl', 'isArtistPage', 'PLATFORMS', 'PLATFORM_ARTIST_PATTERNS']
)
describe('getPlatformFromUrl', () => {
it('identifies each platform from a domain URL', () => {
expect(getPlatformFromUrl('https://www.patreon.com/Atole')).toBe('patreon')
expect(getPlatformFromUrl('https://subscribestar.adult/someone')).toBe('subscribestar')
expect(getPlatformFromUrl('https://www.hentai-foundry.com/user/someone')).toBe('hentaifoundry')
expect(getPlatformFromUrl('https://discord.com/channels/@me')).toBe('discord')
expect(getPlatformFromUrl('https://www.pixiv.net/en/users/123')).toBe('pixiv')
expect(getPlatformFromUrl('https://www.deviantart.com/someone')).toBe('deviantart')
})
it('accepts http as well as https, with or without www', () => {
expect(getPlatformFromUrl('http://patreon.com/Atole')).toBe('patreon')
expect(getPlatformFromUrl('https://www.patreon.com/Atole')).toBe('patreon')
})
it('returns null for unrelated hosts', () => {
expect(getPlatformFromUrl('https://example.com/patreon.com')).toBe(null)
expect(getPlatformFromUrl('https://not-patreon.com/Atole')).toBe(null)
expect(getPlatformFromUrl('')).toBe(null)
})
})
describe('isArtistPage', () => {
// Regression cases from issue #1485: the Add-to-FC button vanished once the
// operator SUBSCRIBED to a creator, because Patreon serves subscribed users
// the /cw/ ("creator workspace") URL and the pattern only matched the bare
// root. All three creator URL shapes must match, plus inner pages — the
// button matters most exactly when you're subscribed.
it('matches all three Patreon creator URL shapes', () => {
expect(isArtistPage('https://www.patreon.com/Atole', 'patreon')).toBe(true)
expect(isArtistPage('https://www.patreon.com/c/Atole', 'patreon')).toBe(true)
expect(isArtistPage('https://www.patreon.com/cw/Atole', 'patreon')).toBe(true)
})
it('matches Patreon creator inner pages', () => {
expect(isArtistPage('https://www.patreon.com/cw/Atole/posts', 'patreon')).toBe(true)
expect(isArtistPage('https://www.patreon.com/Atole/membership', 'patreon')).toBe(true)
})
it('excludes Patreon navigation pages that are not creators', () => {
for (const nav of ['home', 'search', 'messages', 'notifications', 'library', 'settings']) {
expect(isArtistPage(`https://www.patreon.com/${nav}`, 'patreon')).toBe(false)
expect(isArtistPage(`https://www.patreon.com/${nav}/anything`, 'patreon')).toBe(false)
}
})
it('matches SubscribeStar creator roots on both TLDs but not feed pages', () => {
expect(isArtistPage('https://subscribestar.adult/someone', 'subscribestar')).toBe(true)
expect(isArtistPage('https://subscribestar.com/someone', 'subscribestar')).toBe(true)
expect(isArtistPage('https://subscribestar.adult/feed', 'subscribestar')).toBe(false)
expect(isArtistPage('https://subscribestar.adult/messages', 'subscribestar')).toBe(false)
})
it('matches Hentai Foundry user pages only', () => {
expect(isArtistPage('https://www.hentai-foundry.com/user/someone', 'hentaifoundry')).toBe(true)
expect(isArtistPage('https://www.hentai-foundry.com/pictures/popular', 'hentaifoundry')).toBe(
false
)
})
it('matches Pixiv numeric user pages, with or without the /en/ prefix', () => {
expect(isArtistPage('https://www.pixiv.net/users/12345', 'pixiv')).toBe(true)
expect(isArtistPage('https://www.pixiv.net/en/users/12345', 'pixiv')).toBe(true)
expect(isArtistPage('https://www.pixiv.net/en/artworks/999', 'pixiv')).toBe(false)
})
it('excludes DeviantArt navigation roots', () => {
expect(isArtistPage('https://www.deviantart.com/someone', 'deviantart')).toBe(true)
expect(isArtistPage('https://www.deviantart.com/home', 'deviantart')).toBe(false)
expect(isArtistPage('https://www.deviantart.com/watch', 'deviantart')).toBe(false)
})
it('returns false for a platform with no artist pattern (discord)', () => {
expect(isArtistPage('https://discord.com/channels/@me', 'discord')).toBe(false)
})
it('returns false for an unknown platform key', () => {
expect(isArtistPage('https://www.patreon.com/Atole', 'nope')).toBe(false)
})
})
describe('platform table integrity', () => {
it('gives every artist pattern a corresponding platform entry', () => {
// A pattern keyed to a platform that no longer exists is dead code that
// silently never fires; the reverse (a platform with no pattern) is the
// legitimate discord case, so only this direction is an error.
for (const key of Object.keys(PLATFORM_ARTIST_PATTERNS)) {
expect(Object.keys(PLATFORMS)).toContain(key)
}
})
it('gives every platform the fields the popup renders', () => {
for (const [key, platform] of Object.entries(PLATFORMS)) {
expect(platform.name, `${key}.name`).toBeTruthy()
expect(platform.color, `${key}.color`).toMatch(/^#[0-9A-Fa-f]{6}$/)
expect(['cookies', 'token'], `${key}.authType`).toContain(platform.authType)
expect(platform.urlPattern, `${key}.urlPattern`).toBeInstanceOf(RegExp)
expect(Array.isArray(platform.domains), `${key}.domains`).toBe(true)
expect(platform.domains.length, `${key}.domains`).toBeGreaterThan(0)
}
})
it('keeps every artist URL matched by its own platform pattern too', () => {
// isArtistPage is only ever consulted after getPlatformFromUrl resolves a
// key, so an artist pattern matching a URL its platform's urlPattern
// rejects would be unreachable.
const samples = {
patreon: 'https://www.patreon.com/cw/Atole',
subscribestar: 'https://subscribestar.adult/someone',
hentaifoundry: 'https://www.hentai-foundry.com/user/someone',
deviantart: 'https://www.deviantart.com/someone',
pixiv: 'https://www.pixiv.net/en/users/12345'
}
for (const [key, url] of Object.entries(samples)) {
expect(isArtistPage(url, key), `${key} artist pattern`).toBe(true)
expect(getPlatformFromUrl(url), `${key} urlPattern`).toBe(key)
}
})
})
+93
View File
@@ -0,0 +1,93 @@
import { describe, it, expect } from 'vitest'
import { loadLib } from './helpers/loadLib.js'
const { normalizeApiUrl, webRootFromApiUrl } = loadLib('url.js', [
'normalizeApiUrl',
'webRootFromApiUrl'
])
describe('normalizeApiUrl', () => {
// The bug this exists for (issue #2393): the instance root was accepted and
// stored verbatim, so every request went to /credentials instead of
// /api/credentials. That path is a Vue router route, so the SPA catch-all
// answered GET with 200 HTML and rejected POST with 405 — which read as a
// backend bug rather than a URL one.
it('appends /api to an instance root', () => {
expect(normalizeApiUrl('http://curator.traefik.internal')).toBe(
'http://curator.traefik.internal/api'
)
})
it('leaves an API root alone rather than doubling the suffix', () => {
expect(normalizeApiUrl('http://curator.traefik.internal/api')).toBe(
'http://curator.traefik.internal/api'
)
})
it('is idempotent', () => {
const once = normalizeApiUrl('http://curator.example.com')
expect(normalizeApiUrl(once)).toBe(once)
})
it('strips trailing slashes before deciding', () => {
expect(normalizeApiUrl('http://curator.example.com/')).toBe('http://curator.example.com/api')
expect(normalizeApiUrl('http://curator.example.com///')).toBe('http://curator.example.com/api')
expect(normalizeApiUrl('http://curator.example.com/api/')).toBe('http://curator.example.com/api')
})
it('trims surrounding whitespace (paste artifacts)', () => {
expect(normalizeApiUrl(' http://curator.example.com ')).toBe(
'http://curator.example.com/api'
)
})
it('matches the /api suffix case-insensitively', () => {
expect(normalizeApiUrl('http://curator.example.com/API')).toBe('http://curator.example.com/API')
})
it('returns empty string for empty/nullish input, never a bare "/api"', () => {
// isConfigured() gates on truthiness, so a bogus '/api' here would read as
// "configured" and produce a request against the options page's own origin.
expect(normalizeApiUrl('')).toBe('')
expect(normalizeApiUrl(' ')).toBe('')
expect(normalizeApiUrl(null)).toBe('')
expect(normalizeApiUrl(undefined)).toBe('')
})
it('does not treat a path merely containing "api" as the suffix', () => {
expect(normalizeApiUrl('http://curator.example.com/apiary')).toBe(
'http://curator.example.com/apiary/api'
)
})
it('preserves a subpath deployment', () => {
expect(normalizeApiUrl('http://host.internal/curator')).toBe('http://host.internal/curator/api')
})
})
describe('webRootFromApiUrl', () => {
// The SPA root, where the Vue router and the served XPI live. Used by
// OPEN_ARTIST_PAGE and the self-update check — NOT the JSON API.
it('strips the /api suffix', () => {
expect(webRootFromApiUrl('http://curator.example.com/api')).toBe('http://curator.example.com')
})
it('accepts an instance root unchanged', () => {
expect(webRootFromApiUrl('http://curator.example.com')).toBe('http://curator.example.com')
})
it('agrees with normalizeApiUrl in both directions', () => {
for (const input of ['http://curator.example.com', 'http://curator.example.com/api']) {
expect(normalizeApiUrl(webRootFromApiUrl(input))).toBe(normalizeApiUrl(input))
}
})
it('preserves a subpath deployment', () => {
expect(webRootFromApiUrl('http://host.internal/curator/api')).toBe('http://host.internal/curator')
})
it('returns empty string for empty/nullish input', () => {
expect(webRootFromApiUrl('')).toBe('')
expect(webRootFromApiUrl(null)).toBe('')
})
})
+71
View File
@@ -0,0 +1,71 @@
import { describe, it, expect } from 'vitest'
import { readFileSync } from 'node:fs'
import { fileURLToPath } from 'node:url'
import path from 'node:path'
const EXT_DIR = path.join(path.dirname(fileURLToPath(import.meta.url)), '..')
const read = (name) => JSON.parse(readFileSync(path.join(EXT_DIR, name), 'utf8'))
describe('extension version consistency', () => {
// Duplicates check (1) of ci.yml's extension-version job, deliberately.
// That job is the gate that can't be bypassed; this spec is the one that
// fails in a second on the developer's own CI lane with a readable diff.
// The two version strings feed different systems and nothing else reconciles
// them:
// manifest.json -> what `web-ext sign` signs, so what Firefox installs
// (package.json is in --ignore-files, not in the XPI)
// package.json -> build.yml's AMO cache key, the ext-<version> release
// tag, the XPI filename, and therefore the version
// /api/extension/manifest reports to the update prompt
it('keeps manifest.json and package.json in lockstep', () => {
const manifest = read('manifest.json')
const pkg = read('package.json')
expect(manifest.version).toBe(pkg.version)
})
it('uses a plain dotted numeric version AMO will accept', () => {
// AMO rejects exotic version strings, and build.yml embeds this value in a
// release tag and a filename — so anything needing escaping breaks the
// publish path rather than the extension.
expect(read('package.json').version).toMatch(/^\d+(\.\d+)*$/)
})
it('declares manifest v3', () => {
expect(read('manifest.json').manifest_version).toBe(3)
})
it('never lets the CI guard ignore a file that actually ships', () => {
// ci.yml's extension-version job skips its bump check for paths it deems
// non-shipping. If it excludes something web-ext DOES package, a real
// change to shipped code passes the guard unnoticed — precisely the
// silent-stale-ship the guard exists to stop. The reverse drift (guard
// stricter than web-ext) only costs a needless bump, so it isn't asserted.
const ci = readFileSync(path.join(EXT_DIR, '..', '.forgejo', 'workflows', 'ci.yml'), 'utf8')
const lint = read('package.json').scripts.lint
const after = lint.split('--ignore-files')[1] ?? ''
const ignored = new Set(
after
.split(/\s+/)
.filter((tok) => tok && !tok.startsWith('--'))
.map((tok) => tok.replace(/^["']|["']$/g, ''))
)
expect(ignored.size, 'parsed --ignore-files from the lint script').toBeGreaterThan(0)
const guarded = [...ci.matchAll(/:\(exclude\)extension\/(\S+?)'/g)].map((m) => m[1])
expect(guarded.length, 'parsed :(exclude) entries from ci.yml').toBeGreaterThan(0)
for (const entry of guarded) {
expect(ignored, `ci.yml excludes "${entry}" but web-ext packages it`).toContain(entry)
}
})
it('lists every background script that exists, in dependency order', () => {
// url.js must load BEFORE api.js: api.js calls normalizeApiUrl at
// init()-time, and these are classic scripts sharing one scope, so a
// reordering here is a runtime ReferenceError with no build-time signal.
const scripts = read('manifest.json').background.scripts
for (const rel of scripts) {
expect(() => readFileSync(path.join(EXT_DIR, rel)), `missing ${rel}`).not.toThrow()
}
expect(scripts.indexOf('lib/url.js')).toBeLessThan(scripts.indexOf('lib/api.js'))
})
})
+13
View File
@@ -0,0 +1,13 @@
import { defineConfig } from 'vitest/config'
// Mirrors frontend/vitest.config.js, minus the Vue plugin — the extension has
// no SFCs and mounts nothing. Pure-logic specs only, so `node` is enough; the
// libs under test are deliberately the ones with no browser-API surface (see
// test/helpers/loadLib.js).
export default defineConfig({
test: {
environment: 'node',
include: ['test/**/*.spec.js'],
passWithNoTests: true
}
})