feat: Updates for OAUTH implementation#1567
Merged
nikhilsinhaparseable merged 19 commits intoparseablehq:mainfrom Mar 12, 2026
Merged
feat: Updates for OAUTH implementation#1567nikhilsinhaparseable merged 19 commits intoparseablehq:mainfrom
nikhilsinhaparseable merged 19 commits intoparseablehq:mainfrom
Conversation
Contributor
WalkthroughIntroduce a pluggable OAuthProvider abstraction and GlobalClient, migrate OIDC login to code-exchange, add header-aware token refresh (propagating request headers and stripping Basic auth for intra-cluster flows), propagate caller_userid through cluster syncs, and replace broad session invalidation with targeted permission refreshes; expose a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as AppHandler
participant Provider as OAuthProvider
participant IDP as IdentityProvider
participant UserDB as UserStore
Client->>Server: GET /login or callback with code
Server->>Provider: auth_url(...) or exchange_code(code)
Provider->>IDP: token request (code)
IDP-->>Provider: access_token + id_token
Provider->>Provider: validate id_token
alt JWKS validation fails
Provider->>IDP: re-discovery (new JWKS)
IDP-->>Provider: updated JWKS
Provider->>Provider: retry validation
end
Provider->>IDP: fetch userinfo (access_token)
IDP-->>Provider: userinfo
Provider-->>Server: OAuthSession { bearer, claims, userinfo }
Server->>UserDB: provision/update user & roles
Server->>Server: create session (derive expiry from bearer)
Server->>Client: set cookies / redirect or 200 JSON (XHR)
Note over Server,Provider: On refresh, Server forwards request headers -> Provider.refresh_token(oauth, scope, headers)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/http/oidc.rs`:
- Around line 215-225: The code currently uses expect on user_info when
computing username (the variable username built from user_info.name/email/sub),
which can panic; change this to perform the same safe matching as user_id: check
user_info.name, then email, then sub and if none exist log a descriptive error
(e.g., tracing::error!("OAuth provider did not return a usable identifier")) and
return Err(OIDCError::Unauthorized) instead of panicking; ensure the username
computation and the existing user_id match logic both handle missing fields
without unwrap/expect and return the controlled error from the OIDC callback
handler.
- Around line 86-94: The code currently injects the client-provided redirect URL
into the OAuth state (variable redirect and the call
client.read().await.auth_url(&scope, Some(redirect))) which allows
CSRF/session-swapping; change this to generate a server-side random nonce (e.g.,
cryptographically secure random string), store that nonce server-side (session
store, signed cookie, or in-memory map keyed by session id) along with the
intended redirect, then pass only the nonce as the state to auth_url; in the
callback handler validate the incoming state against the stored nonce (look up
and remove it) and only then read the mapped redirect URL to perform the
redirect. Ensure use of a secure RNG, expiry for nonces, and that the symbols to
update are the code that constructs auth_url (client.read().await.auth_url) and
the callback that currently trusts state as redirect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 142adba5-7651-4022-9f31-9a56c6381414
📒 Files selected for processing (9)
src/handlers/http/middleware.rssrc/handlers/http/modal/mod.rssrc/handlers/http/modal/server.rssrc/handlers/http/oidc.rssrc/lib.rssrc/oauth/mod.rssrc/oauth/oidc_client.rssrc/oauth/provider.rssrc/rbac/user.rs
💤 Files with no reviewable changes (1)
- src/handlers/http/middleware.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/handlers/http/oidc.rs (2)
215-220:⚠️ Potential issue | 🟠 MajorAvoid
expectin the OAuth callback path.Line 220 can panic on incomplete provider payloads. Return a controlled auth error instead.
🛠️ Proposed fix
- let username = user_info - .name - .clone() - .or_else(|| user_info.email.clone()) - .or_else(|| user_info.sub.clone()) - .expect("OAuth provider did not return a usable identifier (name, email or sub)"); + let Some(username) = user_info + .name + .clone() + .or_else(|| user_info.email.clone()) + .or_else(|| user_info.sub.clone()) + else { + tracing::error!("OAuth provider did not return a usable identifier (name, email or sub)"); + return Err(OIDCError::Unauthorized); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 215 - 220, The code currently uses expect when deriving username from user_info (the username binding), which can panic if name/email/sub are missing; replace the expect with a controlled error path in the OAuth callback handler: if user_info.name.clone().or_else(|| user_info.email.clone()).or_else(|| user_info.sub.clone()) yields None, return an authentication error (e.g., Err(AuthError::InvalidProviderResponse(...)) or the handler's existing auth error type) with a clear message instead of panicking; locate the username binding and change it to return that Result/Err up the stack so the callback returns a proper auth error rather than calling expect.
86-94:⚠️ Potential issue | 🔴 CriticalStop using redirect URL as OAuth
state; validate server-issued state in callback.Line 89 and Line 146 still set
stateto a client redirect URL, and Line 321-325 still trusts thatstateas the post-login redirect. This keeps the login CSRF/session-swapping risk open.🔐 Suggested fix pattern
- let redirect = query.into_inner().redirect.to_string(); - let mut auth_url: String = client.read().await.auth_url(&scope, Some(redirect)).into(); + let redirect = query.into_inner().redirect.to_string(); + let state = Ulid::new().to_string(); + // Persist {state -> redirect} server-side with short TTL and one-time use. + let mut auth_url: String = client.read().await.auth_url(&scope, Some(state)).into();- let redirect_url = login_query - .state - .clone() - .unwrap_or_else(|| PARSEABLE.options.address.to_string()); + let redirect_url = resolve_redirect_from_validated_state(login_query.state.as_deref()) + .ok_or(OIDCError::Unauthorized)?;Also applies to: 141-151, 321-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 86 - 94, The code is using the client-provided redirect URL as the OAuth "state" (see redirect variable and client.read().await.auth_url(&scope, Some(redirect)) creating auth_url) and later trusting it in the callback, which allows CSRF/session-swapping; instead, generate a cryptographically random server-side state token (store it server-side tied to the user/session, e.g., in session storage or a server-side store keyed by session ID) and pass that token as the OAuth state when building auth_url, then on callback validate the incoming state against the stored token (reject if missing/mismatched) before using any redirect URL; ensure the original client redirect URL is validated against an allowlist before following it and never use it directly as the state value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/http/oidc.rs`:
- Around line 314-320: The XHR success branch (check is_xhr) builds an
HttpResponse::Ok(), sets cookies via response.cookie(cookie), then finishes
without setting cache headers; update that branch to add the non-cache header
before finish by inserting "Cache-Control: no-store" on the response (e.g., via
insert_header or equivalent) so HttpResponse::Ok() with cookies includes
Cache-Control: no-store prior to response.finish().
---
Duplicate comments:
In `@src/handlers/http/oidc.rs`:
- Around line 215-220: The code currently uses expect when deriving username
from user_info (the username binding), which can panic if name/email/sub are
missing; replace the expect with a controlled error path in the OAuth callback
handler: if user_info.name.clone().or_else(||
user_info.email.clone()).or_else(|| user_info.sub.clone()) yields None, return
an authentication error (e.g., Err(AuthError::InvalidProviderResponse(...)) or
the handler's existing auth error type) with a clear message instead of
panicking; locate the username binding and change it to return that Result/Err
up the stack so the callback returns a proper auth error rather than calling
expect.
- Around line 86-94: The code is using the client-provided redirect URL as the
OAuth "state" (see redirect variable and client.read().await.auth_url(&scope,
Some(redirect)) creating auth_url) and later trusting it in the callback, which
allows CSRF/session-swapping; instead, generate a cryptographically random
server-side state token (store it server-side tied to the user/session, e.g., in
session storage or a server-side store keyed by session ID) and pass that token
as the OAuth state when building auth_url, then on callback validate the
incoming state against the stored token (reject if missing/mismatched) before
using any redirect URL; ensure the original client redirect URL is validated
against an allowlist before following it and never use it directly as the state
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6b11945f-a601-4ad4-a08b-c3ca02f963dc
📒 Files selected for processing (1)
src/handlers/http/oidc.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/handlers/http/oidc.rs (3)
314-324:⚠️ Potential issue | 🟡 MinorAdd
Cache-Control: no-storeto the XHR success response.This branch sets auth cookies and returns identity/session data, but does not set non-cache headers.
🧩 Proposed fix
if is_xhr { let mut response = HttpResponse::Ok(); + response.insert_header((actix_web::http::header::CACHE_CONTROL, "no-store")); for cookie in cookies { response.cookie(cookie); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 314 - 324, The XHR success branch (when is_xhr is true) sets cookies and returns JSON but lacks non-cache headers; update the HttpResponse builder used in that branch (the response variable created from HttpResponse::Ok()) to include Cache-Control: no-store (e.g., via response.insert_header(("Cache-Control", "no-store")) or response.header(...)) before adding cookies and calling .json(...), so the final Ok(response.json(...)) returns the non-cache header along with cookies and JSON payload.
215-221:⚠️ Potential issue | 🟠 MajorAvoid panic in callback identity resolution.
Line 220 uses
expect(...); missing provider identifiers should return a controlled auth error, not panic.🛠️ Proposed fix
- let username = user_info - .name - .clone() - .or_else(|| user_info.email.clone()) - .or_else(|| user_info.sub.clone()) - .expect("OAuth provider did not return a usable identifier (name, email or sub)"); + let Some(username) = user_info + .name + .clone() + .or_else(|| user_info.email.clone()) + .or_else(|| user_info.sub.clone()) + else { + tracing::error!("OAuth provider did not return a usable identifier (name, email or sub)"); + return Err(OIDCError::Unauthorized); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 215 - 221, The code currently uses expect when deriving username from user_info (the username variable created from user_info.name/email/sub) which can panic; change this to return a controlled authentication error instead: validate that at least one of user_info.name, user_info.email or user_info.sub exists, and if none are present return an authentication failure (e.g., Err(AuthError::InvalidIdentity) or an appropriate HTTP 400/401 response) from the OIDC callback handler rather than calling expect; update any downstream use (e.g., the creation of user_id from user_info.sub) to handle the same error path so the handler returns a proper error response instead of panicking.
86-94:⚠️ Potential issue | 🔴 CriticalStop using redirect URL as OAuth
state; validate a server-issued nonce instead.Line 89 and Line 146 send client redirect as
state, and Line 325-Line 330 later trustsstateas redirect input. This still enables login CSRF/session-swapping.🔐 Suggested fix pattern
- let redirect = query.into_inner().redirect.to_string(); - let mut auth_url: String = client.read().await.auth_url(&scope, Some(redirect)).into(); + let redirect = query.into_inner().redirect.to_string(); + let state = issue_oauth_state_nonce(&req, redirect)?; // store {state -> redirect} with TTL + let mut auth_url: String = client.read().await.auth_url(&scope, Some(state)).into();- let redirect_url = login_query - .state - .clone() - .unwrap_or_else(|| PARSEABLE.options.address.to_string()); + let redirect_url = consume_validated_redirect_from_state(&req, login_query.state.as_deref()) + .unwrap_or_else(|| PARSEABLE.options.address.to_string());Also applies to: 141-151, 325-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 86 - 94, Do not use the client's redirect parameter as the OAuth state; instead generate a cryptographically-random server nonce when handling the initial request (where redirect is read and auth_url is built), persist that nonce tied to the user's session or a short-lived server-side store, append that server-issued nonce as the state parameter in auth_url (instead of the redirect string), and remove any trust of state later in the callback handler—on callback verify the returned state equals the stored nonce and only then read the original redirect from the server-side store (or session) to perform the final redirect; update the code paths around the redirect variable / auth_url construction and the callback validation logic to implement this nonce generation, storage, and validation flow and ensure proper URL-encoding of the stored redirect when issuing the final Location header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/http/oidc.rs`:
- Around line 319-323: Remove the raw session identifier from the JSON response
to avoid credential leakage: stop returning id.to_string() in the response body
and only return non-sensitive fields (e.g., "username" and "user_id" or a
boolean like "authenticated": true). Update the response generation in the
handler that currently calls response.json(serde_json::json!({ "session":
id.to_string(), ... })) to omit "session" (or replace it with a non-sensitive
flag or masked token) because the real session key is already set in the cookie;
keep the cookie-setting logic untouched and ensure any client-side needs are
satisfied without exposing the full session id.
---
Duplicate comments:
In `@src/handlers/http/oidc.rs`:
- Around line 314-324: The XHR success branch (when is_xhr is true) sets cookies
and returns JSON but lacks non-cache headers; update the HttpResponse builder
used in that branch (the response variable created from HttpResponse::Ok()) to
include Cache-Control: no-store (e.g., via
response.insert_header(("Cache-Control", "no-store")) or response.header(...))
before adding cookies and calling .json(...), so the final
Ok(response.json(...)) returns the non-cache header along with cookies and JSON
payload.
- Around line 215-221: The code currently uses expect when deriving username
from user_info (the username variable created from user_info.name/email/sub)
which can panic; change this to return a controlled authentication error
instead: validate that at least one of user_info.name, user_info.email or
user_info.sub exists, and if none are present return an authentication failure
(e.g., Err(AuthError::InvalidIdentity) or an appropriate HTTP 400/401 response)
from the OIDC callback handler rather than calling expect; update any downstream
use (e.g., the creation of user_id from user_info.sub) to handle the same error
path so the handler returns a proper error response instead of panicking.
- Around line 86-94: Do not use the client's redirect parameter as the OAuth
state; instead generate a cryptographically-random server nonce when handling
the initial request (where redirect is read and auth_url is built), persist that
nonce tied to the user's session or a short-lived server-side store, append that
server-issued nonce as the state parameter in auth_url (instead of the redirect
string), and remove any trust of state later in the callback handler—on callback
verify the returned state equals the stored nonce and only then read the
original redirect from the server-side store (or session) to perform the final
redirect; update the code paths around the redirect variable / auth_url
construction and the callback validation logic to implement this nonce
generation, storage, and validation flow and ensure proper URL-encoding of the
stored redirect when issuing the final Location header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 63bcb549-2f87-4d06-bef5-85975a003d69
📒 Files selected for processing (1)
src/handlers/http/oidc.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/http/oidc.rs (1)
257-257:⚠️ Potential issue | 🔴 CriticalFix:
tenant_idis moved here but used again later (pipeline failure).The
find_existing_userfunction takestenant_idby value, moving it. However,tenant_idis used again at line 302. This causes a compile errorE0382: borrow of moved value.🐛 Proposed fix
- let existing_user = find_existing_user(&user_info, tenant_id); + let existing_user = find_existing_user(&user_info, tenant_id.clone());Alternatively, change
find_existing_userto take a reference:-fn find_existing_user(user_info: &user::UserInfo, tenant_id: Option<String>) -> Option<User> { +fn find_existing_user(user_info: &user::UserInfo, tenant_id: &Option<String>) -> Option<User> {And update the call sites accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` at line 257, The call to find_existing_user(&user_info, tenant_id) moves tenant_id, causing a later use to fail; change find_existing_user to accept a reference to tenant_id (e.g., &TenantId or &str depending on type) and update its signature and all call sites accordingly, or alternatively pass a cloned value (tenant_id.clone()) here to avoid moving; update the function signature of find_existing_user and its implementations/usages so tenant_id is borrowed instead of moved.
♻️ Duplicate comments (3)
src/handlers/http/oidc.rs (3)
215-227:⚠️ Potential issue | 🟠 MajorAvoid panic in OAuth callback path.
Line 220 uses
expect(...)which will panic if the OAuth provider doesn't return a usable identifier (name, email, or sub). This can crash the worker thread instead of returning a controlled auth error.
,🛠️ Proposed fix
- let username = user_info + let Some(username) = user_info .name .clone() .or_else(|| user_info.email.clone()) .or_else(|| user_info.sub.clone()) - .expect("OAuth provider did not return a usable identifier (name, email or sub)"); + else { + tracing::error!("OAuth provider did not return a usable identifier (name, email or sub)"); + return Err(OIDCError::Unauthorized); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 215 - 227, The code currently uses expect(...) when building username from user_info in the OAuth callback (in the username assignment), which can panic; instead check for a usable identifier and return a controlled error: replace the expect-based construction with a safe match or if-let that attempts user_info.name.clone().or_else(|| user_info.email.clone()).or_else(|| user_info.sub.clone()) and if that yields None log an error (tracing::error!) and return Err(OIDCError::Unauthorized) (similar to how user_id is handled), ensuring no panic occurs in the handler.
333-342:⚠️ Potential issue | 🟠 MajorAdd
Cache-Control: no-storeand remove session ID from JSON response.Two issues in the XHR response path:
- Missing
Cache-Control: no-storeheader—the redirect path sets it (line 409), but this path does not. Auth responses should be consistently non-cacheable.- Returning the session key in the JSON body (line 339) expands credential leakage surface (logs, browser tooling, client-side scripts). The session is already set via HttpOnly cookie.
,🔒 Proposed fix
if is_xhr { let mut response = HttpResponse::Ok(); + response.insert_header((actix_web::http::header::CACHE_CONTROL, "no-store")); for cookie in cookies { response.cookie(cookie); } Ok(response.json(serde_json::json!({ - "session": id.to_string(), "username": username, "user_id": user_id, })))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 333 - 342, The XHR branch currently builds an HttpResponse::Ok, sets cookies, and returns JSON including the session id; update this to set the "Cache-Control: no-store" header on the response and remove the "session" field from the JSON body (keep "username" and "user_id"), ensuring you still call response.cookie(...) for each cookie before returning; locate the XHR branch using is_xhr, the HttpResponse::Ok() construction, response.cookie(...), and the .json(...) call to apply these changes.
86-94:⚠️ Potential issue | 🔴 CriticalCSRF vulnerability: OAuth state contains unvalidated redirect URL.
The redirect URL is passed directly as the OAuth
stateparameter without server-side nonce validation. This enables OAuth login CSRF/session-swapping attacks. The callback trustsstateas redirect input without verification.Consider generating a server-side random nonce, storing it with the intended redirect in a short-lived session or signed cookie, and validating the nonce upon callback before using the redirect.
,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 86 - 94, The handler currently passes query.into_inner().redirect directly as the OAuth state (via client.read().await.auth_url(...)), enabling CSRF; instead generate a cryptographically secure random nonce, store a mapping nonce→redirect server-side (short-lived session entry or a signed/encrypted cookie) and pass only that nonce to auth_url as the state; on the OAuth callback handler (the endpoint that consumes state) validate the incoming state nonce against your stored mapping, retrieve the intended redirect, delete/expire the nonce mapping, and only then perform the redirect. Ensure the nonce is unpredictable, has a short TTL, and that you replace the use of query.into_inner().redirect as the state in the code path that constructs auth_url and in the callback verification logic.
🧹 Nitpick comments (1)
src/handlers/http/oidc.rs (1)
353-376: Consider takingtenant_idby reference.The function takes
tenant_id: Option<String>by value, butUsers.get_userlikely only needs a reference. Taking by reference (tenant_id: &Option<String>) would avoid the clone needed at the call site (line 257).♻️ Proposed refactor
-fn find_existing_user(user_info: &user::UserInfo, tenant_id: Option<String>) -> Option<User> { +fn find_existing_user(user_info: &user::UserInfo, tenant_id: &Option<String>) -> Option<User> { if let Some(sub) = &user_info.sub - && let Some(user) = Users.get_user(sub, &tenant_id) + && let Some(user) = Users.get_user(sub, tenant_id) && matches!(user.ty, UserType::OAuth(_)) { return Some(user); } if let Some(name) = &user_info.name - && let Some(user) = Users.get_user(name, &tenant_id) + && let Some(user) = Users.get_user(name, tenant_id) && matches!(user.ty, UserType::OAuth(_)) { return Some(user); } if let Some(email) = &user_info.email - && let Some(user) = Users.get_user(email, &tenant_id) + && let Some(user) = Users.get_user(email, tenant_id) && matches!(user.ty, UserType::OAuth(_)) { return Some(user); } None }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 353 - 376, The function find_existing_user currently takes tenant_id: Option<String> by value causing callers to clone; change its signature to take tenant_id: &Option<String> and update all internal calls to Users.get_user to pass the reference (e.g., Users.get_user(sub, tenant_id)), ensuring pattern matches still work with &Option<String>; update any call sites (such as where find_existing_user is invoked) to pass a reference instead of moving the Option so no unnecessary clones are performed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/http/oidc.rs`:
- Around line 422-429: The session cookie builder in cookie_session (and
similarly cookie_username and cookie_userid) must set HttpOnly and only set
secure when the app is running over HTTPS; add .http_only(true) to each cookie
builder and conditionally call .secure(true) based on the same scheme check used
elsewhere (e.g., PARSEABLE.options.get_scheme() == "https" or get_scheme() ==
"https") so the builder applies secure(true) only when HTTPS is enabled, then
finish the builder with .path("/") and .finish().
- Around line 297-316: The current flow grants Admin fallback when
perms.is_empty() without verifying tenant ownership and uses the client-supplied
tenant header inconsistently; change the logic in the reply_login handling where
rbac::roles_to_permission(...) is evaluated: ignore the incoming tenant_id
header for unauthenticated OIDC callbacks (derive tenant from OAuth claims or
set user.tenant = None), verify the user is actually the tenant owner (e.g.,
check Users.is_first_user_for_tenant(user.userid()) or an explicit owner flag)
before constructing admin_perms via RoleBuilder::from(...).build(), and only
call rbac::map::mut_sessions().track_new(...) with admin_perms if that ownership
check passes; add an audit log entry when the admin fallback branch is taken and
ensure Users.new_session(...) and the stored user.tenant are set consistently
from the validated tenant source rather than the client header.
---
Outside diff comments:
In `@src/handlers/http/oidc.rs`:
- Line 257: The call to find_existing_user(&user_info, tenant_id) moves
tenant_id, causing a later use to fail; change find_existing_user to accept a
reference to tenant_id (e.g., &TenantId or &str depending on type) and update
its signature and all call sites accordingly, or alternatively pass a cloned
value (tenant_id.clone()) here to avoid moving; update the function signature of
find_existing_user and its implementations/usages so tenant_id is borrowed
instead of moved.
---
Duplicate comments:
In `@src/handlers/http/oidc.rs`:
- Around line 215-227: The code currently uses expect(...) when building
username from user_info in the OAuth callback (in the username assignment),
which can panic; instead check for a usable identifier and return a controlled
error: replace the expect-based construction with a safe match or if-let that
attempts user_info.name.clone().or_else(|| user_info.email.clone()).or_else(||
user_info.sub.clone()) and if that yields None log an error (tracing::error!)
and return Err(OIDCError::Unauthorized) (similar to how user_id is handled),
ensuring no panic occurs in the handler.
- Around line 333-342: The XHR branch currently builds an HttpResponse::Ok, sets
cookies, and returns JSON including the session id; update this to set the
"Cache-Control: no-store" header on the response and remove the "session" field
from the JSON body (keep "username" and "user_id"), ensuring you still call
response.cookie(...) for each cookie before returning; locate the XHR branch
using is_xhr, the HttpResponse::Ok() construction, response.cookie(...), and the
.json(...) call to apply these changes.
- Around line 86-94: The handler currently passes query.into_inner().redirect
directly as the OAuth state (via client.read().await.auth_url(...)), enabling
CSRF; instead generate a cryptographically secure random nonce, store a mapping
nonce→redirect server-side (short-lived session entry or a signed/encrypted
cookie) and pass only that nonce to auth_url as the state; on the OAuth callback
handler (the endpoint that consumes state) validate the incoming state nonce
against your stored mapping, retrieve the intended redirect, delete/expire the
nonce mapping, and only then perform the redirect. Ensure the nonce is
unpredictable, has a short TTL, and that you replace the use of
query.into_inner().redirect as the state in the code path that constructs
auth_url and in the callback verification logic.
---
Nitpick comments:
In `@src/handlers/http/oidc.rs`:
- Around line 353-376: The function find_existing_user currently takes
tenant_id: Option<String> by value causing callers to clone; change its
signature to take tenant_id: &Option<String> and update all internal calls to
Users.get_user to pass the reference (e.g., Users.get_user(sub, tenant_id)),
ensuring pattern matches still work with &Option<String>; update any call sites
(such as where find_existing_user is invoked) to pass a reference instead of
moving the Option so no unnecessary clones are performed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 14999131-8ed9-486c-992e-51f805bae818
📒 Files selected for processing (1)
src/handlers/http/oidc.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
src/handlers/http/oidc.rs (6)
333-338:⚠️ Potential issue | 🟡 MinorMark XHR login success responses as non-cacheable.
The 200-with-cookies branch should also include
Cache-Control: no-store(same policy as redirect responses) to avoid caching auth responses.🧩 Minimal fix
if is_xhr { let mut response = HttpResponse::Ok(); + response.insert_header((actix_web::http::header::CACHE_CONTROL, "no-store")); for cookie in cookies { response.cookie(cookie); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 333 - 338, The XHR success branch currently builds an HttpResponse::Ok(), attaches cookies, and returns JSON but doesn't set the Cache-Control header; update the is_xhr branch (where response is created with HttpResponse::Ok() and cookies are iterated) to add header "Cache-Control: no-store" to the response before calling response.json(...) so the 200-with-cookies path uses the same non-cacheable policy as the redirect responses.
338-342:⚠️ Potential issue | 🟠 MajorRemove raw session identifier from JSON response body.
Line 339 exposes the session key in response JSON even though it is already set via cookie, increasing leakage surface (logs/devtools/client scripts).
🔒 Safer response payload
Ok(response.json(serde_json::json!({ - "session": id.to_string(), "username": username, "user_id": user_id, })))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 338 - 342, The response currently includes the raw session identifier (id.to_string()) in the JSON body produced by response.json, which leaks the session token; remove the "session" field from the JSON payload and only return non-sensitive fields (e.g., username and user_id) while leaving the session cookie handling unchanged; update the code that builds the JSON response (the response.json call that references id, username, user_id) to omit id/session so the session is not echoed in the body.
422-429:⚠️ Potential issue | 🔴 CriticalSet
HttpOnlyand makeSecureconditional on HTTPS for auth cookies.These cookies currently miss
.http_only(true)and unconditionally set.secure(true).HttpOnlyshould be explicit, andSecureshould depend on HTTPS deployment mode.🍪 Hardened cookie builder pattern
pub fn cookie_session(id: Ulid) -> Cookie<'static> { - Cookie::build(SESSION_COOKIE_NAME, id.to_string()) + let is_https = PARSEABLE.options.get_scheme() == "https"; + let mut builder = Cookie::build(SESSION_COOKIE_NAME, id.to_string()) .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64)) .same_site(SameSite::None) - .secure(true) - .path("/") - .finish() + .http_only(true) + .path("/"); + if is_https { + builder = builder.secure(true); + } + builder.finish() }Apply the same pattern to
cookie_usernameandcookie_userid.#!/bin/bash # Read-only verification: check cookie attributes and scheme gating in oidc handler rg -n -C3 'fn cookie_(session|username|userid)|\.http_only\(|\.secure\(|get_scheme\(' src/handlers/http/oidc.rsBased on learnings:
.secure(true)cookies are rejected in HTTP contexts and should be conditional/configurable by deployment scheme.Also applies to: 431-438, 440-447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 422 - 429, The three cookie helpers (cookie_session, cookie_username, cookie_userid) must set HttpOnly explicitly and only set Secure when running under HTTPS: add .http_only(true) to each Cookie::build chain and gate .secure(true) behind a scheme check (use the existing scheme detection/get_scheme or a passed-in is_https flag) so cookies use .secure(true) only when scheme == "https" (otherwise omit secure or set false); ensure SameSite, path, and max_age keep existing values and apply the same pattern across all three helper functions.
86-94:⚠️ Potential issue | 🔴 CriticalReplace redirect-in-
statewith server-validated state tokens.Line 89 and Line 146 pass the redirect URL as OAuth
state, and Line 344-347 later trustsstateas the redirect target. This enables login CSRF/session-swapping and unsafe redirect control.🔐 Suggested fix pattern
- let redirect = query.into_inner().redirect.to_string(); - let mut auth_url: String = client.read().await.auth_url(&scope, Some(redirect)).into(); + let redirect = query.into_inner().redirect.to_string(); + let state = Ulid::new().to_string(); + persist_oauth_state_mapping(&req, &state, &redirect)?; + let mut auth_url: String = client.read().await.auth_url(&scope, Some(state)).into();- let redirect_url = login_query - .state - .clone() - .unwrap_or_else(|| PARSEABLE.options.address.to_string()); + let redirect_url = consume_validated_redirect(&req, login_query.state.as_deref()) + .ok_or(OIDCError::Unauthorized)?;Also applies to: 141-151, 344-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 86 - 94, The code currently embeds the user-supplied redirect URL into the OAuth `state` (see redirect variable and the call client.read().await.auth_url(&scope, Some(redirect))) which is later trusted; replace this with a server-generated, cryptographically random state token (store it server-side in session/DB or in a signed/encrypted cookie) and include only that token in auth_url; when handling the callback (the state-consuming logic that reads `state` around earlier lines ~344), validate the returned token against the stored value and then look up the associated, server-validated redirect target (do not accept an arbitrary redirect from the OAuth `state`); update both the auth request generation (auth_url construction) and the callback handler to create/store/validate the token and to fetch the redirect target from the server-side mapping.
220-220:⚠️ Potential issue | 🟠 MajorAvoid
expect(...)in OAuth callback identity extraction.Line 220 can panic the request path when provider identity fields are incomplete; return
OIDCError::Unauthorizedinstead of crashing the worker.🛠️ Safe fallback
- let username = user_info - .name - .clone() - .or_else(|| user_info.email.clone()) - .or_else(|| user_info.sub.clone()) - .expect("OAuth provider did not return a usable identifier (name, email or sub)"); + let Some(username) = user_info + .name + .clone() + .or_else(|| user_info.email.clone()) + .or_else(|| user_info.sub.clone()) + else { + tracing::error!("OAuth provider did not return a usable identifier (name, email or sub)"); + return Err(OIDCError::Unauthorized); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` at line 220, The code currently calls .expect("OAuth provider did not return a usable identifier (name, email or sub)"), which can panic on incomplete provider data; change the identity extraction in the OIDC callback handler (the expression that computes the usable identifier / user id) to return an Err(OIDCError::Unauthorized) instead of panicking when name, email, and sub are all missing — replace the .expect call with a safe match/if-let that maps the missing-identifier case to Err(OIDCError::Unauthorized) and otherwise continues with the found identifier.
196-197:⚠️ Potential issue | 🔴 CriticalDo not trust request tenant context in callback; gate admin fallback by verified ownership.
Line 196 derives tenant context from request input on an unauthenticated callback, while Line 304-313 grants admin permissions whenever role resolution is empty. This is a privilege-escalation path unless tenant ownership is explicitly verified before fallback.
Based on learnings: for non-ingest HTTP paths, tenant must not come from client-controlled headers and must be derived from trusted authenticated context to prevent tenant spoofing.
Also applies to: 293-293, 297-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 196 - 197, The callback handler currently uses get_tenant_id_from_request(&req) to set tenant context on an unauthenticated callback and later performs an admin fallback when role resolution is empty (the empty-role admin grant around the role-resolution block). Replace use of client-controlled request headers for tenant selection in the unauthenticated OIDC callback: derive tenant only from a trusted authenticated context (e.g., verified session/account owner) or explicitly verify tenant ownership (via a lookup that confirms the authenticated user's ownership of that tenant) before applying any admin fallback. Remove or gate the "empty role => grant admin" fallback so it only executes after explicit ownership verification; change logic around the role-resolution empty check to require verified ownership or authenticated tenant resolution instead of trusting get_tenant_id_from_request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/handlers/http/oidc.rs`:
- Around line 333-338: The XHR success branch currently builds an
HttpResponse::Ok(), attaches cookies, and returns JSON but doesn't set the
Cache-Control header; update the is_xhr branch (where response is created with
HttpResponse::Ok() and cookies are iterated) to add header "Cache-Control:
no-store" to the response before calling response.json(...) so the
200-with-cookies path uses the same non-cacheable policy as the redirect
responses.
- Around line 338-342: The response currently includes the raw session
identifier (id.to_string()) in the JSON body produced by response.json, which
leaks the session token; remove the "session" field from the JSON payload and
only return non-sensitive fields (e.g., username and user_id) while leaving the
session cookie handling unchanged; update the code that builds the JSON response
(the response.json call that references id, username, user_id) to omit
id/session so the session is not echoed in the body.
- Around line 422-429: The three cookie helpers (cookie_session,
cookie_username, cookie_userid) must set HttpOnly explicitly and only set Secure
when running under HTTPS: add .http_only(true) to each Cookie::build chain and
gate .secure(true) behind a scheme check (use the existing scheme
detection/get_scheme or a passed-in is_https flag) so cookies use .secure(true)
only when scheme == "https" (otherwise omit secure or set false); ensure
SameSite, path, and max_age keep existing values and apply the same pattern
across all three helper functions.
- Around line 86-94: The code currently embeds the user-supplied redirect URL
into the OAuth `state` (see redirect variable and the call
client.read().await.auth_url(&scope, Some(redirect))) which is later trusted;
replace this with a server-generated, cryptographically random state token
(store it server-side in session/DB or in a signed/encrypted cookie) and include
only that token in auth_url; when handling the callback (the state-consuming
logic that reads `state` around earlier lines ~344), validate the returned token
against the stored value and then look up the associated, server-validated
redirect target (do not accept an arbitrary redirect from the OAuth `state`);
update both the auth request generation (auth_url construction) and the callback
handler to create/store/validate the token and to fetch the redirect target from
the server-side mapping.
- Line 220: The code currently calls .expect("OAuth provider did not return a
usable identifier (name, email or sub)"), which can panic on incomplete provider
data; change the identity extraction in the OIDC callback handler (the
expression that computes the usable identifier / user id) to return an
Err(OIDCError::Unauthorized) instead of panicking when name, email, and sub are
all missing — replace the .expect call with a safe match/if-let that maps the
missing-identifier case to Err(OIDCError::Unauthorized) and otherwise continues
with the found identifier.
- Around line 196-197: The callback handler currently uses
get_tenant_id_from_request(&req) to set tenant context on an unauthenticated
callback and later performs an admin fallback when role resolution is empty (the
empty-role admin grant around the role-resolution block). Replace use of
client-controlled request headers for tenant selection in the unauthenticated
OIDC callback: derive tenant only from a trusted authenticated context (e.g.,
verified session/account owner) or explicitly verify tenant ownership (via a
lookup that confirms the authenticated user's ownership of that tenant) before
applying any admin fallback. Remove or gate the "empty role => grant admin"
fallback so it only executes after explicit ownership verification; change logic
around the role-resolution empty check to require verified ownership or
authenticated tenant resolution instead of trusting get_tenant_id_from_request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2e380d11-306f-4ff2-9c58-417a90c18634
📒 Files selected for processing (1)
src/handlers/http/oidc.rs
nikhilsinhaparseable
force-pushed
the
oauth-update
branch
2 times, most recently
from
b7ba737 to
277316d
Compare
nikhilsinhaparseable
force-pushed
the
oauth-update
branch
from
c30b69a to
95f718e
Compare
nikhilsinhaparseable
force-pushed
the
oauth-update
branch
from
5773ce4 to
e2dc5d7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/handlers/http/middleware.rs (1)
568-584:⚠️ Potential issue | 🔴 CriticalFail closed when the intra-cluster caller cannot be resolved.
If the cluster secret is valid but
Users.get_user(userid, &tenant_id)misses, this branch keeps the originalAuthorizationheader and continues without an error.src/storage/field_stats.rsnow also sends an optionalAuthorizationheader alongside cluster headers, so the target can silently fall back to that credential instead of the intendedintra-cluster-userid. StripAuthorizationbefore the lookup and returnErrorUnauthorizedwhen the user does not exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/middleware.rs` around lines 568 - 584, Before calling Users.get_user(userid, &tenant_id) remove the Authorization header from the incoming request and, if the lookup returns None, fail closed by returning an ErrorUnauthorized instead of continuing; specifically, call req.headers_mut().remove(header::AUTHORIZATION) prior to the get_user call, and if get_user returns None, return an appropriate unauthorized error (do not leave the header present to be used later), otherwise proceed to create Ulid::new(), insert session into req.extensions_mut() as SessionKey::SessionId and call Users.new_session(&user, session, TimeDelta::seconds(20)).src/storage/field_stats.rs (1)
569-607:⚠️ Potential issue | 🟠 MajorDon't panic while building Prism auth headers.
Line 578 and Line 596 unwrap
get_user_from_request, and Line 589 unwraps the resulting header encoding. Any stale/missing caller context on this path turns a normal request failure into a 500. Extract the caller once and map failures intoQueryErrorinstead.Suggested fix
- let auth = if let Some((_, hash)) = CLUSTER_SECRET.get() { + let caller_userid = get_user_from_request(&req) + .map_err(|e| QueryError::CustomError(e.to_string()))?; + let auth = if let Some((_, hash)) = CLUSTER_SECRET.get() { let mut map = actix_web::http::header::HeaderMap::new(); if let Some(header) = TENANT_METADATA.get_global_query_auth(tenant) { map.insert( HeaderName::from_static("authorization"), - HeaderValue::from_str(&header).unwrap(), + HeaderValue::from_str(&header) + .map_err(|e| QueryError::CustomError(e.to_string()))?, ); } - let userid = get_user_from_request(&req).unwrap(); map.insert( HeaderName::from_static(CLUSTER_SECRET_HEADER), HeaderValue::from_str(hash).unwrap(), ); map.insert( HeaderName::from_static("intra-cluster-tenant"), HeaderValue::from_str(tenant).unwrap(), ); map.insert( HeaderName::from_static("intra-cluster-userid"), - HeaderValue::from_str(&userid).unwrap(), + HeaderValue::from_str(&caller_userid) + .map_err(|e| QueryError::CustomError(e.to_string()))?, ); Some(map) } else { let auth = create_intracluster_auth_headermap( req.headers(), "", - &get_user_from_request(&req).unwrap(), + &caller_userid, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/field_stats.rs` around lines 569 - 607, The code panics by unwrapping get_user_from_request and HeaderValue conversions; instead call get_user_from_request(&req) once into a Result (or Option) at the top of this block and propagate errors into the QueryError path rather than unwrap, and replace HeaderValue::from_str / ::from_bytes unwraps with fallible conversions (map_err to QueryError) so header creation fails gracefully; update both branches (the CLUSTER_SECRET.get() branch and the create_intracluster_auth_headermap branch) to use the extracted caller result and return/propagate a QueryError when user extraction or header value parsing fails, referencing get_user_from_request, create_intracluster_auth_headermap, HeaderValue::from_str, and HeaderValue::from_bytes to locate the changes.
♻️ Duplicate comments (2)
src/handlers/http/oidc.rs (2)
436-450:⚠️ Potential issue | 🔴 CriticalMissing
http_only(true)on session cookies.The
build_cookiehelper doesn't set theHttpOnlyattribute. Without this, client-side JavaScript can access session cookies, significantly increasing XSS attack surface. A previous review explicitly requested adding.http_only(true), and the cookie crate does NOT set this by default.🔒 Proposed fix
fn build_cookie(name: &str, value: String) -> Cookie<'static> { let is_https = PARSEABLE.options.get_scheme() == "https"; let mut cookie = Cookie::build(name.to_string(), value) .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64)) - .path("/".to_string()); + .path("/".to_string()) + .http_only(true); if is_https {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 436 - 450, The build_cookie helper (function build_cookie) currently omits the HttpOnly flag; update the Cookie::build chain in build_cookie to call .http_only(true) (for both the HTTPS and non-HTTPS branches) before finishing the cookie so session cookies are marked HttpOnly and not accessible to client-side JavaScript; ensure the .http_only(true) call is applied alongside .same_site(...) and .secure(...) before cookie.finish().
191-206:⚠️ Potential issue | 🟠 MajorTenant header accepted in unauthenticated OIDC callback.
The
reply_loginhandler extractstenant_idfrom the request header (line 192) before any authentication occurs. Per retrieved learnings, client-supplied tenant headers should be ignored or validated only after authentication to prevent tenant spoofing. Currently, thistenant_idis used for user lookup and creation, though the user is ultimately created withtenant: None(line 494-495 input_user), creating an inconsistency.Consider either:
- Ignoring the tenant header entirely in this unauthenticated callback, or
- Deriving tenant from OAuth claims if multi-tenancy via OAuth is required.
Based on learnings: "For non-ingest actions, derive the tenant from the authenticated session and overwrite/inject the tenant header to prevent client-side spoofing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 191 - 206, In reply_login, stop reading client-supplied tenant via get_tenant_id_from_request before authentication; instead derive tenant from the authenticated session/claims after extract_identity (or explicitly set tenant = None) and use that value when calling find_existing_user and when creating/updating via put_user to ensure consistency and prevent client spoofing; specifically, remove or defer the call to get_tenant_id_from_request in reply_login, obtain tenant_id from the OIDC session/claims returned by exchange_code/extract_identity, and pass that derived tenant_id into find_existing_user and put_user (or pass None if no tenant claim exists).
🧹 Nitpick comments (2)
src/rbac/user.rs (2)
501-551: Same tenant_id consistency observation applies to remove_roles and remove_users.Similar to
add_users, these methods derivetenant_idfromgroup_user.tenant_idrather than accepting it as a parameter likeadd_rolesdoes. For consistency and to reinforce the single-tenant invariant, consider aligning these signatures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rbac/user.rs` around lines 501 - 551, Both remove_roles and remove_users derive tenant_id from each GroupUser (like add_users currently does) which breaks the single-tenant invariant that add_roles enforces by accepting a tenant_id parameter; change both function signatures (remove_roles and remove_users) to accept an explicit tenant_id parameter (same type/semantics as add_roles), update internal logic to use that tenant_id (instead of group_user.tenant_id) when computing tid, looking up users (users()/super::map::users()), and when calling sessions.refresh_user_permissions, and update all call sites accordingly so tenant resolution is consistent with add_roles and preserves single-tenant behavior.
468-497: Consider using consistent tenant_id source across group methods.In
add_roles, thetenant_idparameter is used for permission refresh, but inadd_users,group_user.tenant_idis used instead. If the tenant-scoped UserGroup invariant holds (users in a group belong to the same tenant), this works correctly, but using the method'stenant_idparameter consistently would be clearer.♻️ Suggested consistency improvement for add_users
- pub fn add_users(&mut self, users: HashSet<GroupUser>) -> Result<(), RBACError> { + pub fn add_users(&mut self, users: HashSet<GroupUser>, tenant_id: &str) -> Result<(), RBACError> { if users.is_empty() { return Ok(()); } self.users.extend(users.clone()); // refresh permissions for newly added user sessions let mut sessions = mut_sessions(); let all_users = super::map::users(); for group_user in &users { - let tid = group_user.tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); - if let Some(tenant_users) = all_users.get(tid) + if let Some(tenant_users) = all_users.get(tenant_id) && let Some(user) = tenant_users.get(group_user.userid()) { - let new_perms = roles_to_permission(user.roles(), tid); - sessions.refresh_user_permissions(group_user.userid(), tid, &new_perms); + let new_perms = roles_to_permission(user.roles(), tenant_id); + sessions.refresh_user_permissions(group_user.userid(), tenant_id, &new_perms); } } Ok(()) }Based on learnings: "UserGroup must be restricted to a single tenant; do not rely on cross-tenant membership assumptions in group APIs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rbac/user.rs` around lines 468 - 497, The refresh-permissions path in add_users uses group_user.tenant_id while add_roles uses the method parameter tenant_id, causing inconsistent tenant sourcing; update add_users to derive the tenant id the same way add_roles does (i.e., use the group's tenant context rather than each GroupUser's tenant) — replace the tid calculation in add_users with the group's tenant id source (e.g., self.tenant_id.as_deref().unwrap_or(DEFAULT_TENANT) or the same tenant_id parameter approach used by add_roles) so sessions.refresh_user_permissions is called with a consistent tenant identifier across group methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/http/modal/query/querier_rbac.rs`:
- Around line 225-241: The current flow calls
sync_users_with_roles_with_ingestors(...) and logs errors but still proceeds to
update the durable/in-memory state via Users.add_roles(...), causing callers to
receive success even if the cluster fanout failed; change the flow to perform
the cluster fanout before persisting the change (i.e., call
sync_users_with_roles_with_ingestors(...) first and return an error to the
caller if it fails), or implement a durable retry/compensation record persisted
before/alongside Users.add_roles so you can retry failed fanouts; apply the same
fix to the analogous removal/other-role paths referenced around the 330-345
region (the functions that call sync_users_with_roles_with_ingestors and
Users.add_roles/remove roles).
In `@src/handlers/http/modal/query/querier_role.rs`:
- Around line 119-121: The current flow logs and continues on
sync_role_update/sync_role_delete failures after already mutating metadata/local
cache, causing divergent role state across nodes; change this by performing the
cluster sync (call sync_role_update or sync_role_delete) before committing local
state, and if the sync call returns Err, return an error response (surface
partial failure) instead of logging and returning 200; alternatively, implement
persisting a retry/outbox (persist the pending sync entry and return a non-200)
and ensure the local mutation only proceeds once sync is acknowledged—apply the
same change for both sync_role_update and sync_role_delete call sites in
querier_role.rs.
- Around line 107-116: The loop rebuilds permissions using only user.roles(),
which misses roles inherited via groups discovered by read_user_groups(); update
the loop over session_refresh_users (and the code that calls mut_sessions(),
users(), tenant, userid) to compute the effective role set by merging the user's
direct roles with any roles from groups the user belongs to (i.e., include group
roles discovered earlier) before calling roles_to_permission, then pass that
merged role set into sessions.refresh_user_permissions instead of user.roles()
so group-only members get correct permissions.
In `@src/handlers/http/oidc.rs`:
- Around line 308-318: The current role inheritance uses the OAuth userinfo
email (user_info.email) to clone roles from a native account (iterating
metadata.users and matching UserType::Native) without checking email
verification, which can enable privilege escalation; update the logic in the
block that references roles, user_info.email, metadata.users and
UserType::Native to require a verified email claim (check
user_info.email_verified == true or equivalent from the IdP) before copying
native.roles, or gate the feature behind an explicit configuration flag (e.g.,
allow_email_role_inheritance) so deployments can opt-in; alternatively, add a
forced secondary verification path before inheriting roles and ensure the
decision point and flag are clearly named and used where
roles.clone_from(&native.roles) is invoked.
---
Outside diff comments:
In `@src/handlers/http/middleware.rs`:
- Around line 568-584: Before calling Users.get_user(userid, &tenant_id) remove
the Authorization header from the incoming request and, if the lookup returns
None, fail closed by returning an ErrorUnauthorized instead of continuing;
specifically, call req.headers_mut().remove(header::AUTHORIZATION) prior to the
get_user call, and if get_user returns None, return an appropriate unauthorized
error (do not leave the header present to be used later), otherwise proceed to
create Ulid::new(), insert session into req.extensions_mut() as
SessionKey::SessionId and call Users.new_session(&user, session,
TimeDelta::seconds(20)).
In `@src/storage/field_stats.rs`:
- Around line 569-607: The code panics by unwrapping get_user_from_request and
HeaderValue conversions; instead call get_user_from_request(&req) once into a
Result (or Option) at the top of this block and propagate errors into the
QueryError path rather than unwrap, and replace HeaderValue::from_str /
::from_bytes unwraps with fallible conversions (map_err to QueryError) so header
creation fails gracefully; update both branches (the CLUSTER_SECRET.get() branch
and the create_intracluster_auth_headermap branch) to use the extracted caller
result and return/propagate a QueryError when user extraction or header value
parsing fails, referencing get_user_from_request,
create_intracluster_auth_headermap, HeaderValue::from_str, and
HeaderValue::from_bytes to locate the changes.
---
Duplicate comments:
In `@src/handlers/http/oidc.rs`:
- Around line 436-450: The build_cookie helper (function build_cookie) currently
omits the HttpOnly flag; update the Cookie::build chain in build_cookie to call
.http_only(true) (for both the HTTPS and non-HTTPS branches) before finishing
the cookie so session cookies are marked HttpOnly and not accessible to
client-side JavaScript; ensure the .http_only(true) call is applied alongside
.same_site(...) and .secure(...) before cookie.finish().
- Around line 191-206: In reply_login, stop reading client-supplied tenant via
get_tenant_id_from_request before authentication; instead derive tenant from the
authenticated session/claims after extract_identity (or explicitly set tenant =
None) and use that value when calling find_existing_user and when
creating/updating via put_user to ensure consistency and prevent client
spoofing; specifically, remove or defer the call to get_tenant_id_from_request
in reply_login, obtain tenant_id from the OIDC session/claims returned by
exchange_code/extract_identity, and pass that derived tenant_id into
find_existing_user and put_user (or pass None if no tenant claim exists).
---
Nitpick comments:
In `@src/rbac/user.rs`:
- Around line 501-551: Both remove_roles and remove_users derive tenant_id from
each GroupUser (like add_users currently does) which breaks the single-tenant
invariant that add_roles enforces by accepting a tenant_id parameter; change
both function signatures (remove_roles and remove_users) to accept an explicit
tenant_id parameter (same type/semantics as add_roles), update internal logic to
use that tenant_id (instead of group_user.tenant_id) when computing tid, looking
up users (users()/super::map::users()), and when calling
sessions.refresh_user_permissions, and update all call sites accordingly so
tenant resolution is consistent with add_roles and preserves single-tenant
behavior.
- Around line 468-497: The refresh-permissions path in add_users uses
group_user.tenant_id while add_roles uses the method parameter tenant_id,
causing inconsistent tenant sourcing; update add_users to derive the tenant id
the same way add_roles does (i.e., use the group's tenant context rather than
each GroupUser's tenant) — replace the tid calculation in add_users with the
group's tenant id source (e.g.,
self.tenant_id.as_deref().unwrap_or(DEFAULT_TENANT) or the same tenant_id
parameter approach used by add_roles) so sessions.refresh_user_permissions is
called with a consistent tenant identifier across group methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 12e622a7-a760-45b8-bb60-5037a0319880
📒 Files selected for processing (19)
src/handlers/http/cluster/mod.rssrc/handlers/http/middleware.rssrc/handlers/http/modal/ingest/ingestor_role.rssrc/handlers/http/modal/mod.rssrc/handlers/http/modal/query/querier_rbac.rssrc/handlers/http/modal/query/querier_role.rssrc/handlers/http/modal/server.rssrc/handlers/http/oidc.rssrc/lib.rssrc/oauth/mod.rssrc/oauth/oidc_client.rssrc/oauth/provider.rssrc/parseable/mod.rssrc/rbac/map.rssrc/rbac/mod.rssrc/rbac/user.rssrc/storage/field_stats.rssrc/storage/store_metadata.rssrc/tenants/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib.rs
- src/oauth/mod.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
src/handlers/http/oidc.rs (5)
356-365:⚠️ Potential issue | 🟠 MajorDo not return a cacheable raw session token in the XHR success body.
This branch already sets the auth cookie, but it also serializes the same session id into JSON and no longer marks the 200 response as
no-store. That widens credential leakage via logs/devtools/scripts and makes the token-bearing response cacheable. Return only non-sensitive fields here and keep the response non-cacheable.Suggested change
if is_xhr { let mut response = HttpResponse::Ok(); + response.insert_header((actix_web::http::header::CACHE_CONTROL, "no-store")); for cookie in cookies { response.cookie(cookie); } response.json(serde_json::json!({ - "session": session_id.to_string(), "username": username, "user_id": user_id, }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 356 - 365, When handling the is_xhr branch in the OIDC handler, stop returning the raw session token in the JSON body (remove session_id from the serialized response) and ensure the HTTP response is marked non-cacheable (e.g., set Cache-Control: no-store and appropriate Pragma/Expires headers) while still setting the auth cookie(s) via response.cookie(...); return only non-sensitive fields such as username and user_id in the JSON to avoid credential leakage (referencing is_xhr, response, cookies, session_id, username, user_id).
200-215:⚠️ Potential issue | 🔴 CriticalDo not tenant-scope the public callback from
get_tenant_id_from_request().Line 201 runs before any authenticated session exists, so this
tenant_idcan only come from caller-controlled request data. Using it forget_metadata,find_existing_user, andput_userlets an OAuth login read or mutate whichever tenant RBAC metadata the caller names. Derive tenant from validated server-side OAuth state/claims instead, or keep itNoneuntil tenant context is established.Based on learnings: non-ingest handlers under
src/handlers/httpmust derive tenant from the authenticated session and overwrite/inject the tenant header to prevent client-side spoofing.Also applies to: 229-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 200 - 215, Do not call get_tenant_id_from_request() before authentication; the tenant value is attacker-controlled at that point. After exchange_code(&login_query.code) and extract_identity(&session) succeed, derive/validate the tenant from the authenticated session/claims (session/ user_info) and use that server-validated tenant when calling get_metadata, find_existing_user, and put_user (or leave tenant as None until validated). Replace early uses of get_tenant_id_from_request() in the login flow (around exchange_code, extract_identity, get_metadata, find_existing_user and the similar block at lines ~229-236) with the tenant derived from session/claims and ensure you overwrite/inject the tenant header for downstream calls so client-supplied tenant values cannot be used.
317-327:⚠️ Potential issue | 🟠 MajorGate native-role inheritance on a verified/trusted email signal.
This fallback clones roles from a native account purely by matching
user_info.email. If the IdP can return an unverified email, an OAuth account can inherit that native user's privileges. Require a verified-email claim or an explicit opt-in trust setting before copying roles fromUserType::Native.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 317 - 327, The current fallback that clones roles (roles.clone_from(&native.roles)) is gated only by user_info.email and can inherit privileges from unverified emails; update the conditional that finds a native user in metadata.users and the outer guard to require either a verified email claim (check user_info.email_verified or similar claim on the OIDC UserInfo) OR an explicit opt-in trust flag (e.g., a config or tenant-level flag like allow_unverified_email_role_inherit); only when email is present AND (email_verified == true OR the trust flag is enabled) should the code proceed to find UserType::Native entries and clone native.roles into roles.
95-103:⚠️ Potential issue | 🔴 CriticalStop using OAuth
stateas the post-login redirect.These branches serialize the client redirect into
state, and the callback later trusts that value as the redirect target. That reopens OAuth login CSRF/session-swapping and gives the callback an attacker-controlled redirect sink. Store{state -> redirect}server-side (or in an HttpOnly signed cookie), send only the nonce, and resolve/remove it inreply_login.Also applies to: 150-160, 367-372
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 95 - 103, The code currently serializes the client redirect into the OAuth state and later trusts it in reply_login (see the redirect variable from query.into_inner().redirect, the auth_url construction via client.read().await.auth_url(&scope, Some(redirect)), and the reply_login handler) which enables CSRF/session-swapping and open redirect sinks; instead, change the flow to generate a cryptographically random nonce as the state, persist a server-side mapping from that nonce -> redirect (or set an HttpOnly, signed cookie holding the mapping key), send only the nonce as the OAuth state in auth_url, and in reply_login look up and atomically remove the stored redirect by nonce (do not accept an unvalidated redirect from the OAuth state), so auth_url creation uses only the nonce and reply_login resolves the redirect from your server-side store.
445-456:⚠️ Potential issue | 🟠 Major
build_cookie()regresses the cookie security policy.The shared helper dropped
HttpOnly, socookie_session()becomes script-readable again, andSecurenow depends onCOOKIE_REQUIRE_CROSS_SITEinstead of whether the server is actually running over HTTPS. In a same-site HTTPS deployment, the session cookie will now be emitted withoutSecure. Split session vs display-cookie policy if needed, but keep the session cookieHttpOnlyand setSecurewheneverget_scheme() == "https".Suggested change
-fn build_cookie(name: &str, value: String) -> Cookie<'static> { +fn build_cookie(name: &str, value: String, http_only: bool) -> Cookie<'static> { + let is_https = PARSEABLE.options.get_scheme() == "https"; let mut cookie = Cookie::build(name.to_string(), value) .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64)) - .path("/".to_string()); + .path("/") + .http_only(http_only); if COOKIE_REQUIRE_CROSS_SITE.load(Ordering::Relaxed) { - cookie = cookie.same_site(SameSite::None).secure(true); + cookie = cookie.same_site(SameSite::None); } else { cookie = cookie.same_site(SameSite::Lax); } + if is_https { + cookie = cookie.secure(true); + } cookie.finish() } pub fn cookie_session(id: Ulid) -> Cookie<'static> { - build_cookie(SESSION_COOKIE_NAME, id.to_string()) + build_cookie(SESSION_COOKIE_NAME, id.to_string(), true) }Based on learnings: Secure cookies with
.secure(true)cannot be set in HTTP contexts as browsers will reject them; apply cookie security attributes based on HTTPS deployment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/http/oidc.rs` around lines 445 - 456, build_cookie() currently drops HttpOnly and sets Secure only based on COOKIE_REQUIRE_CROSS_SITE; update it so session cookies remain HttpOnly and Secure is set when the server scheme is HTTPS (get_scheme() == "https"), not merely when COOKIE_REQUIRE_CROSS_SITE is true. Concretely: either add a boolean parameter (e.g., is_session) to build_cookie or introduce build_session_cookie/cookie_session that calls a dedicated helper; always call .http_only(true) for session cookies, set .secure(true) when get_scheme() == "https", and still apply SameSite::None/.secure for cross-site requirements via COOKIE_REQUIRE_CROSS_SITE when appropriate. Ensure display/non-session cookies remain readable by scripts if intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/handlers/http/oidc.rs`:
- Around line 356-365: When handling the is_xhr branch in the OIDC handler, stop
returning the raw session token in the JSON body (remove session_id from the
serialized response) and ensure the HTTP response is marked non-cacheable (e.g.,
set Cache-Control: no-store and appropriate Pragma/Expires headers) while still
setting the auth cookie(s) via response.cookie(...); return only non-sensitive
fields such as username and user_id in the JSON to avoid credential leakage
(referencing is_xhr, response, cookies, session_id, username, user_id).
- Around line 200-215: Do not call get_tenant_id_from_request() before
authentication; the tenant value is attacker-controlled at that point. After
exchange_code(&login_query.code) and extract_identity(&session) succeed,
derive/validate the tenant from the authenticated session/claims (session/
user_info) and use that server-validated tenant when calling get_metadata,
find_existing_user, and put_user (or leave tenant as None until validated).
Replace early uses of get_tenant_id_from_request() in the login flow (around
exchange_code, extract_identity, get_metadata, find_existing_user and the
similar block at lines ~229-236) with the tenant derived from session/claims and
ensure you overwrite/inject the tenant header for downstream calls so
client-supplied tenant values cannot be used.
- Around line 317-327: The current fallback that clones roles
(roles.clone_from(&native.roles)) is gated only by user_info.email and can
inherit privileges from unverified emails; update the conditional that finds a
native user in metadata.users and the outer guard to require either a verified
email claim (check user_info.email_verified or similar claim on the OIDC
UserInfo) OR an explicit opt-in trust flag (e.g., a config or tenant-level flag
like allow_unverified_email_role_inherit); only when email is present AND
(email_verified == true OR the trust flag is enabled) should the code proceed to
find UserType::Native entries and clone native.roles into roles.
- Around line 95-103: The code currently serializes the client redirect into the
OAuth state and later trusts it in reply_login (see the redirect variable from
query.into_inner().redirect, the auth_url construction via
client.read().await.auth_url(&scope, Some(redirect)), and the reply_login
handler) which enables CSRF/session-swapping and open redirect sinks; instead,
change the flow to generate a cryptographically random nonce as the state,
persist a server-side mapping from that nonce -> redirect (or set an HttpOnly,
signed cookie holding the mapping key), send only the nonce as the OAuth state
in auth_url, and in reply_login look up and atomically remove the stored
redirect by nonce (do not accept an unvalidated redirect from the OAuth state),
so auth_url creation uses only the nonce and reply_login resolves the redirect
from your server-side store.
- Around line 445-456: build_cookie() currently drops HttpOnly and sets Secure
only based on COOKIE_REQUIRE_CROSS_SITE; update it so session cookies remain
HttpOnly and Secure is set when the server scheme is HTTPS (get_scheme() ==
"https"), not merely when COOKIE_REQUIRE_CROSS_SITE is true. Concretely: either
add a boolean parameter (e.g., is_session) to build_cookie or introduce
build_session_cookie/cookie_session that calls a dedicated helper; always call
.http_only(true) for session cookies, set .secure(true) when get_scheme() ==
"https", and still apply SameSite::None/.secure for cross-site requirements via
COOKIE_REQUIRE_CROSS_SITE when appropriate. Ensure display/non-session cookies
remain readable by scripts if intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bac3bd10-9bd7-403d-a4c7-dbacf3c13675
📒 Files selected for processing (1)
src/handlers/http/oidc.rs
nitisht
approved these changes
Mar 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit