From d238480c7ad9ce8e8d22de66233e2e2cfc6c112e Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Mon, 15 Sep 2025 10:02:10 +0200 Subject: [PATCH] fix: trim whitespace from API tokens (#19814) --- coderd/httpmw/apikey.go | 5 ++-- coderd/httpmw/rfc6750_extended_test.go | 32 +++++++------------------- coderd/httpmw/rfc6750_test.go | 14 ++--------- 3 files changed, 13 insertions(+), 38 deletions(-) diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index 8fb68579a9..164cff6d18 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -730,13 +730,14 @@ func APITokenFromRequest(r *http.Request) string { // Check Authorization: Bearer header (case-insensitive per RFC 6750) authHeader := r.Header.Get("Authorization") if strings.HasPrefix(strings.ToLower(authHeader), "bearer ") { - return authHeader[7:] // Skip "Bearer " (7 characters) + // Skip "Bearer " (7 characters) and trim surrounding whitespace + return strings.TrimSpace(authHeader[7:]) } // Check access_token query parameter accessToken := r.URL.Query().Get("access_token") if accessToken != "" { - return accessToken + return strings.TrimSpace(accessToken) } return "" diff --git a/coderd/httpmw/rfc6750_extended_test.go b/coderd/httpmw/rfc6750_extended_test.go index 3cd6ca312a..b31cfaf72f 100644 --- a/coderd/httpmw/rfc6750_extended_test.go +++ b/coderd/httpmw/rfc6750_extended_test.go @@ -20,6 +20,8 @@ import ( ) // TestOAuth2BearerTokenSecurityBoundaries tests RFC 6750 security boundaries +// +//nolint:tparallel,paralleltest // Subtests share a DB; run sequentially to avoid Windows DB cleanup flake. func TestOAuth2BearerTokenSecurityBoundaries(t *testing.T) { t.Parallel() @@ -41,8 +43,6 @@ func TestOAuth2BearerTokenSecurityBoundaries(t *testing.T) { }) t.Run("TokenIsolation", func(t *testing.T) { - t.Parallel() - // Create middleware middleware := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ DB: db, @@ -78,8 +78,6 @@ func TestOAuth2BearerTokenSecurityBoundaries(t *testing.T) { }) t.Run("CrossTokenAttempts", func(t *testing.T) { - t.Parallel() - middleware := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ DB: db, }) @@ -101,8 +99,6 @@ func TestOAuth2BearerTokenSecurityBoundaries(t *testing.T) { }) t.Run("TimingAttackResistance", func(t *testing.T) { - t.Parallel() - middleware := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ DB: db, }) @@ -150,6 +146,8 @@ func TestOAuth2BearerTokenSecurityBoundaries(t *testing.T) { } // TestOAuth2BearerTokenMalformedHeaders tests handling of malformed Bearer headers per RFC 6750 +// +//nolint:tparallel,paralleltest // Subtests share a DB; run sequentially to avoid Windows DB cleanup flake. func TestOAuth2BearerTokenMalformedHeaders(t *testing.T) { t.Parallel() @@ -215,8 +213,6 @@ func TestOAuth2BearerTokenMalformedHeaders(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - t.Parallel() - req := httptest.NewRequest("GET", "/test", nil) req.Header.Set("Authorization", test.authHeader) rec := httptest.NewRecorder() @@ -234,6 +230,8 @@ func TestOAuth2BearerTokenMalformedHeaders(t *testing.T) { } // TestOAuth2BearerTokenPrecedence tests token extraction precedence per RFC 6750 +// +//nolint:tparallel,paralleltest // Subtests share a DB; run sequentially to avoid Windows DB cleanup flake. func TestOAuth2BearerTokenPrecedence(t *testing.T) { t.Parallel() @@ -257,8 +255,6 @@ func TestOAuth2BearerTokenPrecedence(t *testing.T) { })) t.Run("CookieTakesPrecedenceOverBearer", func(t *testing.T) { - t.Parallel() - req := httptest.NewRequest("GET", "/test", nil) // Set both cookie and Bearer header - cookie should take precedence req.AddCookie(&http.Cookie{ @@ -274,8 +270,6 @@ func TestOAuth2BearerTokenPrecedence(t *testing.T) { }) t.Run("QueryParameterTakesPrecedenceOverBearer", func(t *testing.T) { - t.Parallel() - // Set both query parameter and Bearer header - query should take precedence u, _ := url.Parse("/test") q := u.Query() @@ -292,8 +286,6 @@ func TestOAuth2BearerTokenPrecedence(t *testing.T) { }) t.Run("BearerHeaderFallback", func(t *testing.T) { - t.Parallel() - // Only set Bearer header - should be used as fallback req := httptest.NewRequest("GET", "/test", nil) req.Header.Set("Authorization", "Bearer "+validToken) @@ -305,8 +297,6 @@ func TestOAuth2BearerTokenPrecedence(t *testing.T) { }) t.Run("AccessTokenQueryParameterFallback", func(t *testing.T) { - t.Parallel() - // Only set access_token query parameter - should be used as fallback u, _ := url.Parse("/test") q := u.Query() @@ -322,8 +312,6 @@ func TestOAuth2BearerTokenPrecedence(t *testing.T) { }) t.Run("MultipleAuthMethodsShouldNotConflict", func(t *testing.T) { - t.Parallel() - // RFC 6750 says clients shouldn't send tokens in multiple ways, // but if they do, we should handle it gracefully by using precedence u, _ := url.Parse("/test") @@ -348,6 +336,8 @@ func TestOAuth2BearerTokenPrecedence(t *testing.T) { } // TestOAuth2WWWAuthenticateCompliance tests WWW-Authenticate header compliance with RFC 6750 +// +//nolint:tparallel,paralleltest // Subtests share a DB; run sequentially to avoid Windows DB cleanup flake. func TestOAuth2WWWAuthenticateCompliance(t *testing.T) { t.Parallel() @@ -363,8 +353,6 @@ func TestOAuth2WWWAuthenticateCompliance(t *testing.T) { })) t.Run("UnauthorizedResponse", func(t *testing.T) { - t.Parallel() - req := httptest.NewRequest("GET", "/test", nil) req.Header.Set("Authorization", "Bearer invalid-token") rec := httptest.NewRecorder() @@ -383,8 +371,6 @@ func TestOAuth2WWWAuthenticateCompliance(t *testing.T) { }) t.Run("ExpiredTokenResponse", func(t *testing.T) { - t.Parallel() - // Create an expired API key _, expiredToken := dbgen.APIKey(t, db, database.APIKey{ UserID: user.ID, @@ -406,8 +392,6 @@ func TestOAuth2WWWAuthenticateCompliance(t *testing.T) { }) t.Run("InsufficientScopeResponse", func(t *testing.T) { - t.Parallel() - // For this test, we'll test with an invalid token to trigger the middleware's // error handling which does set WWW-Authenticate headers for 403 responses // In practice, insufficient scope errors would be handled by RBAC middleware diff --git a/coderd/httpmw/rfc6750_test.go b/coderd/httpmw/rfc6750_test.go index 03b7d2d8c8..afad6cf81a 100644 --- a/coderd/httpmw/rfc6750_test.go +++ b/coderd/httpmw/rfc6750_test.go @@ -19,6 +19,8 @@ import ( // TestRFC6750BearerTokenAuthentication tests that RFC 6750 bearer tokens work correctly // for authentication, including both Authorization header and access_token query parameter methods. +// +//nolint:tparallel,paralleltest // Subtests share a DB; run sequentially to avoid Windows DB cleanup flake. func TestRFC6750BearerTokenAuthentication(t *testing.T) { t.Parallel() @@ -44,8 +46,6 @@ func TestRFC6750BearerTokenAuthentication(t *testing.T) { }) t.Run("AuthorizationBearerHeader", func(t *testing.T) { - t.Parallel() - req := httptest.NewRequest("GET", "/test", nil) req.Header.Set("Authorization", "Bearer "+token) @@ -57,8 +57,6 @@ func TestRFC6750BearerTokenAuthentication(t *testing.T) { }) t.Run("AccessTokenQueryParameter", func(t *testing.T) { - t.Parallel() - req := httptest.NewRequest("GET", "/test?access_token="+url.QueryEscape(token), nil) rw := httptest.NewRecorder() @@ -69,8 +67,6 @@ func TestRFC6750BearerTokenAuthentication(t *testing.T) { }) t.Run("BearerTokenPriorityAfterCustomMethods", func(t *testing.T) { - t.Parallel() - // Create a different token for custom header customKey, customToken := dbgen.APIKey(t, db, database.APIKey{ UserID: user.ID, @@ -98,8 +94,6 @@ func TestRFC6750BearerTokenAuthentication(t *testing.T) { }) t.Run("InvalidBearerToken", func(t *testing.T) { - t.Parallel() - req := httptest.NewRequest("GET", "/test", nil) req.Header.Set("Authorization", "Bearer invalid-token") @@ -118,8 +112,6 @@ func TestRFC6750BearerTokenAuthentication(t *testing.T) { }) t.Run("ExpiredBearerToken", func(t *testing.T) { - t.Parallel() - // Create an expired token _, expiredToken := dbgen.APIKey(t, db, database.APIKey{ UserID: user.ID, @@ -144,8 +136,6 @@ func TestRFC6750BearerTokenAuthentication(t *testing.T) { }) t.Run("MissingBearerToken", func(t *testing.T) { - t.Parallel() - req := httptest.NewRequest("GET", "/test", nil) // No authentication provided