Improve token middleware time handling and signature body parsing #88
Conversation
|
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. WalkthroughReplaces full-body JSON reads with streaming decoding (wrapped by a MaxBytesReader) in the signatures handler, updates parse/encode error handling, and adds tests for parse/unknown-field/oversize cases. The token middleware now uses an injectable clock ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Handler as SignaturesHandler
participant MaxReader as baseHttp.MaxBytesReader
participant Decoder as json.Decoder
participant Validator as s.Validator
participant Encoder as json.Encoder
Client->>Handler: POST /signatures (JSON)
Handler->>MaxReader: Wrap r.Body (MaxRequestSize)
Handler->>Decoder: json.NewDecoder(MaxReader).DisallowUnknownFields()
Decoder-->>Handler: Decode into payload.SignatureRequest
alt Decode fails
Handler-->>Client: 400 "could not parse the given data."
else Decode succeeds
Handler->>Validator: s.Validator.Rejects(req)?
alt Rejected
Handler-->>Client: 400 validation error(s)
else Accepted
Handler->>Encoder: Encode response
alt Encode fails
Handler-->>Client: 500 LogInternalError("could not encode signatures response")
else Success
Handler-->>Client: 200 JSON response
end
end
end
sequenceDiagram
autonumber
participant MW as TokenMiddleware
participant Clock as t.now()
participant Verifier as SignatureVerifier
MW->>Clock: now()
Clock-->>MW: current time
MW->>Verifier: HasInvalidSignature(token, current time)
Verifier-->>MW: invalid? (bool)
MW-->>Caller: allow or reject request
note right of MW: Clock is injectable (default: time.Now)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @gocanto, I'm Gemini Code Assist1! 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 robustness and testability of the API by improving how request bodies are handled and how time is managed within the token middleware. It introduces more efficient and secure body parsing for signature requests and allows for more flexible time handling in token validation.
Highlights
- Configurable Time in Token Middleware: The token middleware now allows injecting a configurable time source for signature lookups, enhancing testability and control over time-sensitive operations.
- Improved Signature Body Parsing: The signature request body parsing has been refactored to stream and limit the input, preventing large payload attacks and surfacing encoding errors more explicitly.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable improvements. The change to stream and limit the request body in the signatures handler is a great enhancement for both performance and security, preventing potential DoS attacks via large payloads. The fix to correctly propagate JSON encoding errors as internal server errors is a crucial correctness improvement. Furthermore, injecting a time function into the token middleware is a good design choice that significantly improves the testability of time-sensitive logic. The changes are well-focused and align with Go best practices.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/middleware/token_middleware.go (1)
214-219: Injectable clock is good; add nil-safe fallback to prevent panics when zero-initialized.
t.now()will panic ifTokenCheckMiddleware{}is constructed withoutnow(seen in tests creating a zero value). Use a fallback totime.Nowand reuse it wherever a clock is needed.Apply within this hunk:
- ServerTime: t.now(), + ServerTime: t.currentTime(),Add helper (outside this hunk):
func (t TokenCheckMiddleware) timeNow() func() time.Time { if t.now != nil { return t.now } return time.Now } func (t TokenCheckMiddleware) currentTime() time.Time { return t.timeNow()() }And update timestamp validation to use the same fallback (outside this hunk):
vt := NewValidTimestamp(headers.Timestamp, t.timeNow())
🧹 Nitpick comments (1)
handler/signatures.go (1)
35-38: Var block consolidation looks fine. Also consider injecting time for determinism.LGTM. Optional: mirror the middleware pattern by injecting a time source into the handler to ease testing and avoid
time.Now()flakiness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
handler/signatures.go(2 hunks)pkg/middleware/token_middleware.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/middleware/token_middleware.go (3)
pkg/middleware/token_middleware_additional_test.go (2)
TestTokenMiddleware_SignatureMismatch(108-122)TestTokenMiddleware_PublicTokenMismatch(93-106)pkg/middleware/valid_timestamp.go (1)
ts(11-14)pkg/middleware/token_middleware_test.go (2)
TestTokenMiddlewareErrors(26-64)TestTokenMiddleware_RejectsFutureTimestamps(385-442)
handler/signatures.go (3)
handler/payload/signatures.go (2)
SignatureRequest(3-9)Signature(11-15)pkg/http/request.go (1)
MaxRequestSize(10-10)pkg/http/response.go (1)
LogInternalError(76-83)
🔇 Additional comments (1)
handler/signatures.go (1)
74-77: Good: propagate JSON encode failures as 500 with logging.Returning
http.LogInternalError("could not encode signatures response", err)is the right behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
handler/signatures.go (1)
75-78: Remove duplicate logging; LogInternalError already logs.This avoids double logs. Also drop the slog import.
- if err = resp.RespondOk(response); err != nil { - slog.Error("Error marshaling JSON for signatures response", "error", err) - return http.LogInternalError("could not encode signatures response", err) - } + if err = resp.RespondOk(response); err != nil { + return http.LogInternalError("could not encode signatures response", err) + }And at the top:
-import ( - "encoding/json" - "fmt" - "log/slog" +import ( + "encoding/json" + "fmt"
🧹 Nitpick comments (7)
pkg/middleware/token_middleware_test.go (3)
248-248: Use the same base time for request and expiry to avoid flakiness.Minor: prefer reusing the local now to derive expiry.
- seedSignature(t, repo, apiKey, req, time.Now().Add(time.Hour)) + seedSignature(t, repo, apiKey, req, now.Add(time.Hour))
327-335: Same nit: derive expiry from a captured now.Keeps the request/expiry relation stable within the test.
- req := makeSignedRequest(t, + now := time.Now() + req := makeSignedRequest(t, http.MethodPost, "https://api.test.local/v1/resource?b=2&a=1", "{\"x\":123}", seed.AccountName, seed.PublicKey, seed.PublicKey, - time.Now(), + now, "n-happy-1", "rid-happy-1", ) - seedSignature(t, repo, apiKey, req, time.Now().Add(time.Hour)) + seedSignature(t, repo, apiKey, req, now.Add(time.Hour))
381-401: Optional: strengthen future‐timestamp test Since timestamp validation runs before signature lookup (lines 67–71), seeding a valid API key/signature isn’t required today—but adding it (viarepo.Create+seedSignature) or asserting the timestamp-specificApiError.Codemakes the test unambiguously target the timestamp check.pkg/middleware/token_middleware_additional_test.go (2)
212-235: Custom clock test is solid and exercises the injected time source.Nice coverage of tm.now. Suggest also adding a boundary case (exact skew) in a follow-up.
25-86: Test infra duplication: consolidate DB/container helpers.makeRepo here and setupDB in token_middleware_test.go overlap. Consider a shared testutil (e.g., pkg/testutil/dbtest) to reduce duplication and container setup variance.
handler/signatures.go (1)
39-44: Return 413 Payload Too Large for oversized bodies.Differentiate decoding errors from MaxBytesReader overflows to surface the correct status.
- dec := json.NewDecoder(r.Body) + dec := json.NewDecoder(r.Body) dec.DisallowUnknownFields() - if err = dec.Decode(&req); err != nil { - return http.LogBadRequestError("could not parse the given data.", err) - } + if err = dec.Decode(&req); err != nil { + if _, ok := err.(*baseHttp.MaxBytesError); ok { + return &http.ApiError{ + Message: "request body too large", + Status: baseHttp.StatusRequestEntityTooLarge, + } + } + return http.LogBadRequestError("could not parse the given data.", err) + }If you adopt this, update tests to expect 413.
handler/signatures_test.go (1)
39-47: If handler returns 413 for oversized bodies, align the assertion.Only if you adopt the 413 change above.
- if err := h.Generate(rec, req); err == nil || err.Status != nethttp.StatusBadRequest { + if err := h.Generate(rec, req); err == nil || err.Status != nethttp.StatusRequestEntityTooLarge { t.Fatalf("expected body too large error, got %#v", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
handler/signatures.go(2 hunks)handler/signatures_test.go(1 hunks)pkg/middleware/token_middleware_additional_test.go(4 hunks)pkg/middleware/token_middleware_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
handler/signatures_test.go (3)
handler/signatures.go (1)
SignaturesHandler(19-22)pkg/portal/validator.go (2)
Validator(13-16)GetDefaultValidator(23-33)pkg/http/request.go (1)
MaxRequestSize(10-10)
pkg/middleware/token_middleware_test.go (2)
database/repository/api_keys.go (3)
ApiKeys(16-18)a(58-99)a(148-169)database/repository/repoentity/api_keys.go (1)
Key(9-14)
pkg/middleware/token_middleware_additional_test.go (2)
pkg/middleware/token_middleware.go (1)
MakeTokenMiddleware(35-48)pkg/http/schema.go (1)
ApiError(11-15)
handler/signatures.go (3)
handler/payload/signatures.go (2)
SignatureRequest(3-9)Signature(11-15)pkg/http/request.go (2)
MaxRequestSize(10-10)ParseRequestBody(12-31)pkg/http/response.go (1)
LogInternalError(76-83)
🔇 Additional comments (7)
pkg/middleware/token_middleware_test.go (2)
179-193: SeedSignature: explicit expiresAt is a good test hook.Passing expiresAt makes signature validity deterministic in tests. LGTM.
152-176: Drop this security follow-up: production middleware doesn’t derive HMAC from the public token. It simply looks up the client’s pre-seeded signature (created with the secret) rather than computing it at request time.Likely an incorrect or invalid review comment.
pkg/middleware/token_middleware_additional_test.go (3)
125-126: Seeding with explicit expiry improves determinism.Good change; keeps DB state aligned with test timing.
155-156: Same here: deterministic signature TTL.LGTM.
199-200: Rate limiter test seeding: good.Ensures the final request is valid except for rate limits. Looks good.
handler/signatures_test.go (2)
24-37: Unknown-field path covered—good.Strict decoding via DisallowUnknownFields is validated here. LGTM.
15-22: Parse error test—good negative coverage.Exercises the decoder error path. LGTM.
Summary
Testing
go test ./...https://chatgpt.com/codex/tasks/task_e_68be7a61c75483338a42e70eee040684
Summary by CodeRabbit
Bug Fixes
Chores
Tests