fix(security): block control-char-obfuscated javascript: URLs - #12
Conversation
safeUrl() tested the scheme with /^[a-z][a-z0-9+.-]*:/ and fell through to `return url` when that failed. A scheme containing an ASCII tab, LF or CR fails that test, so "java<TAB>script:alert(1)" was returned unchanged and rendered into an href. The WHATWG URL parser strips ASCII tab/LF/CR before resolving the scheme, so the browser resolves it back to javascript: and executes it on click — same-origin script execution on the Pages site. Reachable from any .rb `homepage` value and from a GitHub release html_url, via both server-rendered hrefs (DetailPage, VersionHistory) and the client-rendered search results. Strip C0 controls and DEL before the scheme test, in both the build-time (src/lib/serialize.mjs) and bundled-client (src/scripts/site.js) copies. Filtered by code point so both sources stay pure ASCII. Tests: new tests/safe-url.test.js covers the blocked/allowed matrix against BOTH copies (tab/LF/CR/NUL/DEL obfuscation, data:, vbscript:, file:, protocol-relative) and the xss fixture now uses the tab-obfuscated payload end-to-end. The previous test only asserted the plain javascript: case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The bash->Node port dropped curl's `--retry 3 --retry-delay 3`, so a single transient CDN error hard-failed the brew job and, since deploy needs it, blocked the Pages deploy. Re-add a bounded retry loop (env-overridable, same shape as src/lib/releases.mjs); the 180s abort is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe PR hardens build-time and client-side URL sanitization, expands security tests, adds configurable timeout and retry handling to checksum downloads, documents the Astro workflow, and authenticates Homebrew CI GitHub API requests. ChangesURL Sanitization
Checksum Download Reliability
Workflow Documentation and Authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/xss.test.js (1)
68-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Cheerio to assert the rendered link attribute.
Parse
htmland assert$("#detail-homepage").attr("href")rather than depending on attribute order and quote serialization in a regex. Confirmcheeriois declared before updating the test. As per coding guidelines, “tests/*.test.js: Write automated tests with Vitest and use Cheerio for HTML DOM assertions where appropriate.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/xss.test.js` around lines 68 - 74, Update the test around the “control-char-obfuscated javascript: homepage URL” case to use Cheerio for DOM assertions: first confirm the test dependencies declare cheerio, parse html, and assert the rendered `#detail-homepage` href via $("`#detail-homepage`").attr("href"). Preserve the existing expectations that the href is "#" and contains no script: payload.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/verify-checksums.mjs`:
- Around line 26-31: Update the fetch/retry logic in the checksum verification
flow so non-ok permanent HTTP responses, including 401, 403, and 404, are
handled immediately outside the retry path; consume the response body before
throwing. Keep retries limited to transient HTTP failures and network errors,
while preserving the existing lastErr handling for retryable failures.
- Around line 11-12: Validate CHECKSUM_RETRIES when initializing RETRIES,
rejecting NaN, Infinity, negative, and non-integer values while preserving 0 as
valid. Ensure invalid configuration fails explicitly before the retry loop can
run.
---
Nitpick comments:
In `@tests/xss.test.js`:
- Around line 68-74: Update the test around the “control-char-obfuscated
javascript: homepage URL” case to use Cheerio for DOM assertions: first confirm
the test dependencies declare cheerio, parse html, and assert the rendered
`#detail-homepage` href via $("`#detail-homepage`").attr("href"). Preserve the
existing expectations that the href is "#" and contains no script: payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 71b0be48-e2e9-464f-883f-49ebab80ee2f
📒 Files selected for processing (5)
scripts/verify-checksums.mjssrc/lib/serialize.mjssrc/scripts/site.jstests/safe-url.test.jstests/xss.test.js
AGENTS.md still described the retired bash pipeline — scripts/build-site.sh,
site/{template,detail-template}.html, site/partials/, site/shared.js's
initDetailPage(), site/input.css, deploy-site.yml — none of which exist. An
agent following it would run a missing script and look for missing files.
Rewrite against the actual tree: Astro static site (base /homebrew-den, outDir
_site), src/lib/*.mjs as the single sources of truth for catalog parsing,
stability and release fetching, npm run dev/build/preview, the build-once
test setup in tests/global-setup.js, and the real two-job-then-gated-deploy
ci.yml. Adds a "Security notes (do not regress)" section covering
serializeJson, the deliberately duplicated safeUrl and its control-char
stripping, and the set:html hazard.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 108-109: Add a package.json engines.node declaration requiring
Node.js 22 or newer, matching the workflow matrix. Do not alter the existing
package manager or dependency configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
brew audit --strict --online and brew livecheck call the GitHub API directly. Unauthenticated, they share a 60 req/hr per-IP limit with every other job on the shared runner, so the job failed with exception while auditing wolfwave: GitHub API Error: API rate limit exceeded ... Rate limit exceeded for core resource (60 limit). on a docs-only commit, with the formula and cask byte-identical to two runs that had passed. Set HOMEBREW_GITHUB_API_TOKEN from the job-scoped secrets.GITHUB_TOKEN, at job level so every brew step is covered. Raises the limit to 5000/hr and makes the job deterministic rather than a coin flip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 57-63: Remove the job-level HOMEBREW_GITHUB_API_TOKEN environment
setting and add it only to the individual brew audit and brew livecheck steps.
Keep checksum verification, formula/cask installation, and repository-controlled
brew test steps free of this token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6719972f-9b74-4c86-995f-0ef9bcd296cb
📒 Files selected for processing (1)
.github/workflows/ci.yml
- ci: scope HOMEBREW_GITHUB_API_TOKEN to the brew audit and livecheck steps
instead of the whole job. Those are the only steps that call the GitHub
API; brew install and brew test execute formula-controlled Ruby and must
not see the token.
- verify-checksums: validate CHECKSUM_RETRIES / CHECKSUM_RETRY_DELAY_MS /
CHECKSUM_TIMEOUT_MS as non-negative integers. Number("abc") is NaN and
`attempt <= NaN` is false, so a typo'd override silently skipped every
attempt and threw an undefined error. 0 remains valid ("no retries").
- verify-checksums: do not retry permanent 4xx responses (bad URL, renamed
asset, private repo); they cannot succeed and only burn RETRIES * delay.
408/429 stay retryable. Drain the body before throwing.
- tests/xss: assert the href with Cheerio rather than a regex over raw HTML,
per the repo's testing guideline.
- package.json: declare engines.node >=22 to match CI and the docs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
A post-migration audit of the Astro rewrite found that
safeUrl()regressed from a strict positive allowlist into a permissive fall-through, reopening a same-origin XSS. Also restores a retry that was lost in the bash→Node port of the checksum verifier.The vulnerability
safeUrl()tested the scheme with/^[a-z][a-z0-9+.-]*:/and fell through toreturn urlwhen that test failed:A scheme containing an ASCII tab, LF or CR fails that character class, so
java<TAB>script:alert(1)was returned unchanged and rendered straight into anhref. The WHATWG URL parser strips ASCII tab/LF/CR before resolving the scheme, so the browser resolves it back tojavascript:and executes it on click.Reproduced end-to-end: a fixture tap with
homepage "java<TAB>script:alert(document.domain)"emittedReachable from any
.rbhomepagevalue and from a GitHub releasehtml_url, through both server-rendered hrefs (DetailPage.astro,VersionHistory.astro) and the client-rendered search results (site.js).The pre-Astro bash
safe_urlwas a strict positive prefix allowlist and blocked this. The migration unified both copies onto the permissive client version — which is load-bearing for internal relative hrefs — costing the external-URL path its strictness.Fix
Strip C0 controls and DEL before the scheme test, in both the build-time (
src/lib/serialize.mjs) and bundled-client (src/scripts/site.js) copies. Filtered by code point so both sources stay pure ASCII, matching the existing convention inserialize.mjs.Relative, anchor,
https:,http:andmailto:URLs are unaffected.Also included
scripts/verify-checksums.mjslost curl's--retry 3 --retry-delay 3in the Node port, so one transient CDN error hard-failed the brew job and blocked the Pages deploy. Restored as a bounded, env-overridable retry loop matchingsrc/lib/releases.mjs. The 180s abort is unchanged.Tests
tests/safe-url.test.js(37 assertions) runs a blocked/allowed matrix against both copies ofsafeUrl: tab/LF/CR/NUL/DEL obfuscation,data:,vbscript:,file:, protocol-relative, plus the legitimate URLs that must pass through.tests/xss.test.jsnow uses the tab-obfuscated payload end-to-end; it previously only asserted the plainjavascript:case, which is why the regression went unnoticed.node scripts/verify-checksums.mjsverifies both real release artifacts.Audit context
This came out of a 5-dimension audit of what the Astro migration preserved from the earlier hardening PR: 13 candidate regressions were flagged, 11 refuted on adversarial verification, and these 2 confirmed. 32 guarantees (XSS containment, U+2028 handling, release-fetch timeouts/retries/pagination/OFFLINE/STRICT, JS-off server-rendering, search-dialog a11y, CI gating and SHA pins) were confirmed intact.
🤖 Generated with Claude Code
Summary by CodeRabbit
javascript:inputs collapse to#.