From 0e3295b746abc2588f80c93ee5826b451ffa4fa9 Mon Sep 17 00:00:00 2001 From: Christophe Vila Date: Mon, 27 Jul 2026 10:59:49 +0200 Subject: [PATCH] fix(auth): stop flipLastChar's tamper tests from flaking Corrupting the true last character of a JWT's base64url-encoded HMAC-SHA256 signature is unreliable: that character encodes only 4 real bits plus 2 unused padding bits, and Go's encoding/base64 ignores those padding bits by default -- about 1 in 4 replacement characters decode to byte-identical signature bytes, so the "tampered" cookie still verifies and the test spuriously passes. Corrupting the second-to-last character instead is deterministic, since HMAC-SHA256's fixed 32-byte digest length means that position is always fully significant. Confirmed via 20 repeated runs (previously ~1/3 failure rate). Co-Authored-By: Claude Sonnet 5 --- backend/internal/auth/middleware_test.go | 2 +- backend/internal/auth/session_test.go | 35 ++++++++++++++++-------- 2 files changed, 25 insertions(+), 12 deletions(-) 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) {