Fix/security OIDC open redirect#1653
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaces regex-based redirect validation with canonical-origin-aware helpers; validates and normalizes redirects in /login and logout flows, changes RedirectAfterLogin.redirect to a defaulted String, updates logout signature to return Result, and adds helm/CLI guidance for the canonical origin. ChangesOIDC Redirect Validation Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@helm/values.yaml`:
- Around line 53-55: The P_ORIGIN_URI example in the env documentation is
indented too far and will break YAML if uncommented; move the commented example
key "P_ORIGIN_URI: \"https://parseable.example.com\"" out to the same
indentation level as other env keys (align with the "env" entries), removing the
extra leading spaces so the comment preserves valid YAML structure when
uncommented and visually matches related env entries.
In `@src/handlers/http/oidc.rs`:
- Around line 666-669: The function is_valid_redirect_url_safe currently rejects
an empty redirect string (due to serde defaulting missing redirect to ""), which
breaks optional redirect behavior; change the empty-check to treat an empty
redirect as "no redirect provided" by returning
Ok(canonical.as_str().to_string()) (i.e., use the canonical URL as the effective
redirect) instead of Err(()). Update the branch that checks redirect.is_empty()
in is_valid_redirect_url_safe to return the canonical URL string and keep the
rest of the validation logic for non-empty redirect values 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c39d35ca-5b88-4d8c-bfa7-cfc685108411
📒 Files selected for processing (3)
helm/values.yamlsrc/cli.rssrc/handlers/http/oidc.rs
Fixes [#XXXX]
Description
This PR fixes a Host-header trust issue in OIDC redirect validation that could allow open
redirects. Previously, OIDC login/logout redirects could derive the allowed origin from request
host information, so a spoofed
Hostheader plus a matching external redirect URL could betreated as same-origin.
The likely intent of the old behavior was convenience: redirects could work automatically across
local, proxied, and multi-host deployments without a configured public URL. The tradeoff was
unsafe for OIDC, because request
Hostis not a reliable trust boundary behind proxies, ingresscontrollers, load balancers, or TLS terminators.
This patch validates OIDC redirect targets against a configured canonical origin instead of the
request
Host. Relative redirects are still allowed, and absolute redirects must match thecanonical Parseable origin. Deployments behind proxies/TLS terminators should set
P_ORIGIN_URIto the public Parseable URL, for example
https://parseable.example.com.The
redirectquery parameter is now optional for OIDC login/logoutto prevent deserialization failures when the frontend omits it.
Key changes:
stateredirects against the canonical origin./oidc-not-configuredpath.redirectquery parameter optional in OIDC login/logout (via#[serde(default)])P_ORIGIN_URIusage for proxied/TLS-terminated deployments.This PR has:
obvious for an unfamiliar reader.
Summary by CodeRabbit
Bug Fixes
Documentation