diff --git a/backend/internal/auth/middleware_test.go b/backend/internal/auth/middleware_test.go index c6733ed..2a4de0d 100644 --- a/backend/internal/auth/middleware_test.go +++ b/backend/internal/auth/middleware_test.go @@ -63,7 +63,7 @@ func TestRequireSession_TamperedCookie(t *testing.T) { if err != nil { t.Fatalf("mint: %v", err) } - cookie.Value = flipLastChar(cookie.Value) + cookie.Value = flipSignatureChar(cookie.Value) req := httptest.NewRequest(http.MethodGet, "/", nil) req.AddCookie(cookie) rec := httptest.NewRecorder() diff --git a/backend/internal/auth/session_test.go b/backend/internal/auth/session_test.go index fbfaf5a..702e963 100644 --- a/backend/internal/auth/session_test.go +++ b/backend/internal/auth/session_test.go @@ -41,25 +41,38 @@ func TestParseSessionCookie_Tampered(t *testing.T) { if err != nil { t.Fatalf("mint: %v", err) } - cookie.Value = flipLastChar(cookie.Value) + cookie.Value = flipSignatureChar(cookie.Value) if _, err := ParseSessionCookie(cookie, []byte(testSecret)); err == nil { t.Fatal("expected error for tampered cookie") } } -// flipLastChar corrupts a signed token for tamper tests, guaranteeing the -// last character actually changes -- blindly overwriting it with a fixed -// character (e.g. "x") would occasionally be a no-op if that character -// already happened to be there (it's derived from the token's embedded -// timestamp, so this isn't as rare as it sounds), silently passing the -// test without having tampered with anything. -func flipLastChar(s string) string { - last := s[len(s)-1] +// flipSignatureChar corrupts a signed JWT for tamper tests by changing the +// second-to-last character of its base64url signature, guaranteeing the +// decoded signature bytes actually change. Two pitfalls to avoid here: +// 1. Blindly overwriting a character with a fixed replacement (e.g. "x") +// would occasionally be a no-op if that character was already there -- +// it's derived from the token's embedded timestamp, so this isn't as +// rare as it sounds. +// 2. Flipping the *last* character of the signature specifically (as this +// helper used to) is flaky in a subtler way: HMAC-SHA256 produces a +// 32-byte digest, which base64url-encodes to 43 characters with a +// final 3-character group covering only a 2-byte remainder -- the +// true last character encodes 4 real bits plus 2 unused padding bits. +// Go's encoding/base64 ignores those padding bits when decoding +// (non-strict by default), so about 1 in 4 replacement characters for +// that position decode to byte-identical signature bytes, silently +// passing the test without having tampered with anything. The +// second-to-last character of that final group has no such unused +// bits, so corrupting it is deterministic. +func flipSignatureChar(s string) string { + pos := len(s) - 2 + orig := s[pos] replacement := byte('x') - if last == replacement { + if orig == replacement { replacement = 'y' } - return s[:len(s)-1] + string(replacement) + return s[:pos] + string(replacement) + s[pos+1:] } func TestParseSessionCookie_WrongSecret(t *testing.T) {