mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(coderd/httpmw): return 500 for internal auth errors (#23352)
## Issue context On `dev.coder.com`, users could successfully log in, briefly see the web UI, and then get redirected back to `/login`. We traced the most reliable repro to viewing Tracy's workspaces on the `/workspaces` page. That page eagerly issues authenticated per-row requests such as: - `POST /api/v2/authcheck` - `GET /api/v2/workspacebuilds/:workspacebuild/parameters` One confirmed failing request was for Tracy's workspace `nav-scroll-fix-1f6b`: - route: `GET /api/v2/workspacebuilds/f2104ae6-7d53-457c-a8df-de831bee76db/parameters` - build owner/workspace: `tracy/nav-scroll-fix-1f6b` The failing response body was: - message: `An internal error occurred. Please try again or contact the system administrator.` - detail: `Internal error fetching API key by id. fetch object: pq: password authentication failed for user "coder"` That showed the request was not actually unauthorized. The server hit an internal database/authentication problem while resolving the session API key. The underlying issue was that DB password rotation had been enabled, it has since been disabled. However, the logout cascade happened because: 1. `APIKeyFromRequest()` returned `ok=false` for both genuine auth failures and internal backend failures. 2. `ValidateAPIKey()` wrapped every `!ok` result as `401 Unauthorized`. 3. `RequireAuth.tsx` signs the user out on any `401` response. So a transient backend/database failure was being misreported as an auth failure, which made the client forcibly log the user out. A useful extra clue was that the installed PWA did not repro. The PWA starts on `/agents`, which avoids the `/workspaces` request fan-out. That helped narrow the problem to the eager authenticated requests on the workspace list rather than to cookies or the login flow itself. ## What changed This PR now fixes the bug without changing the exported `APIKeyFromRequest()` surface: - `ValidateAPIKey()` now uses a new internal helper that returns a typed `ValidateAPIKeyError` - the exported `APIKeyFromRequest()` helper remains compatible for existing callers like `userauth.go` - internal API-key lookup failures are classified as `500 Internal Server Error` plus `Hard: true` - internal `UserRBACSubject()` failures now return `500 Internal Server Error` instead of `401 Unauthorized` - a focused regression test verifies that an internal `GetAPIKeyByID` failure surfaces as `500` This removes the brittle message-based classification and makes the internal-auth-failure path robust for all API-key lookup failures handled by auth middleware.
This commit is contained in:
+50
-28
@@ -248,12 +248,9 @@ func PrecheckAPIKey(cfg ValidateAPIKeyConfig) func(http.Handler) http.Handler {
|
||||
//
|
||||
// Returns (result, nil) on success or (nil, error) on failure.
|
||||
func ValidateAPIKey(ctx context.Context, cfg ValidateAPIKeyConfig, r *http.Request) (*ValidateAPIKeyResult, *ValidateAPIKeyError) {
|
||||
key, resp, ok := APIKeyFromRequest(ctx, cfg.DB, cfg.SessionTokenFunc, r)
|
||||
if !ok {
|
||||
return nil, &ValidateAPIKeyError{
|
||||
Code: http.StatusUnauthorized,
|
||||
Response: resp,
|
||||
}
|
||||
key, valErr := apiKeyFromRequestValidate(ctx, cfg.DB, cfg.SessionTokenFunc, r)
|
||||
if valErr != nil {
|
||||
return nil, valErr
|
||||
}
|
||||
|
||||
// Log the API key ID for all requests that have a valid key
|
||||
@@ -475,7 +472,7 @@ func ValidateAPIKey(ctx context.Context, cfg ValidateAPIKeyConfig, r *http.Reque
|
||||
actor, userStatus, err := UserRBACSubject(ctx, cfg.DB, key.UserID, key.ScopeSet())
|
||||
if err != nil {
|
||||
return nil, &ValidateAPIKeyError{
|
||||
Code: http.StatusUnauthorized,
|
||||
Code: http.StatusInternalServerError,
|
||||
Response: codersdk.Response{
|
||||
Message: internalErrorMessage,
|
||||
Detail: fmt.Sprintf("Internal error fetching user's roles. %s", err.Error()),
|
||||
@@ -492,6 +489,15 @@ func ValidateAPIKey(ctx context.Context, cfg ValidateAPIKeyConfig, r *http.Reque
|
||||
}
|
||||
|
||||
func APIKeyFromRequest(ctx context.Context, db database.Store, sessionTokenFunc func(r *http.Request) string, r *http.Request) (*database.APIKey, codersdk.Response, bool) {
|
||||
key, valErr := apiKeyFromRequestValidate(ctx, db, sessionTokenFunc, r)
|
||||
if valErr != nil {
|
||||
return nil, valErr.Response, false
|
||||
}
|
||||
|
||||
return key, codersdk.Response{}, true
|
||||
}
|
||||
|
||||
func apiKeyFromRequestValidate(ctx context.Context, db database.Store, sessionTokenFunc func(r *http.Request) string, r *http.Request) (*database.APIKey, *ValidateAPIKeyError) {
|
||||
tokenFunc := APITokenFromRequest
|
||||
if sessionTokenFunc != nil {
|
||||
tokenFunc = sessionTokenFunc
|
||||
@@ -499,45 +505,61 @@ func APIKeyFromRequest(ctx context.Context, db database.Store, sessionTokenFunc
|
||||
|
||||
token := tokenFunc(r)
|
||||
if token == "" {
|
||||
return nil, codersdk.Response{
|
||||
Message: SignedOutErrorMessage,
|
||||
Detail: fmt.Sprintf("Cookie %q or query parameter must be provided.", codersdk.SessionTokenCookie),
|
||||
}, false
|
||||
return nil, &ValidateAPIKeyError{
|
||||
Code: http.StatusUnauthorized,
|
||||
Response: codersdk.Response{
|
||||
Message: SignedOutErrorMessage,
|
||||
Detail: fmt.Sprintf("Cookie %q or query parameter must be provided.", codersdk.SessionTokenCookie),
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
keyID, keySecret, err := SplitAPIToken(token)
|
||||
if err != nil {
|
||||
return nil, codersdk.Response{
|
||||
Message: SignedOutErrorMessage,
|
||||
Detail: "Invalid API key format: " + err.Error(),
|
||||
}, false
|
||||
return nil, &ValidateAPIKeyError{
|
||||
Code: http.StatusUnauthorized,
|
||||
Response: codersdk.Response{
|
||||
Message: SignedOutErrorMessage,
|
||||
Detail: "Invalid API key format: " + err.Error(),
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
//nolint:gocritic // System needs to fetch API key to check if it's valid.
|
||||
key, err := db.GetAPIKeyByID(dbauthz.AsSystemRestricted(ctx), keyID)
|
||||
if err != nil {
|
||||
if errors.Is(err, sql.ErrNoRows) {
|
||||
return nil, codersdk.Response{
|
||||
Message: SignedOutErrorMessage,
|
||||
Detail: "API key is invalid.",
|
||||
}, false
|
||||
return nil, &ValidateAPIKeyError{
|
||||
Code: http.StatusUnauthorized,
|
||||
Response: codersdk.Response{
|
||||
Message: SignedOutErrorMessage,
|
||||
Detail: "API key is invalid.",
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
return nil, codersdk.Response{
|
||||
Message: internalErrorMessage,
|
||||
Detail: fmt.Sprintf("Internal error fetching API key by id. %s", err.Error()),
|
||||
}, false
|
||||
return nil, &ValidateAPIKeyError{
|
||||
Code: http.StatusInternalServerError,
|
||||
Response: codersdk.Response{
|
||||
Message: internalErrorMessage,
|
||||
Detail: fmt.Sprintf("Internal error fetching API key by id. %s", err.Error()),
|
||||
},
|
||||
Hard: true,
|
||||
}
|
||||
}
|
||||
|
||||
// Checking to see if the secret is valid.
|
||||
if !apikey.ValidateHash(key.HashedSecret, keySecret) {
|
||||
return nil, codersdk.Response{
|
||||
Message: SignedOutErrorMessage,
|
||||
Detail: "API key secret is invalid.",
|
||||
}, false
|
||||
return nil, &ValidateAPIKeyError{
|
||||
Code: http.StatusUnauthorized,
|
||||
Response: codersdk.Response{
|
||||
Message: SignedOutErrorMessage,
|
||||
Detail: "API key secret is invalid.",
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
return &key, codersdk.Response{}, true
|
||||
return &key, nil
|
||||
}
|
||||
|
||||
// ExtractAPIKey requires authentication using a valid API key. It handles
|
||||
|
||||
@@ -19,12 +19,14 @@ import (
|
||||
"go.uber.org/mock/gomock"
|
||||
"golang.org/x/exp/slices"
|
||||
"golang.org/x/oauth2"
|
||||
"golang.org/x/xerrors"
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
"github.com/coder/coder/v2/coderd/apikey"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||
"github.com/coder/coder/v2/coderd/database/dbgen"
|
||||
"github.com/coder/coder/v2/coderd/database/dbmock"
|
||||
"github.com/coder/coder/v2/coderd/database/dbtestutil"
|
||||
"github.com/coder/coder/v2/coderd/database/dbtime"
|
||||
"github.com/coder/coder/v2/coderd/httpapi"
|
||||
@@ -192,6 +194,31 @@ func TestAPIKey(t *testing.T) {
|
||||
require.Equal(t, http.StatusUnauthorized, res.StatusCode)
|
||||
})
|
||||
|
||||
t.Run("GetAPIKeyByIDInternalError", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctrl := gomock.NewController(t)
|
||||
db := dbmock.NewMockStore(ctrl)
|
||||
id, secret, _ := randomAPIKeyParts()
|
||||
r := httptest.NewRequest("GET", "/", nil)
|
||||
rw := httptest.NewRecorder()
|
||||
r.Header.Set(codersdk.SessionTokenHeader, fmt.Sprintf("%s-%s", id, secret))
|
||||
|
||||
db.EXPECT().GetAPIKeyByID(gomock.Any(), id).Return(database.APIKey{}, xerrors.New("db unavailable"))
|
||||
|
||||
httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
|
||||
DB: db,
|
||||
RedirectToLogin: false,
|
||||
})(successHandler).ServeHTTP(rw, r)
|
||||
res := rw.Result()
|
||||
defer res.Body.Close()
|
||||
require.Equal(t, http.StatusInternalServerError, res.StatusCode)
|
||||
|
||||
var resp codersdk.Response
|
||||
require.NoError(t, json.NewDecoder(res.Body).Decode(&resp))
|
||||
require.NotEqual(t, httpmw.SignedOutErrorMessage, resp.Message)
|
||||
require.Contains(t, resp.Detail, "Internal error fetching API key by id")
|
||||
})
|
||||
|
||||
t.Run("UserLinkNotFound", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
var (
|
||||
|
||||
Reference in New Issue
Block a user