Repository navigation
fix: OAuth connector helpers must enforce the connector's host allowlist before attaching credentials - #313
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a connector host allowlist mechanism, enforces it before materializing/attaching OAuth tokens in authenticated fetch paths (including the prelude sandbox), provides helpers and a specific error type, adds tests covering allowed/disallowed flows, and documents the requirement. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthFetch as createAuthenticatedFetch
participant Allowlist as Connector Host Allowlist
participant TokenSvc as OAuth Token Endpoint
participant Destination as Destination API
Client->>AuthFetch: request via authenticated fetch (url, connectorConfig)
AuthFetch->>AuthFetch: resolve final URL
AuthFetch->>Allowlist: assertConnectorHostAllowed(connectorName, connector, resolvedURL)
alt host allowed
Allowlist-->>AuthFetch: allowed (host in allowed set)
AuthFetch->>TokenSvc: fetch access token
TokenSvc-->>AuthFetch: return access token
AuthFetch->>Destination: outbound request with Authorization: Bearer <token>
Destination-->>AuthFetch: response
AuthFetch-->>Client: response
else host not allowed
Allowlist-->>AuthFetch: throw ConnectorHostNotAllowedError (no token disclosed)
AuthFetch-->>Client: error (no network call to Destination)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
kentcdodds
marked this pull request as ready for review
May 1, 2026 20:01
Contributor
|
🔎 Preview deployed: https://kody-pr-313.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/execute-modules/connector-host-allowlist.ts`:
- Around line 78-79: The current code in connector-host-allowlist.ts returns
early when getConnectorAllowedHosts(connector) yields an empty array, leaving
the connector in a fail-open state; instead, when allowedHosts.length === 0 you
must reject the configuration (throw an Error) so misconfigured connectors fail
closed. Update the logic in connector-host-allowlist.ts to throw a descriptive
error when getConnectorAllowedHosts(connector) returns an empty list
(referencing getConnectorAllowedHosts, createAuthenticatedFetch and
ConnectorConfig) and make the identical change in the mirrored
prelude/codemode-utils.ts copy so both code paths enforce a non-empty allowlist
rather than returning silently.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f22c022-214e-4e8f-8088-7b9652b9a1b5
📒 Files selected for processing (4)
docs/contributing/architecture/index.mdpackages/worker/src/mcp/execute-modules/authenticated-fetch.node.test.tspackages/worker/src/mcp/execute-modules/codemode-utils.tspackages/worker/src/mcp/execute-modules/connector-host-allowlist.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8264cc3. Configure here.
OAuth connector helpers (createAuthenticatedFetch and the sandboxed prelude equivalent) now assert that the outbound request URL targets a host listed in the connector's requiredHosts or derived from its apiBaseUrl before attaching the bearer token. - Add ConnectorHostNotAllowedError and reusable assertConnectorHostAllowed / getConnectorAllowedHosts helpers in connector-host-allowlist.ts. - Enforce the allowlist in both the native createAuthenticatedFetch and the sandboxed __kodyCreateAuthenticatedFetch prelude code. - Mark refreshAccessToken @internal with JSDoc documenting the security boundary (materialized token bypasses the fetch gateway's placeholder-based host check). - Add unit tests in authenticated-fetch.node.test.ts covering rejection for disallowed hosts, success for allowed hosts, and token non-leakage in error messages. - Document the helper-level allowlist invariant in docs/contributing/architecture/index.md.
- assertConnectorHostAllowed now throws when the connector has no allowed hosts configured (requiredHosts and apiBaseUrl both empty) instead of silently allowing all requests. - Protocol-relative URLs (//evil.com/steal) are resolved with a dummy scheme before host extraction, preventing bypass of the allowlist check. - Both the native module and the sandboxed prelude are updated. - Added tests for both edge cases.
cursor
Bot
force-pushed
the
security/group-2-oauth-token-egress
branch
from
May 1, 2026 21:16
c6aa525 to
d9af8ed
Compare
This was referenced May 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a HIGH-severity finding where
createAuthenticatedFetch(and its sandboxed prelude equivalent) would attach a materialized OAuth bearer token to outbound requests targeting any absolute URL, allowing sandboxed packages or generated UIs to exfiltrate connected OAuth tokens to arbitrary attacker-controlled hosts.Changes
New:
connector-host-allowlist.tsReusable enforcement logic extracted into a focused module:
ConnectorHostNotAllowedError— typed error thrown when a request targets a disallowed hostgetConnectorAllowedHosts(connector)— resolves the full set of normalized allowed hosts fromrequiredHosts+apiBaseUrlassertConnectorHostAllowed(connectorName, connector, url)— pre-flight assertion that throws before any credential attachmentModified:
codemode-utils.tscreateAuthenticatedFetchnow callsassertConnectorHostAllowedon the resolved URL before attaching theAuthorizationheader. If the host is not in the connector's allowlist, the request never fires.createExecuteHelperPrelude) includes an equivalent inline implementation so module-mode packages get the same enforcement.refreshAccessTokenis annotated@internalwith a JSDoc comment explaining the security boundary: callers that use the raw token must enforce the host allowlist themselves.New tests:
authenticated-fetch.node.test.tsattacker.example) is rejected withConnectorHostNotAllowedErrorandfetchis never called.apiBaseUrlhost andrequiredHostsentries succeed with the bearer attached.//evil.com/steal) are blocked.Docs:
docs/contributing/architecture/index.mdDocuments the helper-level allowlist invariant for future contributors.
Security Model
The fetch gateway (
fetch-gateway.ts) enforces host allowlists only for secret placeholders ({{secret:NAME}}). Once a token is materialized into a raw string (asrefreshAccessTokendoes), the gateway cannot see it. The fix enforces the same connector allowlist at the helper layer, which is the only code path that materializes and attaches connector tokens to outbound requests.Defense-in-depth measures
requiredHostsnorapiBaseUrlconfigured, the helper throws rather than silently allowing all hosts.//evil.com/stealare resolved with a dummy scheme to extract the host for allowlist checking, preventing bypass through this vector.Summary by CodeRabbit
New Features
Tests
Documentation