From 8beb4921c1bea8ecb478638454e879b69e92fc21 Mon Sep 17 00:00:00 2001 From: Kyle Mendell Date: Mon, 13 Jul 2026 11:25:01 -0500 Subject: [PATCH] coderabbit review stuff --- backend/internal/common/errors.go | 8 +++ backend/internal/dto/scim_dto.go | 10 ++++ backend/internal/dto/user_dto.go | 19 +++++-- backend/internal/dto/user_dto_test.go | 36 ++++++++---- .../internal/middleware/auth_middleware.go | 5 ++ backend/internal/oidc/end_session_handler.go | 2 +- backend/internal/webauthn/handler.go | 14 ++--- backend/internal/webauthn/handler_test.go | 56 +++++++++++++++++++ 8 files changed, 125 insertions(+), 25 deletions(-) diff --git a/backend/internal/common/errors.go b/backend/internal/common/errors.go index 14536541..171dff30 100644 --- a/backend/internal/common/errors.go +++ b/backend/internal/common/errors.go @@ -87,6 +87,14 @@ type NotSignedInError struct{} func (e NotSignedInError) Error() string { return "You are not signed in" } func (e NotSignedInError) HttpStatusCode() int { return http.StatusUnauthorized } +func (e NotSignedInError) Is(target error) bool { + switch target.(type) { + case NotSignedInError, *NotSignedInError: + return true + default: + return false + } +} type MissingPermissionError struct{} diff --git a/backend/internal/dto/scim_dto.go b/backend/internal/dto/scim_dto.go index 52e32e39..3fe46ee4 100644 --- a/backend/internal/dto/scim_dto.go +++ b/backend/internal/dto/scim_dto.go @@ -1,8 +1,10 @@ package dto import ( + "net/url" "time" + "github.com/danielgtaylor/huma/v2" datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" ) @@ -21,6 +23,14 @@ type ScimServiceProviderCreateDTO struct { OidcClientID string `json:"oidcClientId" required:"true"` } +func (d *ScimServiceProviderCreateDTO) Resolve(huma.Context) []error { + endpoint, err := url.Parse(d.Endpoint) + if err != nil || !endpoint.IsAbs() { + return []error{&huma.ErrorDetail{Location: "body.endpoint", Message: "Endpoint must be an absolute URI"}} + } + return nil +} + type ScimUser struct { ScimResourceData UserName string `json:"userName"` diff --git a/backend/internal/dto/user_dto.go b/backend/internal/dto/user_dto.go index a4e3bc9f..b6091606 100644 --- a/backend/internal/dto/user_dto.go +++ b/backend/internal/dto/user_dto.go @@ -4,6 +4,8 @@ import ( "errors" "net/mail" "unicode/utf8" + + "github.com/danielgtaylor/huma/v2" ) type UserDto struct { @@ -36,6 +38,17 @@ type UserCreateDto struct { LdapID string `json:"-"` } +func (u UserCreateDto) Resolve(huma.Context) []error { + if u.Email == nil { + return nil + } + address, err := mail.ParseAddress(*u.Email) + if err != nil || address.Address != *u.Email { + return []error{&huma.ErrorDetail{Location: "body.email", Message: "Field validation for 'Email' failed on the 'email' tag"}} + } + return nil +} + //nolint:staticcheck // LDAP callers and their tests rely on the existing capitalized validation text func (u UserCreateDto) Validate() error { if u.Username == "" { @@ -47,12 +60,6 @@ func (u UserCreateDto) Validate() error { if utf8.RuneCountInString(u.Username) > 50 { return errors.New("Field validation for 'Username' failed on the 'max' tag") } - if u.Email != nil { - address, err := mail.ParseAddress(*u.Email) - if err != nil || address.Address != *u.Email { - return errors.New("Field validation for 'Email' failed on the 'email' tag") - } - } if utf8.RuneCountInString(u.FirstName) > 50 { return errors.New("Field validation for 'FirstName' failed on the 'max' tag") } diff --git a/backend/internal/dto/user_dto_test.go b/backend/internal/dto/user_dto_test.go index 62dd6188..d6a769ae 100644 --- a/backend/internal/dto/user_dto_test.go +++ b/backend/internal/dto/user_dto_test.go @@ -63,17 +63,6 @@ func TestUserCreateDto_Validate(t *testing.T) { }, wantErr: "Field validation for 'Username' failed on the 'username' tag", }, - { - name: "invalid email", - input: UserCreateDto{ - Username: "testuser", - Email: new("not-an-email"), - FirstName: "John", - LastName: "Doe", - DisplayName: "John Doe", - }, - wantErr: "Field validation for 'Email' failed on the 'email' tag", - }, { name: "first name too short", input: UserCreateDto{ @@ -112,3 +101,28 @@ func TestUserCreateDto_Validate(t *testing.T) { }) } } + +func TestUserCreateDtoResolveEmail(t *testing.T) { + testCases := []struct { + name string + email *string + wantErr bool + }{ + {name: "empty email", email: nil}, + {name: "exact address", email: new("test@example.com")}, + {name: "invalid address", email: new("not-an-email"), wantErr: true}, + {name: "display name is not an exact address", email: new("Test User "), wantErr: true}, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + errs := (&UserCreateDto{Email: testCase.email}).Resolve(nil) + if testCase.wantErr { + require.Len(t, errs, 1) + require.ErrorContains(t, errs[0], "Field validation for 'Email' failed on the 'email' tag") + return + } + require.Empty(t, errs) + }) + } +} diff --git a/backend/internal/middleware/auth_middleware.go b/backend/internal/middleware/auth_middleware.go index 6d327866..5a6a8edb 100644 --- a/backend/internal/middleware/auth_middleware.go +++ b/backend/internal/middleware/auth_middleware.go @@ -131,14 +131,18 @@ type authenticationResult struct { } func (m *AuthMiddleware) authenticate(c *gin.Context) (authenticationResult, error) { + // Return immediately after successful JWT authentication userID, isAdmin, authenticationMethod, authenticationTime, err := m.jwtMiddleware.Verify(c, m.options.AdminRequired) if err == nil { return authenticationResult{userID, isAdmin, authenticationMethod, authenticationTime}, nil } + + // Fall back only when JWT verification reports that the request is not signed in if !errors.Is(err, &common.NotSignedInError{}) { return authenticationResult{}, err } + // Handle API-key-disabled routes before considering API-key authentication if !m.options.AllowApiKeyAuth { if m.options.SuccessOptional { return authenticationResult{}, nil @@ -149,6 +153,7 @@ func (m *AuthMiddleware) authenticate(c *gin.Context) (authenticationResult, err return authenticationResult{}, err } + // Attempt API-key authentication after JWT reports that the request is not signed in userID, isAdmin, err = m.apiKeyMiddleware.Verify(c, m.options.AdminRequired) if err == nil { return authenticationResult{UserID: userID, IsAdmin: isAdmin}, nil diff --git a/backend/internal/oidc/end_session_handler.go b/backend/internal/oidc/end_session_handler.go index 9f772bf7..aeb05b11 100644 --- a/backend/internal/oidc/end_session_handler.go +++ b/backend/internal/oidc/end_session_handler.go @@ -35,7 +35,7 @@ func (h *endSessionHandler) endSession(c *gin.Context) { return } - http.SetCookie(c.Writer, cookie.NewAccessTokenCookie(0, "")) + http.SetCookie(c.Writer, cookie.NewAccessTokenCookie(-1, "")) if callbackURL == "" { c.Redirect(http.StatusFound, h.baseURL+"/logout") return diff --git a/backend/internal/webauthn/handler.go b/backend/internal/webauthn/handler.go index 4a8b24c1..8c5da4c4 100644 --- a/backend/internal/webauthn/handler.go +++ b/backend/internal/webauthn/handler.go @@ -143,20 +143,20 @@ func (h *handler) updateCredential(ctx context.Context, input *credentialUpdateI } func (h *handler) logout(_ context.Context, _ *httpapi.EmptyInput) (*emptyOutput, error) { - return &emptyOutput{SetCookie: []http.Cookie{*cookie.NewAccessTokenCookie(0, "")}}, nil + return &emptyOutput{SetCookie: []http.Cookie{*cookie.NewAccessTokenCookie(-1, "")}}, nil } func (h *handler) reauthenticate(ctx context.Context, input *optionalCredentialBodyInput) (*emptyOutput, error) { - sessionID, err := sessionID(ctx) - if err != nil { - return nil, err - } - var token string + var err error if input.Body != nil { assertion, parseErr := protocol.ParseCredentialRequestResponseBody(bytes.NewReader(*input.Body)) if parseErr == nil { - token, err = h.service.CreateReauthenticationTokenWithWebauthn(ctx, sessionID, assertion) + sessionCookieID, sessionErr := sessionID(ctx) + if sessionErr != nil { + return nil, sessionErr + } + token, err = h.service.CreateReauthenticationTokenWithWebauthn(ctx, sessionCookieID, assertion) } else { token, err = h.reauthenticateWithAccessToken(ctx) } diff --git a/backend/internal/webauthn/handler_test.go b/backend/internal/webauthn/handler_test.go index 4934e0a8..167ab80d 100644 --- a/backend/internal/webauthn/handler_test.go +++ b/backend/internal/webauthn/handler_test.go @@ -13,7 +13,10 @@ import ( "github.com/gin-gonic/gin" "github.com/stretchr/testify/require" + "github.com/pocket-id/pocket-id/backend/internal/model" + "github.com/pocket-id/pocket-id/backend/internal/utils/cookie" httpapi "github.com/pocket-id/pocket-id/backend/internal/utils/huma" + testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" ) func TestRequestWithBodyReconstructsUnderlyingRequest(t *testing.T) { @@ -51,3 +54,56 @@ func TestRequestWithBodyReconstructsUnderlyingRequest(t *testing.T) { router.ServeHTTP(response, request) require.Equal(t, http.StatusOK, response.Code) } + +func TestLogoutClearsAccessTokenCookie(t *testing.T) { + output, err := (&handler{}).logout(t.Context(), &httpapi.EmptyInput{}) + require.NoError(t, err) + require.Len(t, output.SetCookie, 1) + require.Equal(t, -1, output.SetCookie[0].MaxAge) + require.Contains(t, output.SetCookie[0].String(), "Max-Age=0") +} + +func TestReauthenticateFallsBackWithoutSessionCookie(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + user := model.User{Base: model.Base{ID: "handler-reauth-user"}, Username: "handler-reauth-user"} + require.NoError(t, db.Create(&user).Error) + + signer := newFakeSigner() + accessToken, err := signer.GenerateAccessToken(user, authenticationMethodPhishingResistant) + require.NoError(t, err) + + gin.SetMode(gin.TestMode) + router := gin.New() + api := httpapi.New(router, router.Group("/")) + h := &handler{service: &Service{db: db, signer: signer}} + httpapi.Register(api, huma.Operation{ + OperationID: "test-reauthenticate-fallback", + Method: http.MethodPost, + Path: "/api/test-reauthenticate-fallback", + DefaultStatus: http.StatusNoContent, + }, h.reauthenticate) + + testCases := []struct { + name string + body io.Reader + }{ + {name: "empty body"}, + {name: "invalid assertion", body: strings.NewReader(`{"invalid":true}`)}, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + request := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/api/test-reauthenticate-fallback", testCase.body) + if testCase.body != nil { + request.Header.Set("Content-Type", "application/json") + } + request.AddCookie(cookie.NewAccessTokenCookie(60, accessToken)) + response := httptest.NewRecorder() + + router.ServeHTTP(response, request) + + require.Equal(t, http.StatusNoContent, response.Code) + require.Contains(t, response.Header().Get("Set-Cookie"), cookie.ReauthenticationTokenCookieName+"=") + }) + } +}