Skip to content

Harden release URL and redirect validation - #1518

Open
peter-research wants to merge 4 commits into
CodebuffAI:mainfrom
peter-research:fix/release-origin-and-redirect-validation
Open

peter-research wants to merge 4 commits into
CodebuffAI:mainfrom
peter-research:fix/release-origin-and-redirect-validation

Conversation

@peter-research

Copy link
Copy Markdown

Summary

  • Reject release download URL overrides that include a path or a nonstandard port on public release hosts.
  • Keep custom ports available for loopback release servers used during local development.
  • Refuse release redirects that embed URL credentials or change the original port.
  • Add regression coverage for malformed origins and unsafe redirects.

These checks keep release requests on the intended origin and prevent redirects from reaching unexpected endpoints on otherwise allowlisted hosts.

Testing

Added regression tests in cli/src/__tests__/release/wrapper-safety.test.ts and cli/src/__tests__/release/proxy-http-get.test.ts. The Bun test suite has not been run in this environment.

@codebuff-team

Copy link
Copy Markdown
Contributor

Good, focused security hardening. The core changes are sound:

  • evaluateRedirect in cli/release-core/http.js now rejects redirects carrying embedded credentials and redirects that change the port versus the original request. Both are classic SSRF/redirect-smuggling vectors, and pinning port equality to originalUrl.port is the right fix.
  • resolveDownloadOrigin in launcher.js now requires pathname === '/' for a 'bare origin' and disallows nonstandard ports on the public HTTPS release host, while still permitting custom ports for loopback dev servers. That matches the stated intent in the PR body and doesn't regress the documented local-dev workflow.
  • Tests cover both the launcher override validation (wrapper-safety.test.ts) and the HTTP redirect refusal path (proxy-http-get.test.ts), including a table-driven check for both credential and port violations in one pass.

Two things worth checking before porting:

  1. target.port !== originalUrl.port compares empty-string ports against each other correctly when both are default (e.g., https://), but confirm this doesn't break the case where a redirect moves from an explicit :443 to the implicit default — URL.port normalizes default ports to '', so https://host:443 and https://host should compare equal, but worth a quick explicit test since this is a subtle area.
  2. The isBareOrigin path-check change (parsed.pathname === '/') will reject any existing config that uses http://host without a trailing slash improperly, but new URL('http://host').pathname is already /, so this should be fine — just flagging it as the single most behavior-changing line for ops to be aware of.

Overall this is a tight, well-tested security fix with a clear cause and no scope creep. Recommend porting after a maintainer double-checks the port-equality edge case above.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Oct 14, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants