Skip to content

Fix/security OIDC open redirect#1653

Open
ygndotgg wants to merge 6 commits into
parseablehq:mainfrom
ygndotgg:fix/security-oidc-open-redirect
Open

Fix/security OIDC open redirect#1653
ygndotgg wants to merge 6 commits into
parseablehq:mainfrom
ygndotgg:fix/security-oidc-open-redirect

Conversation

@ygndotgg
Copy link
Copy Markdown

@ygndotgg ygndotgg commented May 28, 2026

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 Host header plus a matching external redirect URL could be
treated 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 Host is not a reliable trust boundary behind proxies, ingress
controllers, 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 the
canonical Parseable origin. Deployments behind proxies/TLS terminators should set P_ORIGIN_URI
to the public Parseable URL, for example https://parseable.example.com.

The redirect query parameter is now optional for OIDC login/logout
to prevent deserialization failures when the frontend omits it.

Key changes:

  • Validate OIDC login, logout, and callback state redirects against the canonical origin.
  • Redirect the no-OIDC fallback to the canonical /oidc-not-configured path.
  • Make the redirect query parameter optional in OIDC login/logout (via #[serde(default)])
  • Document P_ORIGIN_URI usage for proxied/TLS-terminated deployments.

This PR has:

  • been tested to ensure log ingestion and log query works.
  • added comments explaining the "why" and the intent of the code wherever would not be
    obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

Summary by CodeRabbit

  • Bug Fixes

    • Strengthened OIDC redirect validation to prevent unsafe or malformed redirects
    • Improved logout flow behavior and error responses, including clearer JSON errors for invalid redirect inputs
  • Documentation

    • Clarified CLI help text for the public origin setting
    • Added a commented configuration and guidance for setting the public origin when behind a reverse proxy or TLS terminator

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 28, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 90be1791-da87-4c17-bc40-1e8d5472788f

📥 Commits

Reviewing files that changed from the base of the PR and between 3301320 and f0bec66.

📒 Files selected for processing (1)
  • src/cli.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/cli.rs

Walkthrough

Replaces 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.

Changes

OIDC Redirect Validation Refactoring

Layer / File(s) Summary
Redirect validation helpers
src/handlers/http/oidc.rs
Adds get_canonical_origin, is_valid_redirect_url_safe, and absolute_redirect_url; removes regex-based validator.
Login data model & handler update
src/handlers/http/oidc.rs
Changes RedirectAfterLogin.redirect to String (defaulted) and validates/normalizes login redirect, returning BadRequest for invalid inputs.
Basic-auth redirect handoff
src/handlers/http/oidc.rs
Uses the validated redirect value during basic-auth exchange redirect handoff.
Logout handler refactor & validation
src/handlers/http/oidc.rs
Changes logout to return Result<HttpResponse, OIDCError>, validates logout redirect against canonical origin, early-redirects when no session, and only triggers OIDC logout for configured OAuth users.
OIDC logout URL construction & fallback
src/handlers/http/oidc.rs
redirect_to_oidc_logout now accepts redirect: &str and canonical: &Url, computes post_logout_redirect_uri via absolute_redirect_url, clears existing logout query, appends the redirect param, and uses canonical.join("/oidc-not-configured") for the non-OIDC fallback.
Post-login callback redirect selection
src/handlers/http/oidc.rs
build_login_response validates login_query.state with the safe validator and returns JSON BadRequest for invalid state or defaults to canonical origin.
Configuration & CLI help
helm/values.yaml, src/cli.rs
Add commented P_ORIGIN_URI guidance for reverse-proxy/TLS deployments and update --origin help text to reference the public canonical origin used for OIDC redirects.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • nikhilsinhaparseable

Poem

🐰 A rabbit hops through code and light,
Validates redirects through day and night,
Canonical origins keep paths in line,
Logout flows tidy, and helpers shine,
Helm and CLI whisper the public sign.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix/security OIDC open redirect' directly describes the main security fix in the changeset—addressing an open redirect vulnerability in OIDC validation by switching to canonical-origin-based validation.
Description check ✅ Passed The description is comprehensive and well-structured, covering the problem, solution rationale, key changes, and deployment guidance. It addresses the core vulnerability and includes comments and documentation as required.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between a3949b0 and 71c94d8.

📒 Files selected for processing (3)
  • helm/values.yaml
  • src/cli.rs
  • src/handlers/http/oidc.rs

Comment thread helm/values.yaml Outdated
Comment thread src/handlers/http/oidc.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant