fix: oidc redirect

[parmesant]

• add redirect checks for logout
• add P_ALLOW_ORIGINS env var to allow redirection to other origins

Fixes #XXXX.

Description

---

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

• New Features
  • Added a configurable allowlist for permitted origins via a new command-line option and environment variable.
• Bug Fixes
  • Strengthened login/logout redirect validation by verifying redirect targets against the configured allowed origins.
  • Logout now surfaces invalid-redirect issues more reliably and returns consistent error responses when targets are not permitted.
• Refactor / Chores
  • Reorganized internal imports related to validation without changing behavior.

[coderabbitai]

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: b207875b-7f69-4ab6-822b-022f3301676c

📥 Commits

Reviewing files that changed from the base of the PR and between d3a9902 and 78c39d4.

📒 Files selected for processing (2)

• src/cli.rs
• src/handlers/http/oidc.rs

💤 Files with no reviewable changes (1)

• src/cli.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/handlers/http/oidc.rs

---

Walkthrough

Adds a new CLI option --allowed-origins (P_ALLOW_ORIGINS) storing a Vec<Url> on Options. Updates OIDC redirect validation to use exact-match checks against this allowlist and a scheme-aware base URL, changing logout to return Result<HttpResponse, OIDCError>. Also reorders unrelated imports in option.rs.

Changes

Allowed origins config and OIDC redirect validation

Layer / File(s)	Summary
CLI allowed origins option src/cli.rs	Adds allow_origins: Vec<Url> to Options and parses --allowed-origins/P_ALLOW_ORIGINS as a comma-delimited URL list with validation.
OIDC redirect validation and logout flow src/handlers/http/oidc.rs	Removes regex-based redirect checking, validates redirect origins against configured allowlisted origins or the request base URL, updates login to use scheme plus host, and changes logout to return Result<HttpResponse, OIDCError> while validating redirects and wrapping redirect responses.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant OIDCHandler
  participant RedirectValidator
  participant Config

  Client->>OIDCHandler: login/logout request with redirect
  OIDCHandler->>OIDCHandler: build base_url from scheme+host
  OIDCHandler->>RedirectValidator: is_valid_redirect_url(redirect, base_url)
  RedirectValidator->>Config: read allow_origins
  Config-->>RedirectValidator: allowed origins
  RedirectValidator-->>OIDCHandler: valid or invalid
  OIDCHandler-->>Client: redirect response or BadRequest

Loading

Poem

A rabbit hops through gates anew,
Checking origins, exact and true.
No regex maze, just clean compare,
Logout redirects handled with care.
Hop, hop, hooray — the allowlist grows! 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description leaves the required template sections as placeholders and omits the issue number and rationale.	Replace placeholders with a real issue reference, a clear goal/solution/key-changes description, and complete the checklist items that apply.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and matches the main change: OIDC redirect handling.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/handlers/http/oidc.rs (1)

656-664: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Irrefutable if let pattern and needless boolean returns.

if let allowed_origins = &PARSEABLE.options.allow_origins binds an irrefutable pattern, which triggers the irrefutable_let_patterns lint (a warning that fails CI under -D warnings) and also relies on the let-chains feature unnecessarily. Combined with the true/else { ... } branches, this can be collapsed into a single expression.

♻️ Proposed simplification

 fn is_valid_redirect_url(base_url: &str, redirect_url: &str) -> bool {
-    // check if redirect_url is part of allowed origins
-    if let allowed_origins = &PARSEABLE.options.allow_origins
-        && (allowed_origins
-            .iter()
-            .any(|url| url.as_str().eq(redirect_url)))
-    {
-        true
-    }
-    // if redirect_url is not part of allowed origins, then it must be equal to base url
-    else {
-        base_url == redirect_url
-    }
+    // allowed if redirect_url is an allowed origin, otherwise it must equal base_url
+    PARSEABLE
+        .options
+        .allow_origins
+        .iter()
+        .any(|url| url.as_str() == redirect_url)
+        || base_url == redirect_url
 }

🤖 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 `@src/handlers/http/oidc.rs` around lines 656 - 664, In is_valid_redirect_url,
remove the irrefutable if let let-chain around PARSEABLE.options.allow_origins
and simplify the function to a direct boolean expression; use
allow_origins.iter().any(...) to check the redirect_url against the allowed
list, then return that result together with the existing base_url validation
logic instead of branching on true/false.

🤖 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 `@src/handlers/http/oidc.rs`:
- Around line 656-668: The redirect validation in is_valid_redirect_url is
matching full serialized URLs instead of origins, so allowlisted redirects with
paths or queries are incorrectly rejected. Parse redirect_url as a Url and
compare its origin() against the origins from PARSEABLE.options.allow_origins
rather than using as_str() equality. Apply the same origin-based check to the
base_url fallback so it accepts matching origins consistently. Keep the fix
localized to is_valid_redirect_url in oidc.rs.

---

Nitpick comments:
In `@src/handlers/http/oidc.rs`:
- Around line 656-664: In is_valid_redirect_url, remove the irrefutable if let
let-chain around PARSEABLE.options.allow_origins and simplify the function to a
direct boolean expression; use allow_origins.iter().any(...) to check the
redirect_url against the allowed list, then return that result together with the
existing base_url validation logic instead of branching on true/false.

🪄 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: 0578ff15-d544-4fc4-adbb-41148d83561b

📥 Commits

Reviewing files that changed from the base of the PR and between 5c20b8e and d3a9902.

📒 Files selected for processing (3)

• src/cli.rs
• src/handlers/http/oidc.rs
• src/option.rs