chore: better token middleware + format #77
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. WalkthroughReplaces many imports from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant RateLimiter
participant NonceCache
participant ApiKeysRepo
participant TokenHandler
participant Handler
Client->>Middleware: HTTP request + headers (username,key,signature,ts,nonce,req-id) + body
Middleware->>RateLimiter: TooMany(account|ip)?
RateLimiter-->>Middleware: allowed/limited
Middleware->>NonceCache: Too recently used?
NonceCache-->>Middleware: used/unused
Middleware->>ApiKeysRepo: Load API keys for account
ApiKeysRepo-->>Middleware: api key record
Middleware->>TokenHandler: Decode token / validate
TokenHandler-->>Middleware: token data / error
Middleware->>Middleware: Verify signature, timestamp, body hash
alt All checks pass
Middleware->>NonceCache: Mark(nonce)
Middleware->>Handler: call next with context (account, req-id)
else Any check fails
Middleware->>RateLimiter: Fail(account|ip)
Middleware-->>Client: 401 Unauthorized (or 429 if limited)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
gocanto
force-pushed
the
chore/better-token
branch
from
cd23192 to
8ae6ed6
Compare
gocanto
force-pushed
the
chore/better-token
branch
from
1ffec63 to
72c3101
Compare
![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 (9)
pkg/cache/ttl_cache.go (1)
51-55:Markdoes a full-map sweep on every call – could hurt latency under load
for k, exp := range c.data { … }is O(n) each time a key is written.
With high QPS or large nonce sets this becomes a hotspot.Consider lighter strategies (sampled pruning, background sweeper, or size threshold) to bound per-request cost.
pkg/middleware/token_middleware.go (2)
97-101: Redundant nil-check for logger
slog.With(...)never returnsnil; keepinglogger == nilcomplicates the guard without benefit.- if reqID == "" || logger == nil { + if reqID == "" { return t.getInvalidRequestError() }
257-267: Nonce is marked before signature verification – enables cheap cache-floodingA request with an invalid signature still consumes a fresh nonce, letting an attacker rapidly bloat
nonceCache.Shift
UseOnceafter signature verification or roll the mark back on failure so only successful authentications reserve the nonce.docs/middleware/token_examples.md (1)
50-54: Nit: avoid redefiningpm.sendRequestThe line
pm.sendRequest = pm.sendRequest;is a no-op, can be dropped.docs/middleware/token_analysis_v1.md (5)
23-24: Fix status code inconsistency (legacy returned 403, v1 should return 401)Section 1 says the middleware returns 401, but the legacy code (see provided snippet) returned 403. Keep Section 1 historically accurate and reserve the 401 note for the improvements section.
Apply this diff to Section 1 (legacy behavior):
-7. On success, logs authentication success and calls the next handler; otherwise returns http.ApiError with Status 401 (generic message). +7. On success, logs authentication success and calls the next handler; otherwise returns http.ApiError with Status 403 (generic message).And keep the “Quick wins” item about switching to 401 as-is (since v1 does this).
Also applies to: 45-47
225-233: Fix heading levels and trailing punctuation (markdownlint MD001/MD026) in Section 8Convert sub-section labels into proper H3 headings without trailing colons and keep the increment consistent.
Apply this diff:
-## 8) Example canonical signature spec (for future adoption) +## 8) Example canonical signature spec (for future adoption) -Headers required: +### Headers required - X-API-Username - X-API-Key - X-API-Timestamp (epoch seconds) - X-API-Nonce (UUID v4) - X-API-Signature -Canonical request (string to sign): +### Canonical request (string to sign) METHOD + "\n" + PATH + "\n" + SORTED_QUERY_STRING + "\n" + X-API-Username + "\n" + X-API-Key + "\n" + X-API-Timestamp + "\n" + X-API-Nonce + "\n" + SHA256_HEX(BODY) -Signature: +### Signature - signature = hex(HMAC-SHA256(secretKey, canonical_request)) -Validation rules: +### Validation rules - Accept if |now - timestamp| <= 300s, nonce unused within window, and constant-time comparison passes.Also applies to: 234-244, 245-250
265-278: Address markdownlint issues and improve readability for docker-compose sectionUse proper subheadings rather than list items with colons; remove trailing colon from “Volumes:”.
Apply this diff:
-- Containers and networks (docker-compose.yml): - - Services: +### Containers and networks (docker-compose.yml) + - Services - api: Go API built from docker/dockerfile-api; exposes ENV_HTTP_PORT (default 8080) to the caddy_net and oullin_net networks. DB host is api-db via Docker DNS. Secrets are injected using Docker secrets (pg_username, pg_password, pg_dbname). - api-db: Postgres 17.3-alpine. Port bound to 127.0.0.1:${ENV_DB_PORT:-5432} (not exposed publicly). Uses Docker secrets for credentials. Includes healthcheck and SSL files mounted read-only. - api-db-migrate: Runs migrations from database/infra/migrations via a wrapper script. - api-runner: Convenience container to run Go commands (e.g., seeders) with the code mounted at /app, sharing the network with api-db. - caddy_local (profile local): Reverse proxy for local development. Host ports 8080->80 and 8443->443. Caddyfile: caddy/Caddyfile.local. - caddy_prod (profile prod): Public reverse proxy/terminates TLS via Let’s Encrypt. Host ports 80/443 exposed. Caddyfile: caddy/Caddyfile.prod. - - Networks: + - Networks - caddy_net: Fronting proxy <-> API network. - oullin_net: Internal network for API <-> DB and runner. - - Volumes: + - Volumes - caddy_data, caddy_config, oullin_db_data for persistence; go_mod_cache for cached modules in api-runner.
281-282: Fix bare URLs (markdownlint MD034) by wrapping in angle bracketsWrap URLs to satisfy MD034 and improve consistency.
Apply this diff:
- - Listens on :80 in the container (published as http://localhost:8080 on the host). + - Listens on :80 in the container (published as <http://localhost:8080> on the host). @@ - - API is routed under /api/* and proxied to api:8080. That means production API path = https://oullin.io/api/... while local is http://localhost:8080/.... + - API is routed under /api/* and proxied to api:8080. That means production API path = <https://oullin.io/api/...> while local is <http://localhost:8080/...>. @@ - - In production behind Caddy, the protected routes are under /api (e.g., POST https://oullin.io/api/posts). Locally through caddy_local they are at http://localhost:8080/posts. + - In production behind Caddy, the protected routes are under /api (e.g., POST <https://oullin.io/api/posts>). Locally through caddy_local they are at <http://localhost:8080/posts>.Also applies to: 287-288, 297-301
154-170: Minimize client-visible detail: also avoid leaking normalized account namesThis section is good; add that even normalized account names should be omitted in client-facing messages.
Apply this diff:
- Don’t echo credentials: - - Avoid returning account name, token, or signature in error messages. Use generic client-facing errors. + - Avoid returning account name (even normalized), token, or signature in error messages. Use generic client-facing errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
database/seeder/seeds/users.go(2 hunks)docs/middleware/postman/token-local.postman_collection.json(1 hunks)docs/middleware/postman/token-prod.postman_collection.json(1 hunks)docs/middleware/token_analysis_v1.md(1 hunks)docs/middleware/token_analysis_v2.md(1 hunks)docs/middleware/token_examples.md(1 hunks)metal/cli/main.go(2 hunks)metal/cli/posts/factory.go(1 hunks)metal/cli/posts/handler_test.go(2 hunks)pkg/cache/ttl_cache.go(1 hunks)pkg/middleware/token_middleware.go(1 hunks)pkg/portal/client.go(1 hunks)pkg/portal/client_test.go(1 hunks)pkg/portal/support.go(1 hunks)pkg/portal/validator.go(2 hunks)
✅ Files skipped from review due to trivial changes (6)
- pkg/portal/client_test.go
- pkg/portal/client.go
- metal/cli/main.go
- metal/cli/posts/handler_test.go
- metal/cli/posts/factory.go
- docs/middleware/postman/token-local.postman_collection.json
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/portal/validator.go
- database/seeder/seeds/users.go
- pkg/portal/support.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/middleware/token_middleware.go (3)
pkg/portal/support.go (3)
BuildCanonical(54-74)ParseClientIP(76-91)Sha256Hex(25-28)pkg/http/middleware/token_middleware.go (4)
Handle(30-53)shallReject(55-83)getUnauthenticatedError(106-118)getInvalidRequestError(85-97)pkg/auth/handler_test.go (1)
TestTokenHandlerLifecycle(5-33)
docs/middleware/token_analysis_v1.md (3)
pkg/http/middleware/token_middleware.go (4)
Handle(30-53)shallReject(55-83)MakeTokenMiddleware(23-28)getUnauthenticatedError(106-118)pkg/http/middleware/token_middleware_test.go (2)
TestTokenMiddlewareErrors(12-32)TestTokenMiddlewareHandleInvalid(34-48)pkg/auth/handler_test.go (1)
TestTokenHandlerLifecycle(5-33)
🪛 markdownlint-cli2 (0.17.2)
docs/middleware/token_analysis_v1.md
13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
20-20: Bare URL used
(MD034, no-bare-urls)
21-21: Bare URL used
(MD034, no-bare-urls)
46-46: Bare URL used
(MD034, no-bare-urls)
46-46: Bare URL used
(MD034, no-bare-urls)
107-107: Bare URL used
(MD034, no-bare-urls)
277-277: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
278-278: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (4)
pkg/cache/ttl_cache.go (1)
60-75: Good atomic check-and-set for replay protection
UseOncecloses the race window called out in the earlier review – nice fix.pkg/middleware/token_middleware.go (1)
245-255: Correct constant-time comparison – looks goodHashing both inputs to fixed length before
subtle.ConstantTimeCompareeliminates the length-leak side-channel. Nice catch.docs/middleware/postman/token-prod.postman_collection.json (1)
70-75: Ensure secrets are stored only in environment variablesJust a reminder: keep
secretKeyPostman env values out of version control.docs/middleware/token_analysis_v2.md (1)
118-126: Great roadmap – nothing to addDocumentation is thorough and aligns with the codebase direction.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
pkg/portal/support.go (3)
30-52: Deterministic query building is solid; consider clarifying commentYou sort keys and values for stable canonicalization (Values.Encode only sorts keys). Consider a short code comment to document “value sorting ensures deterministic ordering for multi-value keys”.
Suggested inline note:
- vals := q[k] + // Sort values to ensure deterministic ordering for multi-value keys + vals := q[k]
54-74: Path fallback can be more robust with EscapedPathFor URLs where RawPath is set but Path is empty, the current condition defaults to “/”. Prefer using EscapedPath first, then fallback.
-func BuildCanonical(method string, u *url.URL, username, public, ts, nonce, bodyHash string) string { - path := "/" - - if u != nil && u.Path != "" { - path = u.EscapedPath() - } +func BuildCanonical(method string, u *url.URL, username, public, ts, nonce, bodyHash string) string { + path := "/" + if u != nil { + path = u.EscapedPath() + if path == "" { + path = "/" + } + }
76-91: X-Forwarded-For trust and IP validationParsing the first XFF entry is common, but:
- Only trust XFF if requests come from trusted proxies/load balancers.
- Consider validating the extracted IP via net.ParseIP; if invalid, fall back to RemoteAddr.
Minimal adjustment:
- parts := strings.Split(xff, ",") - return strings.TrimSpace(parts[0]) + parts := strings.Split(xff, ",") + candidate := strings.TrimSpace(parts[0]) + if ip := net.ParseIP(candidate); ip != nil { + return candidate + } + // fallthrough to RemoteAddr parsing belowIf you want, I can add a helper with a “trustProxy bool” parameter and update call sites.
caddy/Caddyfile.local (2)
16-21: Add Vary to ensure correct caching of CORS responsesWithout Vary, proxies/CDNs may cache headers across origins. Recommend adding Vary.
Apply:
header { Access-Control-Allow-Origin "http://localhost:5173" # allows the Vue app (running on localhost:5173) to make requests. Access-Control-Allow-Methods "GET, POST, PUT, DELETE, OPTIONS" # Specifies which methods are allowed. Access-Control-Allow-Headers "X-API-Key, X-API-Username, X-API-Signature, X-API-Timestamp, X-API-Nonce, X-Request-ID, Content-Type, User-Agent, If-None-Match" # allows the custom headers needed by the API. Access-Control-Expose-Headers "ETag, X-Request-ID" + Vary "Origin, Access-Control-Request-Method, Access-Control-Request-Headers" }
24-35: Restrict preflight origin matcher to the allowed origin (avoid reflecting any origin)You reflect Origin for any preflight request, while the main header allows only http://localhost:5173. Tighten the matcher to prevent mixed signals and unnecessary 204s for disallowed origins.
Apply:
@preflight { method OPTIONS - header Origin * + header Origin http://localhost:5173 } handle @preflight { # Reflect the Origin back so it's always allowed header Access-Control-Allow-Origin "{http.request.header.Origin}" header Access-Control-Allow-Methods "GET, POST, PUT, DELETE, OPTIONS" header Access-Control-Allow-Headers "X-API-Key, X-API-Username, X-API-Signature, X-API-Timestamp, X-API-Nonce, X-Request-ID, Content-Type, User-Agent, If-None-Match" header Access-Control-Max-Age "86400" + header Vary "Origin, Access-Control-Request-Method, Access-Control-Request-Headers" respond 204 }caddy/Caddyfile.prod (2)
34-39: Add Vary to guard against incorrect CORS caching in intermediariesAdd Vary to ensure correctness under caching/CDNs.
Apply:
header { Access-Control-Allow-Origin "https://oullin.io" Access-Control-Allow-Methods "GET, POST, PUT, DELETE, OPTIONS" Access-Control-Allow-Headers "X-API-Key, X-API-Username, X-API-Signature, X-API-Timestamp, X-API-Nonce, X-Request-ID, Content-Type, User-Agent, If-None-Match" Access-Control-Expose-Headers "ETag, X-Request-ID" + Vary "Origin, Access-Control-Request-Method, Access-Control-Request-Headers" }
41-53: Preflight: restrict origin matcher to your allowed originMirror the allow-list to the preflight matcher for consistency and a smaller attack surface.
Apply:
@preflight { method OPTIONS - header Origin * + header Origin https://oullin.io } handle @preflight { # Reflect the Origin back so it's always allowed header Access-Control-Allow-Origin "{http.request.header.Origin}" header Access-Control-Allow-Methods "GET, POST, PUT, DELETE, OPTIONS" header Access-Control-Allow-Headers "X-API-Key, X-API-Username, X-API-Signature, X-API-Timestamp, X-API-Nonce, X-Request-ID, Content-Type, User-Agent, If-None-Match" header Access-Control-Max-Age "86400" + header Vary "Origin, Access-Control-Request-Method, Access-Control-Request-Headers" respond 204 }docs/middleware/token_analysis_v1.md (2)
289-304: Fix markdownlint MD034 (no bare URLs) by wrapping URLs in angle bracketsWrap raw URLs to satisfy markdownlint.
Apply:
- - Listens on :80 in the container (published as http://localhost:8080 on the host). + - Listens on :80 in the container (published as <http://localhost:8080> on the host). - - CORS configured for https://oullin.io within the /api handler. For preflight, echoes Access-Control-Allow-Origin back. + - CORS configured for <https://oullin.io> within the /api handler. For preflight, echoes Access-Control-Allow-Origin back. - - API is routed under /api/* and proxied to api:8080. That means production API path = https://oullin.io/api/... while local is http://localhost:8080/.... + - API is routed under /api/* and proxied to api:8080. That means production API path = <https://oullin.io/api/...> while local is <http://localhost:8080/...>. - - build-local (build.mk): docker compose --profile local up --build -d (starts api, api-db, caddy_local). After this, the API is reachable at http://localhost:8080. + - build-local (build.mk): docker compose --profile local up --build -d (starts api, api-db, caddy_local). After this, the API is reachable at <http://localhost:8080>.
309-313: Fix remaining bare URLs (examples section) for markdownlintWrap example URLs.
Apply:
- - In production behind Caddy, the protected routes are under /api (e.g., POST https://oullin.io/api/posts). Locally through caddy_local they are at http://localhost:8080/posts. + - In production behind Caddy, the protected routes are under /api (e.g., POST <https://oullin.io/api/posts>). Locally through caddy_local they are at <http://localhost:8080/posts>.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
caddy/Caddyfile.local(2 hunks)caddy/Caddyfile.prod(3 hunks)docs/middleware/token_analysis_v1.md(1 hunks)main.go(2 hunks)pkg/markdown/handler.go(2 hunks)pkg/middleware/token_middleware.go(1 hunks)pkg/portal/client.go(2 hunks)pkg/portal/support.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/portal/client.go
- pkg/middleware/token_middleware.go
- main.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/markdown/handler.go (2)
pkg/portal/support.go (1)
ReadWithSizeLimit(96-108)pkg/markdown/handler_test.go (1)
TestParserFetch(9-22)
pkg/portal/support.go (1)
pkg/support_test.go (2)
closed(8-11)TestCloseWithLog(19-35)
docs/middleware/token_analysis_v1.md (2)
pkg/http/middleware/token_middleware.go (1)
Handle(30-53)pkg/http/middleware/token_middleware_test.go (1)
TestTokenMiddlewareErrors(12-32)
🪛 markdownlint-cli2 (0.17.2)
docs/middleware/token_analysis_v1.md
291-291: Bare URL used
(MD034, no-bare-urls)
292-292: Bare URL used
(MD034, no-bare-urls)
298-298: Bare URL used
(MD034, no-bare-urls)
298-298: Bare URL used
(MD034, no-bare-urls)
299-299: Bare URL used
(MD034, no-bare-urls)
304-304: Bare URL used
(MD034, no-bare-urls)
312-312: Bare URL used
(MD034, no-bare-urls)
312-312: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (7)
pkg/markdown/handler.go (1)
8-10: Import migration to portal looks correctAdopts shared helpers under github.com/oullin/pkg/portal. No issues.
pkg/portal/support.go (1)
15-23: Nil-safety for CloseWithLog is now handledGuarding nil closers prevents panics. Looks good.
caddy/Caddyfile.local (1)
19-21: CORS headers updated correctly for new auth fieldsAllow/Expose lists include X-API-Timestamp, X-API-Nonce, X-Request-ID. Looks good.
caddy/Caddyfile.prod (2)
37-39: CORS headers correctly include new auth headers and X-Request-ID exposureAllow/Expose sets look consistent with middleware requirements.
60-62: Header forwarding for new auth headers is correctX-API-Timestamp, X-API-Nonce, and X-Request-ID are forwarded upstream as required.
docs/middleware/token_analysis_v1.md (2)
3-11: Doc scope and file paths are now clearly split between legacy and v1Legacy vs current locations are explicit. Good fix.
107-114: Canonicalization rules documented clearlyDeterministic rules should prevent signature drift. Nicely done.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/middleware/token_middleware.go (2)
195-210: Good: bounded body read and body restorationReadWithSizeLimit addresses prior DoS risk; body is restored for downstream handlers.
249-256: Good: constant-time compare without length leakHashing both inputs before subtle.ConstantTimeCompare fixes the early-exit length side-channel.
🧹 Nitpick comments (14)
pkg/portal/support_test.go (6)
11-27: Add tests for special characters and percent-encoding in query parametersTo harden the contract of SortedQuery, also cover:
- Keys/values requiring escaping (space, +, %, /, unicode).
- Idempotence when the query is already sorted.
Example additions:
t.Run("escapes special chars", func(t *testing.T) { u, _ := url.Parse("https://x.test/api?sp= a+b&path=a/b&perc=%25&uni=é") got := SortedQuery(u) // url.QueryEscape encodes as: " " -> "+", "+" -> "%2B", "/" -> "%2F", "%" -> "%25" expected := "path=a%2Fb&perc=%2525&sp=+a%2Bb&uni=%C3%A9" if got != expected { t.Fatalf("expected %q, got %q", expected, got) } }) t.Run("idempotent on already-sorted query", func(t *testing.T) { u, _ := url.Parse("https://x.test/api?a=0&a=1&b=2") if got := SortedQuery(u); got != "a=0&a=1&b=2" { t.Fatalf("unexpected: %q", got) } })
38-43: Add coverage for path escaping and empty path edge casesBuildCanonical uses u.EscapedPath() and defaults to "/". Add tests for:
- URL with RawPath (percent-encoded path) to ensure EscapedPath is preserved.
- Empty path (e.g., u := &url.URL{}) producing "/".
Example:
t.Run("preserves escaped path", func(t *testing.T) { u := &url.URL{Path: "/a b", RawPath: "/a%20b"} got := BuildCanonical("get", u, "u", "p", "1", "n", "h") want := "GET\n/a%20b\n\nu\np\n1\nn\nh" if got != want { t.Fatalf("expected %q, got %q", want, got) } }) t.Run("empty path becomes slash", func(t *testing.T) { u := &url.URL{} got := BuildCanonical("get", u, "u", "p", "1", "n", "h") want := "GET\n/\n\nu\np\n1\nn\nh" if got != want { t.Fatalf("expected %q, got %q", want, got) } })
53-55: Prefer errors.Is for error comparisonsUse errors.Is to remain robust if the implementation wraps io.ErrUnexpectedEOF.
Apply this diff:
- if err != io.ErrUnexpectedEOF { - t.Errorf("expected io.ErrUnexpectedEOF for nil reader, got %v", err) - } + if err == nil || !errors.Is(err, io.ErrUnexpectedEOF) { + t.Errorf("expected io.ErrUnexpectedEOF for nil reader, got %v", err) + }
58-76: Tighten the comment to reflect the next custom-limit testThe note about testing the 5MB default is already addressed by the following custom-limit test. Consider removing or rewording to avoid redundancy.
Minimal cleanup:
- // We can't easily test the default 5MB limit in a unit test, - // but we can test the logic by using a smaller custom limit + // Exceed-path is covered in the following custom-limit test.
112-127: Use errors.Is instead of substring matching for wrapped errorThe implementation wraps the underlying read error with %w, so errors.Is is a better fit than string contains.
Apply this diff:
- if err == nil || !strings.Contains(err.Error(), expectedErr.Error()) { - t.Errorf("expected error containing %q, got %v", expectedErr, err) - } + if err == nil || !errors.Is(err, expectedErr) { + t.Errorf("expected wrapped error %v, got %v", expectedErr, err) + }
78-110: Consider adding a test for zero/negative custom limits falling back to defaultReadWithSizeLimit ignores non-positive overrides and uses the default. A quick sanity check would lock this contract.
Add this test:
func TestReadWithSizeLimit_NonPositiveCustomLimitFallsBackToDefault(t *testing.T) { // 1MB payload; should be allowed by default 5MB payload := strings.Repeat("x", 1<<20) for _, override := range []int64{0, -1} { t.Run(strings.TrimSpace(strings.ReplaceAll(strings.TrimPrefix(strings.TrimSpace(string(override)), "-"), " ", "")), func(t *testing.T) { data, err := ReadWithSizeLimit(strings.NewReader(payload), override) if err != nil { t.Fatalf("unexpected error with override=%d: %v", override, err) } if got := len(data); got != len(payload) { t.Fatalf("unexpected size: got=%d want=%d", got, len(payload)) } }) } }pkg/middleware/valid_timestamp.go (2)
35-38: Nil logger should not cause 401; fall back to default logger insteadFailing auth when the logger is nil is surprising. Degrade gracefully by using slog.Default() (or a no-op) rather than returning 401.
- if v.logger == nil { - return &http.ApiError{Message: "Invalid timestamp headers tracker", Status: baseHttp.StatusUnauthorized} - } + if v.logger == nil { + v.logger = slog.Default() // fallback; don't fail auth on missing logger + }
36-36: Message wording nit"Invalid timestamp headers tracker" reads odd. Consider aligning with other generic errors.
- return &http.ApiError{Message: "Invalid timestamp headers tracker", Status: baseHttp.StatusUnauthorized} + return &http.ApiError{Message: "Invalid authentication headers", Status: baseHttp.StatusUnauthorized}docs/middleware/token_analysis_v2.md (2)
64-69: Doc-code mismatch: disallowFuture default is true in codeMakeTokenMiddleware sets disallowFuture = true by default, but docs state false. Update docs to reflect the code.
- - clockSkew: 5m; disallowFuture: false + - clockSkew: 5m; disallowFuture: true
60-62: Fix unordered list indentation (MD007)markdownlint flags the indentation on these bullets. Unindent to the top-level list indentation.
- - Uses structured slog logger with request_id, method, path. - - Logs warnings for missing headers, invalid format, too many failures, account not found, replay detected, signature mismatch. - - Errors are generic to clients (HTTP 401 with neutral messages) to avoid leaking details. + - Uses structured slog logger with request_id, method, path. + - Logs warnings for missing headers, invalid format, too many failures, account not found, replay detected, signature mismatch. + - Errors are generic to clients (HTTP 401 with neutral messages) to avoid leaking details.pkg/middleware/token_middleware.go (3)
168-169: Minor logging nit: add space after comma when joiningFor readability, join with ", " instead of ",".
- logger.Error("token middleware missing dependencies", "missing", strings.Join(missing, ",")) + logger.Error("token middleware missing dependencies", "missing", strings.Join(missing, ", "))
104-106: Redundant nil check for loggerslog.With(...) never returns nil. You can simplify to just check reqID.
- if reqID == "" || logger == nil { + if reqID == "" { return t.getInvalidRequestError() }
84-97: Dead config fields or duplication: failWindow/maxFailPerScope not wiredfailWindow and maxFailPerScope are set but not used to configure rateLimiter (hardcoded to 1m/10). Either remove the fields or pass them into the limiter ctor to avoid drift.
- rateLimiter: limiter.NewMemoryLimiter(1*time.Minute, 10), - failWindow: 1 * time.Minute, - maxFailPerScope: 10, + failWindow: 1 * time.Minute, + maxFailPerScope: 10, + rateLimiter: limiter.NewMemoryLimiter(failWindow, maxFailPerScope),pkg/middleware/token_middleware_test.go (1)
235-311: Solid end-to-end path; consider adding replay and limiter testsGreat coverage of happy/negative DB paths. Please add:
- Nonce replay test: reuse same nonce for a signed request and expect 401 on the second try.
- Rate limiter test: trigger repeated failures until TooMany and assert 429.
I can draft these tests with minimal scaffolding if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
caddy/Caddyfile.prod(3 hunks)docs/middleware/token_analysis_v2.md(1 hunks)pkg/markdown/handler.go(3 hunks)pkg/middleware/token_middleware.go(1 hunks)pkg/middleware/token_middleware_test.go(1 hunks)pkg/middleware/valid_timestamp.go(1 hunks)pkg/middleware/valid_timestamp_test.go(1 hunks)pkg/portal/support.go(1 hunks)pkg/portal/support_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/middleware/valid_timestamp_test.go
- pkg/markdown/handler.go
- caddy/Caddyfile.prod
- pkg/portal/support.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
pkg/portal/support_test.go (1)
pkg/portal/support.go (3)
SortedQuery(31-53)BuildCanonical(55-75)ReadWithSizeLimit(97-118)
pkg/middleware/valid_timestamp.go (1)
pkg/http/schema.go (1)
ApiError(10-13)
docs/middleware/token_analysis_v2.md (1)
pkg/http/middleware/token_middleware_test.go (1)
TestTokenMiddlewareErrors(12-32)
pkg/middleware/token_middleware_test.go (9)
pkg/http/schema.go (1)
ApiError(10-13)database/connection.go (2)
Connection(12-17)MakeConnection(19-32)metal/env/env.go (1)
Environment(9-15)metal/env/db.go (1)
DBEnvironment(5-14)database/model.go (2)
DriverName(9-9)APIKey(26-35)pkg/auth/encryption.go (1)
CreateSignatureFrom(80-85)pkg/auth/handler.go (1)
MakeTokensHandler(16-26)database/repository/api_keys.go (1)
ApiKeys(11-13)pkg/http/middleware/token_middleware.go (2)
Handle(30-53)shallReject(55-83)
🪛 markdownlint-cli2 (0.17.2)
docs/middleware/token_analysis_v2.md
60-60: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
61-61: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
62-62: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
🔇 Additional comments (8)
pkg/portal/support_test.go (4)
11-18: Good coverage of basic sorting behavior in SortedQueryInputs exercise multi-value sorting and verify deterministic ordering. Looks correct.
29-37: Canonical builder happy path looks solidValidates method uppercasing, EscapedPath usage, and query sorting in the canonical string. Looks good.
78-110: Custom limit path is well coveredVerifies both under- and over-limit behavior with a small limit. This is clear and effective.
129-136: Simple mock reader is fineMinimal, readable, and adequate for exercising error propagation.
pkg/middleware/token_middleware.go (1)
118-123: Good: explicit timestamp validation via helperClear separation of concerns with ValidTimestamp and configurable skew/future policy.
pkg/middleware/token_middleware_test.go (3)
93-111: Dependency: slogNoop must exist in this packageThese tests call slogNoop(); ensure it’s defined in the same package (e.g., in valid_timestamp_test.go) and exported within the test package scope (not middleware_test vs middleware).
369-378: Good: asserts default disallowFuture trueMatches code defaults in MakeTokenMiddleware.
380-438: Good: rejects future timestamps with disallowFuture=trueCovers the policy well and ensures next is not invoked.
Summary by CodeRabbit
New Features
New Components
Tests
Chores
Documentation