feat: Updates for OAUTH implementation#1567
feat: Updates for OAUTH implementation#1567parmesant wants to merge 8 commits intoparseablehq:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplace concrete OpenID client usage with a pluggable Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as OAuthHandler
participant Provider as OAuthProvider
participant IDP as IdentityProvider
participant UserDB as User System
Client->>Server: GET /login or callback with code
Server->>Provider: auth_url(scope, state) / 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-->>Client: set cookies / redirect or 200 JSON (XHR)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
| let redirect = query.into_inner().redirect.to_string(); | ||
|
|
||
| let scope = PARSEABLE.options.scope.to_string(); | ||
| let mut auth_url: String = client.read().await.auth_url(&scope, Some(redirect)).into(); | ||
|
|
||
| auth_url.push_str("&access_type=offline&prompt=consent"); | ||
| return Ok(HttpResponse::TemporaryRedirect() | ||
| .insert_header((actix_web::http::header::LOCATION, auth_url)) | ||
| .finish()); |
There was a problem hiding this comment.
Replace redirect-in-state with CSRF-validated state tokens.
Line 89 and Line 146 set OAuth state to the redirect URL. The callback later trusts state as redirect input, but there is no server-issued nonce validation. That enables OAuth 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 = Ulid::new().to_string();
+ // Persist {state -> redirect} in short-lived server session or HttpOnly cookie.
+ 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(&req, login_query.state.as_deref())
+ .ok_or(OIDCError::Unauthorized)?;Also applies to: 141-151
🤖 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 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.
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
| if is_xhr { | ||
| let mut response = HttpResponse::Ok(); | ||
| for cookie in cookies { | ||
| response.cookie(cookie); | ||
| } | ||
| Ok(response.finish()) | ||
| } else { |
There was a problem hiding this comment.
Add Cache-Control: no-store to the XHR success response.
The redirect path sets no-store, but this 200-with-cookies path does not. Keep auth responses consistently non-cacheable.
🧩 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.finish())🤖 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 - 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().
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
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
src/handlers/http/oidc.rs
Outdated
| // Create session: try normal role resolution first. | ||
| // If roles resolve to empty permissions (e.g. tenant roles not in ROLES map), | ||
| // grant admin permissions so the tenant owner can access their workspace. | ||
| let perms = rbac::roles_to_permission( | ||
| user.roles(), | ||
| tenant_id.as_deref().unwrap_or(DEFAULT_TENANT), | ||
| ); | ||
| if perms.is_empty() { | ||
| use crate::rbac::role::{self, RoleBuilder}; | ||
| let admin_perms = RoleBuilder::from(&role::model::DefaultPrivilege::Admin).build(); | ||
| rbac::map::mut_sessions().track_new( | ||
| user.userid().to_owned(), | ||
| SessionKey::SessionId(id), | ||
| Utc::now() + expires_in, | ||
| admin_perms.into_iter().collect(), | ||
| &user.tenant, | ||
| ); | ||
| } else { | ||
| Users.new_session(&user, SessionKey::SessionId(id), expires_in); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the roles_to_permission function definition
rg -n "fn roles_to_permission" --type rust -A 20Repository: parseablehq/parseable
Length of output: 911
🏁 Script executed:
# Check for tenant owner identification and validation logic
rg -n "tenant.*owner\|owner.*tenant\|is_owner\|is_tenant_owner" --type rust -B 2 -A 5Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Examine the user structure to understand tenant scoping
rg -n "struct.*User\|trait.*User" src/handlers/http/oidc.rs -B 2 -A 10Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check how DEFAULT_TENANT is defined and used in this context
rg -n "DEFAULT_TENANT" --type rust -B 2 -A 2 | head -40Repository: parseablehq/parseable
Length of output: 2269
🏁 Script executed:
# Look for audit logging around session creation or admin privilege granting
rg -n "log\|trace\|debug\|info\|warn" src/handlers/http/oidc.rs -B 1 -A 1 | grep -A 3 -B 3 "session\|admin\|privilege"Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Examine the OIDC handler context around the code in question
sed -n '280,320p' src/handlers/http/oidc.rsRepository: parseablehq/parseable
Length of output: 1533
🏁 Script executed:
# Check the User struct definition to understand tenant scoping
rg -n "struct User\|impl User" src/ --type rust | head -20Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Look for tenant owner validation or tenant admin checks
rg -n "owner\|admin\|privilege.*grant\|tenant.*admin" src/handlers/http/oidc.rs -B 2 -A 2Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check how the tenant_id is derived in OIDC for this user
sed -n '200,320p' src/handlers/http/oidc.rs | head -80Repository: parseablehq/parseable
Length of output: 2684
🏁 Script executed:
# Look for any authorization/validation before the session creation block
rg -n "authorize\|validate\|check.*permission\|verify.*user" src/handlers/http/oidc.rs -B 2 -A 2Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check where tenant_id comes from in the OIDC handler - trace the function parameter
sed -n '140,180p' src/handlers/http/oidc.rsRepository: parseablehq/parseable
Length of output: 1668
🏁 Script executed:
# Look at the reply_login function signature to understand tenant_id source
rg -n "async fn reply_login\|fn reply_login" src/handlers/http/oidc.rs -A 10Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check if there's any tenant owner validation logic in the codebase
rg -n "tenant.*owner\|is_owner\|owner_id\|tenant_admin" src/rbac/ --type rust -B 3 -A 3Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Examine the User struct and tenant field
rg -n "pub struct User\|pub.*tenant" src/rbac/user.rs -B 2 -A 5 | head -50Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check if there's a concept of "first user" or "tenant owner" setup
rg -n "first.*user\|setup.*tenant\|owner.*init\|admin.*init" src/ --type rust -B 2 -A 2 | head -60Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Find the reply_login function definition
rg -n "fn reply_login\|async fn reply_login" src/handlers/http/oidc.rs -B 3 -A 15Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check how tenant_id is obtained in the OIDC flow - look at the full function
sed -n '1,150p' src/handlers/http/oidc.rsRepository: parseablehq/parseable
Length of output: 5752
🏁 Script executed:
# Check what tenant_id is passed to reply_login and where it comes from
rg -n "reply_login\|tenant_id" src/handlers/http/oidc.rs | head -40Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Look at the route definition to understand tenant_id parameter
rg -n "\.route\|\.service\|reply_login" src/handlers/http/mod.rs -B 2 -A 2 | grep -A 5 -B 5 "oidc\|login"Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check the user structure for tenant field
sed -n '1,100p' src/rbac/user.rsRepository: parseablehq/parseable
Length of output: 2943
🏁 Script executed:
# Look for where reply_login is called to understand tenant_id source
rg -n "reply_login" src/handlers/http/oidc.rs -B 5 -A 5Repository: parseablehq/parseable
Length of output: 777
🏁 Script executed:
# Check the full reply_login function signature and its tenant_id parameter
rg -n "async fn reply_login\|pub async fn reply_login" src/handlers/http/oidc.rs -A 30 | head -50Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Search for where the tenant_id variable is defined in the OIDC reply handler
sed -n '200,250p' src/handlers/http/oidc.rsRepository: parseablehq/parseable
Length of output: 1613
🏁 Script executed:
# Check if there's a route handler that calls reply_login
rg -n "\.route.*reply_login\|reply_login.*route" src/ --type rust -B 3 -A 3Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Look for tenant_id extraction/derivation in the OIDC handler
rg -n "tenant_id" src/handlers/http/oidc.rs | head -20Repository: parseablehq/parseable
Length of output: 1318
🏁 Script executed:
# Check the get_tenant_id_from_request function to see where it gets tenant_id from
rg -n "fn get_tenant_id_from_request" src/ --type rust -A 15Repository: parseablehq/parseable
Length of output: 839
🏁 Script executed:
# Look at the full reply_login function from beginning to end
sed -n '187,330p' src/handlers/http/oidc.rsRepository: parseablehq/parseable
Length of output: 4883
🏁 Script executed:
# Check if tenant_id comes from request headers or query params
rg -n "get_tenant_id_from_request\|extract.*tenant\|X-Tenant\|tenant.*header" src/utils/ --type rust -B 3 -A 8 | head -60Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Look at the middleware to understand how it handles tenant for unauthenticated requests
sed -n '1,100p' src/handlers/http/middleware.rsRepository: parseablehq/parseable
Length of output: 3132
🏁 Script executed:
# Check if OIDC endpoint requires authentication or is public
rg -n "reply_login\|Action::Login" src/rbac/ --type rust -B 3 -A 5Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check how TENANT_ID header is defined
rg -n "TENANT_ID\|X-P-Tenant\|tenant.*header" src/handlers/ --type rust -B 2 -A 2 | head -40Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Look at the middleware auth logic to see if it applies to the login endpoint
sed -n '100,300p' src/handlers/http/middleware.rs | head -120Repository: parseablehq/parseable
Length of output: 4656
🏁 Script executed:
# Check what routes use the reply_login handler
rg -n "reply_login\|\.post.*login" src/handlers/http/mod.rs -B 5 -A 5Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Verify if reply_login endpoint is protected or public
rg -n "reply_login\|/login\|\.route.*login" src/handlers/ --type rust -B 3 -A 3 | head -50Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check the put_user function to understand how tenant is handled
rg -n "fn put_user\|async fn put_user" src/handlers/http/oidc.rs -A 20Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Look at put_user function to see how it handles tenant parameter
sed -n '500,600p' src/handlers/http/oidc.rsRepository: parseablehq/parseable
Length of output: 3416
🏁 Script executed:
# Check the comment about "LET TENANT BE NONE FOR NOW"
rg -n "LET TENANT BE NONE\|tenant be None" src/handlers/http/oidc.rs -B 5 -A 5Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# See what user.tenant is used for and how None vs Some differs
rg -n "user\.tenant\|\.tenant" src/rbac/user.rs -B 2 -A 2 | head -60Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check the sessions tracking to see if tenant: None creates issues
rg -n "track_new\|mut_sessions" src/rbac/map.rs -B 3 -A 8 | head -80Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Look for any validation of tenant_id from request headers
rg -n "TENANT_ID\|tenant.*validate\|validate.*tenant" src/handlers/ --type rust -B 3 -A 5Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check how the login endpoint is configured - find the route definition
rg -n "reply_login\|/callback\|/oidc\|.post\(" src/handlers/http/mod.rs -B 3 -A 3 | grep -A 8 -B 8 "reply_login"Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check if reply_login is wrapped with Auth middleware
rg -n "\.authorize\|\.wrap\|Auth\|reply_login" src/handlers/http/mod.rs -B 5 -A 5 | head -60Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Look at the complete route setup for the OIDC endpoints
sed -n '1,200p' src/handlers/http/mod.rs | tail -100Repository: parseablehq/parseable
Length of output: 2863
🏁 Script executed:
# Verify what TENANT_ID constant is
rg -n "const TENANT_ID\|TENANT_ID\s*=" src/handlers/http/ --type rust -B 2 -A 2Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check if there's any validation that tenant_id from header matches the user's tenant
rg -n "tenant.*header\|header.*tenant\|x-p-tenant\|X-P-Tenant" src/handlers/http/ --type rust -i -B 3 -A 3Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check the routes configuration in mod.rs to see if reply_login has auth
sed -n '1,500p' src/handlers/http/mod.rs | tail -300Repository: parseablehq/parseable
Length of output: 4239
🏁 Script executed:
# Look for web::scope or similar route grouping
rg -n "\.service\|web::\|\.route" src/handlers/http/mod.rs | head -60Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check for oidc-specific routes and scope
rg -n "oidc\|login\|reply_login\|callback" src/handlers/http/mod.rs -i -B 2 -A 2Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Verify if there's tenant spoofing protection in middleware
sed -n '200,400p' src/handlers/http/middleware.rsRepository: parseablehq/parseable
Length of output: 7624
🏁 Script executed:
# Check what happens when tenant_id from request header is used vs user.tenant
rg -n "user\.tenant\|&user\.tenant" src/handlers/http/oidc.rs -B 3 -A 3Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Look for tenant validation in put_user function
sed -n '375,450p' src/handlers/http/oidc.rsRepository: parseablehq/parseable
Length of output: 2432
🏁 Script executed:
# Check if there's any tenant owner or authorization concept
rg -n "first_user\|no.*user\|owner\|admin.*grant" src/rbac/user.rs -B 3 -A 3Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Verify how find_existing_user works and tenant scoping
sed -n '353,375p' src/handlers/http/oidc.rsRepository: parseablehq/parseable
Length of output: 738
🏁 Script executed:
# Check if OIDC reply_login is protected by Auth middleware
git log -1 --oneline -- src/handlers/http/mod.rsRepository: parseablehq/parseable
Length of output: 113
🏁 Script executed:
# Look for where the routes are wired up
find src/handlers -name "*.rs" -type f -exec grep -l "reply_login\|web::scope" {} \;Repository: parseablehq/parseable
Length of output: 205
🏁 Script executed:
# Check the main server.rs to see how routes are configured
sed -n '1,200p' src/handlers/http/modal/server.rsRepository: parseablehq/parseable
Length of output: 7658
🏁 Script executed:
# Look for the reply_login route configuration
grep -n "reply_login\|/callback\|\.route" src/handlers/http/modal/server.rs -A 3 -B 3Repository: parseablehq/parseable
Length of output: 21358
🏁 Script executed:
# Check the learnings context - verify tenant-scoped design
rg -n "tenant.*scope\|cross.*tenant" src/rbac/ --type rust -B 2 -A 2 | head -40Repository: parseablehq/parseable
Length of output: 47
Add explicit tenant owner validation before granting admin fallback permissions and remove client-supplied tenant header from unauthenticated OIDC flow.
The reply_login endpoint is completely unprotected (no .authorize() middleware), allowing any unauthenticated OAuth user to trigger a privilege escalation. When a user's roles map to empty permissions, the code grants full Admin privileges with only a comment suggesting this is "for the tenant owner"—but there is no actual validation that the user is a tenant owner. Additionally, the endpoint derives tenant_id from the client-supplied request header, yet creates the user with tenant: None, creating an inconsistency.
Required fixes:
- Validate that the user is actually a tenant owner (first user, or explicitly designated) before granting Admin permissions.
- Remove or ignore client-supplied tenant header in the OIDC callback; derive tenant from session context only (currently there is no authenticated session yet, so consider whether tenant should be
Noneor derive from OAuth claims). - Add audit logging when the admin fallback is triggered.
- Ensure user
tenantfield is set consistently based on actual tenant ownership, not client input.
🧰 Tools
🪛 GitHub Actions: Lint, Test and Coverage Report
[error] 302-302: borrow of moved value: tenant_id. In find_existing_user(&user_info, tenant_id), tenant_id is moved because find_existing_user takes tenant_id by value. Consider passing by reference or cloning: e.g., find_existing_user(&user_info, tenant_id.clone()) or change the function to take a borrowed value.
🤖 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 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.
| pub fn cookie_session(id: Ulid) -> Cookie<'static> { | ||
| Cookie::build(SESSION_COOKIE_NAME, id.to_string()) | ||
| .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64)) | ||
| .same_site(SameSite::Strict) | ||
| .same_site(SameSite::None) | ||
| .secure(true) | ||
| .path("/") | ||
| .finish() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Rust actix-web cookie crate http_only default behavior Cookie::build
💡 Result:
In actix-web, Cookie/Cookie::build() comes from the cookie crate. By default, Cookie::build(...) does not set HttpOnly; the http_only attribute is unset (None) unless you explicitly set it. When it’s unset, the Set-Cookie header will not include HttpOnly. [1][2]
To add it, do:
use actix_web::cookie::Cookie;
let c = Cookie::build(("session", "abc"))
.http_only(true) // <-- explicitly sets HttpOnly
.finish();Cookie::http_only() returns:
Some(true)if explicitly set true (or parsed withHttpOnly)Some(false)if explicitly set falseNoneif never set (the default) [2]
Sources: [1] [2]
🏁 Script executed:
# Search for cookie configurations in the file and related HTTPS context
rg -n "cookie_session|secure|http_only|https|http" src/handlers/http/oidc.rs -B2 -A2Repository: parseablehq/parseable
Length of output: 3514
🏁 Script executed:
# Check if there are any environment-based configurations for HTTPS
rg -n "HTTPS|https|secure" src/ --type rs | head -30Repository: parseablehq/parseable
Length of output: 91
🏁 Script executed:
# Look for other cookie usage in the codebase to understand patterns
fd -e rs -x rg -l "Cookie::build|http_only|secure" {} \; | head -10Repository: parseablehq/parseable
Length of output: 92
🏁 Script executed:
# Check for HTTPS enforcement in the codebase
rg -n "https|HTTPS|tls|TLS|secure" src/handlers/http/oidc.rsRepository: parseablehq/parseable
Length of output: 218
🏁 Script executed:
# Look for environment-based configuration for secure cookies
rg -n "cfg|env|secure|http" src/handlers/ --type rs | grep -i "secure\|https" | head -20Repository: parseablehq/parseable
Length of output: 91
🏁 Script executed:
# Check if secure(true) appears conditionally elsewhere
rg -n "\.secure\(" src/ -B3 -A1Repository: parseablehq/parseable
Length of output: 1110
🏁 Script executed:
# Look for any configuration files that might control HTTPS
fd -e toml -e yaml -e yml -e json -e conf | head -10Repository: parseablehq/parseable
Length of output: 326
🏁 Script executed:
# Find main entry point and server configuration
fd -name "main.rs" src/ | xargs catRepository: parseablehq/parseable
Length of output: 295
🏁 Script executed:
# Check for any HTTPS/TLS configuration in application setup
rg -n "HttpServer|TcpListener|listen|bind|https|tls" src/main.rs -B2 -A2Repository: parseablehq/parseable
Length of output: 948
🏁 Script executed:
# Look for environment or config handling related to protocol
rg -n "HTTP|HTTPS|protocol|scheme" src/handlers/ | head -20Repository: parseablehq/parseable
Length of output: 1651
🏁 Script executed:
# Check if there's conditional logic anywhere for cookie security based on deployment
rg -n "deployment|environment|dev|prod" src/handlers/http/oidc.rsRepository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Find the get_scheme method implementation
rg -n "fn get_scheme" src/Repository: parseablehq/parseable
Length of output: 120
🏁 Script executed:
# Look for where secure(true) might need to be conditional
rg -n "get_scheme|scheme" src/handlers/http/oidc.rs -B2 -A2Repository: parseablehq/parseable
Length of output: 764
🏁 Script executed:
# Check if there's configuration that allows HTTP or makes HTTPS optional
rg -n "P_ADDR|address|port|scheme" src/options.rs -A5Repository: parseablehq/parseable
Length of output: 119
🏁 Script executed:
# View get_scheme implementation
sed -n '540,560p' src/cli.rsRepository: parseablehq/parseable
Length of output: 654
🏁 Script executed:
# Search for where get_scheme is used to understand context
rg -n "get_scheme" src/ -B2 -A2Repository: parseablehq/parseable
Length of output: 1571
🏁 Script executed:
# Look for configuration structure definition
rg -n "pub struct.*Options\|pub struct.*Config" src/ | head -10Repository: parseablehq/parseable
Length of output: 47
🏁 Script executed:
# Check if secure(true) is used conditionally anywhere in codebase
rg -n "if.*https|if.*tls" src/ -i | head -10Repository: parseablehq/parseable
Length of output: 1075
Add http_only(true) and make secure conditional on HTTPS deployment.
The session cookie lacks the HttpOnly attribute, allowing client-side JavaScript access and increasing XSS attack surface. Additionally, secure(true) is unconditionally set, but the application supports HTTP deployment when TLS certificates are not provided. Secure cookies cannot be set in HTTP contexts and will be rejected by browsers. Both attributes should be properly configured:
- Add
http_only(true)to all session-related cookies - Make
secure()conditional based on the deployment scheme
The application already conditionally determines the scheme via get_scheme(), which returns "https" only when TLS certs are present. Use this pattern to conditionally apply secure().
🔒 Proposed approach
pub fn cookie_session(id: Ulid) -> Cookie<'static> {
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)
.http_only(true);
if is_https {
builder = builder.secure(true);
}
builder.path("/").finish()
}Apply the same pattern to cookie_username() and cookie_userid().
🤖 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 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().
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
2 participants
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor