Conversation
nahimterrazas
requested review from
NicoMolinaOZ,
pepebndc and
shahnami
as code owners
|
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:
📝 WalkthroughWalkthroughAdds optional API JWT auth: new seed override Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server (axum)
participant Jwt as JwtService
participant Store as RateLimitStore
Client->>Server: POST /api/v1/auth/token (Basic auth or body creds)
Server->>Store: check/increment client/IP rate limit
Store-->>Server: allow / deny
alt allowed
Server->>Jwt: generate_access_token_with_ttl_seconds(client_id, scopes, ttl)
Jwt-->>Server: access_token
Server-->>Client: 200 { access_token, token_type, expires_in, scope }
else denied
Server-->>Client: 429 or OAuth error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/solver-service/src/config_merge.rs (1)
1340-1372:⚠️ Potential issue | 🔴 CriticalAdd
Some()wrapper aroundAuthConfigin the if-branch.The
authvariable is assigned a value based on a condition: if the condition is true, it gets anAuthConfig; otherwise, it getsNone. SinceApiConfig.authis typed asOption<AuthConfig>(confirmed in crates/solver-config/src/lib.rs:249), the if-branch must returnSome(AuthConfig { ... })to match the else-branch which returnsNone.The code at line 870 in the same file correctly uses
Some(solver_types::AuthConfig { ... }).Proposed fix
- AuthConfig { + Some(AuthConfig { enabled: auth_enabled.unwrap_or(false), jwt_secret: SecretString::new(jwt_secret), access_token_expiry_hours: 1, refresh_token_expiry_hours: 720, // 30 days issuer: "oif-solver".to_string(), admin: admin_config, - } + })
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/solver-service/src/config_merge.rs (1)
2853-2890: 🛠️ Refactor suggestion | 🟠 MajorMissing test for the core new scenario: API auth enabled without admin.
The PR's main addition is the ability to enable API auth independently of admin. Yet there's no test for
admin.enabled = false, auth_enabled = true. This case should produceauth = Some(AuthConfig { enabled: true, admin: None, … }). Without it, the primary feature path is untested.Suggested test
#[test] fn test_build_api_config_from_operator_auth_enabled_without_admin() { let admin = OperatorAdminConfig { enabled: false, domain: "".to_string(), chain_id: 0, nonce_ttl_seconds: 0, admin_addresses: vec![], withdrawals: OperatorWithdrawalsConfig::default(), }; let api = build_api_config_from_operator(&admin, true); assert!(api.enabled); let auth = api.auth.as_ref().expect("auth should be Some when auth_enabled=true"); assert!(auth.enabled); assert!(auth.admin.is_none(), "admin should be None when admin.enabled=false"); }
🧹 Nitpick comments (1)
crates/solver-service/src/config_merge.rs (1)
848-896: Auth wiring looks correct — the two concerns (admin vs API auth) are properly separated.The OR gate on line 849 ensures the JWT infrastructure (secret, expiry, issuer) is provisioned whenever either admin or API auth is needed, while
AuthConfig.enabledon line 871 independently controls JWT enforcement on the/ordersendpoint.AdminConfigpresence is gated solely byadmin.enabled. This is consistent with the comment on lines 1340-1342.One gap: there's no test for
admin.enabled = true, auth_enabled = false— the scenario where admin wallet-signature auth is active but JWT on/ordersis off. Consider adding one to lock in that theauthblock isSome(…)withenabled == falseandadmin == Some(…).
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
- Added new environment variables for JWT and client credentials. - Introduced endpoint for issuing access tokens using client credentials. - Updated README and API specifications to reflect new authentication flow. - Improved configuration handling for authentication settings.
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 (3)
README.md (1)
893-899:⚠️ Potential issue | 🟡 MinorFix invalid JSON in example request body.
The
userfield is missing the closing quote, which breaks copy‑paste usage.✅ Proposed fix
- "user": "0x74..., + "user": "0x74...",api-spec/auth-api.yaml (2)
22-26:⚠️ Potential issue | 🟡 MinorScope list is inconsistent between documentation and schema.
The "Scopes" documentation section (lines 22–26) lists only
read-orders,create-orders, andadmin-all. However, theRegisterRequest.scopesenum at line 352 also includesread-quotesandcreate-quotes. Additionally, the inline description ofscopes(lines 346–349) only documentsread-ordersandcreate-orders.Either add
read-quotesandcreate-quotesto the top-level Scopes documentation and the inline description, or remove them from the enum if they're not yet supported.Also applies to: 343-353
415-424:⚠️ Potential issue | 🟡 Minor
ErrorResponseschema doesn't includeerror_descriptionused by/auth/tokenexamples.The
ErrorResponseschema (lines 415–424) only defines anerrorproperty. However, the/auth/tokenerror examples (e.g., lines 87–88, 100–101, 109–110) return botherroranderror_descriptionfields. Either adderror_descriptionas an optional property toErrorResponse, or align the examples with the existing schema.Proposed fix — add `error_description` to ErrorResponse
ErrorResponse: type: object description: Error response for authentication failures required: - error properties: error: type: string description: Error message example: "Client ID cannot be empty" + error_description: + type: string + description: Human-readable explanation of the error + example: "grant_type must be client_credentials"Also applies to: 85-110
🤖 Fix all issues with AI agents
In `@api-spec/auth-api.yaml`:
- Around line 279-299: TokenRequest schema currently has no required array so
all properties are optional; update the TokenRequest definition to include a
required array with at least ["grant_type","client_id","client_secret"]
(optionally include "scope" if you want to enforce it) so OpenAPI validators and
code generators will enforce the client-credentials fields; locate the
TokenRequest object in the spec and add the required property next to
type/description/properties to list the required property names.
In `@README.md`:
- Around line 239-251: The README mixes two config key names—auth_enabled and
auth.enabled—which can cause misconfigurations; choose one canonical key (e.g.,
auth_enabled) and update all occurrences in the document to that single name
(including the admin note and the other mentions referenced in the comment), and
clarify in the nearby text that this key is used in seed overrides to enable API
auth so the docs are consistent and unambiguous.
🧹 Nitpick comments (4)
crates/solver-service/src/config_merge.rs (1)
2948-2996: Environment variable mutation in tests is inherently racy.These tests use
env::set_var/env::remove_varon shared process-wide state. Whencargo testruns tests in parallel (the default), concurrent tests that also read these env vars (e.g.,JWT_SECRET,AUTH_CLIENT_SECRET) can interfere with each other, causing flaky failures.Consider using
#[serial]from theserial_testcrate for tests that mutate environment variables, or extract the env-dependent logic behind a trait/closure so tests can inject values without touching the process environment.crates/solver-service/src/apis/auth.rs (3)
585-590: The length check beforect_eqleaks secret length via timing.The early return on length mismatch defeats the purpose of constant-time comparison—an attacker can binary-search the expected secret's length by timing responses. Since
ct_eqalready returnsfalsefor different-length slices (it compares byte-by-byte after asserting equal length internally), you can remove the length guard and pad/hash both sides, or simply accept the minor leak given the 32-char minimum.A more robust approach:
🔒 Proposed fix using HMAC-based comparison
+use hmac::{Hmac, Mac}; +use sha2::Sha256; + fn secrets_match(provided: &str, expected: &str) -> bool { - if provided.len() != expected.len() { - return false; - } - provided.as_bytes().ct_eq(expected.as_bytes()).into() + // Use HMAC comparison to avoid leaking length information + type HmacSha256 = Hmac<Sha256>; + let mut mac = HmacSha256::new_from_slice(expected.as_bytes()) + .expect("HMAC accepts any key length"); + mac.update(provided.as_bytes()); + // verify_slice uses constant-time comparison internally + mac.verify_slice( + &HmacSha256::new_from_slice(expected.as_bytes()) + .expect("HMAC accepts any key length") + .chain_update(expected.as_bytes()) + .finalize() + .into_bytes(), + ) + .is_ok() }That said, given the 32-char minimum enforcement and rate limiting in place, the practical risk is very low. This could be deferred.
28-29: In-memory rate limit stores grow unbounded under sustained attack.
FAILED_AUTH_BY_CLIENTandFAILED_AUTH_BY_IPareDashMap<String, VecDeque<i64>>that only prune timestamps from within each entry on access. Entries for distinct client IDs or IPs are never removed. Under a distributed brute-force attack from many unique IPs, the map grows without bound.Consider adding periodic cleanup (e.g., a background task or a max-size eviction policy) or switching to a bounded LRU/TTL cache. For a pilot deployment this is likely fine, but worth tracking.
Also applies to: 558-583
540-556:extract_source_ipis spoofable and falls back to a shared bucket.Two concerns:
- When no proxy sets
x-forwarded-fororx-real-ip, all clients map to"unknown", sharing a single IP-rate-limit bucket of 120 req/min. This effectively becomes a global limit.- Without a trusted proxy stripping client-supplied headers, attackers can forge
x-forwarded-forto spread their attempts across many fake IPs, bypassing IP-based rate limiting entirely.If the service is always behind a reverse proxy (e.g., nginx, cloud LB), consider using Axum's
ConnectInfoto get the peer address as a fallback, or document the proxy requirement.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api-spec/auth-api.yaml (2)
419-428:⚠️ Potential issue | 🟠 Major
ErrorResponseschema is missing theerror_descriptionfield used in/auth/tokenexamples.The
/auth/tokenerror examples (lines 87–88, 91–92, 100–101, 109–110) include anerror_descriptionfield, but theErrorResponseschema only defineserror. This mismatch means validators and code generators won't includeerror_descriptionin the generated types.Since
error+error_descriptionis the standard pattern from OAuth 2.0 (RFC 6749 §5.2), adderror_descriptionas an optional property.Proposed fix: add `error_description` to ErrorResponse
ErrorResponse: type: object description: Error response for authentication failures required: - error properties: error: type: string description: Error message example: "Client ID cannot be empty" + error_description: + type: string + description: Optional human-readable description providing additional detail about the error + example: "grant_type must be client_credentials"
22-26:⚠️ Potential issue | 🟡 MinorScopes documentation is inconsistent with the
RegisterRequestenum.The "Scopes" section (lines 22–26) documents three scopes:
read-orders,create-orders, andadmin-all. However, theRegisterRequest.scopesenum on line 356 includesread-quotesandcreate-quotes, which are not documented anywhere in the spec.Either add the missing scopes to the documentation section or remove them from the enum if they're not yet supported.
Option A: Update the Scopes documentation
## Scopes - `read-orders`: Read access to order information (GET /orders/{id}) - `create-orders`: Permission to create new orders (POST /orders) + - `read-quotes`: Read access to quote information + - `create-quotes`: Permission to create new quotes - `admin-all`: Full administrative access to all endpointsOption B: Remove undocumented scopes from the enum
items: type: string - enum: ["read-orders", "create-orders", "read-quotes", "create-quotes"] + enum: ["read-orders", "create-orders"] example: ["read-orders", "create-orders"]Also applies to: 356-356
🤖 Fix all issues with AI agents
In `@api-spec/auth-api.yaml`:
- Around line 300-303: The description for the 'scope' field on the /auth/token
endpoint is ambiguous about omission; update the 'scope' description for the
field in api-spec/auth-api.yaml to state the default behavior when omitted
(e.g., "If omitted, defaults to 'admin-all' and will be issued with admin-all
privileges") and, if applicable, also add an allowed enum or note what error
(e.g., 400) the server returns when an unsupported value is provided so
implementers know whether omission is accepted or rejected.
In `@crates/solver-service/src/apis/auth.rs`:
- Around line 1067-1071: Tests manipulate global DashMap state
(FAILED_AUTH_BY_CLIENT, FAILED_AUTH_BY_IP) causing flakiness when run in
parallel; fix by either marking tests that touch these globals with
#[serial_test::serial] (apply to functions like
test_issue_client_token_success_with_basic_auth and the other listed tests) so
they run sequentially, or refactor the rate-limit storage out of globals into an
injectable dependency (e.g., add a field to State or pass a RateLimitStore into
the auth functions/handlers and update tests to construct isolated instances) so
each test gets its own isolated maps instead of clearing shared globals.
- Around line 400-411: The code is incorrectly defaulting request.grant_type to
"client_credentials"; change the handling so grant_type is treated as required:
if request.grant_type is None or empty after trimming, return an
oauth_error_response with StatusCode::BAD_REQUEST and error "invalid_request"
(or "unsupported_grant_type" per your conventions) indicating grant_type is
required; otherwise compare the trimmed value to "client_credentials" and return
the existing error response path if it differs. Update references around
grant_type, request.grant_type, and the oauth_error_response call accordingly.
- Around line 526-538: The extract_basic_credentials function rejects valid
credentials when the auth scheme case differs because it uses
strip_prefix("Basic "); change the logic to compare the scheme
case-insensitively (e.g., convert the scheme portion to ASCII lowercase and
check for "basic") or split the header on the first ASCII whitespace to separate
scheme and credentials, verify scheme.eq_ignore_ascii_case("basic"), then decode
the remainder exactly as before; keep the same validation of
client_id/client_secret and return Some((client_id.to_string(),
client_secret.to_string())) only on success.
- Around line 585-590: The function secrets_match currently leaks the secret
length via the early length check and then relies on ct_eq which short-circuits
on unequal lengths; remove the length check in secrets_match and instead
normalize both inputs to a fixed-size cryptographic representation (e.g.,
compute SHA-256 of provided and expected using sha2::Sha256 or HMAC-SHA256) and
then perform a constant-time comparison on the fixed-size digests (use the
subtle::ConstantTimeEq/ct_eq on the digest byte arrays). Update secrets_match to
hash both strings to 32-byte digests and compare those, ensuring no early
returns based on length.
🧹 Nitpick comments (3)
api-spec/auth-api.yaml (1)
216-251:/auth/refreshresponse reusesRegisterResponse— verify this is intentional.The refresh endpoint (line 243) references
RegisterResponseas the response schema, which includesclient_id,scopes, and both token expiry timestamps. This works if the refresh response always mirrors the registration response shape, but the schema nameRegisterResponseis somewhat misleading when used for refresh responses.Consider renaming to something like
AuthTokensResponseto better reflect its dual use, or this is fine if you prefer keeping the schema count small.crates/solver-service/src/apis/auth.rs (2)
28-29: Global rate-limit maps never evict empty entries — unbounded key growth.
FAILED_AUTH_BY_CLIENTandFAILED_AUTH_BY_IPonly prune timestamps from the innerVecDeque, but theDashMapkey itself is never removed once the deque is empty. Under sustained probing from diverse client IDs or spoofed IPs, key count grows without bound — a slow memory leak.Consider a periodic background task or an
retain-based sweep (e.g., on every Nth request) to drop entries whoseVecDequeis empty after pruning. Alternatively, use a crate likemokawith TTL-based eviction.
540-556:X-Forwarded-Foris client-controllable — IP-based rate limiting is bypassable without a trusted proxy.If the service is exposed without a reverse proxy that sanitizes
X-Forwarded-For, an attacker can spoof arbitrary values on each request to evict IP-based rate limiting entirely. Consider using axum'sConnectInfoextractor to obtain the peer address, or document that a trusted proxy must set these headers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/solver-service/src/apis/auth.rs`:
- Around line 567-592: The extract_source_ip function currently trusts
X-Forwarded-For/X-Real-IP headers unconditionally (affecting
TOKEN_RATE_LIMIT_IP_PER_MINUTE); update it to only read these headers when the
incoming connection is from a trusted proxy—add a configurable trusted proxy
list or boolean (e.g., TrustedProxyIps or trust_forwarded_headers) and check
peer_addr.ip() against that list before using X-Forwarded-For/X-Real-IP,
otherwise always use peer_addr.ip() (or "unknown"); also document the
expectation that the service must sit behind a proxy that overwrites those
headers if trust is enabled and add a defensive comment in extract_source_ip
referencing TOKEN_RATE_LIMIT_IP_PER_MINUTE.
🧹 Nitpick comments (2)
crates/solver-service/src/apis/auth.rs (2)
615-630: Minor TOCTOU inrecord_failed_attempt, but bounded by DashMap safety.Lines 616 and 619 each call
store.contains_key(key)separately from the eventualstore.entry(...)on line 624. Between these checks another thread could insert a new key, potentially pushing the store slightly overMAX_RATE_LIMIT_KEYS. Since DashMap is safe against data races and the overshoot is bounded (at most one extra key per concurrent call), this isn't a correctness bug — just worth noting that the cap is approximate rather than strict.
433-440: Defaultingscopeto"admin-all"silently grants maximum privileges.When
scopeis omitted, the endpoint defaults toadmin-all(line 433). While this is documented and the endpoint is for privileged clients, silently granting the highest privilege level when a field is missing goes against the principle of least privilege. Consider requiringscopeexplicitly, similar to howgrant_typeis now required.Proposed fix
- let requested_scope = request.scope.as_deref().unwrap_or("admin-all").trim(); + let requested_scope = match request.scope.as_deref() { + Some(s) if !s.trim().is_empty() => s.trim(), + _ => { + return oauth_error_response( + StatusCode::BAD_REQUEST, + "invalid_scope", + "scope is required", + ); + }, + };
shahnami
left a comment
shahnami
left a comment
There was a problem hiding this comment.
LGTM, just few comments
…authentication handling
There was a problem hiding this comment.
Wouldn't you just use https://crates.io/crates/siwe?
shahnami
left a comment
shahnami
left a comment
There was a problem hiding this comment.
LGTM. but I would just use siwe crate instead of a custom impl.
3 participants
Summary
Testing Process
Checklist
Summary by CodeRabbit
New Features
Chores
Documentation