feat: dynamic redirect uri

[nickpismenkov]

Note

Adds validated frontend_callback to OAuth login/callback to redirect users back to allowed frontends with tokens in URL fragment, persisting state in DB.

• Auth/OAuth:
  • Add optional frontend_callback query param to GET /v1/auth/github and GET /v1/auth/google; persist in oauth_states and use on /v1/auth/callback to redirect back to frontend with token and refresh_token in URL fragment (encoded).
  • New validate_frontend_callback enforces HTTPS (except localhost), blocks userinfo, traversal, queries/fragments, and checks origin via is_origin_allowed (now pub).
• Database:
  • Migration V0034__add_frontend_callback_to_oauth_states.sql adds frontend_callback column.
  • OAuthStateRepository: extend create/get_and_delete to handle frontend_callback; return it from queries.
• Tests:
  • New E2E tests e2e_oauth_frontend_callback.rs for storing/retrieving callbacks and replay protection.
  • Update repository E2E tests for new create signature and callback scenarios.
• Dependencies:
  • Add urlencoding for encoding tokens in callback fragment.

^{Written by Cursor Bugbot for commit 6a0013a. This will update automatically on new commits. Configure here.}

[gemini-code-assist]

Summary of Changes

Hello @nickpismenkov, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the OAuth authentication system by enabling dynamic frontend callback URLs. This feature allows client applications to specify where they should be redirected after completing the OAuth flow, providing greater flexibility in integrating with various frontend environments. The changes involve updating the database schema, modifying the OAuth initiation and callback logic, and adding robust testing to ensure proper functionality and security.

Highlights

• Dynamic OAuth Redirects: Introduced the ability for frontend applications to specify a dynamic callback URL during OAuth initiation for both GitHub and Google. This allows for flexible redirection after successful authentication.
• Database Schema Update: A new frontend_callback column has been added to the oauth_states table in the database to store the dynamic redirect URL provided by the frontend.
• OAuth Flow Integration: The github_login and google_login endpoints now accept an optional frontend_callback query parameter. If provided, the OAuth callback will redirect to this URL with the access and refresh tokens; otherwise, it defaults to a JSON response.
• URL Encoding: The urlencoding crate has been added and utilized to properly encode the access and refresh tokens when constructing the redirect URL to the frontend.
• Comprehensive Testing: New end-to-end tests have been added specifically for the dynamic frontend callback functionality, covering various scenarios including different providers, None values, special characters in URLs, and replay protection.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[claude]

Code Review - PR #273: feat: dynamic redirect uri

⚠️ CRITICAL SECURITY ISSUE: Open Redirect Vulnerability

Location: crates/api/src/routes/auth.rs:390-396

Issue: The frontend_callback parameter is accepted from user input and used directly in a redirect without any validation. This creates an open redirect vulnerability that attackers can exploit to redirect users to malicious sites after OAuth authentication.

Attack Vector:

1. Attacker crafts URL: /v1/auth/github/login?frontend_callback=https://evil.com
2. User completes OAuth flow
3. Server redirects to: https://evil.com/auth/callback?token=...&refresh_token=...
4. Attacker now has user's session tokens

Current Code:

// Line 390-396 in auth.rs
if let Some(frontend_url) = oauth_state_row.frontend_callback {
    let callback_url = format\!(
        "{}/auth/callback?token={}&refresh_token={}",
        frontend_url,
        urlencoding::encode(&access_token),
        urlencoding::encode(&refresh_token)
    );
    return Redirect::temporary(&callback_url).into_response();
}

Required Fix:
Validate frontend_callback against allowed origins using the existing CorsConfig validation:

// In github_login and google_login handlers (lines 175, 208)
// Add validation before storing state:
if let Some(ref callback_url) = params.frontend_callback {
    // Parse URL and extract origin
    let url = url::Url::parse(callback_url)
        .map_err(|_| StatusCode::BAD_REQUEST)?;
    let origin = format\!("{}://{}", url.scheme(), url.host_str().ok_or(StatusCode::BAD_REQUEST)?);
    
    // Validate against CORS config
    if \!is_origin_allowed(&origin, &config.cors) {
        error\!("Rejected frontend_callback from untrusted origin: {}", origin);
        return Err(StatusCode::BAD_REQUEST);
    }
}

You already have the is_origin_allowed() function at crates/api/src/lib.rs:498 that validates against CorsConfig. Use it to protect this redirect.

---

OTHER ISSUES

2. Privacy Violation - Logging Sensitive Data

Location: crates/api/src/routes/auth.rs:182-183, 215-216, 398

Issue: Logging frontend_callback URL at debug level and the full frontend URL at info level violates the CLAUDE.md privacy policy. URLs can contain query parameters with customer data.

Current Code:

debug\!("Initiating GitHub OAuth flow - frontend_callback: {:?}", params.frontend_callback);
info\!("Redirecting to frontend: {}", frontend_url);

Fix: Log only IDs or sanitized versions:

debug\!("Initiating GitHub OAuth flow - has_frontend_callback: {}", params.frontend_callback.is_some());
info\!("Redirecting to frontend callback");

---

3. Database Migration - Missing Backward Compatibility

Location: crates/database/src/migrations/sql/V0034__add_frontend_callback_to_oauth_states.sql

Issue: The migration adds a nullable column which is good for backward compatibility, but there's a potential issue during rolling deployments:

• Old code calling create() with 3 parameters will fail when new schema expects 4 parameters
• New code is deployed but some instances still running old code

Current Migration:

ALTER TABLE oauth_states ADD COLUMN frontend_callback VARCHAR(2048);

Recommendation: This is acceptable since the column is nullable, but document that this requires coordinated deployment or use blue-green deployment strategy. In multi-cluster deployments, ensure all instances are updated atomically.

---

SUMMARY

BLOCKING ISSUES:

1. ❌ CRITICAL: Open redirect vulnerability - must validate frontend_callback against allowed origins
2. ⚠️ Privacy violation: Remove or sanitize URL logging per CLAUDE.md guidelines

Non-blocking observations:

• Migration is backward compatible (nullable column)
• Tests are comprehensive and well-structured
• URL encoding of tokens is correct

Verdict: ⚠️ Changes Required Before Merge

The open redirect vulnerability is a critical security flaw that must be fixed. Once validation is added and logging is sanitized, this PR will be good to merge.

[gemini-code-assist]

Code Review

This pull request introduces a dynamic redirect URI for OAuth flows by adding an optional frontend_callback parameter. The changes span the API routes, database repository, and include a new migration and tests. The implementation correctly persists the callback URL and adds logic to redirect users after authentication.

My review has identified a few issues:

• A critical security vulnerability (Open Redirect) exists because the frontend_callback URL is not validated against a whitelist. This could allow an attacker to steal user tokens.
• There is a bug in how the final redirect URL is constructed, which will fail for callback URLs that already contain query parameters.
• The comments in the new test file are slightly misleading about what is being tested.

I have provided specific comments and suggestions to address these points. Overall, this is a valuable feature, but the security and correctness issues should be addressed before merging.

[cursor]

This is the final PR Bugbot will review for you during this billing cycle

Your free Bugbot reviews will reset on January 9

Details

Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

[PierreLeGuen]

@claude review

[PierreLeGuen]

/gemini review

[claude]

PR Review - Critical Issues Found ⚠️

1. Security - Open Redirect Vulnerability (HIGH SEVERITY)

Location: crates/api/src/routes/auth.rs:400-407

The redirect URL construction is vulnerable to path manipulation:

let callback_url = format!(
    "{}/auth/callback?token={}&refresh_token={}",
    frontend_url,
    urlencoding::encode(&access_token),
    urlencoding::encode(&refresh_token)
);

Issue: If frontend_url is https://app.example.com/existing/path, the redirect becomes https://app.example.com/existing/path/auth/callback?..., which may not be the intended endpoint.

More Critical: An attacker could provide a URL like https://allowed-origin.com@evil.com or use other URL parsing edge cases to bypass origin validation while redirecting to a malicious site.

Recommended Fix: After validating the origin, construct the full URL using proper URL parsing:

// After origin validation succeeds
let mut redirect_url = url; // reuse the parsed URL
redirect_url.set_path("/auth/callback");
redirect_url.set_query(Some(&format!(
    "token={}&refresh_token={}",
    urlencoding::encode(&access_token),
    urlencoding::encode(&refresh_token)
)));
let callback_url = redirect_url.to_string();

This ensures the redirect URL is always {origin}/auth/callback?... regardless of the input path.

---

2. Privacy Violation - Logging Sensitive Data (CRITICAL per CLAUDE.md)

Locations:

• Line 183-186: debug!("Initiating GitHub OAuth flow - frontend_callback: {:?}", params.frontend_callback)
• Line 216-219: debug!("Initiating Google OAuth flow - frontend_callback: {:?}", params.frontend_callback)
• Line 407: info!("Redirecting to frontend: {}", frontend_url)

Issue: CLAUDE.md explicitly states production runs at INFO level and above, and frontend callback URLs may contain query parameters with user data (e.g., https://app.com/callback?next=/user/profile&session=xyz). Logging these URLs violates the privacy rules:

✗ NEVER LOG: Metadata that reveals customer information - Custom fields, tags, labels that could expose user activity

Impact: The INFO-level log (line 407) runs in production and could leak sensitive user navigation data.

Recommended Fix:

// Remove the debug logs or only log that it was provided (not the value):
debug!("Initiating GitHub OAuth flow - has_frontend_callback: {}", params.frontend_callback.is_some());

// For INFO log, only log the origin (already validated):
info!("Redirecting to frontend origin: {}", origin_to_check);

---

3. Database Migration - Missing Index (PERFORMANCE)

Location: V0034__add_frontend_callback_to_oauth_states.sql

The migration adds frontend_callback VARCHAR(2048) but OAuth states are temporary (expire in 10 minutes). No index needed, but consider:

• Potential issue: 2048 chars is very large for a URL. Consider if 1024 is sufficient to reduce storage overhead.
• Verification: Confirm this column size aligns with other URL storage in the system.

This is minor but worth considering for consistency.

---

4. Logic - URL Construction Edge Case (MEDIUM)

Location: Line 400-407

If frontend_url already has query parameters (e.g., https://app.com/auth?foo=bar), the constructed URL becomes:
https://app.com/auth?foo=bar/auth/callback?token=...&refresh_token=...

This creates a malformed URL.

Recommended Fix: The URL parsing approach from issue nearai/chat-api#105 also fixes this.

---

Summary

• 1 HIGH SEVERITY security issue: Open redirect vulnerability
• 1 CRITICAL privacy violation: Logging sensitive URLs in production
• 2 MEDIUM issues: URL construction edge cases

Recommendation: ⚠️ Do not merge until security and privacy issues are resolved.

The logging violations directly conflict with CLAUDE.md requirements for production privacy, and the open redirect vulnerability could be exploited for phishing attacks.

[gemini-code-assist]

Code Review

This pull request introduces a dynamic redirect URI feature for OAuth flows, which is a great enhancement for supporting multiple frontends. The changes are well-structured, including database migration, repository updates, and new tests for the data layer. However, I've found a critical issue in how the redirect URL is constructed, which could break the feature in many cases. I've provided a suggestion to fix this. I also noticed that with the fix, a newly added dependency is no longer necessary and can be removed.

[nickpismenkov]

@claude review

[claude]

Code Review - PR #273: Dynamic OAuth Frontend Redirects

Critical Issues Found ⚠️

1. Security: Path Override Vulnerability (High Severity)

Location: crates/api/src/routes/auth.rs:405-407

The code intentionally overwrites the path from frontend_callback with hardcoded /auth/callback:

let mut callback_url = url.clone();
callback_url.set_path("/auth/callback");  // Overwrites user-provided path
callback_url.set_query(Some(&format!(
    "token={}&refresh_token={}",
    urlencoding::encode(&access_token),
    urlencoding::encode(&refresh_token)
)));

Problem: The comment claims this "prevents path manipulation attacks", but this defeats the purpose of dynamic redirects. If frontends can only receive callbacks at /auth/callback, why accept a frontend_callback parameter at all?

Scenarios this breaks:

• Frontend at https://app.example.com/oauth/complete → Redirected to https://app.example.com/auth/callback (wrong path, 404)
• Frontend at https://app.example.com/dashboard/callback → Redirected to https://app.example.com/auth/callback (wrong path)

Recommendation: Either:

1. Use the full URL as provided (validate origin only, preserve path):

   if is_origin_allowed(origin_to_check, &config.cors) {
       let mut callback_url = Url::parse(&frontend_url)?;
       callback_url.set_query(Some(&format!(
           "token={}&refresh_token={}",
           urlencoding::encode(&access_token),
           urlencoding::encode(&refresh_token)
       )));
       return Redirect::temporary(callback_url.as_str()).into_response();
   }

2. Restrict paths via configuration if path manipulation is a real concern:
   • Add allowed path patterns to CORS config
   • Validate both origin AND path before redirecting

The current approach provides false security while breaking legitimate use cases.

---

2. Database Migration: Missing Backward Compatibility Check

Location: crates/database/src/migrations/sql/V0034__add_frontend_callback_to_oauth_states.sql

ALTER TABLE oauth_states
ADD COLUMN frontend_callback TEXT;

Concern: During rolling deployment in a multi-cluster environment:

• Old instances may still be inserting rows without frontend_callback (using old 5-param INSERT)
• New instances expect 6-param INSERT

Verification needed: Confirm that PostgreSQL handles mixed-version writes gracefully. Since the new column is nullable and has no default constraint, this should be safe, but explicit verification is recommended.

Recommendation: Test migration on a staging cluster with mixed versions to ensure no INSERT failures occur during rollout.

---

3. Logic: Query Parameter Encoding May Fail

Location: crates/api/src/routes/auth.rs:408-412

callback_url.set_query(Some(&format!(
    "token={}&refresh_token={}",
    urlencoding::encode(&access_token),
    urlencoding::encode(&refresh_token)
)));

Problem: urlencoding::encode() returns a Cow<str>, but if tokens contain characters that need encoding (e.g., +, /, `="), the format string construction may produce malformed query strings.

Example: If access_token = "abc+def=", this becomes token=abc%2Bdef%3D, which is correct. However, using format! with Cow can be error-prone.

Recommendation: Use proper query parameter building or verify token format:

use url::form_urlencoded;

let query = form_urlencoded::Serializer::new(String::new())
    .append_pair("token", &access_token)
    .append_pair("refresh_token", &refresh_token)
    .finish();
callback_url.set_query(Some(&query));

This is safer and more idiomatic for URL construction.

---

4. Test Coverage Gap: No End-to-End Redirect Tests

Location: crates/api/tests/e2e_oauth_frontend_callback.rs

All tests verify database storage/retrieval but none test the actual redirect behavior through the HTTP layer.

Missing test scenarios:

• ✅ Valid frontend_callback → Should redirect with tokens in query params
• ✅ Invalid origin (not in CORS config) → Should return 400
• ✅ Malformed URL → Should return 400
• ✅ No frontend_callback → Should return JSON (fallback behavior)
• ✅ Path override behavior → Verify /auth/callback is enforced

Recommendation: Add integration tests that call /v1/auth/github?frontend_callback=... and verify the redirect response, not just database state.

---

Minor Issues (Non-Critical)

5. Code Quality: Unnecessary .clone() calls

Location: crates/api/src/routes/auth.rs:199, 232

params.frontend_callback.clone(),  // Unnecessary clone

Since params is consumed here and frontend_callback is Option<String>, you can move it:

params.frontend_callback,  // No clone needed

---

6. Logging: Privacy Compliance

Location: crates/api/src/routes/auth.rs:184, 217

debug!("Initiating GitHub OAuth flow - frontend_callback: {}", params.frontend_callback.is_some());

✅ Good: Logs only boolean presence, not the actual URL (which could contain sensitive redirect info).

However, at line 414:

info!("Redirecting to frontend: {}", origin_to_check);

Concern: Logging the origin may reveal customer frontend domains. Per CLAUDE.md privacy policy, this is borderline. Consider:

info!("Redirecting to validated frontend origin");

Or log only in debug mode if needed for troubleshooting.

---

Summary

Blocking Issues:

1. ❌ Path override logic defeats the purpose of dynamic redirects - This will break legitimate frontend integrations
2. ⚠️ Missing E2E tests for redirect behavior - Database tests are insufficient

Recommended Actions Before Merge:

1. Decide on path handling strategy (preserve vs. override) and update implementation
2. Add HTTP-level integration tests for redirect flows
3. Remove unnecessary .clone() calls
4. Consider reducing info-level logging of origins

Sign-off: ⚠️ Issues Found - Requires Changes

---

Overall Assessment: The database layer and migration look solid, but the redirect logic needs clarification. The path override behavior appears to be a security measure that may break real-world use cases. Please clarify the intended design before merging.

[nickpismenkov]

@claude review

[claude]

Security & Logic Review - CRITICAL ISSUES FOUND ⚠️

I've identified several critical security vulnerabilities and logic issues that must be fixed before merging:

---

🔴 CRITICAL: Open Redirect Vulnerability (High Severity)

Location: crates/api/src/routes/auth.rs, lines 388-435 (in the diff)

Issue: The code redirects to user-controlled frontend_callback URLs with authentication tokens in query parameters. While there's origin validation, the path component is not validated, allowing attackers to redirect users to attacker-controlled paths on allowed domains.

Attack Scenario:

1. Attacker sets frontend_callback to: https://app.example.com/../../evil.com/steal?
2. Origin validation passes (origin is https://app.example.com)
3. Code appends: ?token=...&refresh_token=...
4. Final URL could be manipulated via path traversal or @ tricks

Additional Issues:

• Path traversal: https://app.example.com/../../../attacker.com/steal
• Username in URL: https://app.example.com@evil.com/steal
• Fragment bypasses: URL parsing edge cases

Fix Required:

// After origin validation, also validate the full URL structure
match Url::parse(&frontend_url) {
    Ok(url) => {
        let origin_str = url.origin().unicode_serialization();
        let origin_to_check = origin_str.strip_suffix('/').unwrap_or(&origin_str);
        
        if is_origin_allowed(origin_to_check, &config.cors) {
            // CRITICAL: Validate path doesn't contain suspicious patterns
            let path = url.path();
            if path.contains("../") || path.contains("%2e%2e") {
                error\!("Path traversal attempt in frontend_callback");
                return (StatusCode::BAD_REQUEST, ...).into_response();
            }
            
            // Verify domain hasn't been spoofed via @ or other tricks
            if url.username() \!= "" || frontend_url.contains('@') {
                error\!("Invalid characters in frontend_callback URL");
                return (StatusCode::BAD_REQUEST, ...).into_response();
            }
            
            // Now safe to redirect
            let mut callback_url = url.clone();
            callback_url.set_query(...);
            ...
        }
    }
}

---

🟡 MEDIUM: Token Exposure in URL Query Parameters

Location: Same as above

Issue: Tokens are passed in URL query parameters (?token=...&refresh_token=...), which:

• Appear in browser history
• Are logged in web server access logs
• Can leak via Referer headers
• Are visible in browser address bar

Recommendation: Use POST redirect (form auto-submit) or fragment-based approach:

// Option 1: Fragment-based (more secure, not sent to server)
callback_url.set_fragment(Some(&format\!(
    "token={}&refresh_token={}",
    urlencoding::encode(&access_token),
    urlencoding::encode(&refresh_token)
)));

// Option 2: POST redirect with form auto-submit (requires HTML response)

---

🟡 MEDIUM: Missing Input Validation on frontend_callback

Location: github_login and google_login functions

Issue: The frontend_callback parameter from query string is accepted without validation before storage. Malicious values are only caught later in oauth_callback.

Fix: Validate at input time:

pub async fn github_login(
    Query(params): Query<OAuthInitQuery>,
    State((oauth, state_store, _auth_service, config)): State<AuthState>,
) -> Result<Redirect, StatusCode> {
    // Validate frontend_callback early
    if let Some(ref callback_url) = params.frontend_callback {
        match Url::parse(callback_url) {
            Ok(url) => {
                let origin = url.origin().unicode_serialization();
                if \!is_origin_allowed(origin.strip_suffix('/').unwrap_or(&origin), &config.cors) {
                    error\!("frontend_callback origin not allowed at login");
                    return Err(StatusCode::BAD_REQUEST);
                }
            }
            Err(_) => {
                error\!("Invalid frontend_callback URL format");
                return Err(StatusCode::BAD_REQUEST);
            }
        }
    }
    // ... rest of function
}

---

🟢 MINOR: Logging May Expose Sensitive URLs

Location: Lines 183, 217 in diff

Current Code:

debug\!(
    "Initiating GitHub OAuth flow - frontend_callback: {}",
    params.frontend_callback.is_some()
);

Issue: While you're correctly logging only the presence (not the value), the later line logs the origin which could reveal customer activity patterns.

Per CLAUDE.md:

✗ NEVER LOG: Metadata that reveals customer information

Fix: Consider whether logging the presence of frontend_callback reveals customer behavior. If it does, remove or reduce to trace level.

---

🟢 MINOR: Database Migration Missing Constraints

Location: V0034__add_frontend_callback_to_oauth_states.sql

Issue: The frontend_callback column has no length constraint. While TEXT is fine, consider:

• Adding a reasonable length limit (e.g., 2048 characters for URLs)
• Adding a check constraint for valid URL format (optional)

---

✅ GOOD PRACTICES OBSERVED:

• Proper replay protection (get_and_delete)
• Origin validation against CORS config
• Comprehensive E2E tests
• URL encoding of tokens

---

📋 REQUIRED ACTIONS BEFORE MERGE:

1. FIX CRITICAL: Implement path traversal and URL spoofing checks
2. FIX MEDIUM: Consider POST redirect or fragment-based token passing
3. FIX MEDIUM: Add early validation of frontend_callback in login endpoints
4. Review logging for privacy compliance

Overall Status: ⚠️ DO NOT MERGE until critical open redirect vulnerability is fixed.

---

cc @PR-author - Please address the open redirect vulnerability before merging. Happy to review the fixes!

[nickpismenkov]

@claude review

[claude]

Code Review - PR #273: Dynamic redirect URI

I've reviewed this PR focusing on critical security, logic, and production safety issues. Here are my findings:

CRITICAL ISSUES

1. Open Redirect Vulnerability - Missing Query Parameter Validation

Location: crates/api/src/routes/auth.rs, lines ~476-479

The redirect URL validation checks origin, path traversal, and username, but does NOT validate query parameters. An attacker can inject malicious query parameters that could execute XSS if frontend blindly trusts query params, perform CSRF attacks via carefully crafted query strings, or bypass frontend security via parameter pollution.

Example Attack: frontend_callback=https://allowed-origin.com/auth?redirect=https://evil.com

The validation would pass (origin is allowed, no traversal), but the frontend might redirect to evil.com based on the redirect parameter.

Fix: Add query parameter validation or reject URLs with query strings.

2. Token Exposure in URL Fragments

Location: crates/api/src/routes/auth.rs, lines ~476-484

While URL fragments don't appear in server logs or Referer headers (good!), they ARE stored in browser history (contrary to the comment), ARE accessible via JavaScript to any script on the page, and CAN be leaked via browser extensions, analytics tools, or compromised frontend code.

This is particularly dangerous in a multi-tenant OAuth flow where frontends may have varying security postures.

Recommendation: Consider using one of these more secure approaches:

1. POST request with body - Return a form that auto-submits via POST
2. Session cookie - Set HttpOnly cookie and redirect without tokens
3. One-time exchange code - Redirect with a short-lived exchange code that frontend uses to fetch tokens via API

Note: If fragment approach is intentional (for mobile/SPA deep-linking), document the security tradeoffs and ensure frontends immediately extract and clear the fragment.

3. Race Condition in OAuth State Validation

Location: crates/api/src/routes/auth.rs, lines ~177-229

The frontend_callback validation happens in two separate places during OAuth flow: at login initiation and at callback. Between these steps, the CORS configuration could change (e.g., during a rolling deployment). An attacker could start OAuth with a currently-allowed origin, wait for config update that removes that origin, then complete OAuth and get redirected despite origin no longer being allowed.

Fix: Consider storing the allowed status in the oauth_states table or accept that callback-time validation is authoritative.

4. Database Migration - Missing Backward Compatibility Analysis

Location: V0034__add_frontend_callback_to_oauth_states.sql

The migration adds a nullable column frontend_callback TEXT to oauth_states. During rolling deployment, old instances won't process frontend_callback redirects correctly. What happens if an old instance handles the callback after a new instance stored the frontend_callback?

Recommendation: Document rollout strategy or add a feature flag to ensure all instances are upgraded before frontend_callback is used.

MEDIUM PRIORITY ISSUES

5. Insufficient URL Validation Edge Cases

Current validation misses several edge cases: javascript: and data: URIs, IP addresses, homograph attacks, and URL normalization issues. Add scheme validation to only allow HTTP/HTTPS.

6. Missing Rate Limiting on OAuth Endpoints

The github_login and google_login endpoints create database rows without rate limiting. An attacker could spam oauth_states table with junk entries causing database bloat.

GOOD PRACTICES OBSERVED

1. Replay protection via get_and_delete
2. Defense in depth with validation at both stages
3. CORS enforcement
4. URL encoding of tokens
5. Comprehensive test coverage

VERDICT: Changes Requested

This PR has good security awareness but needs fixes for:

1. Critical: Query parameter validation in frontend_callback
2. Critical: Document token-in-fragment security tradeoffs or use alternative approach
3. Important: Backward compatibility plan for rolling deployment
4. Nice-to-have: Additional URL validation edge cases

[nickpismenkov]

@claude review

[nickpismenkov]

@claude review

[claude]

Code Review - PR #273

I've reviewed this PR and found one critical security issue that must be addressed before merge.

⚠️ CRITICAL: Open Redirect Vulnerability

Location: crates/api/src/routes/auth.rs:443-473

Issue: The validation re-runs at callback time but on failure, only logs an error and returns a generic 400 response. Meanwhile, the already-validated URL from database is used directly for redirect without re-validation success check:

if let Some(frontend_url) = oauth_state_row.frontend_callback {
    let validated_url = match validate_frontend_callback(&frontend_url, &config.cors) {
        Ok(url) => url,
        Err(err_msg) => {
            error\!("frontend_callback validation failed at callback: {}", err_msg);
            return (
                StatusCode::BAD_REQUEST,
                Json(serde_json::json\!({
                    "error": "invalid_request",
                    "error_description": "Invalid frontend callback URL"
                })),
            )
                .into_response();
        }
    };

    let mut callback_url = validated_url.clone();  // Uses validated_url
    callback_url.set_fragment(Some(&format\!(
        "token={}&refresh_token={}",
        urlencoding::encode(&access_token),
        urlencoding::encode(&refresh_token)
    )));

    info\!("Redirecting to frontend callback");
    return Redirect::temporary(callback_url.as_str()).into_response();
}

Attack Scenario:
If CORS configuration changes between OAuth initiation and callback (config reload, deployment), an attacker could:

1. Initiate OAuth with a malicious URL when it was allowed
2. Wait for CORS config to tighten (remove the attacker's domain)
3. Complete OAuth flow - validation fails but DB value is already stored
4. While the code attempts to use validated_url, if there's any code path where the original URL is used, tokens could leak to attacker domain

Impact: Session token and refresh token exposure to unauthorized domains = full account takeover.

Fix Required:
The current code structure looks correct (uses validated_url), but this defensive validation pattern is problematic:

• If validation at callback fails, the entire flow should abort (current behavior is correct)
• However, validation should happen ONCE at initiation - there's no security benefit to re-validating at callback since the URL is already stored
• The dual validation creates confusion and code smell

Recommended approach:

1. Keep validation at initiation (lines 238-245 for GitHub, 268-275 for Google) ✅
2. Remove re-validation at callback - if URL was validated and stored, trust it
3. This simplifies code and removes the confusing "validate but maybe use anyway" pattern

Alternative (if config changes must be detected):
If CORS config changes must invalidate stored URLs, then validate at callback but ABORT the entire flow on failure - do not attempt to use any URL value:

if let Some(frontend_url) = oauth_state_row.frontend_callback {
    // Validate - if config changed, reject the flow
    let validated_url = validate_frontend_callback(&frontend_url, &config.cors)
        .map_err(|err_msg| {
            error\!("frontend_callback validation failed at callback: {}", err_msg);
            // Return error response - DO NOT proceed with redirect
        })?;
    
    // Now safe to use validated_url for redirect
}

But honestly, the simpler approach is to remove callback validation entirely - validation at storage time is sufficient.

---

Other Observations (Non-blocking)

✅ Database Migration: Backward compatible (nullable column), safe for rolling deployments
✅ Privacy/Logging: No sensitive data logged, only booleans and error messages
✅ Tests: Comprehensive coverage including edge cases and replay protection
✅ URL Validation: Thorough checks for path traversal, credentials, query params, fragments

---

Verdict: ⚠️ Changes Required

Please address the open redirect concern before merge. The fix is straightforward - either trust stored URLs or properly abort on validation failure.

[nickpismenkov]

@claude review

[claude]

Code Review - Critical Issues Found ⚠️

I've identified several CRITICAL security and production safety issues that must be addressed before merge:

---

1. 🔴 CRITICAL: Open Redirect Vulnerability - Username Bypass

Location: crates/api/src/routes/auth.rs - validate_frontend_callback function

Issue: The username check occurs AFTER origin validation, allowing potential bypass:

// Current order allows bypass attempts
let origin_str = url.origin().unicode_serialization();
let origin = origin_str.strip_suffix('/').unwrap_or(&origin_str);
if \!crate::is_origin_allowed(origin, cors_config) {
    return Err("Origin not allowed");
}

// This comes TOO LATE
if \!url.username().is_empty() || url_str.contains('@') {
    return Err("Invalid callback URL format");
}

Attack Vector: URLs like https://allowed.com@evil.com/path or URL-encoded %40 could bypass CORS checks.

Fix: Reorder checks - validate URL structure (no userinfo, HTTPS-only scheme) BEFORE extracting origin:

// Validate scheme first
if url.scheme() \!= "https" {
    return Err("Callback URL must use HTTPS");
}

// Block userinfo BEFORE origin check
if \!url.username().is_empty() || url.password().is_some() {
    return Err("Invalid callback URL format");
}

// Then check origin
let origin_str = url.origin().unicode_serialization();
// ...

---

2. 🔴 CRITICAL: Path Traversal Check Incomplete

Location: validate_frontend_callback function

Issue: Path traversal check misses edge cases:

if path.contains("../ ") || path.contains("%2e%2e") || path.contains("%2E%2E") {
    return Err("Invalid path in callback URL");
}

Problems:

• Missing mixed-case encodings: %2E%2e, %2e%2E
• Missing double-encoded: %252e%252e
• The Url crate auto-normalizes, so check after parsing is insufficient

Fix: Simplify by checking decoded path:

let path = url.path();
if path.contains("..") {
    return Err("Invalid path in callback URL");
}

---

3. 🔴 CRITICAL PRIVACY BREACH: Token Exposure Risk

Location: OAuth callback redirect with tokens in URL fragment

CLAUDE.md Violation: Your project's CLAUDE.md explicitly forbids logging tokens:

✗ NEVER LOG (Forbidden)

• Security credentials - API keys, session tokens, passwords, secrets, OAuth tokens

Code:

callback_url.set_fragment(Some(&format\!(
    "token={}&refresh_token={}",
    urlencoding::encode(&access_token),
    urlencoding::encode(&refresh_token)
)));

info\!("Redirecting to frontend callback");  // PRIVACY RISK
return Redirect::temporary(callback_url.as_str()).into_response();

Risk: If HTTP access logging middleware is enabled, this redirect URL will be logged, exposing customer session tokens.

Fix:

1. Verify NO access logging middleware captures this response
2. Add security comment explaining risk
3. Consider alternative token delivery (POST to callback, or separate API endpoint)

---

4. 🔴 PRODUCTION BREAKING: Rolling Deployment Incompatibility

Location: Database migration + repository signature change

Issue: The function signature changed from 3 to 4 parameters:

// OLD (main branch)
repo.create(state, provider, pkce_verifier).await

// NEW (this PR)  
repo.create(state, provider, pkce_verifier, frontend_callback).await

Breaking Change: During rolling deployment in your multi-cluster environment:

• Migration adds frontend_callback column ✅ (safe, nullable)
• Old code still running will fail to compile/call new function signature ❌
• Result: Service disruption during deployment

Safe Migration Path:

Option A (Recommended): Two-phase deployment

// Phase 1: Keep backward compatibility
pub async fn create(
    state: String,
    provider: String,
    pkce_verifier: Option<String>,
) -> Result<OAuthStateRow> {
    self.create_with_callback(state, provider, pkce_verifier, None).await
}

pub async fn create_with_callback(
    state: String,
    provider: String,
    pkce_verifier: Option<String>,
    frontend_callback: Option<String>,
) -> Result<OAuthStateRow> {
    // Existing implementation
}

Option B: Update all call sites to pass None first, THEN change signature in separate PR

---

5. ⚠️ Minor: Redundant Validation

Location: validate_frontend_callback called twice

The callback URL is validated at login initiation AND at OAuth callback completion. Since it's stored server-side and attackers can't modify it, the second validation is redundant.

Recommendation: Remove second validation OR add comment explaining defense-in-depth reasoning.

---

6. ⚠️ Code Quality: Missing HTTPS Enforcement

Issue: No explicit check that callback URL uses HTTPS scheme.

Fix:

if url.scheme() \!= "https" {
    return Err("Callback URL must use HTTPS");
}

---

Summary

MUST FIX before merge:

1. ✅ Reorder username/origin validation (security)
2. ✅ Fix path traversal check (security)
3. ✅ Address token logging risk (privacy/compliance)
4. ✅ Fix rolling deployment compatibility (production safety)

Should fix:

• Add HTTPS scheme validation
• Remove redundant validation or document reasoning

Test Coverage: E2E tests look comprehensive ✅, but add negative test cases for:

• Malicious URLs with @ in various positions
• Path traversal attempts with different encodings
• Non-HTTPS URLs

---

Great work on the comprehensive test coverage and feature implementation! These issues are fixable with targeted changes to the validation logic and deployment strategy. Let me know if you need clarification on any of these points.

[nickpismenkov]

@claude review

[claude]

PR Review: Dynamic Redirect URI

I've analyzed this PR focusing on critical issues. Here are my findings:

⚠️ CRITICAL ISSUES

1. Open Redirect Vulnerability - Token Leakage via Referer Header

Location: crates/api/src/routes/auth.rs:489-493

The PR redirects to user-controlled URLs with tokens in the fragment:

callback_url.set_fragment(Some(&format!(
    "token={}&refresh_token={}",
    urlencoding::encode(&access_token),
    urlencoding::encode(&refresh_token)
)));

Problem: If the frontend callback page contains any external resources (images, scripts, stylesheets, fonts, analytics, ads, etc.) or any links that users click, the fragment may be leaked via:

• Referer header (browser-dependent, but many browsers leak fragments to same-origin navigations)
• JavaScript on the page accessing window.location.hash
• Redirect chains where intermediate hops can capture the fragment

Why validation isn't enough: Even with CORS allowlist validation, if an allowed origin is compromised or has XSS vulnerabilities, attackers can exfiltrate tokens.

Impact: Session hijacking, account takeover. This is a CRITICAL security vulnerability.

Recommended fixes:

1. Best: Use authorization code flow with backend exchange (don't put tokens in URL at all)
2. Alternative: Use POST form with application/x-www-form-urlencoded body instead of redirect
3. If redirect required: Add explicit warning in docs about secure fragment handling

---

2. Race Condition: Time-of-Check to Time-of-Use (TOCTOU)

Location: crates/api/src/routes/auth.rs:245-258 and 469-486

The validation flow has a TOCTOU vulnerability:

// At login time (github_login/google_login):
if let Some(ref callback_url) = params.frontend_callback {
    validate_frontend_callback(callback_url, &config.cors)?;  // Validation nearai/chat-api#105
}
state_store.create(..., params.frontend_callback.clone()).await; // Store

// Later at callback time (oauth_callback):
if let Some(frontend_url) = oauth_state_row.frontend_callback {
    validate_frontend_callback(&frontend_url, &config.cors)?;  // Validation nearai/cloud-api#2 (redundant)
    // Redirect
}

Problem: Between validation nearai/chat-api#105 and callback, the CORS configuration could change (e.g., removing an allowed origin). This creates a window where:

• URL validated at login time with old config
• URL used at callback time with new config
• Second validation might fail, breaking OAuth flow for legitimate users

Impact: Denial of service for legitimate users during config updates in multi-instance deployments.

Recommended fix:
Only validate at callback time (remove validation at login). The callback validation is authoritative since that's when the redirect happens. This also simplifies rollback scenarios.

---

3. Database Migration Missing IF NOT EXISTS

Location: crates/database/src/migrations/sql/V0034__add_frontend_callback_to_oauth_states.sql:3-4

ALTER TABLE oauth_states
ADD COLUMN frontend_callback TEXT;

Problem: In rolling deployments, if migration runs twice (e.g., multiple instances starting concurrently), this will fail with "column already exists" error.

Impact: Deployment failures, service outage during rollout.

Recommended fix:

ALTER TABLE oauth_states
ADD COLUMN IF NOT EXISTS frontend_callback TEXT;

---

4. Missing Timeout on OAuth State Validation

Location: crates/api/src/routes/auth.rs:29-87 (validate_frontend_callback)

The validation function performs multiple string operations and URL parsing without explicit resource limits beyond the 2048 character check.

Problem: While 2048 bytes is checked, the path traversal validation does multiple .contains() calls which are O(n*m). A carefully crafted 2048-byte URL with many repeated patterns could cause CPU exhaustion.

Impact: Minor DoS risk (low severity, but worth noting).

Recommendation: Current 2048 limit is reasonable, but consider adding rate limiting on the login endpoints.

---

📋 NON-CRITICAL OBSERVATIONS

1. Unnecessary clone: crates/api/src/routes/auth.rs:486 - validated_url.clone() could be avoided by using validated_url directly (it's not used after)

2. Test coverage gap: E2E tests only validate database storage, not actual HTTP redirect behavior with validation. Consider adding integration test that calls the actual endpoints.

3. Privacy logging compliance: Good adherence to CLAUDE.md - no customer data in logs ✅

---

🔒 PRIORITY RECOMMENDATION

Issue nearai/chat-api#105 (Token Leakage) is CRITICAL and should block merge. This is a fundamental security architecture issue that cannot be mitigated with validation alone.

Please address at minimum issues nearai/chat-api#105 and #3 before merging.

---

⚠️ CRITICAL ISSUES FOUND - DO NOT MERGE