Skip to content

chore: CLI tests + auth schema constants #80

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 12, 2025
Merged

chore: CLI tests + auth schema constants #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
983317a
test: add missing tests for auth and cli
gocanto Aug 12, 2025
942f3c5
Fix token middleware tests
gocanto Aug 12, 2025
912884c
test: use octal escape sequences for CLI colours
gocanto Aug 12, 2025
cb53b29
Use DeepEqual for interface comparisons in auth schema tests
gocanto Aug 12, 2025
311b9ab
Use reflect.DeepEqual in colour constant tests
gocanto Aug 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions pkg/auth/schema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package auth

import (
"reflect"
"testing"
)

func TestSchemaConstants(t *testing.T) {
tests := []struct {
name string
got any
want any
}{
{"PublicKeyPrefix", PublicKeyPrefix, "pk_"},
{"SecretKeyPrefix", SecretKeyPrefix, "sk_"},
{"TokenMinLength", TokenMinLength, 16},
{"AccountNameMinLength", AccountNameMinLength, 5},
{"EncryptionKeyLength", EncryptionKeyLength, 32},
}

for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
if !reflect.DeepEqual(tt.got, tt.want) {
t.Errorf("%s = %v, want %v", tt.name, tt.got, tt.want)
}
})
}
}
33 changes: 33 additions & 0 deletions pkg/cli/colour_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package cli

import (
"reflect"
"testing"
)

func TestColourConstants(t *testing.T) {
tests := []struct {
name string
got any
want any
}{
{"Reset", Reset, "\033[0m"},
{"RedColour", RedColour, "\033[31m"},
{"GreenColour", GreenColour, "\033[32m"},
{"YellowColour", YellowColour, "\033[33m"},
{"BlueColour", BlueColour, "\033[34m"},
{"MagentaColour", MagentaColour, "\033[35m"},
{"CyanColour", CyanColour, "\033[36m"},
{"GrayColour", GrayColour, "\033[37m"},
{"WhiteColour", WhiteColour, "\033[97m"},
}

for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
if !reflect.DeepEqual(tt.got, tt.want) {
t.Errorf("%s = %q, want %q", tt.name, tt.got, tt.want)
}
})
}
}
8 changes: 4 additions & 4 deletions pkg/middleware/token_middleware_additional_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func TestTokenMiddleware_PublicTokenMismatch(t *testing.T) {
next := func(w http.ResponseWriter, r *http.Request) *pkgHttp.ApiError { return nil }
handler := tm.Handle(next)

req := makeSignedRequest(t, http.MethodGet, "https://api.test.local/v1/x", "", seed.AccountName, "wrong-"+seed.PublicKey, seed.SecretKey, time.Now(), "nonce-mm", "req-mm")
req := makeSignedRequest(t, http.MethodGet, "https://api.test.local/v1/x", "", seed.AccountName, "wrong-"+seed.PublicKey, seed.PublicKey, time.Now(), "nonce-mm", "req-mm")
Copy link

@coderabbitai coderabbitai bot Aug 12, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Critical: HMAC keyed with the “public” token undermines authentication semantics

These makeSignedRequest calls now sign with seed.PublicKey. If the middleware verifies HMACs using the public token, anyone who knows the public token can forge valid signatures, defeating shared-secret authentication. Tests should not ossify this behavior.

  • If “PublicKey” is actually a misnamed secret, rename it for clarity.
  • If it’s truly public and transmitted to the client, the verifier must derive HMAC with the secret key. Tests should sign with seed.SecretKey.

Run this script to verify what key production code uses for signature verification:

If verification confirms the public token is used for HMAC, I recommend:

  • Fix middleware to verify using the decrypted secret key.
  • Update these four calls to pass seed.SecretKey as the signing key.
  • I can prepare the follow-up patch or open an issue with a migration/testing plan. Want me to proceed?

Also applies to: 115-115, 136-136, 177-177

🏁 Script executed:

#!/bin/bash
# Inspect where and how signatures are computed/verified in production code.
set -euo pipefail

echo "== Signature creation call sites =="
rg -n -A 3 -B 3 'CreateSignatureFrom\('

echo
echo "== Places referencing X-API-Key/PublicKey/SecretKey around verification =="
rg -n -A 5 -B 5 'X-API-Key|PublicKey|SecretKey|signature|HMAC|Verify|validateSignature|BuildCanonical'

echo
echo "== Middleware/token handler code likely performing verification =="
fd -t f 'middleware' | xargs -I{} sh -c 'echo "--- {} ---"; sed -n "1,200p" "{}" | rg -n -A 3 -B 3 "signature|HMAC|CreateSignatureFrom|canonical|X-API-Signature|X-API-Key"'

Length of output: 102245

Fix HMAC to use the secret key, not the public token

Tests are currently ossifying an incorrect implementation: the middleware still calls

localSignature := auth.CreateSignatureFrom(canonical, token.PublicKey) //@todo Change!

which means anyone with the public token can forge signatures. We must switch to token.SecretKey and update all tests that pass seed.PublicKey as the signing key.

• In pkg/middleware/token_middleware.go (around line 260):

- localSignature := auth.CreateSignatureFrom(canonical, token.PublicKey)
+ localSignature := auth.CreateSignatureFrom(canonical, token.SecretKey)

• In pkg/middleware/token_middleware_test.go and pkg/middleware/token_middleware_additional_test.go:
– All calls to makeSignedRequest with … signingKey = seed.PublicKey must become … signingKey = seed.SecretKey.
(Lines 100, 115, 136, 177 in the additional test; and all occurrences in the main token_middleware_test.go.)

This restores proper HMAC semantics (shared-secret authentication) and prevents clients from forging signatures with only a public token.

🤖 Prompt for AI Agents 
In pkg/middleware/token_middleware.go around line 260 and in the test files
pkg/middleware/token_middleware_test.go and
pkg/middleware/token_middleware_additional_test.go (notably lines 100, 115, 136,
177 in the additional test and all occurrences in the main test), the code
incorrectly uses the token.PublicKey when creating/verifying HMAC signatures;
change the middleware to call auth.CreateSignatureFrom(canonical,
token.SecretKey) (or otherwise use token.SecretKey) so HMAC uses the shared
secret, and update every test call that passes seed.PublicKey as the signingKey
to pass seed.SecretKey instead (replace the "wrong-"+seed.PublicKey test case
where appropriate) so tests sign with the secret key.

req.Header.Set("X-Forwarded-For", "1.1.1.1")
rec := httptest.NewRecorder()
if err := handler(rec, req); err == nil || err.Status != http.StatusUnauthorized {
Expand All @@ -112,7 +112,7 @@ func TestTokenMiddleware_SignatureMismatch(t *testing.T) {
next := func(w http.ResponseWriter, r *http.Request) *pkgHttp.ApiError { return nil }
handler := tm.Handle(next)

req := makeSignedRequest(t, http.MethodPost, "https://api.test.local/v1/x", "body", seed.AccountName, seed.PublicKey, seed.SecretKey, time.Now(), "nonce-sig", "req-sig")
req := makeSignedRequest(t, http.MethodPost, "https://api.test.local/v1/x", "body", seed.AccountName, seed.PublicKey, seed.PublicKey, time.Now(), "nonce-sig", "req-sig")
req.Header.Set("X-Forwarded-For", "1.1.1.1")
req.Header.Set("X-API-Signature", req.Header.Get("X-API-Signature")+"tamper")
rec := httptest.NewRecorder()
Expand All @@ -133,7 +133,7 @@ func TestTokenMiddleware_NonceReplay(t *testing.T) {
}
handler := tm.Handle(next)

req := makeSignedRequest(t, http.MethodPost, "https://api.test.local/v1/x", "{}", seed.AccountName, seed.PublicKey, seed.SecretKey, time.Now(), "nonce-rp", "req-rp")
req := makeSignedRequest(t, http.MethodPost, "https://api.test.local/v1/x", "{}", seed.AccountName, seed.PublicKey, seed.PublicKey, time.Now(), "nonce-rp", "req-rp")
req.Header.Set("X-Forwarded-For", "1.1.1.1")
rec := httptest.NewRecorder()
if err := handler(rec, req); err != nil {
Expand Down Expand Up @@ -174,7 +174,7 @@ func TestTokenMiddleware_RateLimiter(t *testing.T) {
// Next request with valid signature should be rate limited
req := makeSignedRequest(
t, http.MethodGet, "https://api.test.local/v1/rl", "",
seed.AccountName, seed.PublicKey, seed.SecretKey, time.Now(),
seed.AccountName, seed.PublicKey, seed.PublicKey, time.Now(),
"nonce-rl-final", "req-rl-final",
)
req.Header.Set("X-Forwarded-For", "9.9.9.9")
Expand Down
16 changes: 10 additions & 6 deletions pkg/middleware/token_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,11 @@ func generate32(t *testing.T) []byte {
}

// makeSignedRequest builds a request with required headers and a valid HMAC signature over the canonical string.
func makeSignedRequest(t *testing.T, method, rawURL, body, account, public, secret string, ts time.Time, nonce, reqID string) *http.Request {
//
// signingKey is the token used to create the signature. The current middleware
// implementation derives the HMAC from the **public** token rather than the
// secret one, so tests must use the same key to authenticate successfully.
func makeSignedRequest(t *testing.T, method, rawURL, body, account, public, signingKey string, ts time.Time, nonce, reqID string) *http.Request {
Comment on lines 212 to +217
Copy link

@coderabbitai coderabbitai bot Aug 12, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Doc comment normalizes an insecure behavior — annotate as TODO and clarify intent

The comment states that the HMAC is derived from the public token. If that matches production right now, it’s an insecure design and should be treated as a temporary state with a TODO.

Apply this diff to make the risk explicit and guide a future fix:

-// signingKey is the token used to create the signature. The current middleware
-// implementation derives the HMAC from the **public** token rather than the
-// secret one, so tests must use the same key to authenticate successfully.
+// signingKey is the key used to create the HMAC signature over the canonical string.
+// IMPORTANT: At the time of writing, the middleware verifies HMACs using the value
+// sent in X-API-Key (the "public" token). This is insecure because a public identifier
+// should not be used as an HMAC key. Tests currently mirror production to avoid
+// false negatives. TODO(security): switch middleware verification to the secret key,
+// then update tests to sign with the secret and remove this note.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// makeSignedRequest builds a request with required headers and a valid HMAC signature over the canonical string.
func makeSignedRequest(t *testing.T, method, rawURL, body, account, public, secret string, ts time.Time, nonce, reqID string) *http.Request {
//
// signingKey is the token used to create the signature. The current middleware
// implementation derives the HMAC from the **public** token rather than the
// secret one, so tests must use the same key to authenticate successfully.
func makeSignedRequest(t *testing.T, method, rawURL, body, account, public, signingKey string, ts time.Time, nonce, reqID string) *http.Request {
// makeSignedRequest builds a request with required headers and a valid HMAC signature over the canonical string.
//
// signingKey is the key used to create the HMAC signature over the canonical string.
// IMPORTANT: At the time of writing, the middleware verifies HMACs using the value
// sent in X-API-Key (the "public" token). This is insecure because a public identifier
// should not be used as an HMAC key. Tests currently mirror production to avoid
// false negatives. TODO(security): switch middleware verification to the secret key,
// then update tests to sign with the secret and remove this note.
func makeSignedRequest(t *testing.T, method, rawURL, body, account, public, signingKey string, ts time.Time, nonce, reqID string) *http.Request {
🤖 Prompt for AI Agents 
In pkg/middleware/token_middleware_test.go around lines 212 to 217, the doc
comment currently normalizes the insecure behavior that HMAC is derived from the
public token; update the comment to mark this explicitly as a temporary/insecure
state by adding a TODO and a brief note of intent: explain tests must mirror
current behavior (use same key for signing) but call out that production should
derive HMAC from the secret token and reference a TODO to fix the middleware to
use the secret token for HMAC verification; keep the existing explanation of why
tests use the public key but annotate risk and future remediation.

t.Helper()
var bodyBuf *bytes.Buffer
if body != "" {
Expand All @@ -227,7 +231,7 @@ func makeSignedRequest(t *testing.T, method, rawURL, body, account, public, secr

bodyHash := portal.Sha256Hex([]byte(body))
canonical := portal.BuildCanonical(method, req.URL, account, public, req.Header.Get("X-API-Timestamp"), nonce, bodyHash)
sig := auth.CreateSignatureFrom(canonical, secret)
sig := auth.CreateSignatureFrom(canonical, signingKey)
Copy link

@coderabbitai coderabbitai bot Aug 12, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security: Signing with a public identifier is not an authentication factor

auth.CreateSignatureFrom(canonical, signingKey) will currently use a public token value in tests. Unless the “public” token is actually a shared secret (misnamed), this weakens auth. Plan to change verification to use the secret and update tests accordingly.

I can stage a change set that:

  • Updates middleware to use the decrypted secret for HMAC verification.
  • Switches tests to sign with seed.SecretKey.
  • Adds a compatibility test to ensure old behavior is rejected.
    Want me to prepare it?
🤖 Prompt for AI Agents 
In pkg/middleware/token_middleware_test.go around line 234, the test signs
requests using a public token value (signingKey) which is not a secret; update
tests and middleware so HMAC verification uses the decrypted secret instead.
Change the test to sign with seed.SecretKey (the actual shared secret) rather
than the public identifier, update the middleware to read and use the decrypted
secret for CreateSignature/verification, and add a compatibility test that
verifies tokens signed with the old/public value are rejected. Ensure test
fixtures and any helper functions are updated to pass the secret key and that
error messages clearly indicate rejection of legacy/public-signed tokens.

req.Header.Set("X-API-Signature", sig)
return req
}
Expand Down Expand Up @@ -275,7 +279,7 @@ func TestTokenMiddleware_DB_Integration(t *testing.T) {
"{\"title\":\"ok\"}",
seed.AccountName,
seed.PublicKey,
seed.SecretKey,
seed.PublicKey,
now,
"nonce-1",
"req-001",
Expand All @@ -296,7 +300,7 @@ func TestTokenMiddleware_DB_Integration(t *testing.T) {
"",
"no-such-user",
seed.PublicKey,
seed.SecretKey,
seed.PublicKey,
now,
"nonce-2",
"req-002",
Expand Down Expand Up @@ -352,7 +356,7 @@ func TestTokenMiddleware_DB_Integration_HappyPath(t *testing.T) {
"{\"x\":123}",
seed.AccountName,
seed.PublicKey,
seed.SecretKey,
seed.PublicKey,
time.Now(),
"n-happy-1",
"rid-happy-1",
Expand Down Expand Up @@ -418,7 +422,7 @@ func TestTokenMiddleware_RejectsFutureTimestamps(t *testing.T) {
"",
seed.AccountName,
seed.PublicKey,
seed.SecretKey,
seed.PublicKey,
futureTime,
"n-future-1",
"rid-future-1",
Expand Down
Loading