Apply public middleware to signature route #91
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. Caution Review failedThe pull request is closed. WalkthroughRemoves in-handler timestamp validation from signature generation and inserts PublicMiddleware into the public request pipeline; adds IP-whitelist and production flags to environment and middleware, wires middleware into app/router initialization, and adds tests for signature route and public middleware behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as Router
participant P as PublicMiddleware
participant H as SignatureHandler
C->>R: POST /generate-signature
R->>P: PublicMiddleware.Handle(request)
alt PublicMiddleware allows
P->>H: Forward request
H->>H: Parse body → CreateSignature (no timestamp check)
H-->>C: 200 OK / 400 Bad Request
else Rejected (e.g., prod & IP mismatch / rate limit)
P-->>C: 401 Unauthorized or relevant ApiError
end
note right of P: PublicMiddleware enforces IP whitelist when IsProduction=true and applies rate-limit/replay checks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✨ 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 refines the signature generation endpoint by removing redundant time-based validation from the handler, as this responsibility is now handled by the newly enforced public middleware. The change ensures consistent application of public request requirements and includes new tests to validate this behavior, improving the robustness and clarity of the API.
Highlights
- Signature Handler Simplification: The
isRequestWithinTimeframevalidation logic has been removed from theSignaturesHandler.Generatemethod, streamlining the handler's responsibilities. - Public Middleware Enforcement: The
/generate-signatureroute now explicitly enforces the public middleware, ensuring that requests to this endpoint adhere to public header requirements. - New Test Coverage: A new test file has been added to verify that the signature route correctly applies the public middleware and rejects requests that do not meet its header criteria.
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 refactors the signature generation route to use a public middleware for handling concerns like timestamp validation, which was previously done in the handler. The changes look good overall, moving shared logic to a more appropriate place and adding a test to verify the middleware is applied correctly. I've identified a potential issue with how the middleware is instantiated, which could affect rate limiting if more public routes are added. I've also suggested an improvement to the new test for better readability and structure.
![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
🧹 Nitpick comments (4)
metal/kernel/router.go (1)
33-41: Consider reusing a single PublicMiddleware instance across routesMakePublicMiddleware builds in-memory limiter/cache; creating a new instance per public route yields per-route rate limits and caches. If you want global limits/replay protection across all public endpoints, instantiate once (e.g., on Router) and reuse here.
metal/kernel/router_signature_test.go (3)
16-21: Verify handler dependencies to avoid future nil derefr.Pipeline.ApiKeys is nil while Signature() passes it into MakeSignaturesHandler. If that handler ever dereferences ApiKeys before JSON validation, this test could start failing. Consider injecting a lightweight stub or fixture for ApiKeys in the test.
30-37: Tighten the test by setting Content-Type and asserting JSON error shapeSet Content-Type to application/json and optionally assert Content-Type in the response to lock in the contract from http.MakeApiHandler.
Apply this diff to strengthen the test:
@@ - req2 := httptest.NewRequest("POST", "/generate-signature", strings.NewReader("{")) + req2 := httptest.NewRequest("POST", "/generate-signature", strings.NewReader("{")) + req2.Header.Set("Content-Type", "application/json") @@ r.Mux.ServeHTTP(rec2, req2) if rec2.Code != http.StatusBadRequest { t.Fatalf("expected status %d, got %d", http.StatusBadRequest, rec2.Code) } + if got := rec2.Header().Get("Content-Type"); got != "application/json" { + t.Fatalf("expected Content-Type application/json, got %q", got) + }
30-37: Optional: add subtests for missing/invalid public headersAdd cases for: missing X-Request-ID only, missing X-API-Timestamp only, and future timestamp (if middleware disallows future). This isolates failure modes and guards regressions in PublicMiddleware.
@@ func TestSignatureRoute_PublicMiddleware(t *testing.T) { + t.Run("missing request-id only -> 401", func(t *testing.T) { + req := httptest.NewRequest("POST", "/generate-signature", strings.NewReader(`{}`)) + req.Header.Set(portal.TimestampHeader, fmt.Sprintf("%d", time.Now().Unix())) + rec := httptest.NewRecorder() + r.Mux.ServeHTTP(rec, req) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("expected %d, got %d", http.StatusUnauthorized, rec.Code) + } + }) + + t.Run("missing timestamp only -> 401", func(t *testing.T) { + req := httptest.NewRequest("POST", "/generate-signature", strings.NewReader(`{}`)) + req.Header.Set(portal.RequestIDHeader, "req-2") + rec := httptest.NewRecorder() + r.Mux.ServeHTTP(rec, req) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("expected %d, got %d", http.StatusUnauthorized, rec.Code) + } + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
handler/signatures.go(0 hunks)metal/kernel/router.go(1 hunks)metal/kernel/router_signature_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- handler/signatures.go
🧰 Additional context used
🧬 Code graph analysis (2)
metal/kernel/router.go (3)
pkg/middleware/public_middleware.go (1)
MakePublicMiddleware(30-39)pkg/http/handler.go (1)
MakeApiHandler(9-28)pkg/middleware/pipeline.go (2)
Pipeline(10-14)m(16-22)
metal/kernel/router_signature_test.go (4)
metal/kernel/router.go (1)
Router(25-31)pkg/middleware/pipeline.go (1)
Pipeline(10-14)pkg/portal/validator.go (1)
GetDefaultValidator(23-33)pkg/portal/consts.go (2)
RequestIDHeader(13-13)TimestampHeader(11-11)
🔇 Additional comments (2)
metal/kernel/router.go (1)
33-41: Public middleware correctly inserted into the pipelinepm.Handle wrapping apiHandler via Pipeline.Chain is the right integration; this ensures requests hit PublicMiddleware before the handler.
metal/kernel/router_signature_test.go (1)
23-28: Good: asserts middleware enforces headers before body parsingExpecting 401 with no body confirms PublicMiddleware is in the chain ahead of the handler.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
.env.gh.example (1)
5-5: Clarify format and production behavior for ENV_PUBLIC_ALLOWED_IP.Add a short comment so users know it's a single IP (IPv4/IPv6) and only enforced in production.
-ENV_PUBLIC_ALLOWED_IP= +# Optional: single client IP whitelist for public routes (enforced only when ENV_APP_ENV_TYPE=production). +# Accepts a single IPv4 or IPv6 address (no CIDR, no lists). +ENV_PUBLIC_ALLOWED_IP=.env.example (1)
9-12: Be explicit about single-IP requirement and prod enforcement.Tighten the comment to prevent CIDR/multi-IP misconfiguration.
-# Optional IP whitelist for production-only public routes -ENV_PUBLIC_ALLOWED_IP= +# IP whitelist for public routes; required when ENV_APP_ENV_TYPE=production. +# Single IP only (IPv4 or IPv6). Example: 203.0.113.10 or 2001:db8::1 +ENV_PUBLIC_ALLOWED_IP=.env.prod.example (1)
8-10: Mark this as required in production and show an example.Since validation enforces this in prod, make the requirement obvious.
-# --- Public middleware -ENV_PUBLIC_ALLOWED_IP= +# --- Public middleware +# Required in production; single IP only (no CIDR, no lists). +# Example: +# ENV_PUBLIC_ALLOWED_IP=203.0.113.10 +ENV_PUBLIC_ALLOWED_IP=metal/env/network.go (1)
5-5: Relax HTTP port validation (8080-only is too restrictive).Limiting to
oneof=8080blocks common ports (80/443) and custom setups. Consider permitting a small set or switching to numeric range validation.- HttpPort string `validate:"required,numeric,oneof=8080"` + HttpPort string `validate:"required,numeric,oneof=80 443 8080"`If broader flexibility is desired later, migrate
HttpPorttointand validatemin=1,max=65535.pkg/middleware/public_middleware.go (1)
64-66: Use net.IP equality to avoid IPv6 string-format mismatches.String compare can fail for equivalent IPv6 addresses with different textual forms. Parse and compare using
net.IP.Equal.- if p.isProduction && ip != p.allowedIP { - return mwguards.InvalidRequestError("Invalid client IP", "unauthorised ip: "+ip) - } + if p.isProduction { + allowed := net.ParseIP(p.allowedIP) + client := net.ParseIP(ip) + if allowed == nil || client == nil || !allowed.Equal(client) { + return mwguards.InvalidRequestError("Invalid client IP", "unauthorised ip: "+ip) + } + }Add import:
import "net"pkg/middleware/public_middleware_test.go (2)
16-21: Freeze time in this test to isolate header failures.Currently the base timestamp (Line 19) can be far from
pm.now()and conflate reasons. Movebasebeforepmand bindpm.nowto it.- pm := MakePublicMiddleware("", false) - - base := time.Unix(1_700_000_000, 0) + base := time.Unix(1_700_000_000, 0) + pm := MakePublicMiddleware("", false) + pm.now = func() time.Time { return base }
115-156: Use RFC 5737 example IPs instead of real-looking public IPs.Swapping to documentation-only ranges avoids confusion and accidental geo-assumptions.
- pm := MakePublicMiddleware("31.97.60.190", true) + pm := MakePublicMiddleware("203.0.113.10", true) @@ - req.Header.Set("X-Forwarded-For", "31.97.60.190") + req.Header.Set("X-Forwarded-For", "203.0.113.10") @@ - pm := MakePublicMiddleware("31.97.60.190", false) + pm := MakePublicMiddleware("203.0.113.10", false)metal/kernel/kernel_test.go (1)
53-65: Tighten panic assertion to ensure cause.Optional: assert the panic message contains “invalid [NETWORK] model” to prove the production-IP rule is what failed.
- defer func() { - if r := recover(); r == nil { - t.Fatalf("expected panic") - } - }() + defer func() { + if r := recover(); r == nil { + t.Fatalf("expected panic") + } else if msg := fmt.Sprint(r); !strings.Contains(msg, "invalid [NETWORK] model") { + t.Fatalf("unexpected panic: %s", msg) + } + }()metal/kernel/router_signature_test.go (2)
34-43: Stabilize timestamp to avoid edge flakiness.Use a slight past timestamp to dodge “future” checks around clock boundaries.
- req.Header.Set(portal.TimestampHeader, fmt.Sprintf("%d", time.Now().Unix())) + req.Header.Set(portal.TimestampHeader, fmt.Sprintf("%d", time.Now().Add(-1*time.Minute).Unix()))
45-64: Production non-whitelist rejection covered; add positive case.Add a subtest where X-Forwarded-For equals the allowed IP and assert 400 (invalid body) to prove the request passes middleware.
t.Run("production allows whitelisted IP (then fails body)", func(t *testing.T) { r := Router{ Mux: http.NewServeMux(), Pipeline: middleware.Pipeline{ PublicMiddleware: middleware.MakePublicMiddleware("203.0.113.10", true), }, validator: portal.GetDefaultValidator(), } r.Signature() req := httptest.NewRequest("POST", "/generate-signature", strings.NewReader("{")) req.Header.Set(portal.RequestIDHeader, "req-1") req.Header.Set(portal.TimestampHeader, fmt.Sprintf("%d", time.Now().Add(-1*time.Minute).Unix())) req.Header.Set("X-Forwarded-For", "203.0.113.10") rec := httptest.NewRecorder() r.Mux.ServeHTTP(rec, req) if rec.Code != http.StatusBadRequest { t.Fatalf("expected status %d, got %d", http.StatusBadRequest, rec.Code) } })I can push this subtest if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.env.example(1 hunks).env.gh.example(1 hunks).env.prod.example(1 hunks)metal/env/network.go(1 hunks)metal/kernel/app.go(1 hunks)metal/kernel/factory.go(1 hunks)metal/kernel/kernel_test.go(4 hunks)metal/kernel/router.go(1 hunks)metal/kernel/router_signature_test.go(1 hunks)pkg/middleware/pipeline.go(1 hunks)pkg/middleware/public_middleware.go(2 hunks)pkg/middleware/public_middleware_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- metal/kernel/router.go
🧰 Additional context used
🧬 Code graph analysis (8)
pkg/middleware/pipeline.go (2)
pkg/middleware/public_middleware.go (1)
PublicMiddleware(20-29)pkg/middleware/pipeline_test.go (1)
TestPipelineChainOrder(12-49)
metal/kernel/router_signature_test.go (5)
metal/kernel/router.go (1)
Router(25-31)pkg/middleware/pipeline.go (1)
Pipeline(10-15)pkg/middleware/public_middleware.go (2)
PublicMiddleware(20-29)MakePublicMiddleware(34-45)pkg/portal/validator.go (1)
GetDefaultValidator(23-33)pkg/portal/consts.go (2)
RequestIDHeader(13-13)TimestampHeader(11-11)
metal/kernel/factory.go (4)
metal/env/env.go (2)
GetEnvVar(21-23)App(9-15)metal/env/env_test.go (2)
TestNetEnvironment(88-105)TestAppEnvironmentChecks(43-67)metal/env/app.go (1)
Name(7-11)metal/kernel/helpers.go (1)
a(34-36)
pkg/middleware/public_middleware.go (1)
pkg/middleware/mwguards/mw_response_messages.go (1)
InvalidRequestError(36-47)
metal/env/network.go (3)
metal/env/env_test.go (1)
TestNetEnvironment(88-105)metal/env/app.go (1)
Name(7-11)metal/env/env.go (1)
App(9-15)
pkg/middleware/public_middleware_test.go (3)
pkg/middleware/public_middleware.go (1)
MakePublicMiddleware(34-45)pkg/http/schema.go (1)
ApiError(11-15)pkg/portal/consts.go (2)
RequestIDHeader(13-13)TimestampHeader(11-11)
metal/kernel/kernel_test.go (7)
metal/kernel/factory.go (1)
MakeEnv(55-131)metal/kernel/router.go (1)
Router(25-31)pkg/middleware/pipeline.go (1)
Pipeline(10-15)pkg/middleware/public_middleware.go (2)
PublicMiddleware(20-29)MakePublicMiddleware(34-45)database/repository/api_keys.go (1)
ApiKeys(16-18)database/connection.go (1)
Connection(13-18)pkg/auth/handler.go (1)
TokenHandler(10-14)
metal/kernel/app.go (8)
metal/env/env.go (1)
Environment(9-15)pkg/auth/handler.go (2)
MakeTokensHandler(16-26)TokenHandler(10-14)metal/kernel/factory.go (3)
MakeDbConnection(35-43)MakeLogs(45-53)MakeSentry(15-33)metal/kernel/router.go (1)
Router(25-31)pkg/middleware/pipeline.go (1)
Pipeline(10-15)database/repository/api_keys.go (1)
ApiKeys(16-18)pkg/middleware/public_middleware.go (2)
PublicMiddleware(20-29)MakePublicMiddleware(34-45)metal/env/app.go (2)
Name(7-11)e(13-15)
🔇 Additional comments (18)
metal/env/network.go (1)
6-7: Good: Production-only IP requirement is enforced at the model level.The
required_if=IsProduction true,omitempty,iptag cleanly prevents accidental open access in prod.pkg/middleware/public_middleware.go (3)
27-29: Private fields for whitelist config look good.Keeps surface area small and ties config to instance state.
32-34: Constructor docs are clear.The semantics of allowedIP/isProduction are well stated.
42-44: Trim and persist config at construction time—good.Avoids per-request normalization overhead.
pkg/middleware/pipeline.go (1)
11-15: Wiring PublicMiddleware into the Pipeline SGTM.The value-type field plus internal dependency guard is fine given construction via MakeApp.
metal/kernel/factory.go (1)
87-91: Environment wiring is correct and validated.Reads, trims, and exposes
PublicAllowedIPandIsProductionfor downstream middleware setup.metal/kernel/app.go (2)
50-54: Public middleware correctly instantiated with env-provided config.This should enforce headers + IP whitelist on public routes as intended.
45-55: Confirm PublicPipelineFor applies PublicMiddleware firstSignature route uses
r.PublicPipelineFor(abstract.Generate)(metal/kernel/router.go:76–80); I couldn’t locatePublicPipelineFor’s definition—please verify it invokesPipeline.PublicMiddleware.Handleas the first middleware in the chain.pkg/middleware/public_middleware_test.go (3)
61-65: Good: deterministic timestamp window.Binding
pm.nowremoves flakiness and proves expiry logic.
78-83: Replay + rate limit coverage looks solid.Limiter tightened and clock fixed; status checks are precise (401 then 429).
16-17: No stale MakePublicMiddleware constructor calls detected; all call sites use the updated two-argument signature.metal/kernel/kernel_test.go (4)
36-36: ENV_PUBLIC_ALLOWED_IP added to fixtures.Good addition to keep env in-sync with new middleware config.
48-51: Assert load of PublicAllowedIP.Catches wiring regressions from env → kernel.
118-119: Router test now exercises PublicMiddleware presence.Helps ensure helpers don’t regress when middleware becomes mandatory.
159-163: Pipeline wiring complete (Env, ApiKeys, TokenHandler, PublicMiddleware).Good end-to-end coverage setup for route registration.
metal/kernel/router_signature_test.go (3)
15-23: Router initialized with PublicMiddleware for this suite.Correctly scopes middleware to the tested mux.
25-33: Unauthorized without public headers.Clear, isolated assertion of the middleware gate.
45-64: Resolved: Signature route is correctly wrapped with PublicMiddleware.
gocanto
deleted the
codex/remove-isrequestwithintimeframe-from-signatures.go
branch
Summary
/generate-signatureTesting
go test ./...https://chatgpt.com/codex/tasks/task_e_68bea17a9f308333a44f1d9d683eda3b
Summary by CodeRabbit