Repository navigation
Conversation
redirectURIAllowed compared the decoded url.Path, so a percent-encoded variant of an allowed path, such as https://claude.ai/api%2Fmcp/auth_callback or http://127.0.0.1:53211/call%62ack, was accepted despite the function documenting a byte-for-byte match. Compare EscapedPath() instead. It also accepted a trailing empty fragment ("...callback#"), because url.Parse reports that as no fragment at all; RFC 6749 section 3.1.2 forbids fragments in redirect URIs, so reject any URI containing '#'. Both mirror the hardening in pgEdge/pgedge-mcp-bridge@05bb3e20. The userinfo bypass that commit fixed does not apply here, since the host was already taken from the parsed URI and userinfo rejected; a test case now pins that down too.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe redirect URI validator rejects URIs containing ChangesOAuth Redirect URI Validation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes both main changes: encoded redirect URI path comparison and rejection of all fragments. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
AntTheLimey
left a comment
AntTheLimey
left a comment
There was a problem hiding this comment.
Approving. The new cases fail on main (api%2Fmcp, call%62ack,
trailing #) and pass here. I probed 62 URIs against 10 allowed
entries. 14 went from accepted to rejected and none the other way.
The probe included %23, dot segments, backslash and userinfo host
tricks, punycode, encoded hosts, IPv6 loopback forms and control
characters. Go's EscapedPath() re-encoding fallback is not a bypass,
because handleAuthorize rebuilds the redirect with u.String(), so
every accepted URI lands on an allowed entry's exact path. make test,
make lint, -race, gofmt and go vet are clean. internal/oauth
coverage is unchanged at 89.6%. No changelog entry is needed while
OAuth sits under Unreleased. This merges cleanly with #293.
Follow-up hardening to the unreleased OAuth server (#287), mirroring pgEdge/pgedge-mcp-bridge@05bb3e20.
redirectURIAllowedcompared the decodedurl.Path, so a percent-encoded variant of an allowed path (https://claude.ai/api%2Fmcp/auth_callback,http://127.0.0.1:53211/call%62ack) was accepted, despite the function documenting a byte-for-byte match. It now comparesEscapedPath()..../auth_callback#) was accepted becauseurl.Parsereports it as no fragment; RFC 6749 section 3.1.2 forbids fragments, so any URI containing#is now rejected.The userinfo bypass the bridge fixed (
http://localhost:@attacker.example/callback) did not apply here, as we already took the host from the parsed URI and rejected userinfo; a test case now pins that down.Server and client Go suites pass; the live-database tests were skipped (no connection string set), and this touches no database code.
Summary by CodeRabbit