fix: preserve query string from X-Forwarded-Uri in decision handler#1265
fix: preserve query string from X-Forwarded-Uri in decision handler#1265LennartKoot wants to merge 1 commit into
Conversation
|
|
e127fb1 to
569188b
Compare
569188b to
5246bef
Compare
📝 WalkthroughWalkthroughThis change refines how the decision handler processes the ChangesX-Forwarded-Uri Query String Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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 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: 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 `@api/decision.go`:
- Around line 74-82: When X-Forwarded-Uri is present (check using xForwardedUri)
the code sets r.URL.Path but currently only assigns r.URL.RawQuery when a "?"
and second part exist, leaving any previous RawQuery intact; update the block in
the handler that inspects fwdUri (the if fwdUri := r.Header.Get(xForwardedUri) {
... } branch) so that when parts := strings.SplitN(fwdUri, "?", 2) yields
len(parts) == 2 you set r.URL.RawQuery = parts[1], and otherwise explicitly
clear r.URL.RawQuery = "" (so the forwarded URI is the sole source of path and
query, and previous queries from the /decisions prefix don’t leak). Ensure the
else branch that trims DecisionPath remains unchanged.
🪄 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: dfe6e8d7-68df-459b-8603-ece9e1af1621
📒 Files selected for processing (2)
api/decision.goapi/decision_test.go
| if fwdUri := r.Header.Get(xForwardedUri); fwdUri != "" { | ||
| parts := strings.SplitN(fwdUri, "?", 2) | ||
| r.URL.Path = parts[0] | ||
| if len(parts) == 2 { | ||
| r.URL.RawQuery = parts[1] | ||
| } | ||
| } else { | ||
| r.URL.Path = r.URL.Path[len(DecisionPath):] | ||
| } |
There was a problem hiding this comment.
Clear RawQuery when X-Forwarded-Uri has no query string.
When X-Forwarded-Uri is present but contains no query string (no ?), the current code doesn't clear r.URL.RawQuery. This means if the original request to /decisions included a query string (e.g., /decisions?foo=bar), it would persist and be incorrectly combined with the forwarded path.
For example: a request to /decisions?original=query with X-Forwarded-Uri: /target/path would result in /target/path?original=query, which is likely unintended.
When X-Forwarded-Uri is set, it should be the sole source of truth for both path and query.
🔧 Proposed fix
if fwdUri := r.Header.Get(xForwardedUri); fwdUri != "" {
parts := strings.SplitN(fwdUri, "?", 2)
r.URL.Path = parts[0]
if len(parts) == 2 {
r.URL.RawQuery = parts[1]
+ } else {
+ r.URL.RawQuery = ""
}
} else {
r.URL.Path = r.URL.Path[len(DecisionPath):]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if fwdUri := r.Header.Get(xForwardedUri); fwdUri != "" { | |
| parts := strings.SplitN(fwdUri, "?", 2) | |
| r.URL.Path = parts[0] | |
| if len(parts) == 2 { | |
| r.URL.RawQuery = parts[1] | |
| } | |
| } else { | |
| r.URL.Path = r.URL.Path[len(DecisionPath):] | |
| } | |
| if fwdUri := r.Header.Get(xForwardedUri); fwdUri != "" { | |
| parts := strings.SplitN(fwdUri, "?", 2) | |
| r.URL.Path = parts[0] | |
| if len(parts) == 2 { | |
| r.URL.RawQuery = parts[1] | |
| } else { | |
| r.URL.RawQuery = "" | |
| } | |
| } else { | |
| r.URL.Path = r.URL.Path[len(DecisionPath):] | |
| } |
🤖 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 `@api/decision.go` around lines 74 - 82, When X-Forwarded-Uri is present (check
using xForwardedUri) the code sets r.URL.Path but currently only assigns
r.URL.RawQuery when a "?" and second part exist, leaving any previous RawQuery
intact; update the block in the handler that inspects fwdUri (the if fwdUri :=
r.Header.Get(xForwardedUri) { ... } branch) so that when parts :=
strings.SplitN(fwdUri, "?", 2) yields len(parts) == 2 you set r.URL.RawQuery =
parts[1], and otherwise explicitly clear r.URL.RawQuery = "" (so the forwarded
URI is the sole source of path and query, and previous queries from the
/decisions prefix don’t leak). Ensure the else branch that trims DecisionPath
remains unchanged.
2 participants
This PR sets the
r.URL.RawQueryto a potential query string provided within theX-Forwarded-Uriheader. This was dropped in an earlier version because including it in the path broke the URL matcher, but fully stripping the query string breaks authenticator that rely on query parameters.Related issue(s)
#1264
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.com) from the maintainers to push
the changes.
works.
Further Comments
Summary by CodeRabbit