From e8c78f496dea5145ef577bd32a5c5e40b14bb275 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Tue, 6 Oct 2026 08:35:45 +0200 Subject: [PATCH] Do not leak information about tokens in errors --- services/auth-guest/pkg/server/http/errors.go | 5 -- .../auth-guest/pkg/server/http/redeem_test.go | 7 -- .../pkg/service/authguest/service.go | 15 +++- .../pkg/service/authguest/service_test.go | 82 +++++++++++++++++++ 4 files changed, 93 insertions(+), 16 deletions(-) diff --git a/services/auth-guest/pkg/server/http/errors.go b/services/auth-guest/pkg/server/http/errors.go index 869d0f6234..61952c91ee 100644 --- a/services/auth-guest/pkg/server/http/errors.go +++ b/services/auth-guest/pkg/server/http/errors.go @@ -9,7 +9,6 @@ import ( "net/http" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/authguest" - "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/storage" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/token" ) @@ -39,10 +38,6 @@ func writeRedeemError(w http.ResponseWriter, err error) { status, errorType = http.StatusUnauthorized, "tokenExpired" case errors.Is(re.ErrorType, token.ErrInvalidToken): status, errorType = http.StatusUnauthorized, "tokenInvalid" - case errors.Is(re.ErrorType, storage.ErrNotFound): - status, errorType = http.StatusNotFound, "tokenNotFound" - case errors.Is(re.ErrorType, storage.ErrInvalidHash): - status, errorType = http.StatusUnauthorized, "tokenInvalid" case errors.Is(re.ErrorType, authguest.ErrAlreadyRedeemed): status, errorType = http.StatusConflict, "tokenAlreadyRedeemed" case errors.Is(re.ErrorType, authguest.ErrShareNotFound): diff --git a/services/auth-guest/pkg/server/http/redeem_test.go b/services/auth-guest/pkg/server/http/redeem_test.go index 28a9320601..faf2449f60 100644 --- a/services/auth-guest/pkg/server/http/redeem_test.go +++ b/services/auth-guest/pkg/server/http/redeem_test.go @@ -15,7 +15,6 @@ import ( "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/config" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/authguest" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/authguest/mocks" - "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/storage" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/token" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -83,12 +82,6 @@ func TestRedeemHandlerErrorMapping(t *testing.T) { wantStatus: http.StatusUnauthorized, wantType: "tokenInvalid", }, - { - name: "token not found", - err: &authguest.RedeemError{ErrorType: storage.ErrNotFound}, - wantStatus: http.StatusNotFound, - wantType: "tokenNotFound", - }, { name: "token already redeemed", err: &authguest.RedeemError{ErrorType: authguest.ErrAlreadyRedeemed}, diff --git a/services/auth-guest/pkg/service/authguest/service.go b/services/auth-guest/pkg/service/authguest/service.go index c33e016871..f7ae0be9a3 100644 --- a/services/auth-guest/pkg/service/authguest/service.go +++ b/services/auth-guest/pkg/service/authguest/service.go @@ -134,19 +134,26 @@ func (s *AuthGuestService) CleanupShare(shareID string) error { } // VerifyToken validates a token and returns its stored record. +// +// Until the secret has been verified, all failures are reported as +// token.ErrInvalidToken without a share id, so that a caller holding only part +// of a token learns neither the share id nor whether a record exists. func (s *AuthGuestService) verifyToken(tokenString string) (*storage.Record, error) { tok, err := s.tokenSvc.Parse(tokenString) if err != nil { - return nil, &RedeemError{ErrorType: err} + return nil, &RedeemError{ErrorType: token.ErrInvalidToken} } rec, err := s.store.Get(tok.ShareIDHash) - if err != nil { - return nil, &RedeemError{ErrorType: err} + switch { + case errors.Is(err, storage.ErrNotFound), errors.Is(err, storage.ErrInvalidHash): + return nil, &RedeemError{ErrorType: token.ErrInvalidToken} + case err != nil: + return nil, err } if err := s.tokenSvc.Verify(*tok, rec.SecretHash); err != nil { - return nil, &RedeemError{ErrorType: err, ShareID: rec.ShareID} + return nil, &RedeemError{ErrorType: token.ErrInvalidToken} } if !rec.Expiry.IsZero() && rec.Expiry.Before(time.Now()) { diff --git a/services/auth-guest/pkg/service/authguest/service_test.go b/services/auth-guest/pkg/service/authguest/service_test.go index df033dc596..03e1e20847 100644 --- a/services/auth-guest/pkg/service/authguest/service_test.go +++ b/services/auth-guest/pkg/service/authguest/service_test.go @@ -5,6 +5,8 @@ package authguest import ( "context" + "errors" + "strings" "testing" "time" @@ -158,6 +160,86 @@ func TestVerifyToken(t *testing.T) { } } +func TestVerifyTokenDoesNotDiscloseShare(t *testing.T) { + tok, rec := newToken(t) + // flip the last character of the secret + last := "x" + if strings.HasSuffix(tok, last) { + last = "y" + } + tamperedSecret := tok[:len(tok)-1] + last + + tests := []struct { + name string + token string + setup func(store *storagemocks.Manager) + }{ + { + name: "malformed token", + token: "not-a-token", + }, + { + name: "unknown record", + token: tok, + setup: func(store *storagemocks.Manager) { + store.On("Get", rec.ShareIDHash).Return(storage.Record{}, storage.ErrNotFound) + }, + }, + { + name: "invalid hash", + token: "v1.ab.secret", + setup: func(store *storagemocks.Manager) { + store.On("Get", "ab").Return(storage.Record{}, storage.ErrInvalidHash) + }, + }, + { + name: "wrong secret", + token: tamperedSecret, + setup: func(store *storagemocks.Manager) { + store.On("Get", rec.ShareIDHash).Return(rec, nil) + }, + }, + { + name: "wrong secret on expired and redeemed record", + token: tamperedSecret, + setup: func(store *storagemocks.Manager) { + r := rec + r.Expiry = time.Now().Add(-time.Hour) + r.Redeemed = true + store.On("Get", rec.ShareIDHash).Return(r, nil) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := storagemocks.NewManager(t) + if tt.setup != nil { + tt.setup(store) + } + s := NewAuthGuestService(token.NewTokenService(), store) + + _, err := s.verifyToken(tt.token) + var re *RedeemError + require.ErrorAs(t, err, &re) + assert.ErrorIs(t, re.ErrorType, token.ErrInvalidToken) + assert.Empty(t, re.ShareID) + }) + } +} + +func TestVerifyTokenStorageFailure(t *testing.T) { + tok, rec := newToken(t) + store := storagemocks.NewManager(t) + store.On("Get", rec.ShareIDHash).Return(storage.Record{}, errors.New("disk on fire")) + s := NewAuthGuestService(token.NewTokenService(), store) + + _, err := s.verifyToken(tok) + require.Error(t, err) + var re *RedeemError + assert.False(t, errors.As(err, &re), "storage failures must surface as internal errors") +} + func TestValidateShare(t *testing.T) { share := &collaboration.Share{Id: &collaboration.ShareId{OpaqueId: testShareID}} notExpiredShare := &collaboration.Share{