Skip to content

Conversation

@gocanto
Copy link
Collaborator

@gocanto gocanto commented Aug 12, 2025

Summary by CodeRabbit

  • New Features

    • Public endpoint to generate API account signatures (returns signature, max tries, timing metadata).
    • CORS expanded to allow and forward X-API-Intended-Origin.
    • Backend support added for per-API-key signatures (retry/expires semantics).
  • Refactor

    • Streamlined auth/token middleware and standardized guard-style error paths.
    • API error responses now include an additional data field with contextual details.
  • Chores

    • CLI menu updated: consolidated account key generation and removed app encryption key option.

@coderabbitai
Copy link

coderabbitai bot commented Aug 12, 2025

Note

Other AI code review bot(s) detected

CodeRabbit 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 failed

The pull request is closed.

Walkthrough

Adds API key signature support (DB migration, model, repo), a /generate-signature endpoint and payloads, guard-based token middleware refactor with new portal header constants, expanded CORS to include X-API-Intended-Origin, richer HTTP error payloads/logging, HMAC signature utilities, and assorted CLI/router/test/import tweaks.

Changes

Cohort / File(s) Summary
CORS & proxy
caddy/Caddyfile.local, caddy/Caddyfile.prod, main.go
Add X-API-Intended-Origin to Access-Control-Allow-Headers and preflight; forward header in prod; centralize allowed headers in main.go.
Signatures: schema & model
database/infra/migrations/000003_api_keys_signatures.up.sql, database/model.go
New api_key_signatures migration and APIKeySignatures GORM model; APIKey gets APIKeySignature []APIKeySignatures association; indexes and constraints added.
Signatures: repository & DTOs
database/repository/api_keys.go, database/repository/repoentity/api_keys.go
New repo methods to create/find/disable/increase signature tries; DTOs APIKeyCreateSignatureFor and FindSignatureFrom.
DB indices & attrs cleanup
database/infra/migrations/000002_api_keys.up.sql, database/attrs.go
Rename api_keys indices (add idx_api_keys_ prefix); remove GORM struct tags from LikesAttrs fields.
HTTP endpoint & routing
handler/signatures.go, handler/payload/signatures.go, metal/kernel/router.go, metal/kernel/app.go
Add SignaturesHandler, request/response payloads, constructor; register POST /generate-signature via new PublicPipelineFor and wire router validator on boot.
Token middleware & guards
pkg/middleware/token_middleware.go, pkg/middleware/headers.go, pkg/middleware/mwguards/mw_token_guard.go, pkg/middleware/mwguards/mw_response_messages.go, pkg/middleware/valid_timestamp.go, pkg/middleware/pipeline.go
Replace per-header flow with AuthTokenHeaders; introduce guard-based MWTokenGuard, standardized error builders, public middleware helpers (ValidateAndGetHeaders, AttachContext, GuardDependencies, HasInvalidFormat, HasInvalidSignature); simplify timestamp helper; tests updated accordingly.
HTTP errors & schema
pkg/http/schema.go, pkg/http/handler.go, pkg/http/response.go
Add Data map[string]any to error payloads; include err.Data in responses; add logging helpers and UnprocessableEntity.
Auth utilities
pkg/auth/signature.go, pkg/auth/encryption.go
New HMAC-SHA256 signature helpers (CreateSignature, SignatureToString, VerifySignature); adjust AES key generation error return value.
Portal constants & helpers
pkg/portal/consts.go, pkg/portal/support.go
Add DatesLayout, MaxSignaturesTries, X-API-* header constants and context keys; add GenerateURL respecting forwarded headers.
CLI/menu & minor UX
metal/cli/main.go, metal/cli/panel/menu.go
Remove app encryption key generator, collapse menu options, rename menu item to “Generate API account HTTP key pair”.
Minor formatting/imports
database/repository/posts.go, handler/categories.go, metal/cli/accounts/*, pkg/gorm/support.go, database/connection.go, pkg/cache/ttl_cache_useonce_test.go
Import reorderings, blank-line insertions, and formatting-only edits.
Tests updated
pkg/middleware/token_middleware_test.go, pkg/middleware/token_middleware_additional_test.go, pkg/portal/client_test.go
Tests switched to exported middleware methods and small test input/format adjustments; semantics unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant Caddy
  participant API as API Server
  participant Router
  participant Handler as SignaturesHandler
  participant Repo as ApiKeys Repo
  participant DB

  Client->>Caddy: POST /generate-signature (JSON + X-API-Intended-Origin)
  Caddy->>API: Reverse proxy (forwards Intended-Origin)
  API->>Router: route /generate-signature
  Router->>Handler: Generate(request)
  Handler->>Handler: validate payload & timeframe
  Handler->>Repo: CreateSignatureFor(key, seed, origin, expires)
  Repo->>DB: SELECT/INSERT/UPDATE api_key_signatures
  DB-->>Repo: result
  Repo-->>Handler: APIKeySignatures
  Handler->>Client: 200 JSON {signature, max_tries, cadence}
Loading
sequenceDiagram
  autonumber
  participant Client
  participant Caddy
  participant API as API Server
  participant Router
  participant MW as Token Middleware
  participant Guard as MWTokenGuard
  participant Repo as ApiKeys Repo
  participant DB
  participant Handler

  Client->>Caddy: Request with X-API-* headers
  Caddy->>API: Forward request
  API->>Router: apply pipeline
  Router->>MW: ValidateAndGetHeaders(r, reqID)
  MW->>MW: GuardDependencies()
  MW->>Repo: lookup API key by username
  Repo->>DB: SELECT api_keys
  DB-->>Repo: APIKey
  MW->>Guard: HasInvalidFormat(headers)
  Guard-->>MW: OK / error
  alt signature verification needed
    MW->>Repo: FindSignatureFrom(...) / IncreaseSignatureTries(...)
    Repo->>DB: SELECT/UPDATE api_key_signatures
    DB-->>Repo: result
  end
  MW->>Router: AttachContext(headers)
  Router->>Handler: invoke handler
  Handler-->>Client: response (errors include Data)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

Suggested labels

codex

Poem

Thump-thump I tap with tiny paws,
I sign the keys and mind the cause.
Headers hop through proxy gates,
Guards keep watch while time awaits.
Bugs beware — the rabbits code with claws. 🥕


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c452421 and 58f3066.

📒 Files selected for processing (5)
  • database/infra/migrations/000003_api_keys_signatures.up.sql (1 hunks)
  • database/repository/api_keys.go (2 hunks)
  • handler/signatures.go (1 hunks)
  • main.go (1 hunks)
  • pkg/auth/signature.go (1 hunks)
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/token-endpoint

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

gemini-code-assist[bot]

This comment was marked as resolved.

gemini-code-assist[bot]

This comment was marked as resolved.

@gocanto gocanto force-pushed the feature/token-endpoint branch from e07d58b to 27a9f73 Compare September 4, 2025 08:26
@gocanto gocanto marked this pull request as ready for review September 5, 2025 09:20
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 24

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
metal/cli/accounts/handler.go (1)

51-53: Mask secrets in CLI output (leaks plaintext keys).

You're printing full Public/Secret keys. This is a security footgun—CreateSignature already uses SafeDisplay. Align here to avoid accidental exposure.

Apply this diff:

-	cli.Blueln("   > " + fmt.Sprintf("Public Key: %s", token.PublicKey))
-	cli.Blueln("   > " + fmt.Sprintf("Secret Key: %s", token.SecretKey))
+	cli.Blueln("   > " + fmt.Sprintf("Public Key: %s", auth.SafeDisplay(token.PublicKey)))
+	cli.Blueln("   > " + fmt.Sprintf("Secret Key: %s", auth.SafeDisplay(token.SecretKey)))
♻️ Duplicate comments (2)
database/infra/migrations/000003_api_keys_signatures.up.sql (2)

23-24: Consider more descriptive index names.

The index names could be more specific to better indicate they are single-column indexes.

Apply this diff to rename the indexes for better clarity:

-CREATE INDEX idx_api_key_signatures_created_at ON api_key_signatures(created_at);
-CREATE INDEX idx_api_key_signatures_deleted_at ON api_key_signatures(deleted_at);
+CREATE INDEX idx_created_at ON api_key_signatures(created_at);
+CREATE INDEX idx_deleted_at ON api_key_signatures(deleted_at);

19-19: Redundant index on composite columns.

The index idx_api_key_signatures_signature_created_at on (signature, created_at) is redundant because the unique constraint api_key_signatures_unique_signature on (signature, api_key_id, created_at) already creates an index that can be efficiently used for queries filtering by signature or (signature, created_at).

Remove the redundant index to reduce write overhead and simplify the schema:

-CREATE INDEX idx_api_key_signatures_signature_created_at ON api_key_signatures(signature, created_at);
🧹 Nitpick comments (36)
pkg/cache/ttl_cache_useonce_test.go (1)

28-33: Harden timing to reduce flakiness on slow CI.

The 50ms buffer over ttl can still flake under load. Consider doubling the TTL window or polling. Minimal change:

-        time.Sleep(ttl + 50*time.Millisecond)
+        // Add a larger buffer to account for scheduler jitter in CI.
+        time.Sleep(2*ttl + 50*time.Millisecond)
metal/cli/accounts/factory_test.go (1)

10-13: Optional: run tests in parallel if DB/env are isolated

If clitest.MakeTestConnection creates an isolated DB per test, consider t.Parallel() to speed up the suite.

 func TestMakeHandler(t *testing.T) {
+	t.Parallel()
 	conn := clitest.MakeTestConnection(t, &database.APIKey{})
 	h, err := MakeHandler(conn, clitest.MakeTestEnv())
@@
 func TestMakeHandlerInvalidKey(t *testing.T) {
+	t.Parallel()
 	conn := clitest.MakeTestConnection(t)
 	env := clitest.MakeTestEnv()

Also applies to: 33-36

metal/cli/accounts/handler.go (2)

82-88: Fix mislabeled section and mask signature.

"----- Encrypted Values -----" precedes a non-encrypted Signature. Move it under its own header and mask.

Apply this diff:

-	cli.Successln("\nThe given account has been found successfully!\n")
-	cli.Blueln("   > " + fmt.Sprintf("Account name: %s", token.AccountName))
-	cli.Blueln("   > " + fmt.Sprintf("Public Key: %s", auth.SafeDisplay(token.PublicKey)))
-	cli.Blueln("   > " + fmt.Sprintf("Secret Key: %s", auth.SafeDisplay(token.SecretKey)))
-	cli.Warningln("----- Encrypted Values -----")
-	cli.Magentaln("   > " + fmt.Sprintf("Signature: %s", signature))
+	cli.Successln("\nThe given account has been found successfully!\n")
+	cli.Blueln("   > " + fmt.Sprintf("Account name: %s", token.AccountName))
+	cli.Blueln("   > " + fmt.Sprintf("Public Key: %s", auth.SafeDisplay(token.PublicKey)))
+	cli.Blueln("   > " + fmt.Sprintf("Secret Key: %s", auth.SafeDisplay(token.SecretKey)))
+	cli.Warningln("----- Signature -----")
+	cli.Magentaln("   > " + fmt.Sprintf("Signature: %s", auth.SafeDisplay(signature)))
+	cli.Warningln("----- Encrypted Values -----")

51-57: Minor: avoid string concat around Sprintf.

Use a single Sprintf with the prefix to reduce allocations.

Example:

-	cli.Blueln("   > " + fmt.Sprintf("Account name: %s", token.AccountName))
+	cli.Blueln(fmt.Sprintf("   > Account name: %s", token.AccountName))

Also applies to: 83-87

metal/cli/accounts/factory.go (2)

24-26: Prefer %w for error wrapping.

Keeps original error for unwrapping and tracing.

-		return nil, fmt.Errorf("failed to make token handler: %v", err)
+		return nil, fmt.Errorf("failed to make token handler: %w", err)

12-17: Consider wiring IsDebugging from env.

Field is hardcoded false; optionally derive from env to control masking/verbosity behavior suggested elsewhere.

Also applies to: 28-33

pkg/gorm/support.go (1)

17-19: Simplify HasDbIssues.

Equivalent to err != nil; reduces double checks.

-func HasDbIssues(err error) bool {
-	return IsNotFound(err) || IsFoundButHasErrors(err)
-}
+func HasDbIssues(err error) bool {
+	return err != nil
+}
database/infra/migrations/000002_api_keys.up.sql (1)

7-9: updated_at won't auto-change on UPDATE.

Consider a trigger to maintain updated_at.

Example (Postgres):

CREATE OR REPLACE FUNCTION set_updated_at()
RETURNS TRIGGER AS $$
BEGIN
  NEW.updated_at = CURRENT_TIMESTAMP;
  RETURN NEW;
END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER trg_api_keys_updated_at
BEFORE UPDATE ON api_keys
FOR EACH ROW EXECUTE FUNCTION set_updated_at();
database/repository/posts.go (2)

34-41: Guard against non-positive pages; avoid negative offsets.

If Page is 0, offset becomes negative.

-	offset := (paginate.Page - 1) * paginate.Limit
+	page := paginate.Page
+	if page < 1 {
+		page = 1
+	}
+	offset := (page - 1) * paginate.Limit

42-44: Distinct() can be expensive.

If JOINs cause dupes, consider Select("posts.*").Group("posts.id") or DISTINCT ON(posts.id) to let the planner use indexes more effectively.

pkg/auth/encryption.go (1)

18-20: Return nil key on failure and keep error context.

Returning a non-nil empty slice can be misleading; also the previous wrapped context was useful.

-	if _, err := rand.Read(key); err != nil {
-		return []byte(""), err
-	}
+	if _, err := rand.Read(key); err != nil {
+		return nil, fmt.Errorf("failed to generate random key: %w", err)
+	}
handler/categories.go (1)

48-52: Set JSON Content-Type header before encoding.

Helps clients and tools interpret the body correctly.

-	if err := json.NewEncoder(w).Encode(items); err != nil {
+	w.Header().Set("Content-Type", "application/json; charset=utf-8")
+	if err := json.NewEncoder(w).Encode(items); err != nil {
pkg/portal/support.go (1)

26-42: Harden X-Forwarded- handling and add safety checks.*

Normalize/sanitize header values, take first when comma-separated, and guard nil requests. This reduces header-injection risk when not behind a trusted proxy.

-func GenerateURL(r *baseHttp.Request) string {
-	scheme := "http"
-	if r.TLS != nil {
-		scheme = "https"
-	}
-
-	if v := r.Header.Get("X-Forwarded-Proto"); v != "" {
-		scheme = v
-	}
-
-	host := r.Host
-	if v := r.Header.Get("X-Forwarded-Host"); v != "" {
-		host = v
-	}
-
-	return scheme + "://" + host + r.URL.RequestURI()
-}
+func GenerateURL(r *baseHttp.Request) string {
+	if r == nil {
+		return ""
+	}
+
+	scheme := "http"
+	if r.TLS != nil {
+		scheme = "https"
+	}
+	if v := strings.ToLower(strings.TrimSpace(r.Header.Get("X-Forwarded-Proto"))); v == "https" || v == "http" {
+		scheme = v
+	}
+
+	host := strings.TrimSpace(r.Host)
+	if v := strings.TrimSpace(r.Header.Get("X-Forwarded-Host")); v != "" {
+		host = strings.TrimSpace(strings.SplitN(v, ",", 2)[0])
+	}
+	if host == "" && r.URL != nil && r.URL.Host != "" {
+		host = r.URL.Host
+	}
+
+	return scheme + "://" + host + r.URL.RequestURI()
+}

Would this run exclusively behind trusted reverse proxies in prod? If not, consider whitelisting allowed hosts.

pkg/middleware/headers.go (1)

3-12: Add doc comment for exported type and align with header constants.

Improves linting and discoverability; ensure field usage aligns with portal header constants.

 type AuthTokenHeaders struct {
+	// AuthTokenHeaders carries request authentication header values extracted by middleware.
 	AccountName       string
 	PublicKey         string
 	Signature         string
 	Timestamp         string
 	Nonce             string
 	ClientIP          string
 	RequestID         string
 	IntendedOriginURL string
 }

Please confirm these map 1:1 with constants in pkg/portal/consts.go.

pkg/http/schema.go (1)

6-8: Prefer omitting empty error data in JSON responses

Avoid "data": null in responses by tagging the field with omitempty on both structs.

 type ErrorResponse struct {
-	Error  string         `json:"error"`
-	Status int            `json:"status"`
-	Data   map[string]any `json:"data"`
+	Error  string         `json:"error"`
+	Status int            `json:"status"`
+	Data   map[string]any `json:"data,omitempty"`
 }

 type ApiError struct {
-	Message string         `json:"message"`
-	Status  int            `json:"status"`
-	Data    map[string]any `json:"data"`
+	Message string         `json:"message"`
+	Status  int            `json:"status"`
+	Data    map[string]any `json:"data,omitempty"`
 }

Also applies to: 12-14

pkg/auth/signature.go (1)

9-14: Ignore of Write return is fine, but make it explicit

hmac.Hash.Write never returns a non-nil error; underscore the return to document intent.

-	mac.Write(message)
+	_, _ = mac.Write(message)
pkg/http/handler.go (1)

20-20: Forwarding error data is good; consider omitting nulls and enriching logs

  • With the current tags, responses will include "data": null. Pair this with omitempty on the struct (see schema.go suggestion) to omit empty data.
  • Consider logging attached error data to aid debugging.

Additional log improvement (outside this hunk):

// before:
slog.Error("API Error", "message", err.Message, "status", err.Status)
// after:
slog.Error("API Error", "message", err.Message, "status", err.Status, "data", err.Data)
pkg/middleware/valid_timestamp.go (3)

33-36: Default the clock in the constructor to remove runtime nil-check.

Set now in the ctor when nil so Validate can assume it’s non-nil.

 func NewValidTimestamp(ts string, now func() time.Time) ValidTimestamp {
-  return ValidTimestamp{
-    ts:  ts,
-    now: now,
-  }
+  if now == nil {
+    now = time.Now
+  }
+  return ValidTimestamp{ts: ts, now: now}
 }
 
 func (v ValidTimestamp) Validate(skew time.Duration, disallowFuture bool) *http.ApiError {
@@
-  nowFn := v.now
-  if nowFn == nil {
-    nowFn = time.Now
-  }
-  now := nowFn().Unix()
+  now := v.now().Unix()

28-31: Consider accepting millisecond-precision timestamps.

If clients send Unix ms, current parse treats them as seconds and fails window checks. Simple normalization avoids client confusion.

- epoch, err := strconv.ParseInt(v.ts, 10, 64)
+ epoch, err := strconv.ParseInt(v.ts, 10, 64)
   if err != nil {
     return &http.ApiError{Message: "Invalid authentication headers", Status: baseHttp.StatusUnauthorized}
   }
+ // Normalize ms → s if needed (13+ digits)
+ if len(v.ts) >= 13 {
+   epoch = epoch / 1000
+ }

11-14: Add brief GoDoc on the exported type.

Add a one-line comment to appease linters and clarify expected epoch units (seconds vs ms).

pkg/portal/consts.go (1)

3-4: Prefer RFC3339 for API date serialization.

If DatesLayout is used in payloads/logs, RFC3339/RFC3339Nano improves interoperability and timezone clarity.

-const DatesLayout = "2006-01-02 15:04:05"
+const DatesLayout = time.RFC3339

Note: requires importing time and updating formatters accordingly.

caddy/Caddyfile.local (2)

16-21: Add Vary to prevent cache poisoning across origins.

Helps shared proxies/CDNs cache distinct variants by Origin and requested headers.

 header {
@@
-    Access-Control-Expose-Headers "ETag, X-Request-ID"
+    Access-Control-Expose-Headers "ETag, X-Request-ID"
+    Vary "Origin, Access-Control-Request-Method, Access-Control-Request-Headers"
 }

33-34: Mirror Vary on preflight for completeness.

-    header Access-Control-Max-Age "86400"
+    header Access-Control-Max-Age "86400"
+    header Vary "Origin, Access-Control-Request-Method, Access-Control-Request-Headers"
caddy/Caddyfile.prod (2)

34-39: Add Vary to resource responses to harden caching.

 		header {
@@
-			Access-Control-Expose-Headers "ETag, X-Request-ID"
+			Access-Control-Expose-Headers "ETag, X-Request-ID"
+			Vary "Origin, Access-Control-Request-Method, Access-Control-Request-Headers"
 		}

46-53: Echo Vary on preflight.

 			header Access-Control-Max-Age "86400"
+			header Vary "Origin, Access-Control-Request-Method, Access-Control-Request-Headers"
 			respond 204
metal/kernel/router.go (1)

75-80: Harden Signature() against nil deps and consider basic rate limiting

Guard against nil validator; public endpoint may need rate limiting to prevent abuse.

Apply this diff:

 func (r *Router) Signature() {
+	if r.validator == nil {
+		r.validator = portal.GetDefaultValidator()
+	}
 	abstract := handler.MakeSignaturesHandler(r.validator, r.Pipeline.ApiKeys)
 	generate := r.PublicPipelineFor(abstract.Generate)

 	r.Mux.HandleFunc("POST /generate-signature", generate)
 }

Additionally, consider adding a lightweight IP-based limiter in the public pipeline.

main.go (1)

60-80: Use portal constant for Intended-Origin header

Prevents string drift across code and Caddy config.

Apply this diff:

-		"X-API-Intended-Origin", //new
+		portal.IntendedOriginHeader, // X-API-Intended-Origin
database/model.go (1)

36-39: Pluralize association field for clarity.
The field name reads singular but holds a slice.

-	APIKeySignature []APIKeySignatures `gorm:"foreignKey:APIKeyID"`
+	Signatures []APIKeySignatures `gorm:"foreignKey:APIKeyID"`
database/repository/repoentity/api_keys.go (1)

9-21: Add brief doc comments and consider Params-style names.
Exported types without comments will trip linters; Params suffix clarifies intent.

+// APIKeyCreateSignatureFor carries the inputs to create a signature for a given API key.
 type APIKeyCreateSignatureFor struct {
 	Key       *database.APIKey
 	ExpiresAt time.Time
 	Seed      []byte
 	Origin    string
 }
 
+// FindSignatureFrom carries the inputs to look up/validate a signature.
 type FindSignatureFrom struct {
 	Key        *database.APIKey
 	Signature  []byte
 	Origin     string
 	ServerTime time.Time
 }

If you prefer, rename to APIKeyCreateSignatureParams and FindSignatureParams.

handler/payload/signatures.go (1)

17-21: Prefer RFC3339 time format (or time.Time) for cadence fields.
String without timezone is ambiguous. Either emit RFC3339 strings or switch to time.Time and let JSON marshalling format them.

Option A (keep strings, specify RFC3339 in handler):

  • Ensure values are time.Now().UTC().Format(time.RFC3339)

Option B (stronger typing):

+import "time"
 
 type SignatureCadenceResponse struct {
-	ReceivedAt string `json:"received_at"`
-	CreatedAt  string `json:"created_at"`
-	ExpiresAt  string `json:"expires_at"`
+	ReceivedAt time.Time `json:"received_at"`
+	CreatedAt  time.Time `json:"created_at"`
+	ExpiresAt  time.Time `json:"expires_at"`
 }
pkg/middleware/mwguards/mw_response_messages.go (1)

16-21: Potential allocation inefficiency in normaliseData

The function pre-allocates a map with len(data) capacity, but this only accounts for the number of maps, not the total number of key-value pairs. If each map contains multiple entries, the resulting map will need to grow during iteration.

Consider calculating the actual total capacity needed:

-	result := make(map[string]any, len(data))
+	totalCap := 0
+	for _, d := range data {
+		totalCap += len(d)
+	}
+	result := make(map[string]any, totalCap)
handler/signatures.go (2)

116-116: Short signature expiration window

The signature expires in 30 seconds, which might be too short for clients with network latency or retry logic.

Consider making the expiration time configurable via a constant or environment variable:

-	expiresAt := serverTime.Add(time.Second * 30)
+	expiresAt := serverTime.Add(portal.SignatureExpirationDuration)

111-114: Rename GenerateAESKey to reflect its role as a random seed generator
The function produces cryptographically secure random bytes (length EncryptionKeyLength) and is used only as a seed in CreateSignature, not as an AES encryption key. Rename it—e.g. GenerateRandomSeed or GenerateRandomBytes—and update all its callsites (including tests) for clarity.

pkg/middleware/mwguards/mw_token_guard.go (1)

33-50: Error handling could be more explicit

The Rejects method sets g.Error as a side effect but also returns a boolean. This dual-state error handling could lead to confusion about which should be checked.

Consider returning (bool, error) instead of storing error as state:

-func (g *MWTokenGuard) Rejects(data MWTokenGuardData) bool {
+func (g *MWTokenGuard) Rejects(data MWTokenGuardData) (bool, error) {
 	if g.HasInvalidDependencies() {
-		g.Error = fmt.Errorf("invalid mw-token guard dependencies")
-
-		return true
+		return true, fmt.Errorf("invalid mw-token guard dependencies")
 	}

 	if err := g.AccountNotFound(data.Username); err != nil {
-		g.Error = err
-
-		return true
+		return true, err
 	}

-	if g.HasInvalidFormat(data.PublicKey) {
-		return true
+	if invalid, err := g.HasInvalidFormat(data.PublicKey); invalid {
+		return true, err
 	}

-	return false
+	return false, nil
 }
database/repository/api_keys.go (1)

80-80: Inconsistent error message formatting.

The error message has incorrect formatting with an extra comma and space.

Apply this diff to fix the error message:

-		return nil, fmt.Errorf("issue creating the given api keys signature [%s, %s]: ", username, result.Error)
+		return nil, fmt.Errorf("issue creating the given api keys signature [%s]: %s", username, result.Error)
pkg/middleware/token_middleware.go (1)

129-134: Consider allowing optional Intended-Origin header.

The intendedOriginURL is required in all cases (line 129), but there might be legitimate use cases where API calls don't need origin restrictions (e.g., server-to-server calls).

Consider making the Intended-Origin header optional and handling empty origins appropriately in the signature validation logic:

-	if accountName == "" || publicToken == "" || signature == "" || ts == "" || nonce == "" || ip == "" || intendedOriginURL == "" {
+	if accountName == "" || publicToken == "" || signature == "" || ts == "" || nonce == "" || ip == "" {
 		return AuthTokenHeaders{}, mwguards.InvalidRequestError(
 			"Invalid authentication headers / or missing headers",
 			"",
 		)
 	}
+	
+	// intendedOriginURL can be optional for certain use cases
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d4f46b2 and 8c16cc4.

📒 Files selected for processing (38)
  • caddy/Caddyfile.local (2 hunks)
  • caddy/Caddyfile.prod (3 hunks)
  • database/attrs.go (1 hunks)
  • database/infra/migrations/000002_api_keys.up.sql (1 hunks)
  • database/infra/migrations/000003_api_keys_signatures.up.sql (1 hunks)
  • database/model.go (2 hunks)
  • database/repository/api_keys.go (2 hunks)
  • database/repository/posts.go (1 hunks)
  • database/repository/repoentity/api_keys.go (1 hunks)
  • handler/categories.go (1 hunks)
  • handler/payload/signatures.go (1 hunks)
  • handler/signatures.go (1 hunks)
  • main.go (1 hunks)
  • metal/cli/accounts/factory.go (1 hunks)
  • metal/cli/accounts/factory_test.go (1 hunks)
  • metal/cli/accounts/handler.go (1 hunks)
  • metal/cli/main.go (0 hunks)
  • metal/cli/panel/menu.go (1 hunks)
  • metal/kernel/app.go (2 hunks)
  • metal/kernel/router.go (3 hunks)
  • pkg/auth/encryption.go (1 hunks)
  • pkg/auth/signature.go (1 hunks)
  • pkg/cache/ttl_cache_useonce_test.go (2 hunks)
  • pkg/gorm/support.go (1 hunks)
  • pkg/http/handler.go (1 hunks)
  • pkg/http/response.go (2 hunks)
  • pkg/http/schema.go (1 hunks)
  • pkg/middleware/headers.go (1 hunks)
  • pkg/middleware/mwguards/mw_response_messages.go (1 hunks)
  • pkg/middleware/mwguards/mw_token_guard.go (1 hunks)
  • pkg/middleware/pipeline.go (0 hunks)
  • pkg/middleware/token_middleware.go (2 hunks)
  • pkg/middleware/token_middleware_additional_test.go (1 hunks)
  • pkg/middleware/token_middleware_test.go (3 hunks)
  • pkg/middleware/valid_timestamp.go (1 hunks)
  • pkg/portal/client_test.go (2 hunks)
  • pkg/portal/consts.go (1 hunks)
  • pkg/portal/support.go (1 hunks)
💤 Files with no reviewable changes (2)
  • pkg/middleware/pipeline.go
  • metal/cli/main.go
🧰 Additional context used
🧬 Code graph analysis (20)
pkg/gorm/support.go (1)
pkg/gorm/support_test.go (3)
  • TestHasDbIssues (29-41)
  • TestIsFoundButHasErrors (19-27)
  • TestIsNotFound (9-17)
database/repository/repoentity/api_keys.go (1)
database/connection_test.go (1)
  • TestApiKeysWithTestContainer (17-94)
pkg/auth/signature.go (2)
pkg/auth/encryption_test.go (1)
  • TestCreateSignatureFrom (76-83)
metal/cli/main.go (1)
  • generateApiAccountsHTTPSignature (146-164)
pkg/auth/encryption.go (3)
metal/cli/main.go (1)
  • generateAppEncryptionKey (166-183)
pkg/auth/encryption_test.go (3)
  • TestDecryptWrongKey (34-56)
  • TestEncryptDecrypt (5-32)
  • TestDecryptShortCipher (58-68)
pkg/auth/handler_test.go (1)
  • TestTokenHandlerLifecycle (5-33)
pkg/portal/support.go (1)
metal/env/network.go (1)
  • e (16-18)
handler/categories.go (1)
handler/categories_test.go (1)
  • TestCategoriesHandlerIndex_Success (18-81)
pkg/http/response.go (2)
pkg/http/schema.go (1)
  • ApiError (11-15)
pkg/http/response_test.go (1)
  • TestApiErrorHelpers (59-71)
pkg/http/schema.go (2)
pkg/http/schema_test.go (1)
  • TestApiErrorError (5-20)
handler/tests/helpers.go (1)
  • Version (9-12)
pkg/middleware/mwguards/mw_response_messages.go (2)
pkg/http/schema.go (1)
  • ApiError (11-15)
pkg/http/response_test.go (1)
  • TestApiErrorHelpers (59-71)
pkg/middleware/valid_timestamp.go (1)
pkg/middleware/valid_timestamp_test.go (3)
  • TestValidate_NilLogger (102-108)
  • TestNewValidTimestampConstructor (14-28)
  • slogNoop (111-113)
database/model.go (1)
database/connection_test.go (1)
  • TestApiKeysWithTestContainer (17-94)
pkg/middleware/token_middleware_additional_test.go (4)
database/repository/api_keys.go (1)
  • ApiKeys (15-17)
pkg/auth/handler.go (1)
  • TokenHandler (10-14)
pkg/cache/ttl_cache.go (1)
  • NewTTLCache (18-22)
pkg/limiter/limiter.go (1)
  • NewMemoryLimiter (21-27)
pkg/middleware/mwguards/mw_token_guard.go (2)
pkg/auth/handler.go (1)
  • TokenHandler (10-14)
database/repository/api_keys.go (1)
  • ApiKeys (15-17)
metal/kernel/router.go (5)
pkg/middleware/pipeline.go (1)
  • Pipeline (10-14)
pkg/portal/validator.go (1)
  • Validator (13-16)
pkg/http/schema.go (1)
  • ApiHandler (25-25)
pkg/http/handler.go (1)
  • MakeApiHandler (9-28)
database/repository/api_keys.go (1)
  • ApiKeys (15-17)
pkg/portal/client_test.go (1)
pkg/portal/client.go (4)
  • MakeDefaultClient (26-43)
  • GetDefaultTransport (18-24)
  • f (45-79)
  • UserAgent (10-16)
metal/kernel/app.go (2)
metal/kernel/kernel_test.go (1)
  • TestAppBootRoutes (119-177)
metal/kernel/helpers.go (1)
  • a (10-12)
handler/signatures.go (2)
pkg/auth/handler.go (1)
  • SetupNewAccount (49-72)
pkg/auth/handler_test.go (1)
  • TestTokenHandlerLifecycle (5-33)
pkg/cache/ttl_cache_useonce_test.go (2)
pkg/cache/ttl_cache.go (5)
  • NewTTLCache (18-22)
  • c (63-75)
  • mu (12-15)
  • c (44-58)
  • c (26-41)
pkg/cache/ttl_cache_test.go (1)
  • TestTTLCache_UsedAndMark (8-22)
database/repository/api_keys.go (3)
database/repository/repoentity/api_keys.go (2)
  • APIKeyCreateSignatureFor (9-14)
  • FindSignatureFrom (16-21)
pkg/portal/consts.go (1)
  • MaxSignaturesTries (4-4)
pkg/gorm/support.go (1)
  • HasDbIssues (17-19)
pkg/middleware/token_middleware.go (11)
pkg/cache/ttl_cache.go (2)
  • TTLCache (12-15)
  • NewTTLCache (18-22)
database/repository/api_keys.go (1)
  • ApiKeys (15-17)
pkg/limiter/limiter.go (2)
  • MemoryLimiter (12-17)
  • NewMemoryLimiter (21-27)
pkg/portal/consts.go (9)
  • RequestIDHeader (13-13)
  • IntendedOriginHeader (14-14)
  • UsernameHeader (9-9)
  • SignatureHeader (10-10)
  • TokenHeader (8-8)
  • TimestampHeader (11-11)
  • NonceHeader (12-12)
  • AuthAccountNameKey (20-20)
  • RequestIdKey (21-21)
pkg/middleware/mwguards/mw_response_messages.go (5)
  • InvalidRequestError (36-46)
  • UnauthenticatedError (60-70)
  • InvalidTokenFormatError (48-58)
  • RateLimitedError (72-82)
  • NotFound (84-94)
pkg/middleware/valid_timestamp.go (1)
  • NewValidTimestamp (16-21)
pkg/middleware/headers.go (1)
  • AuthTokenHeaders (3-12)
pkg/portal/support.go (1)
  • ParseClientIP (95-110)
pkg/auth/encryption.go (1)
  • ValidateTokenFormat (87-99)
pkg/middleware/mwguards/mw_token_guard.go (2)
  • NewMWTokenGuard (25-30)
  • MWTokenGuardData (20-23)
database/repository/repoentity/api_keys.go (1)
  • FindSignatureFrom (16-21)
🪛 GitHub Check: test (1.24.6)
pkg/middleware/token_middleware_test.go

[failure] 98-98:
cannot use logger (variable of type *slog.Logger) as string value in argument to tm.ValidateAndGetHeaders


[failure] 98-98:
assignment mismatch: 6 variables but tm.ValidateAndGetHeaders returns 2 values

pkg/middleware/token_middleware_additional_test.go

[failure] 88-88:
too many arguments in call to tm.GuardDependencies


[failure] 82-82:
too many arguments in call to tm.GuardDependencies

🪛 GitHub Actions: Tests
pkg/middleware/token_middleware_additional_test.go

[error] 82-82: Go test failed: too many arguments in call to tm.GuardDependencies. Command: 'go test -coverpkg=./... ./pkg/... -coverprofile=coverage-pkg-1.24.6.out'

🔇 Additional comments (29)
pkg/cache/ttl_cache_useonce_test.go (1)

11-14: Resolved: No subtest-level t.Parallel detected
Confirmed only the parent test invokes t.Parallel(); there are no subtests calling t.Parallel(). Safe as-is—just avoid adding Parallel in subtests.

metal/cli/accounts/factory_test.go (1)

7-7: LGTM: import order

Import reordering is fine and consistent with goimports sorting; no functional impact.

metal/cli/accounts/handler.go (2)

5-5: Import grouping LGTM.

Stdlib vs. external separation improves readability.


54-54: Avoid printing full API signature by default.

Signatures are sensitive (derived from secret). Either gate behind a debug flag or mask.

Option A (mask):

-	cli.Blueln("   > " + fmt.Sprintf("API Signature: %s", auth.CreateSignatureFrom(token.AccountName, token.SecretKey)))
+	cli.Blueln("   > " + fmt.Sprintf("API Signature: %s", auth.SafeDisplay(auth.CreateSignatureFrom(token.AccountName, token.SecretKey))))

Option B (gate):

-	cli.Blueln("   > " + fmt.Sprintf("API Signature: %s", auth.CreateSignatureFrom(token.AccountName, token.SecretKey)))
+	if h.IsDebugging {
+		cli.Blueln("   > " + fmt.Sprintf("API Signature: %s", auth.CreateSignatureFrom(token.AccountName, token.SecretKey)))
+	}
metal/cli/accounts/factory.go (1)

5-5: Import grouping LGTM.

Consistent formatting.

pkg/gorm/support.go (1)

5-5: Import spacing LGTM.

No functional change.

database/infra/migrations/000002_api_keys.up.sql (2)

14-17: Index name normalization LGTM.

Consistent idx_api_keys_* naming improves clarity.


1-12: Verify uniqueness constraints reflect intent.

You have UNIQUE(account_name) and UNIQUE(account_name, public_key, secret_key). The first subsumes many use-cases and makes the composite largely redundant. If multiple keys per account are intended, drop the single-column unique and rely on the composite.

Happy to draft a follow-up migration if desired.

database/repository/posts.go (1)

5-5: Import spacing LGTM.

handler/categories.go (1)

5-7: Import alias choice looks good.

Using baseHttp prevents name clashes with the local http package.

pkg/auth/signature.go (1)

16-18: Hex helper looks good

Encoding via hex.EncodeToString is correct and allocation-minimal. No changes needed.

pkg/http/response.go (1)

101-107: 422 helper with attached data looks good

Status and payload shape align with the new ApiError.Data field.

database/attrs.go (1)

42-44: Migrations already enforce NOT NULL and UNIQUE constraints on likes
The likes table migration defines uuid UUID UNIQUE NOT NULL, post_id BIGINT NOT NULL, user_id BIGINT NOT NULL, and a UNIQUE (post_id, user_id) constraint.

caddy/Caddyfile.local (1)

19-19: Header allow-list update looks good.

Adding X-API-Intended-Origin to CORS allow-headers is consistent with backend usage.

caddy/Caddyfile.prod (2)

37-37: CORS allow-list update is consistent.


66-66: Forwarding X-API-Intended-Origin upstream — LGTM.

Ensures upstream can factor intended origin into auth/signature checks.

pkg/portal/client_test.go (2)

15-15: LGTM — write error safely ignored.


29-29: Nil-receiver test still valid with https URL.

Change is benign; test continues to cover the nil client guard.

metal/kernel/router.go (2)

26-31: Ensure validator is always initialized

Router now depends on validator; nil in tests or custom wiring will panic when registering Signature().

Optionally default it when missing (see diff below in Signature()) and update tests to set it.


33-39: PublicPipelineFor looks good

Wrapper cleanly reuses the pipeline sans token middleware.

pkg/middleware/token_middleware_test.go (1)

132-139: AttachContext public API usage looks correct

Non-nil new request with updated context is asserted.

metal/kernel/app.go (2)

45-49: Good: Router now carries validator

Wiring from MakeApp ensures non-nil in prod paths.


77-78: Boot registers new route; update kernel tests to set validator and assert route

Avoid nil validator in tests and add route assertion.

Apply in metal/kernel/kernel_test.go:

 router := Router{
 	Env: env,
 	Mux: http.NewServeMux(),
+	validator: portal.GetDefaultValidator(),
 	Pipeline: middleware.Pipeline{
 		Env:          env,
 		ApiKeys:      &repository.ApiKeys{DB: &database.Connection{}},
 		TokenHandler: handler,
 	},
 	Db: &database.Connection{},
 }
@@
 	routes := []struct {
 		method string
 		path   string
 	}{
+		{"POST", "/generate-signature"},
 		{"GET", "/profile"},
 		{"GET", "/experience"},
 		{"GET", "/projects"},
 		{"GET", "/social"},
 		{"GET", "/talks"},
 		{"GET", "/education"},
 		{"GET", "/recommendations"},
 		{"POST", "/posts"},
 		{"GET", "/posts/slug"},
 		{"GET", "/categories"},
 	}
database/model.go (1)

6-8: LGTM: import placement is fine.

database/repository/repoentity/api_keys.go (1)

6-7: Import path is correct per module path
go.mod declares module github.com/oullin, so github.com/oullin/database is the right import and no change is needed.

Likely an incorrect or invalid review comment.

handler/signatures.go (2)

86-100: Commented-out timestamp validation logic

The isRequestWithinTimeframe function contains commented-out validation logic that would check if request timestamps are within a 5-second window. This is a security feature that should either be enabled or removed entirely.

Is timestamp validation intentionally disabled? If this is temporary, consider:

  1. Adding a TODO comment explaining why it's disabled
  2. Creating a feature flag to toggle this validation
  3. Or removing the dead code if it's no longer needed

If you plan to enable it, the 5-second window might be too restrictive for clients with clock drift.


117-117: Remove this comment: CreateSignature correctly implements HMAC-SHA256 using crypto/hmac and crypto/sha256.

pkg/middleware/mwguards/mw_token_guard.go (2)

81-84: Inefficient byte array conversions for comparison

Converting strings to byte arrays just to compute SHA-256 hashes is unnecessary when comparing the strings directly would suffice, unless there's a specific security requirement for constant-time comparison of hashes.

Is there a security requirement for hashing before comparison? If not, consider simplifying:

-	pBytes := []byte(publicKey)
-	eBytes := []byte(token.PublicKey)
-	hP := sha256.Sum256(pBytes)
-	hE := sha256.Sum256(eBytes)
-
-	if subtle.ConstantTimeCompare(hP[:], hE[:]) != 1 {
+	if subtle.ConstantTimeCompare([]byte(publicKey), []byte(token.PublicKey)) != 1 {

If hashing is required for security (e.g., to prevent timing attacks on long keys), please document this requirement.


52-54: Nil receiver check might be too late

The HasInvalidDependencies method checks if g == nil, but if the receiver is nil, calling this method would already cause a panic before reaching this check.

Remove the unnecessary nil check for the receiver:

 func (g *MWTokenGuard) HasInvalidDependencies() bool {
-	return g == nil || g.KeysRepository == nil || g.TokenHandler == nil
+	return g.KeysRepository == nil || g.TokenHandler == nil
 }

Likely an incorrect or invalid review comment.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (2)
pkg/middleware/mwguards/mw_token_guard.go (1)

86-88: Don’t include key material in error messages.

Even masked, returning any part of a token/key risks leakage in logs/clients. Use a generic message.

Apply:

-    g.Error = fmt.Errorf("invalid provided public token: %s", auth.SafeDisplay(publicKey))
+    g.Error = fmt.Errorf("invalid provided public token")
database/repository/api_keys.go (1)

81-89: Transaction is ineffective (not using tx) and error messages are malformed.

All DB ops inside the Transaction use a.DB.Sql() instead of tx, so the txn provides no isolation. Error formatting also drops the wrapped error and has dangling punctuation.

Apply:

-	err := a.DB.Sql().Transaction(func(tx *baseGorm.DB) error {
+	err := a.DB.Sql().Transaction(func(tx *baseGorm.DB) error {
 		username := entity.Key.AccountName
-		if result := a.DB.Sql().Create(&signature); gorm.HasDbIssues(result.Error) {
-			return fmt.Errorf("issue creating the given api keys signature [%s, %s]: ", username, result.Error)
-		}
+		if result := tx.Create(&signature); gorm.HasDbIssues(result.Error) {
+			return fmt.Errorf("issue creating api key signature for [%s]: %w", username, result.Error)
+		}
 
-		if result := a.DisablePreviousSignatures(entity.Key, signature.UUID, entity.Origin); result != nil {
-			return fmt.Errorf("issue disabling previous api keys signature [%s, %s]: ", username, result)
-		}
+		if err := a.DisablePreviousSignaturesTx(tx, entity.Key, signature.UUID, entity.Origin); err != nil {
+			return fmt.Errorf("issue disabling previous api key signatures for [%s]: %w", username, err)
+		}
 
 		return nil
 	})

Add this helper in the same file (uses the same logic but bound to tx):

func (a ApiKeys) DisablePreviousSignaturesTx(tx *baseGorm.DB, key *database.APIKey, signatureUUID, origin string) error {
	q := tx.
		Model(&database.APIKeySignatures{}).
		Where(tx.
			Where("expired_at IS NULL").
			Or("current_tries > max_tries"),
		).
		Where("api_key_id = ?", key.ID).
		Where(tx.Where("origin = ?", origin).Or("origin = '' OR origin IS NULL")).
		Where("uuid NOT IN (?)", []string{signatureUUID}).
		Update("expired_at", time.Now())
	if gorm.HasDbIssues(q.Error) {
		return q.Error
	}
	return nil
}
🧹 Nitpick comments (12)
database/model.go (1)

37-55: Model conventions, uniqueness scope, and time types.

  • Type/field naming: Go prefers singular type names. Consider renaming APIKeySignatures (type) and APIKey.APIKeySignature (field) for clarity.
  • Uniqueness: uniqueIndex on Signature alone enforces global uniqueness across all API keys. If you want uniqueness per API key, switch to a composite unique (APIKeyID, Signature).
  • Time columns: other parts of the schema use timestamptz; consider aligning for ExpiresAt/ExpiredAt/CreatedAt/UpdatedAt.

Apply if desired:

-   //Associations
-   APIKeySignature []APIKeySignatures `gorm:"foreignKey:APIKeyID"`
+   // Associations
+   Signatures []APIKeySignature `gorm:"foreignKey:APIKeyID"`
-type APIKeySignatures struct {
+type APIKeySignature struct {
   ID           int64          `gorm:"primaryKey"`
   UUID         string         `gorm:"type:uuid;unique;not null"`
-  APIKeyID     int64          `gorm:"not null;index:idx_api_key_signatures_api_key_id"`
+  APIKeyID     int64          `gorm:"not null;index:idx_api_key_signatures_api_key_id;uniqueIndex:uq_api_key_signatures_key_signature,priority:1"`
   MaxTries     int            `gorm:"not null"`
   CurrentTries int            `gorm:"not null"`
   APIKey       APIKey         `gorm:"foreignKey:APIKeyID;references:ID;constraint:OnDelete:CASCADE"`
-  Signature    []byte         `gorm:"not null;uniqueIndex:uq_api_key_signatures_signature"`
+  Signature    []byte         `gorm:"not null;uniqueIndex:uq_api_key_signatures_key_signature,priority:2"`
   Origin       string         `gorm:"type:varchar(255);not null;index:idx_api_key_signatures_origin"`
-  ExpiresAt    time.Time      `gorm:"index:idx_api_key_signatures_expires_at"`
-  ExpiredAt    *time.Time     `gorm:"index:idx_api_key_signatures_expired_at"`
-  CreatedAt    time.Time      `gorm:"index:idx_api_key_signatures_created_at"`
-  UpdatedAt    time.Time      `gorm:"index:idx_api_key_signatures_updated_at"`
+  ExpiresAt    time.Time      `gorm:"type:timestamptz;index:idx_api_key_signatures_expires_at"`
+  ExpiredAt    *time.Time     `gorm:"type:timestamptz;index:idx_api_key_signatures_expired_at"`
+  CreatedAt    time.Time      `gorm:"type:timestamptz;index:idx_api_key_signatures_created_at"`
+  UpdatedAt    time.Time      `gorm:"type:timestamptz;index:idx_api_key_signatures_updated_at"`
   DeletedAt    gorm.DeletedAt `gorm:"index:idx_api_key_signatures_deleted_at"`
 }

If you intentionally want global uniqueness of Signature, keep current tags but confirm the migration mirrors it.

pkg/middleware/mwguards/mw_token_guard.go (4)

52-54: Nil receiver check is ineffective here.

Calling a method on a nil receiver panics before this check runs. Just validate dependencies.

-func (g *MWTokenGuard) HasInvalidDependencies() bool {
-    return g == nil || g.KeysRepository == nil || g.TokenHandler == nil
-}
+func (g *MWTokenGuard) HasInvalidDependencies() bool {
+    return g.KeysRepository == nil || g.TokenHandler == nil
+}

99-104: Avoid emitting full public key in maps/logs.

If this map is logged or returned, mask the key.

-    return map[string]any{
-        "username":   receiver.Username,
-        "public_key": receiver.PublicKey,
-    }
+    return map[string]any{
+        "username":   receiver.Username,
+        "public_key": auth.SafeDisplay(receiver.PublicKey),
+    }

25-30: Constructor style nits (optional).

Prefer returning a pointer and use lower-camel local names.

-func NewMWTokenGuard(apiKeys *repository.ApiKeys, TokenHandler *auth.TokenHandler) MWTokenGuard {
-    return MWTokenGuard{
-        KeysRepository: apiKeys,
-        TokenHandler:   TokenHandler,
-    }
-}
+func NewMWTokenGuard(apiKeys *repository.ApiKeys, tokenHandler *auth.TokenHandler) *MWTokenGuard {
+    return &MWTokenGuard{
+        KeysRepository: apiKeys,
+        TokenHandler:   tokenHandler,
+    }
+}

75-77: Unify external error semantics (optional).

“unable to decode the given account [...] keys” can aid account enumeration. Consider a generic decode error for client-facing paths and keep details for debug logs.

database/repository/api_keys.go (3)

149-163: Origin filter: consider NULLs and avoid TRIM for index use.

Using TRIM(origin) = '' can bypass indexes and ignores NULLs. Prefer origin = '' OR origin IS NULL, which also matches your intent.

Apply:

-		Where(
-			a.DB.Sql().
-				Where("origin = ?", origin).
-				Or("TRIM(origin) = ''"),
-		).
+		Where(a.DB.Sql().Where("origin = ?", origin).Or("origin = '' OR origin IS NULL")).

104-111: Inconsistent expiry comparator (> now vs >= server time).

Active: expires_at > now; Lookup: expires_at >= serverTime. Pick one policy to avoid off-by-one acceptance at the boundary (recommend consistently using >=).

Also applies to: 127-135


68-79: Signature uniqueness scope — confirm intent.

Signature has a unique index globally (model.go). If Seed can repeat across accounts, inserts will fail. Confirm that Seed is per-key unique or change the index to (signature, api_key_id).

pkg/middleware/token_middleware.go (4)

35-47: Wire limiter config to fields; remove hardcoded params.

maxFailPerScope is unused and rateLimiter uses literals; bind them to the configured fields for consistency.

Apply:

-func MakeTokenMiddleware(tokenHandler *auth.TokenHandler, apiKeys *repository.ApiKeys) TokenCheckMiddleware {
-	return TokenCheckMiddleware{
-		maxFailPerScope: 10,
-		disallowFuture:  true,
-		ApiKeys:         apiKeys,
-		now:             time.Now,
-		TokenHandler:    tokenHandler,
-		clockSkew:       5 * time.Minute,
-		failWindow:      1 * time.Minute,
-		nonceTTL:        5 * time.Minute,
-		nonceCache:      cache.NewTTLCache(),
-		rateLimiter:     limiter.NewMemoryLimiter(1*time.Minute, 10),
-	}
-}
+func MakeTokenMiddleware(tokenHandler *auth.TokenHandler, apiKeys *repository.ApiKeys) TokenCheckMiddleware {
+	m := TokenCheckMiddleware{
+		maxFailPerScope: 10,
+		disallowFuture:  true,
+		ApiKeys:         apiKeys,
+		now:             time.Now,
+		TokenHandler:    tokenHandler,
+		clockSkew:       5 * time.Minute,
+		failWindow:      1 * time.Minute,
+		nonceTTL:        5 * time.Minute,
+		nonceCache:      cache.NewTTLCache(),
+	}
+	m.rateLimiter = limiter.NewMemoryLimiter(m.failWindow, m.maxFailPerScope)
+	return m
+}

120-134: Validate Intended-Origin as a URL.

Only checking non-empty allows garbage. Parse and require scheme+host.

Apply:

-import (
+import (
 	"context"
 	"encoding/hex"
 	"fmt"
 	baseHttp "net/http"
+	"net/url"
 	"strings"
 	"time"
 )
 	intendedOriginURL := strings.TrimSpace(r.Header.Get(portal.IntendedOriginHeader))
@@
-	if accountName == "" || publicToken == "" || signature == "" || ts == "" || nonce == "" || ip == "" || intendedOriginURL == "" {
+	if accountName == "" || publicToken == "" || signature == "" || ts == "" || nonce == "" || ip == "" || intendedOriginURL == "" {
 		return AuthTokenHeaders{}, mwguards.InvalidRequestError(
 			"Invalid authentication headers / or missing headers",
 			"",
 		)
 	}
+	if u, e := url.Parse(intendedOriginURL); e != nil || u.Scheme == "" || u.Host == "" {
+		return AuthTokenHeaders{}, mwguards.InvalidRequestError("Invalid intended origin", "")
+	}

208-213: 400 fits better than 404 for bad signature encoding.

Decoding failure is a client format error, not a missing resource. Consider InvalidRequestError.

Apply:

-	if byteSignature, err = hex.DecodeString(headers.Signature); err != nil {
-		t.rateLimiter.Fail(limiterKey)
-
-		return mwguards.NotFound("error decoding signature string", "")
-	}
+	if byteSignature, err = hex.DecodeString(headers.Signature); err != nil {
+		t.rateLimiter.Fail(limiterKey)
+		return mwguards.InvalidRequestError("error decoding signature string", "")
+	}

214-219: Use t.now for determinism and testability.

Avoid direct time.Now to keep behavior injectable.

Apply:

-		ServerTime: time.Now(),
+		ServerTime: t.now(),
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c16cc4 and c452421.

📒 Files selected for processing (8)
  • database/connection.go (1 hunks)
  • database/model.go (2 hunks)
  • database/repository/api_keys.go (2 hunks)
  • handler/payload/signatures.go (1 hunks)
  • metal/cli/panel/menu.go (1 hunks)
  • pkg/middleware/mwguards/mw_token_guard.go (1 hunks)
  • pkg/middleware/token_middleware.go (2 hunks)
  • pkg/portal/consts.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • database/connection.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • handler/payload/signatures.go
  • pkg/portal/consts.go
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/middleware/mwguards/mw_token_guard.go (4)
database/model.go (1)
  • APIKey (27-39)
pkg/auth/handler.go (1)
  • TokenHandler (10-14)
database/repository/api_keys.go (1)
  • ApiKeys (16-18)
pkg/auth/render.go (1)
  • SafeDisplay (5-20)
database/repository/api_keys.go (4)
database/repository/repoentity/api_keys.go (2)
  • APIKeyCreateSignatureFor (9-14)
  • FindSignatureFrom (16-21)
database/model.go (2)
  • APIKeySignatures (41-55)
  • APIKey (27-39)
pkg/portal/consts.go (1)
  • MaxSignaturesTries (4-4)
pkg/gorm/support.go (1)
  • HasDbIssues (17-19)
database/model.go (1)
database/connection_test.go (1)
  • TestApiKeysWithTestContainer (17-94)
pkg/middleware/token_middleware.go (12)
pkg/cache/ttl_cache.go (2)
  • TTLCache (12-15)
  • NewTTLCache (18-22)
database/repository/api_keys.go (1)
  • ApiKeys (16-18)
pkg/limiter/limiter.go (2)
  • MemoryLimiter (12-17)
  • NewMemoryLimiter (21-27)
pkg/http/schema.go (2)
  • ApiHandler (25-25)
  • ApiError (11-15)
pkg/portal/consts.go (9)
  • RequestIDHeader (13-13)
  • IntendedOriginHeader (14-14)
  • UsernameHeader (9-9)
  • SignatureHeader (10-10)
  • TokenHeader (8-8)
  • TimestampHeader (11-11)
  • NonceHeader (12-12)
  • AuthAccountNameKey (20-20)
  • RequestIDKey (21-21)
pkg/middleware/mwguards/mw_response_messages.go (5)
  • InvalidRequestError (36-46)
  • UnauthenticatedError (60-70)
  • InvalidTokenFormatError (48-58)
  • RateLimitedError (72-82)
  • NotFound (84-94)
database/model.go (1)
  • APIKey (27-39)
pkg/middleware/headers.go (1)
  • AuthTokenHeaders (3-12)
pkg/portal/support.go (1)
  • ParseClientIP (95-110)
pkg/auth/encryption.go (1)
  • ValidateTokenFormat (87-99)
pkg/middleware/mwguards/mw_token_guard.go (2)
  • NewMWTokenGuard (25-30)
  • MWTokenGuardData (20-23)
database/repository/repoentity/api_keys.go (1)
  • FindSignatureFrom (16-21)
🔇 Additional comments (2)
metal/cli/panel/menu.go (1)

92-92: Copy fix LGTM.

The menu text now reads “Generate API account HTTP key pair.” Good catch and consistent with the rest of the CLI.

pkg/middleware/token_middleware.go (1)

159-167: Rate limiting coverage looks correct.

Limiter is applied on format rejection, nonce replay, signature miss, and increment failure. Good.

Also applies to: 176-184, 189-197, 223-233

@gocanto gocanto merged commit db3f72a into main Sep 8, 2025
3 of 6 checks passed
@gocanto gocanto deleted the feature/token-endpoint branch September 8, 2025 04:41
@coderabbitai coderabbitai bot mentioned this pull request Sep 23, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants