fix(aver-server): shared store, consent recovery, OAuth hardening - #7
fix(aver-server): shared store, consent recovery, OAuth hardening#75queezer wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughOAuth token rotation and consent revocation are added, MCP sessions share one tools store, and scope and recall contracts are tightened. Error responses, authentication handling, documentation, and integration coverage are updated accordingly. ChangesOAuth integrity
Shared MCP storage
Scope and tool contracts
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuthToken
participant AuthDb
participant TokenFamily
Client->>OAuthToken: Submit authorization_code or refresh_token grant
OAuthToken->>AuthDb: Validate grant and load token state
AuthDb-->>OAuthToken: Return authorization or refresh data
OAuthToken->>TokenFamily: Rotate token or revoke reused family
TokenFamily-->>OAuthToken: Return token pair or error
OAuthToken-->>Client: Return JSON token response
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
🧹 Nitpick comments (1)
crates/aver-server/src/oauth.rs (1)
11-24: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDo not maintain a home-grown constant-time primitive.
Use a vetted implementation already approved by the project, such as a
ConstantTimeEqabstraction, instead of making two security boundaries depend on this local loop. Thesubtlecrate provides that dedicated contract. (docs.rs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aver-server/src/oauth.rs` around lines 11 - 24, Replace the local constant_time_eq byte loop with the project-approved subtle::ConstantTimeEq implementation, updating callers in the PKCE and consent-flow comparisons to use its constant-time equality contract and convert the result to bool as needed. Remove the home-grown helper while preserving the existing equality behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aver-server/src/auth.rs`:
- Around line 577-582: Update the refresh-token model and rotation flow around
the revoked-token handling in the authentication implementation to persist a
unique family identifier, inherit the existing identifier when issuing rotated
tokens, and assign a new identifier for a fresh login or consent generation.
Change revoke_token_family and its callers to target that family identifier
rather than the broad user/client pair, ensuring reuse revokes only the affected
lineage while preserving current-token behavior.
- Line 531: Update refresh_access_token() so token revocation and
issue_token_pair() persistence execute within one database transaction. Make
revocation conditional on the presented token, require exactly one affected row
before minting, and commit both the revocation and replacement-row inserts
together so any failure rolls back the entire rotation.
In `@crates/aver-server/src/consent.rs`:
- Around line 490-495: Update the error branches in the authentication paths,
including the branches around html_error at the shown locations, to log the
detailed anyhow error server-side and pass a fixed generic message to html_error
instead of interpolating err. Apply the same behavior to all referenced paths
while preserving the existing HTTP status and response structure.
- Around line 989-995: Update AuthDb::revoke_consent in auth.rs to execute the
client_consents, access_tokens, and refresh_tokens updates within a single
database transaction, committing only after all succeed and rolling back on any
failure. Keep the existing error propagation used by the caller in consent.rs.
In `@crates/aver-server/src/http.rs`:
- Around line 452-457: Update the token response around the JSON containing
access_token and refresh_token to include Cache-Control: no-store and Pragma:
no-cache headers, while preserving the existing response body and status
behavior.
- Around line 423-429: Update the "authorization_code" validation branch in the
token request handler to include request.redirect_uri.is_empty() alongside the
existing required-field checks. Return token_error with BAD_REQUEST and
"invalid_request" before database exchange when redirect_uri is omitted, while
preserving validation for the other fields.
- Around line 414-417: Update the oauth_token extractor handling so form parsing
failures, including invalid content types, malformed encoding, and missing
required fields, are handled inside oauth_token rather than short-circuiting.
Route every such failure through token_error and return the token JSON contract
with error set to invalid_request, while preserving normal TokenRequest
processing.
---
Nitpick comments:
In `@crates/aver-server/src/oauth.rs`:
- Around line 11-24: Replace the local constant_time_eq byte loop with the
project-approved subtle::ConstantTimeEq implementation, updating callers in the
PKCE and consent-flow comparisons to use its constant-time equality contract and
convert the result to bool as needed. Remove the home-grown helper while
preserving the existing equality behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdb3d5cd-d82d-4dc5-a676-525b844513ea
📒 Files selected for processing (17)
README.mdcrates/aver-server/src/auth.rscrates/aver-server/src/config.rscrates/aver-server/src/consent.rscrates/aver-server/src/http.rscrates/aver-server/src/mcp.rscrates/aver-server/src/oauth.rscrates/aver-server/src/origin.rscrates/aver-server/src/scope_resolution.rscrates/aver-server/src/tools.rscrates/aver-server/tests/http_routes.rscrates/aver-server/tests/oauth_consent_flow.rscrates/aver-server/tests/refresh_and_cors.rscrates/aver-server/tests/scope_mcp.rscrates/aver-server/tests/scope_resolution.rscrates/aver-server/tests/shared_store.rscrates/aver-server/tests/tools.rs
💤 Files with no reviewable changes (2)
- crates/aver-server/tests/scope_mcp.rs
- crates/aver-server/tests/tools.rs
146d9bd to
a3a9966
Compare
|
Addressed all seven review findings and rebased onto current Highlights: refresh rotation is transactional and family-scoped; browser database details are logged server-side but replaced with fixed client-safe errors; consent plus access/refresh revocation is atomic; malformed token forms and missing Validation passed: format; — Hermes Agent |
- Share one Store across MCP sessions: build_router opens AverTools once and hands every session the same Arc<Mutex<_>>, preserving the single-writer invariant (per-session Store::open raced log rotation and claim-id pre-allocation on the same memory_dir). - Consent empty-grant trap: zero-scope approvals no longer skip the consent screen (consent_covers([],[]) == true trapped users forever), and POST /oauth/consent/revoke now exposes AuthDb::revoke_consent so users can revoke + re-consent over HTTP. - recall drops the advertised-but-discarded alpha parameter; hybrid weighting needs an embedding client the server does not have. - Token endpoint: RFC 6749 expires_in, section 5.2 JSON error bodies; refresh tokens rotate on use with RFC 6819 reuse detection (family revocation); 401s carry WWW-Authenticate: Bearer. - Hardening: auth-DB upsert failures surface as 500 instead of a swallowed stub; ALTER TABLE migrations only ignore duplicate-column errors; PKCE verify reuses the constant-time comparison; scope validation rejects empty path segments and caps length at 256; serialization failures map to McpError instead of an empty success block; stale origin/consolidate docs fixed; embedding-model label is a named const; AVER_PORT parse errors carry context.
a3a9966 to
6b2d90a
Compare
1 participant
Fixes the server-hardening code-review findings against
crates/aver-server. Re-verified each finding against master after 35a36be (token TTLs + loopback CORS already landed upstream;AuthDb::revoke_consentalready revoked tokens at the DB layer — the missing pieces were the HTTP route and the skip-loop gate).Findings → fixes
1. Per-session
Storebroke single-writer (MAJOR)build_routernow opensAverToolsonce at startup and the session factory clones the sameArc<Mutex<AverTools>>intoAverMcpService::from_shared_tools(http.rs,mcp.rs). rusqlite connections are Send-not-Sync, so the existingArc<Mutex<_>>pattern (same asAuthDb) is used. Per-session state stays limited to auth/scope request extensions. Nobusy_timeoutadded — that belongs to the core PR.tests/shared_store.rs— two concurrent MCP sessions write claims and recall sees both;mcp.rsunit testservices_sharing_tools_observe_each_others_writes(threaded writers).2. Consent empty-grant trap (MAJOR)
Zero-scope approvals recorded
granted_scopes=""andconsent_covers([], []) == trueskipped the consent screen forever, minting tokens that fail everyrequire_scope. Conservative fix (chosen over mapping empty→all SUPPORTED, which would grant more than asked): the skip path now requires a non-empty grant (consent.rs), so the screen re-renders andrecord_consent's upsert overwrites the empty row. NewPOST /oauth/consent/revokeroute (session-cookie + origin validated) exposes the previously unreachableAuthDb::revoke_consent.empty_scope_approval_does_not_trap_user_in_skip_loop(regression for the stuck loop),revoke_route_revokes_consent_and_tokens_and_allows_reconsent. Existingapprove_..._skips_screenupdated to grantclaims:read(it previously pinned the trap).3.
alphavalidated then discarded (MAJOR)recallno longer advertisesalpha(removed from the MCP schema andtools::RecallParams). Threading it was not viable: hybrid recall needs anEmbeddingClientand aver-server has none (aver-core built without theollamafeature; wiring one would break the offline/deterministic test rule). aver-core'srecall_hybrid_claims_with_alphais untouched for CLI/BEAM use. Instructions card needed no change (it never mentioned alpha).alphafields removed fromtests/tools.rs/tests/scope_mcp.rs; obsoleterecall_tool_rejects_alpha_outside_unit_intervaldeleted.4. OAuth gaps (MAJOR)
expires_in(ACCESS_TOKEN_TTL_SECS, nowpub).(user, client)token family (RFC 6819 §5.2.2.3 reuse detection).tests/refresh_and_cors.rsupdated (it pinned reuse) + newrefresh_token_reuse_revokes_token_family.AuthDb::revoke_consent— now reachable via the revoke route (covered end-to-end by the route test).5. Minor batch
/oauth/tokenreturns RFC 6749 §5.2 JSON errors (invalid_grant/invalid_request/unsupported_grant_type) —oauth_token_route_returns_rfc6749_json_errors.WWW-Authenticate: Bearer(RFC 6750 §3) — asserted inhttp_routes.rs.tracing_unavailable_warnstub removed:authenticate_requestreturnsResult; auth-DB failures surface as HTML 500.let _ =ALTER TABLE migrations →apply_column_migration: ignores only the duplicate-column error (rusqlite 0.32 maps it toErrorCode::Unknown/ext 1 + message; comment explains), propagates everything else; expiry backfills propagate too.constant_time_eq(moved tooauth.rs, shared with the CSRF check).///,a//b) and caps length at 256 bytes —rejects_empty_path_segments,rejects_overlong_scope.json_tool_resultmaps serialization failure toMcpErrorinstead of an empty success block.origin.rsmodule doc corrected;consolidatescope doc reworded to match the"all"-only implementation;"nomic-embed-text"is nowDEFAULT_EMBEDDING_MODEL;AVER_PORTparse has.context("invalid AVER_PORT").6.
adapters.rswiringStill test-only (only
tests/adapter_boundaries.rsreferences it; no production path inmain.rs/http.rs/mcp.rs). Left as-is — intentional ADR-0016 boundary.Verification
cargo fmtapplied.cargo clippy -p aver-server --all-targets -- -D warnings— clean.cargo test -p aver-server— 172 passed, 0 failed, 0 ignored across all test binaries (deterministic/offline, no#[ignore]).Docs: README endpoint list + consent/token flow paragraph updated (rotation, reuse detection,
expires_in, revoke route, empty-grant behavior). Nodoc/adr/*.mdedits; noautoresearch.jsonlchanges; aver-core untouched (privacy filtering / log-first ordering preserved).Summary by CodeRabbit
New Features
Bug Fixes
Documentation