diff --git a/.changeset/safe-urls-and-prose.md b/.changeset/safe-urls-and-prose.md new file mode 100644 index 000000000..e88e205c7 --- /dev/null +++ b/.changeset/safe-urls-and-prose.md @@ -0,0 +1,5 @@ +--- +"catalog": patch +--- + +Reject `javascript:`, `data:` and other non-http(s) schemes in every `url` field (E128) and raw HTML in prose (E305); the build now escapes markup it is handed. The one `ftp://` link (dbx 160A) is removed. diff --git a/.github/workflows/url-health.yml b/.github/workflows/url-health.yml index 486438b79..68c759291 100644 --- a/.github/workflows/url-health.yml +++ b/.github/workflows/url-health.yml @@ -15,19 +15,31 @@ on: permissions: contents: read +# Two jobs, split by what they hold, on the same reasoning as +# assign-ids.yml. `check-urls` runs `pnpm install` (the repo's lifecycle +# scripts) and then thousands of guarded requests to contributor-chosen +# URLs. Anything running in that job can read every credential the job +# references, so it references a read-only token and nothing else, and +# hands its one product, the refreshed cache, to `commit-cache` as an +# artifact. `commit-cache` is the only job that holds a contents-write +# token: it installs nothing, runs no script from the tree, checks the +# artifact is the JSON it expects, and pushes that one file. jobs: check-urls: runs-on: ubuntu-latest timeout-minutes: 60 permissions: - contents: write + contents: read issues: write + outputs: + cache: ${{ steps.cache.outputs.present }} steps: - name: Checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: fetch-depth: 1 + persist-credentials: false - name: Setup Node.js uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7 @@ -40,28 +52,41 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile + # The input is a workflow_dispatch boolean, so it reaches the shell + # through env rather than `${{ }}` inside `run:`, like every other + # value these workflows did not choose. - name: Run URL validation id: urls continue-on-error: true + env: + FORCE_RECHECK: ${{ inputs.force_recheck }} run: | - if [ "${{ inputs.force_recheck }}" = "true" ]; then + if [ "$FORCE_RECHECK" = "true" ]; then pnpm validate-urls --update-cache --ignore-cache else pnpm validate-urls --use-cache --update-cache fi - - name: Commit cache updates + # Only the cache file is handed on. The first run may not create it. + - name: Check for cache updates + id: cache if: always() run: | - git config user.name "github-actions[bot]" - git config user.email "github-actions[bot]@users.noreply.github.com" - # Only add cache file if it exists (first run may not create it) if [ -f .github/url-cache.json ]; then - git add .github/url-cache.json - git diff --staged --quiet || git commit -m "chore: update URL cache" - git push || echo "Push failed - may need manual intervention" + echo "present=true" >> "$GITHUB_OUTPUT" + else + echo "present=false" >> "$GITHUB_OUTPUT" fi + - name: Upload cache + if: always() && steps.cache.outputs.present == 'true' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: url-cache + path: .github/url-cache.json + retention-days: 1 + if-no-files-found: error + - name: Create or update issue for broken URLs if: steps.urls.outcome == 'failure' uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 @@ -181,3 +206,50 @@ jobs: }); console.log(`Closed issue #${existingIssue.number}`); } + + commit-cache: + needs: check-urls + if: always() && needs.check-urls.outputs.cache == 'true' + runs-on: ubuntu-latest + permissions: + contents: write + + steps: + # No `pnpm install` runs here, so no husky hooksPath gets configured + # either; hooks are disabled below all the same. + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + fetch-depth: 1 + + - name: Download cache + uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7.0.0 + with: + name: url-cache + path: .github + + # The artifact was produced by a job that ran contributor-reachable + # code, so it is data to check, not a file to trust: it has to parse + # as the cache shape `scripts/lib/url-cache.ts` writes, and nothing + # else may have changed. + - name: Verify cache artifact + run: | + node -e ' + const fs = require("node:fs"); + const cache = JSON.parse(fs.readFileSync(".github/url-cache.json", "utf-8")); + const ok = cache && cache.version === 1 && cache.entries && typeof cache.entries === "object" && !Array.isArray(cache.entries); + if (!ok) { console.error("::error::url-cache.json is not a version 1 cache; refusing to commit it"); process.exit(1); } + ' + if git status --porcelain | grep -vE '^.. \.github/url-cache\.json$'; then + echo "::error::artifact changed something other than .github/url-cache.json; refusing to commit" + exit 1 + fi + + - name: Commit cache updates + run: | + git config user.name "github-actions[bot]" + git config user.email "github-actions[bot]@users.noreply.github.com" + git add .github/url-cache.json + git diff --staged --quiet && exit 0 + git -c core.hooksPath=/dev/null commit -m "chore: update URL cache" + git -c core.hooksPath=/dev/null push || echo "Push failed - may need manual intervention" diff --git a/CLAUDE.md b/CLAUDE.md index 0e488d5e8..ef2e60a72 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -171,6 +171,27 @@ cohort working: with an argument array, never `execSync` with an interpolated string. Quoting does not disarm `$(...)` or a backtick, and catalog filenames arrive through pull requests. +- Every data file is read through `readRegularFile` (`loadYamlFile`, + `loadYamlFileWithPositions`), which `lstat`s first and refuses a symlink + or anything that is not a regular file. A directory listing is a list of + names, and `readFileSync` follows a name wherever it points, so a + committed `data/software/x.yaml -> /dev/zero` was an unbounded read in + `pnpm validate` and `pnpm build` until this existed. +- Prose is Markdown and a `url` is an http(s) page, at both ends. The build + renders `description`/`details`/`specs` with marked, which passes raw + HTML through by design, and Studio shows the result; `z.url()` accepts + `javascript:` and `data:` because the URL parser does. `pnpm validate` + refuses raw HTML (E305) and any scheme but http and https (E128), on + `url` fields and on links inside prose, read off marked's own lexer so + the verdict is the renderer's. `markdownToHtml` in `build-sqlite.ts` + escapes raw HTML and drops unsafe link destinations regardless, because + the build reads whatever is on disk and a validator is only a statement + about files that went through it. The shared rule lives in + `scripts/lib/prose-markup.ts`. +- `fix-urls` never writes a redirect target that leaves the maker's + registrable domain. The target is whatever the server at the old address + says today, and a lapsed domain answers with its buyer's store; a + cross-site redirect is reported and left for a person. - A workflow that lays a catalog value out in a Markdown table passes it through that workflow's `cell()` encoder. The value is data, not markup: a newline ends the row, a pipe adds a column, a backtick opens a code diff --git a/data/hardware/dbx-160a.yaml b/data/hardware/dbx-160a.yaml index 13cf99be3..bce759e65 100644 --- a/data/hardware/dbx-160a.yaml +++ b/data/hardware/dbx-160a.yaml @@ -104,9 +104,6 @@ links: - type: resource title: 160A Owner's Manual - English url: https://dbxpro.com/en-US/product_documents/dbx160a-18-2040v-cpdf - - type: resource - title: Compression 101 White Paper - url: ftp://ftp.hmgsl.com/dbxpro.com/pub/PDFs/WhitePapers/Compression_101.pdf - type: support title: Contact Technical Support url: https://dbxpro.com/en-US/support diff --git a/docs/VALIDATION_ERRORS.md b/docs/VALIDATION_ERRORS.md index a324c5117..0f833fec6 100644 --- a/docs/VALIDATION_ERRORS.md +++ b/docs/VALIDATION_ERRORS.md @@ -494,6 +494,38 @@ read is omitted, never written as a placeholder. --- +### E128: URL Scheme Is Not http or https + +A `url` field (the entry's own, a link's, a version's, a manufacturer's) carries a +scheme other than `http` or `https`: `javascript:`, `data:`, `file:`, `ftp:` and so +on. `z.url()` accepts any of them, because the URL parser does, and nothing +rejected them before. A `url` is a page Studio sends a person to, so a +`javascript:` or `data:` value is a payload rather than an address once a client +opens it, and the catalog's signature would be vouching for it. + +The same rule applies to a link written inside prose (`[text](javascript:...)` in +`description`, `details` or `specs`): the build renders those with marked, and a +link it refuses is rendered as its text alone. + +**Examples:** + +```yaml +# Wrong +url: javascript:alert(1) +links: + - type: resource + title: White paper + url: ftp://ftp.example.com/paper.pdf + +# Correct +url: https://example.com/product +``` + +**Fix:** Point the field at the maker's http(s) page. A document only reachable +over another scheme has no place in `links`; mention it in `details` instead. + +--- + ### E199: Validation Error A generic validation error that doesn't fall into a more specific category. @@ -700,6 +732,41 @@ description: Use the `foo` command to run it --- +### E305: Raw HTML in Prose + +A prose field (`description`, `details`, `specs`, or the same under +`translations`) contains raw HTML: a tag, a comment, an inline element. The build +renders these fields with marked, which passes HTML through unchanged by design, +and Studio displays the result as HTML. So until this rule existed, a `")).toBe( + "<script>alert(2)</script>" + ); + expect(markdownToHtml("[click](javascript:alert(3))")).toBe("

click

"); + expect(markdownToHtml("![alt](javascript:alert(4))")).toBe("

alt

"); + expect(markdownToHtml("[site](https://x.example/) and [mail](mailto:a@x.example)")).toBe( + '

site and mail

' + ); + }); + it("normalizeCategory maps an alias to its canonical value and passes the rest through", () => { expect(normalizeCategory("eq")).toBe("equalizer"); expect(normalizeCategory("equalizer")).toBe("equalizer"); diff --git a/scripts/__tests__/utils.test.ts b/scripts/__tests__/utils.test.ts index 3a883247f..48a664ae2 100644 --- a/scripts/__tests__/utils.test.ts +++ b/scripts/__tests__/utils.test.ts @@ -1,5 +1,52 @@ -import { describe, expect, it } from "vitest"; -import { escapeSQL, findClosestMatch, formatValidOptions, parseErrorPath } from "../lib/utils.js"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { + escapeSQL, + findClosestMatch, + formatValidOptions, + loadYamlFile, + parseErrorPath, + readRegularFile, +} from "../lib/utils.js"; + +describe("readRegularFile", () => { + const dirs: string[] = []; + afterEach(() => { + for (const dir of dirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); + }); + const tmp = (): string => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "catalog-utils-")); + dirs.push(dir); + return dir; + }; + + it("reads a regular file", () => { + const dir = tmp(); + const file = path.join(dir, "a.yaml"); + fs.writeFileSync(file, "name: A\n"); + expect(readRegularFile(file)).toBe("name: A\n"); + expect(loadYamlFile<{ name: string }>(file)).toEqual({ name: "A" }); + }); + + it("refuses a symlink rather than reading through it", () => { + // A committed `data/software/x.yaml -> /dev/zero` was an unbounded + // read in validate and build; the link is refused as a link. + const dir = tmp(); + const target = path.join(dir, "real.yaml"); + fs.writeFileSync(target, "name: A\n"); + const link = path.join(dir, "link.yaml"); + fs.symlinkSync(target, link); + expect(() => readRegularFile(link)).toThrow(/symbolic link/); + expect(() => loadYamlFile(link)).toThrow(/symbolic link/); + }); + + it("refuses something that is not a regular file", () => { + const dir = tmp(); + expect(() => readRegularFile(dir)).toThrow(/not a regular file/); + }); +}); describe("findClosestMatch", () => { it("returns exact match when found", () => { diff --git a/scripts/__tests__/validate.test.ts b/scripts/__tests__/validate.test.ts index 7ce97870a..4c4498a8a 100644 --- a/scripts/__tests__/validate.test.ts +++ b/scripts/__tests__/validate.test.ts @@ -70,6 +70,34 @@ describe("getErrorCodeFromZodIssue", () => { ).toBe(ValidationErrorCode.E127_INVALID_PRICE_AMOUNT); }); + it("classifies an unsafe scheme as E128 before the generic url branch", () => { + expect( + getErrorCodeFromZodIssue( + issue({ message: "Unsafe URL scheme 'javascript:': only http and https are allowed." }) + ) + ).toBe(ValidationErrorCode.E128_UNSAFE_URL_SCHEME); + expect( + getErrorCodeFromZodIssue( + issue({ + message: + "Unsafe URL scheme in prose link 'data:x': only http, https and mailto links render.", + path: ["details"], + }) + ) + ).toBe(ValidationErrorCode.E128_UNSAFE_URL_SCHEME); + }); + + it("classifies raw html as E305 before the markdown branch", () => { + expect( + getErrorCodeFromZodIssue( + issue({ + message: + "Raw HTML in prose: '\n` + ); + expect(validateFile(block, COLLECTION_SCHEMAS.hardware, MAKERS)?.details?.[0]).toMatchObject({ + code: ValidationErrorCode.E305_RAW_HTML_IN_PROSE, + path: "details", + }); + + const link = writeEntry("hardware", `${HARDWARE_OK}specs: "- [Click](javascript:alert(3))"\n`); + expect(validateFile(link, COLLECTION_SCHEMAS.hardware, MAKERS)?.details?.[0]).toMatchObject({ + code: ValidationErrorCode.E128_UNSAFE_URL_SCHEME, + path: "specs", + }); + }); + + it("reaches raw html and unsafe links inside table cells", () => { + // marked keeps a table's cells under `header` and `rows`, not `tokens`. + const table = + "| Mode | Note |\n|------|------|\n" + + '| Hall | |\n' + + "| Room | [more](javascript:alert(2)) |"; + const file = writeEntry( + "hardware", + `${HARDWARE_OK}details: |-\n ${table.replace(/\n/g, "\n ")}\n` + ); + const result = validateFile(file, COLLECTION_SCHEMAS.hardware, MAKERS); + expect(result?.details?.map((d) => d.code).sort()).toEqual([ + ValidationErrorCode.E128_UNSAFE_URL_SCHEME, + ValidationErrorCode.E305_RAW_HTML_IN_PROSE, + ]); + }); + + it("reads angle brackets the lexer takes as text as text, not html", () => { + // In the corpus: a tracklist entry written as . + const file = writeEntry( + "hardware", + `${HARDWARE_OK}details: |-\n Tracks include and a [page](https://x.example).\n` + ); + expect(validateFile(file, COLLECTION_SCHEMAS.hardware, MAKERS)).toBeNull(); + }); + + it("refuses to read an entry through a symlink", () => { + const real = writeEntry("hardware", HARDWARE_OK); + const link = path.join(path.dirname(real), "linked.yaml"); + fs.symlinkSync(real, link); + const result = validateFile(link, COLLECTION_SCHEMAS.hardware, MAKERS); + expect(result?.errors[0]).toMatch(/symbolic link/); + }); + it("keeps a wrong-typed price amount on E101, not E127", () => { // The error map fires only when the input really was a number, so a // string keeps Zod's own wrong-type message and its existing code. diff --git a/scripts/build-sqlite.ts b/scripts/build-sqlite.ts index a841cbf4a..acb571788 100644 --- a/scripts/build-sqlite.ts +++ b/scripts/build-sqlite.ts @@ -10,8 +10,8 @@ import fs from "node:fs"; import path from "node:path"; import { pathToFileURL } from "node:url"; import Database from "better-sqlite3"; -import { marked } from "marked"; - +import { Marked, type Tokens } from "marked"; +import { escapeHtml, isSafeHref } from "./lib/prose-markup.js"; import type { Accessory, CategoryAliasesSchema, @@ -31,10 +31,38 @@ import { slugify, } from "./lib/utils.js"; -// Configure marked for safe HTML output -marked.setOptions({ +/** + * The renderer behind every `*_html` column. Its own `Marked` instance, + * so the overrides never reach the global `marked` that `validate.ts` + * lexes with. + * + * marked passes raw HTML through by design, and Studio shows these + * columns as HTML, so until this renderer existed a `