Conversation
|
PR Review: fix: set cookie properly CRITICAL ISSUES:
In oauth.rs (oauth_callback, mock_login, near_auth), the old code logged errors and continued. The new pattern using Recommended fix: use a match expression to handle all three variants explicitly - the Err arm should log at warn level (matching the previous behavior), and a parse failure for the cookie value should also log at warn level.
In service.rs around line 563, the anyhow error includes response_text from the proxy-session endpoint, which propagates into error chains logged at warn level by callers. A session endpoint response body may contain cookie values or session tokens. Per CLAUDE.md, security credentials must never be logged. The response body should be omitted from the error string. Note: the removed tracing::info! log in this PR is the correct call. The direction is correct - forwarding Set-Cookie is needed. Issues 1 and 2 are critical blockers. Signed off: warning found |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the OAuth, mock login, and NEAR authentication routes to forward Set-Cookie headers from the gateway session setup to the client. It also removes an informational log in the agent service. The reviewer recommends restoring the error logging for gateway session setup that was removed in these routes to maintain observability and facilitate debugging.
|
@claude review |
PR Review: fix: set cookie properlySummary: Correctly forwards the Set-Cookie header from compose-api to the browser across all three auth flows (OAuth callback, mock login, NEAR auth). The service.rs changes are good CLAUDE.md compliance improvements. Issue 1: response_text always consumed even on success path (service.rs) response.text().await is called unconditionally, reading and allocating the full response body on every successful login. The body is only ever used in the error branch. Move the .text() call inside the error branch: Issue 2: Only first Set-Cookie header forwarded headers.get(set-cookie) returns only the first value. HTTP allows multiple Set-Cookie headers, and headers.insert(SET_COOKIE, ...) overwrites any prior value. If compose-api is known to set exactly one cookie, add a comment. Otherwise use headers.get_all + response_headers.append. Issue 3: Cookie parse failure is a silent auth degradation When set_cookie.parse() fails, the user completes login to chat-api but the gateway cookie is never set in the browser, leaving them in a broken state. Since parse failure indicates a compose-api bug (invalid header bytes), returning an error is preferable to silently degrading. Otherwise the approach is correct: moving HeaderMap initialization before the match block and threading it through the response tuple is the right fix. Issues found - items 1 and 3 are worth addressing before merge. |
There was a problem hiding this comment.
Pull request overview
Adjusts auth flows to correctly propagate the gateway session cookie back to clients while avoiding leaking upstream response bodies in error messages.
Changes:
- Forward
Set-Cookiereturned bycompose-apifromoauth_callback,mock_login, andnear_authresponses. - Reduce error detail for
/auth/proxy-sessionfailures and move response body logging to debug-only logs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/services/src/agent/service.rs | Avoids embedding upstream response bodies in returned errors; logs response body at debug level on failure. |
| crates/api/src/routes/oauth.rs | Captures and forwards Set-Cookie from gateway session setup in several auth routes (including a near_auth response signature change). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Capture response body for debug logging only (do not include in errors per CLAUDE.md) | ||
| let response_text = response | ||
| .text() | ||
| .await | ||
| .unwrap_or_else(|_| "(unable to read response body)".to_string()); |
There was a problem hiding this comment.
This reads the full response body unconditionally, even when status.is_success() and the body is never used. That adds latency and memory overhead and also consumes the response stream. Consider only reading response.text() inside the if !status.is_success() branch (or when debug logging is enabled) so successful calls don't pay this cost.
| let mut headers = HeaderMap::new(); | ||
| match app_state | ||
| .agent_service | ||
| .setup_gateway_session_for_user(session.user_id) | ||
| .await | ||
| { | ||
| tracing::warn!( | ||
| "Failed to set up gateway session for user: user_id={}, error={}", | ||
| session.user_id, | ||
| e | ||
| ); | ||
| // Continue anyway - not critical for OAuth callback | ||
| Ok(Some(set_cookie)) => { | ||
| // Forward Set-Cookie header from compose-api to browser | ||
| if let Ok(cookie_value) = set_cookie.parse() { | ||
| headers.insert(http::header::SET_COOKIE, cookie_value); | ||
| } else { | ||
| tracing::warn!( | ||
| "Failed to parse Set-Cookie header value in oauth_callback: set_cookie_len={}", | ||
| set_cookie.len() | ||
| ); | ||
| } | ||
| } | ||
| Ok(None) => { | ||
| tracing::debug!( | ||
| "No Set-Cookie header returned from gateway session setup in oauth_callback" | ||
| ); | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| "Failed to set up gateway session for user in oauth_callback: user_id={}, error={}", | ||
| session.user_id, | ||
| e | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The Set-Cookie forwarding logic is duplicated across oauth_callback, mock_login, and near_auth with only the log context changing. To reduce repetition and the chance of behavior drifting, consider extracting a small helper (e.g., a function that takes &mut HeaderMap, an Option<String>, and a context label) that handles parse + insert + consistent logging.
No description provided.