Conversation
|
Warning Rate limit exceeded@twangodev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds optional HMAC-SHA256 query-signature authentication: new validator and tests, Axum middleware, config/settings and AppState wiring, route-layer protection and OpenAPI updates, dependency additions, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Router
participant HmacMiddleware
participant Validator
participant Handler
Client->>Router: GET /generate?url=...&signature=...
Router->>HmacMiddleware: enter protected layer
HmacMiddleware->>Validator: validate(query_string)
alt signature present and valid
Validator-->>HmacMiddleware: Ok
HmacMiddleware->>Handler: proceed
Handler-->>Client: 200 OK
else missing signature
Validator-->>HmacMiddleware: Err(MissingSignature)
HmacMiddleware-->>Client: 401 Unauthorized
else invalid hex format
Validator-->>HmacMiddleware: Err(InvalidHexFormat)
HmacMiddleware-->>Client: 400 Bad Request
else invalid signature
Validator-->>HmacMiddleware: Err(InvalidSignature)
HmacMiddleware-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR implements optional HMAC-SHA256 authentication for the API. When OGIS_HMAC_SECRET is configured, requests to the main image generation endpoint (/) require a valid HMAC signature computed over the canonical query string (alphabetically sorted parameters excluding the signature itself).
Key Changes:
- New authentication middleware that validates HMAC signatures on protected routes
- HMAC validator using SHA256 with constant-time comparison
- Configuration support for optional HMAC secret key via environment variable
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/auth/mod.rs | New authentication module exports |
| src/auth/middleware.rs | Axum middleware for HMAC validation |
| src/auth/hmac.rs | HMAC validator implementation with error handling and tests |
| src/routes/mod.rs | Splits routes into protected (with HMAC) and public paths |
| src/main.rs | Initializes HMAC validator from config and adds to AppState |
| src/config.rs | Adds HMAC settings with secret configuration |
| src/params.rs | Adds optional signature parameter to request params |
| src/routes/index.rs | Updates OpenAPI documentation for new auth responses |
| Cargo.toml | Adds hmac, sha2, hex, and thiserror dependencies |
| .env.example | Documents HMAC secret configuration option |
| CLAUDE.md | Documents HMAC authentication feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cargo.toml (1)
4-4: Critical: Invalid Rust edition.Rust edition "2024" does not exist. Valid editions are 2015, 2018, and 2021. This will cause compilation to fail.
Apply this diff to fix:
-edition = "2024" +edition = "2021"
🧹 Nitpick comments (3)
.env.example (1)
21-21: Consider adding the secret generation command in the comment.The placeholder value is clear, but users might benefit from seeing the generation command here (as mentioned in src/config.rs:
openssl rand -hex 32).Consider this improvement:
-#OGIS_HMAC_SECRET=your-secret-key-here +#OGIS_HMAC_SECRET=your-secret-key-here # Generate with: openssl rand -hex 32CLAUDE.md (1)
47-54: LGTM! Clear documentation of the HMAC feature.The documentation accurately describes the HMAC authentication feature with key details about its optional nature, algorithm, and canonical query string format.
Consider adding a brief example of the canonical query string format to help developers understand the signing process:
### HMAC Authentication - Optional signature-based authentication using HMAC-SHA256 - Enabled when `OGIS_HMAC_SECRET` is set - Signature computed over canonical query string (params sorted alphabetically, excluding signature param) - Example: `description=Hello&title=World` → HMAC signature - Code in `src/auth/` modulesrc/config.rs (1)
102-112: Consider validating secret format and length.The current implementation doesn't validate the secret's format or enforce minimum length requirements. While the suggestion to use
openssl rand -hex 32produces a 64-character hex string (32 bytes), users might provide weaker secrets.Consider adding validation in
secret_bytes()or during Config parsing:impl HmacSettings { /// Check if HMAC authentication is enabled pub fn is_enabled(&self) -> bool { self.secret.is_some() } /// Get secret as bytes (returns None if not enabled or invalid) /// Validates that secret is at least 32 characters (16 bytes of entropy if hex) pub fn secret_bytes(&self) -> Option<Vec<u8>> { self.secret.as_ref().and_then(|s| { if s.len() < 32 { tracing::warn!("HMAC secret too short (minimum 32 characters recommended)"); return None; } Some(s.as_bytes().to_vec()) }) } }Alternatively, validate during application startup in main.rs and fail fast with a clear error message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.env.example(1 hunks)CLAUDE.md(1 hunks)Cargo.toml(1 hunks)src/auth/hmac.rs(1 hunks)src/auth/middleware.rs(1 hunks)src/auth/mod.rs(1 hunks)src/config.rs(2 hunks)src/main.rs(4 hunks)src/params.rs(1 hunks)src/routes/index.rs(1 hunks)src/routes/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/routes/mod.rs (3)
src/auth/middleware.rs (1)
hmac_auth_middleware(11-33)src/routes/index.rs (2)
new(25-39)generate(95-190)src/routes/health.rs (1)
health_check(12-15)
src/auth/mod.rs (1)
src/auth/middleware.rs (1)
hmac_auth_middleware(11-33)
🔇 Additional comments (17)
Cargo.toml (2)
13-13: LGTM! Standard dependencies for HMAC implementation.The added dependencies (hex, hmac, sha2, thiserror) are appropriate for HMAC-SHA256 authentication. The versions selected are current and widely used.
Also applies to: 15-16, 24-25
16-16: Update is compatible with infer 0.19.0—no breaking changes detected.The infer crate in version 0.19.0 includes the same
infer::get()function andmime_type()method used in the codebase. Both usages insrc/image/validate.rsandsrc/image/mod.rsalign with the current API. No breaking changes were found across the version bump.Note: Since infer is a 0.x crate, minor version updates carry inherent instability risk per SemVer. The upgrade appears safe based on API compatibility, but monitor for any runtime behavior changes in testing.
.env.example (1)
3-3: LGTM! Appropriate default log level.Changing the default from
debugtoinfois a better production default while still providing useful operational visibility.src/routes/index.rs (1)
89-90: LGTM! Appropriate HTTP status codes for authentication.The updated OpenAPI documentation correctly reflects the HMAC authentication feature:
- 400 for malformed input (including invalid signature format)
- 401 for authentication failures (missing or invalid signatures)
This follows RFC 7235 conventions for authentication status codes.
src/routes/mod.rs (2)
12-28: LGTM! Well-structured router with proper authentication layering.The router architecture correctly separates concerns:
- Protected routes (/) with HMAC middleware
- Public routes (/health) without authentication
- Public Swagger UI documentation (/docs)
The state cloning for middleware is appropriate since AppState is Arc-based (cheap to clone). The merge order ensures middleware is properly applied before state attachment.
14-14: Current route protection is correctly scoped and intentional.The codebase intentionally separates protected and public routes:
- Protected: "/" (with HMAC authentication middleware)
- Public: "/health" (monitoring endpoint)
This architecture is sound and follows standard practices. The health check endpoint is appropriately public, and the main route is properly protected. No additional routes are present that would require authentication evaluation.
src/params.rs (1)
31-35: LGTM! Proper integration of signature parameter.The signature field is correctly added with appropriate annotations:
Option<String>allows optional authenticationserde(default)handles missing values#[allow(dead_code)]is justified since middleware consumes this before the handler- Properly excluded from validation and empty checks
src/auth/mod.rs (1)
1-5: LGTM! Clean module organization.The module follows Rust conventions with clear separation of concerns:
hmacmodule for validation logicmiddlewaremodule for Axum integration- Public re-exports make the API surface explicit
Note: The core implementation files (hmac.rs and middleware.rs) are referenced in the AI summary but not included in this review. Ensure those files are properly reviewed for security and correctness, particularly:
- Constant-time signature comparison (timing attack resistance)
- Proper canonical query string construction
- Error handling and logging
src/config.rs (1)
93-100: LGTM! Clear configuration structure for HMAC settings.The HmacSettings struct is well-designed:
- Optional secret enables feature toggling
- Clear documentation with generation command
- Environment variable naming follows project conventions
src/auth/middleware.rs (1)
11-33: LGTM! Well-structured middleware implementation.The middleware correctly implements HMAC authentication with proper error handling and pass-through behavior when authentication is disabled. The use of
unwrap_or("")for missing query strings is safe, and error responses appropriately use the status codes fromHmacError.src/main.rs (2)
27-27: LGTM! Appropriate use ofOption<Arc<...>>for optional shared state.The optional validator pattern allows graceful degradation when HMAC authentication is disabled, and
Arcenables efficient sharing across request handlers.
58-69: No action required—the.expect()call is safe.The contract between
is_enabled()andsecret_bytes()is guaranteed by the implementation. Both methods operate on the same immutable field (self.secret):
is_enabled()returnsself.secret.is_some()secret_bytes()returnsself.secret.as_ref().map(...), which returnsSomewhen the field isSomeWhen
is_enabled()returnstrue,secret_bytes()will always returnSome, so the.expect()will never panic.src/auth/hmac.rs (5)
9-29: LGTM! Appropriate error types and status code mappings.The error enum correctly distinguishes between authentication failures (401) and malformed requests (400), with proper use of
thiserrorfor ergonomic error handling.
31-38: LGTM! Simple and appropriate validator structure.The
Vec<u8>type for the secret is correct for arbitrary byte sequences.
80-88: LGTM! Appropriate test-only helper.The
signmethod is correctly gated with#[cfg(test)]and provides a useful utility for test cases. Note that it signs the input string as-is, so tests correctly pass canonical query strings to it.
90-152: LGTM! Comprehensive test coverage.The test suite covers all major code paths: successful validation, missing signature, invalid format, wrong signature, and canonical query construction with proper sorting. Well done!
52-68: The current implementation is correct and the review concern is based on a misunderstanding.The code correctly implements HMAC-SHA256 over a decoded canonical query string. Here's why:
form_urlencoded::parse()decodes URL-encoded values, storing them as decoded strings in the BTreeMap.The canonical form is explicitly documented in the algorithm comments to use decoded values: "Canonical: description=World&title=Hello"
Test cases confirm clients must compute signatures over the decoded canonical form, not the raw URL-encoded query string.
CLAUDE.md documents: "Signature computed over canonical query string (params sorted alphabetically, excluding signature param)"—this refers to the decoded form.
The proposed code fix would be incorrect because it would add URL encoding to the canonical form, creating a cryptographically different value and breaking all existing valid clients that follow the documented behavior.
Minor improvement opportunity: The README mentions HMAC but lacks client-side signing guidance. The algorithm is well-documented in the docstring, but external API consumers might benefit from explicit examples in the README about how to generate valid signatures.
Likely an incorrect or invalid review comment.
…on prevention tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/auth/hmac.rs (1)
87-92: Consider clarifying the expected input format.The parameter name
query_stringmight suggest any query string is acceptable, but the method expects a canonical query string (sorted, no signature parameter). Consider renaming tocanonical_queryor adding a doc comment to clarify this expectation.- pub fn sign(&self, query_string: &str) -> String { + /// Generate signature for a canonical query string (sorted params, no signature field). + pub fn sign(&self, canonical_query: &str) -> String { let mut mac = HmacSha256::new_from_slice(&self.secret).expect("HMAC can take key of any size"); - mac.update(query_string.as_bytes()); + mac.update(canonical_query.as_bytes()); hex::encode(mac.finalize().into_bytes()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/auth/hmac.rs(1 hunks)
🔇 Additional comments (4)
src/auth/hmac.rs (4)
1-8: LGTM! Clean imports and type alias.The imports cover all necessary dependencies, and the type alias improves code readability.
9-29: LGTM! Well-structured error handling.The error variants cover all failure modes appropriately, and the HTTP status code mappings are sensible (401 for auth failures, 400 for malformed input).
72-83: LGTM! URL encoding properly implemented.The use of
url::form_urlencoded::Serializercorrectly addresses the previous review concern about URL-encoding parameter keys and values. This prevents injection attacks where special characters like&or=could break the canonical format.
54-56: The review comment is incorrect; no changes are needed.The current implementation is sound. The protocol requires clients to canonicalize queries before signing—the test at line 100-109 demonstrates this:
validator.sign(canonical)receives a pre-canonicalized string. On the server side,validate()canonicalizes before verification, using identical logic (BTreeMap sorts and deduplicates). Both sides deduplicate consistently, so signatures match correctly.Silently keeping the last value of duplicate parameters is acceptable and common practice across web frameworks. The asymmetry between
sign()andvalidate()is intentional:sign()is client-side and expects pre-canonical input, whilevalidate()normalizes on the server. This design prevents signature forgery and maintains protocol integrity. There is no security vulnerability here.Likely an incorrect or invalid review comment.
…eware application
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/config.rs (1)
102-115: Implementation is correct; consider clarifying hex string usage.The methods work correctly. Note that
openssl rand -hex 32generates a 64-character hex string (e.g., "a1b2c3..."), which is used directly as the HMAC key (64 ASCII bytes) rather than being decoded to 32 random bytes. This is perfectly valid for HMAC—just ensure consistency between signing and validation.If you intended to use the decoded 32-byte value, you'd need to add hex decoding. Otherwise, the current approach is fine.
Optional: If hex decoding was intended, add a dependency like
hexand decode the secret:pub fn secret_bytes(&self) -> Option<Vec<u8>> { self.secret .as_ref() .filter(|s| !s.is_empty()) .and_then(|s| hex::decode(s).ok()) }Otherwise, consider updating the documentation to clarify:
- /// HMAC secret key for request signing (if set, auth is required) - /// Generate with: openssl rand -hex 32 + /// HMAC secret key for request signing (if set, auth is required) + /// Generate with: openssl rand -hex 32 + /// Note: The hex string is used directly as the key (64 bytes), not decoded
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/config.rs(2 hunks)
🔇 Additional comments (2)
src/config.rs (2)
93-100: LGTM! Clean struct definition for optional HMAC configuration.The struct is well-documented and properly integrates with clap for CLI/env configuration. The optional secret field appropriately enables opt-in HMAC authentication.
136-137: LGTM! Clean integration of HMAC settings.The flattened
hmacfield properly integrates the new settings into the existing configuration structure.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/params.rs:45
- The
is_emptymethod does not check thesignaturefield, which means a request with only a signature parameter (e.g.,?signature=abc123) will be considered 'empty' and trigger default value behavior. This is likely unintended as signature-only requests should fail validation. Include the signature field in the emptiness check:&& self.signature.is_none().
fn is_empty(&self) -> bool {
self.title.is_none()
&& self.description.is_none()
&& self.subtitle.is_none()
&& self.logo.is_none()
&& self.image.is_none()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let params: BTreeMap<String, String> = url::form_urlencoded::parse(query_string.as_bytes()) | ||
| .into_owned() | ||
| .collect(); |
There was a problem hiding this comment.
Duplicate query parameter handling is undefined. When parsing query strings into a BTreeMap, if a parameter appears multiple times (e.g., ?title=A&title=B), only one value will be retained (the behavior depends on iteration order). This could allow signature bypasses where an attacker adds duplicate parameters. Consider validating that parameters don't appear multiple times, or use a different data structure like HashMap<String, Vec<String>> and reject requests with duplicate keys.
| /// HMAC signature (required when authentication is enabled) | ||
| #[serde(default)] | ||
| #[schema(example = "a1b2c3d4e5f6...")] | ||
| #[allow(dead_code)] // Used by middleware, not by route handler |
There was a problem hiding this comment.
The comment 'Used by middleware, not by route handler' is misleading. The signature field is not actually used by the middleware - the middleware reads the raw query string directly from the request URI. The field exists for API documentation purposes (OpenAPI schema). Consider updating the comment to reflect this: 'Used in OpenAPI schema documentation, not by code'.
| #[allow(dead_code)] // Used by middleware, not by route handler | |
| #[allow(dead_code)] // Used in OpenAPI schema documentation, not by code |
Summary by CodeRabbit
New Features
Documentation
Chores