Skip to content

fix(orb): validate ORB_BROKER_URL to prevent enrollment-secret leakage#1401

Open
JSONbored wants to merge 1 commit into
mainfrom
codex/propose-fix-for-broker-url-vulnerability
Open

fix(orb): validate ORB_BROKER_URL to prevent enrollment-secret leakage#1401
JSONbored wants to merge 1 commit into
mainfrom
codex/propose-fix-for-broker-url-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Prevent accidental leakage of the long-lived ORB_ENROLLMENT_SECRET when an operator overrides ORB_BROKER_URL with an unsafe destination.
  • Enforce a fail-closed policy: require valid URLs, reject userinfo/query/fragment, and require https: except for explicit localhost development hosts.

Description

  • Add orbBrokerBaseUrl() which parses ORB_BROKER_URL, rejects invalid URLs, userinfo, query/fragment, and disallows plaintext http: except for localhost, 127.0.0.1, and [::1] development hosts.
  • Replace raw string normalization with orbBrokerBaseUrl() in fetchBrokeredInstallationToken and registerOrbRelayTarget so the enrollment secret is never sent to an unvalidated origin.
  • Add unit tests in test/unit/orb-broker-client.test.ts covering unsafe URL errors (invalid URL, userinfo, query/fragment, non-HTTPS), localhost HTTP allowance, and relay-registration fail-closed behavior.
  • Small host handling tweak to accept IPv6 loopback string form for localhost development ([::1]).

Testing

  • Ran unit tests with npx vitest run test/unit/orb-broker-client.test.ts test/unit/github-app.test.ts and observed both test files pass (39 tests passed).
  • Ran static checks with npm run typecheck and git diff --check, both of which succeeded locally and showed no type or diff issues.
  • Ran repository-wide coverage (npm run test:coverage) which started but failed during coverage remapping due to an upstream dependency mismatch (TypeError: jsTokens is not a function) unrelated to the PR logic; vitest unit runs themselves passed.
  • Attempted npm audit/npm ci which hit registry access errors in this environment (403), so dependency-audit and full CI audit steps could not complete here.

Codex Task

@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jun 26, 2026
@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.34%. Comparing base (6138b9e) to head (c06513e).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1401   +/-   ##
=======================================
  Coverage   95.33%   95.34%           
=======================================
  Files         192      192           
  Lines       20750    20763   +13     
  Branches     7500     7504    +4     
=======================================
+ Hits        19783    19796   +13     
  Misses        383      383           
  Partials      584      584           
Files with missing lines Coverage Δ
src/orb/broker-client.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

aardvark codex size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant