diff --git a/api/bundled/openapi.json b/api/bundled/openapi.json index c46fef1a7..f3af48f50 100644 --- a/api/bundled/openapi.json +++ b/api/bundled/openapi.json @@ -151,6 +151,11 @@ "responses": { "200": { "description": "The new access token.", + "headers": { + "Cache-Control": { + "$ref": "#/components/headers/CacheControlNoStore" + } + }, "content": { "application/json": { "schema": { @@ -372,6 +377,11 @@ "responses": { "200": { "description": "The new grant.", + "headers": { + "Cache-Control": { + "$ref": "#/components/headers/CacheControlNoStore" + } + }, "content": { "application/json": { "schema": { @@ -423,6 +433,11 @@ "responses": { "201": { "description": "The admin was created.", + "headers": { + "Cache-Control": { + "$ref": "#/components/headers/CacheControlNoStore" + } + }, "content": { "application/json": { "schema": { @@ -1137,6 +1152,15 @@ "schema": { "type": "string" } + }, + "CacheControlNoStore": { + "description": "Always `no-store`, because the response carries a secret or a token.", + "schema": { + "type": "string", + "enum": [ + "no-store" + ] + } } } } diff --git a/api/bundled/openapi.yaml b/api/bundled/openapi.yaml index 5b07e614d..2134268eb 100644 --- a/api/bundled/openapi.yaml +++ b/api/bundled/openapi.yaml @@ -108,6 +108,9 @@ paths: responses: '200': description: The new access token. + headers: + Cache-Control: + $ref: '#/components/headers/CacheControlNoStore' content: application/json: schema: @@ -243,6 +246,9 @@ paths: responses: '200': description: The new grant. + headers: + Cache-Control: + $ref: '#/components/headers/CacheControlNoStore' content: application/json: schema: @@ -276,6 +282,9 @@ paths: responses: '201': description: The admin was created. + headers: + Cache-Control: + $ref: '#/components/headers/CacheControlNoStore' content: application/json: schema: @@ -793,3 +802,9 @@ components: description: Entity tag for `If-None-Match` revalidation. schema: type: string + CacheControlNoStore: + description: Always `no-store`, because the response carries a secret or a token. + schema: + type: string + enum: + - no-store diff --git a/api/openapi/components/headers/CacheControlNoStore.yaml b/api/openapi/components/headers/CacheControlNoStore.yaml new file mode 100644 index 000000000..092a03fda --- /dev/null +++ b/api/openapi/components/headers/CacheControlNoStore.yaml @@ -0,0 +1,4 @@ +description: Always `no-store`, because the response carries a secret or a token. +schema: + type: string + enum: [no-store] diff --git a/api/openapi/paths/auth.yaml b/api/openapi/paths/auth.yaml index 5f7e3d58d..b6d8c1d9c 100644 --- a/api/openapi/paths/auth.yaml +++ b/api/openapi/paths/auth.yaml @@ -17,6 +17,9 @@ token: responses: '200': description: The new access token. + headers: + Cache-Control: + $ref: ../components/headers/CacheControlNoStore.yaml content: application/json: schema: @@ -130,6 +133,9 @@ login: responses: '200': description: The new grant. + headers: + Cache-Control: + $ref: ../components/headers/CacheControlNoStore.yaml content: application/json: schema: @@ -163,6 +169,9 @@ setup: responses: '201': description: The admin was created. + headers: + Cache-Control: + $ref: ../components/headers/CacheControlNoStore.yaml content: application/json: schema: diff --git a/core/apiauth/cache.go b/core/apiauth/cache.go index d19d53dd5..ab4aaa1e1 100644 --- a/core/apiauth/cache.go +++ b/core/apiauth/cache.go @@ -62,10 +62,6 @@ func (c *livenessCache) put(id string, e livenessEntry, now time.Time, started u break } } - if len(c.evicted) >= maxLivenessEntries { - clear(c.evicted) - c.floor = c.gen - } e.expires = now.Add(c.ttl) c.entries[id] = e } @@ -73,6 +69,10 @@ func (c *livenessCache) put(id string, e livenessEntry, now time.Time, started u func (c *livenessCache) evict(id string) { c.mu.Lock() defer c.mu.Unlock() + if len(c.evicted) >= maxLivenessEntries { + clear(c.evicted) + c.floor = c.gen + } c.gen++ c.evicted[id] = c.gen delete(c.entries, id) diff --git a/core/apiauth/cache_test.go b/core/apiauth/cache_test.go index a3429897d..fde2066f5 100644 --- a/core/apiauth/cache_test.go +++ b/core/apiauth/cache_test.go @@ -65,6 +65,22 @@ var _ = Describe("livenessCache", func() { Expect(ok).To(BeFalse()) }) + It("stays bounded under eviction-only traffic and still drops a fill started before the trim", func() { + started := c.begin() + c.evict("g1") + for i := range 3 * maxLivenessEntries { + c.evict(fmt.Sprint("other", i)) + } + Expect(len(c.evicted)).To(BeNumerically("<=", maxLivenessEntries)) + + c.put("g1", livenessEntry{userID: "u1"}, t0, started) + _, ok := c.get("g1", t0) + Expect(ok).To(BeFalse()) + c.put("g1", livenessEntry{userID: "u1"}, t0, c.begin()) + _, ok = c.get("g1", t0) + Expect(ok).To(BeTrue()) + }) + It("records the last use without extending the TTL", func() { c.put("g1", livenessEntry{userID: "u1"}, t0, c.begin()) c.markUsed("g1", t0.Add(10*time.Second)) diff --git a/core/apiauth/service.go b/core/apiauth/service.go index f16fef5fb..08dc53332 100644 --- a/core/apiauth/service.go +++ b/core/apiauth/service.go @@ -155,8 +155,8 @@ func (s *Service) ResolveGrant(ctx context.Context, secret, ip string) (*Princip if err != nil { return nil, err } - if !s.now().Before(g.LastActivity().Add(IdleExpiry)) { - s.dropGrant(ctx, g.ID) + if idleSince := s.now().Add(-IdleExpiry); g.LastActivity().Before(idleSince) { + s.dropIdle(ctx, g.ID, idleSince) return nil, model.ErrInvalidAuth } u, err := s.loadUser(ctx, g.UserID) @@ -194,16 +194,17 @@ func (s *Service) loadUser(ctx context.Context, userID string) (*model.User, err return u, err } -// dropGrant evicts after deleting, so a concurrent fill cannot re-cache the dead grant. -func (s *Service) dropGrant(ctx context.Context, id string) { - if err := s.ds.Grant().Delete(ctx, id); err != nil { - log.Warn(ctx, "API v1: could not delete dead grant", "grant", id, err) +// dropIdle deletes only still-idle grants, sparing one renewed meanwhile, and evicts after deleting so a +// concurrent fill cannot re-cache the dead grant. +func (s *Service) dropIdle(ctx context.Context, id string, idleSince time.Time) { + if _, err := s.ds.Grant().DeleteIdle(ctx, idleSince); err != nil { + log.Warn(ctx, "API v1: could not delete idle grants", "grant", id, err) } s.cache.evict(id) } -// settleEpoch re-reads grant and user in one read transaction: separate reads can straddle a password -// change and make a kept grant look dead. The delete only fires while the grant is on the epoch seen here. +// settleEpoch re-reads grant and user in one read transaction: separate reads can straddle a password change +// and make a kept grant look dead. Deleting below the snapshot's epoch is safe: a later change only moves kept grants up. func (s *Service) settleEpoch(ctx context.Context, grantID string) (*model.Grant, *model.User, error) { var g *model.Grant var u *model.User @@ -224,8 +225,8 @@ func (s *Service) settleEpoch(ctx context.Context, grantID string) (*model.Grant } if g.UserEpoch != u.TokenEpoch { s.cache.evict(grantID) - if err := s.ds.Grant().DeleteIfEpoch(ctx, grantID, g.UserEpoch); err != nil { - log.Warn(ctx, "API v1: could not delete dead grant", "grant", grantID, err) + if err := s.ds.Grant().DeleteStaleEpochs(ctx, u.ID, u.TokenEpoch); err != nil { + log.Warn(ctx, "API v1: could not delete the user's grants from older epochs", "user", u.ID, "grant", grantID, err) } return nil, nil, model.ErrInvalidAuth } @@ -297,6 +298,7 @@ func (s *Service) liveGrant(ctx context.Context, id string, u *model.User) (live return e, u, nil } +// ListGrants shows only the current epoch: grants left on an older one are dead but only deleted when presented. func (s *Service) ListGrants(ctx context.Context, p *Principal, offset, limit int) (model.Grants, int64, error) { idleSince := s.now().Add(-IdleExpiry) grants, err := s.ds.Grant().GetAllForUser(ctx, p.User.ID, p.User.TokenEpoch, idleSince, offset, limit) @@ -374,6 +376,6 @@ func (s *Service) ChangePassword(ctx context.Context, p *Principal, current, new if err := tx.Grant().SetEpoch(ctx, u.ID, oldEpoch, updated.TokenEpoch, keep); err != nil { return err } - return tx.Grant().DeleteOtherEpochs(ctx, u.ID, updated.TokenEpoch) + return tx.Grant().DeleteStaleEpochs(ctx, u.ID, updated.TokenEpoch) }) } diff --git a/core/apiauth/service_session_test.go b/core/apiauth/service_session_test.go index 1a83d2ee4..573173402 100644 --- a/core/apiauth/service_session_test.go +++ b/core/apiauth/service_session_test.go @@ -63,7 +63,7 @@ var _ = Describe("Service: sessions", func() { _, err := svc.Authenticate(ctx, tok.Token, "") Expect(err).ToNot(HaveOccurred()) - Expect(realDS.Grant().Delete(ctx, issued.Grant.ID)).To(Succeed()) // another node + Expect(realDS.Grant().DeleteForUser(ctx, u.ID, issued.Grant.ID)).To(Succeed()) // another node _, err = svc.Authenticate(ctx, tok.Token, "") Expect(err).ToNot(HaveOccurred()) // still cached now = now.Add(cacheTTL) @@ -223,7 +223,7 @@ var _ = Describe("Service: sessions", func() { _, p, tok := login(ctx, svc, u) _, err := svc.Authenticate(ctx, tok.Token, "") Expect(err).ToNot(HaveOccurred()) - Expect(realDS.Grant().Delete(ctx, p.GrantID)).To(Succeed()) // another node + Expect(realDS.Grant().DeleteForUser(ctx, u.ID, p.GrantID)).To(Succeed()) // another node Expect(svc.Logout(ctx, p)).To(Succeed()) _, err = svc.Authenticate(ctx, tok.Token, "") @@ -302,7 +302,7 @@ var _ = Describe("Service: sessions", func() { It("rejects a caller whose grant was revoked before the change ran", func() { u := createUser(ctx, "pw", false) _, p, _ := login(ctx, svc, u) - Expect(realDS.Grant().Delete(ctx, p.GrantID)).To(Succeed()) + Expect(realDS.Grant().DeleteForUser(ctx, u.ID, p.GrantID)).To(Succeed()) err := svc.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", true) Expect(err).To(MatchError(model.ErrInvalidAuth)) _, err = svc.Login(ctx, u.UserName, "pw", meta, nil) diff --git a/core/apiauth/service_test.go b/core/apiauth/service_test.go index ce6622914..03469e486 100644 --- a/core/apiauth/service_test.go +++ b/core/apiauth/service_test.go @@ -38,6 +38,26 @@ func (p *flakyProps) Get(ctx context.Context, id string) (string, error) { return p.PropertyRepository.Get(ctx, id) } +// renewingDS runs renew right before DeleteIdle, as a node resolving the grant meanwhile would. +type renewingDS struct { + model.DataStore + renew func() +} + +func (d renewingDS) Grant() model.GrantRepository { + return renewingGrants{GrantRepository: d.DataStore.Grant(), renew: d.renew} +} + +type renewingGrants struct { + model.GrantRepository + renew func() +} + +func (g renewingGrants) DeleteIdle(ctx context.Context, idleSince time.Time) (int64, error) { + g.renew() + return g.GrantRepository.DeleteIdle(ctx, idleSince) +} + var meta = ClientMeta{Name: "Living room", Client: "TestApp", ClientVersion: "1.0"} var _ = Describe("Service: grants and tokens", func() { @@ -174,13 +194,32 @@ var _ = Describe("Service: grants and tokens", func() { Expect(err).To(MatchError(model.ErrNotFound)) }) - It("rejects a grant whose epoch is behind the user's", func() { + It("keeps an idle grant that another node renewed before the delete ran", func() { + u := createUser(ctx, "pw", false) + issued, _ := svc.Login(ctx, u.UserName, "pw", meta, nil) + renewedAt := now.Add(IdleExpiry - time.Minute) + racing := New(renewingDS{DataStore: realDS, renew: func() { + Expect(realDS.Grant().Touch(ctx, issued.Grant.ID, "10.0.0.2", renewedAt, renewedAt)).To(Succeed()) + }}) + now = now.Add(IdleExpiry + time.Second) + racing.SetClock(func() time.Time { return now }) + + _, err := racing.ResolveGrant(ctx, issued.Secret, "") + Expect(err).To(MatchError(model.ErrInvalidAuth)) + g, err := realDS.Grant().Get(ctx, issued.Grant.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(g.LastUsedIP).To(Equal("10.0.0.2")) + }) + + It("rejects and deletes a grant whose epoch is behind the user's", func() { u := createUser(ctx, "pw", false) issued, _ := svc.Login(ctx, u.UserName, "pw", meta, nil) u.NewPassword = "changed-elsewhere" Expect(realDS.User().Put(ctx, &u)).To(Succeed()) _, err := svc.ResolveGrant(ctx, issued.Secret, "") Expect(err).To(MatchError(model.ErrInvalidAuth)) + _, err = realDS.Grant().Get(ctx, issued.Grant.ID) + Expect(err).To(MatchError(model.ErrNotFound)) }) }) diff --git a/model/grant.go b/model/grant.go index 1e8fe46e1..ecbf30e41 100644 --- a/model/grant.go +++ b/model/grant.go @@ -59,11 +59,9 @@ type GrantRepository interface { FindBySecretHash(ctx context.Context, hash string) (*Grant, error) GetAllForUser(ctx context.Context, userID string, epoch int, idleSince time.Time, offset, limit int) (Grants, error) CountForUser(ctx context.Context, userID string, epoch int, idleSince time.Time) (int64, error) - Delete(ctx context.Context, id string) error DeleteForUser(ctx context.Context, userID, id string) error - DeleteOtherEpochs(ctx context.Context, userID string, epoch int) error + DeleteStaleEpochs(ctx context.Context, userID string, currentEpoch int) error SetEpoch(ctx context.Context, userID string, fromEpoch, toEpoch int, onlyID string) error - DeleteIfEpoch(ctx context.Context, id string, epoch int) error Touch(ctx context.Context, id, ip string, at, notSince time.Time) error DeleteIdle(ctx context.Context, idleSince time.Time) (int64, error) } diff --git a/persistence/grant_repository.go b/persistence/grant_repository.go index 0e561315c..349099983 100644 --- a/persistence/grant_repository.go +++ b/persistence/grant_repository.go @@ -60,7 +60,6 @@ func (r *grantRepository) findOne(ctx context.Context, cond Sqlizer) (*model.Gra return &g, nil } -// activeForUser skips grants left on an older epoch: they are dead but only deleted when presented. func activeForUser(userID string, epoch int, idleSince time.Time) Sqlizer { return And{Eq{"user_id": userID, "user_epoch": epoch}, Expr(grantLastActivity+" >= ?", idleSince.UTC())} } @@ -78,10 +77,6 @@ func (r *grantRepository) CountForUser(ctx context.Context, userID string, epoch return r.count(ctx, Select().Where(activeForUser(userID, epoch, idleSince))) } -func (r *grantRepository) Delete(ctx context.Context, id string) error { - return r.delete(ctx, Eq{"id": id}) -} - func (r *grantRepository) DeleteForUser(ctx context.Context, userID, id string) error { n, err := r.executeSQL(ctx, Delete(r.tableName).Where(Eq{"id": id, "user_id": userID})) if err != nil { @@ -93,8 +88,8 @@ func (r *grantRepository) DeleteForUser(ctx context.Context, userID, id string) return nil } -func (r *grantRepository) DeleteOtherEpochs(ctx context.Context, userID string, epoch int) error { - return r.delete(ctx, And{Eq{"user_id": userID}, NotEq{"user_epoch": epoch}}) +func (r *grantRepository) DeleteStaleEpochs(ctx context.Context, userID string, currentEpoch int) error { + return r.delete(ctx, And{Eq{"user_id": userID}, Lt{"user_epoch": currentEpoch}}) } // SetEpoch only moves grants still on fromEpoch, so grants killed by an earlier change never come back. @@ -107,10 +102,6 @@ func (r *grantRepository) SetEpoch(ctx context.Context, userID string, fromEpoch return err } -func (r *grantRepository) DeleteIfEpoch(ctx context.Context, id string, epoch int) error { - return r.delete(ctx, Eq{"id": id, "user_epoch": epoch}) -} - func (r *grantRepository) Touch(ctx context.Context, id, ip string, at, notSince time.Time) error { upd := Update(r.tableName).Set("last_used_at", at.UTC()).Set("last_used_ip", ip). Where(And{Eq{"id": id}, Or{Eq{"last_used_at": nil}, Lt{"last_used_at": notSince.UTC()}}}) diff --git a/persistence/grant_repository_test.go b/persistence/grant_repository_test.go index bb140ebd6..2436f8bb4 100644 --- a/persistence/grant_repository_test.go +++ b/persistence/grant_repository_test.go @@ -96,20 +96,18 @@ var _ = Describe("GrantRepository", func() { Expect(err).To(MatchError(model.ErrNotFound)) }) - It("moves epochs forward and deletes grants left on other epochs", func() { + It("moves epochs forward", func() { keep := newGrant(adminUser.ID, "h-keep") - drop := newGrant(adminUser.ID, "h-drop") + stay := newGrant(adminUser.ID, "h-stay") Expect(repo.Put(ctx, keep)).To(Succeed()) - Expect(repo.Put(ctx, drop)).To(Succeed()) + Expect(repo.Put(ctx, stay)).To(Succeed()) Expect(repo.SetEpoch(ctx, adminUser.ID, 0, 3, keep.ID)).To(Succeed()) - Expect(repo.DeleteOtherEpochs(ctx, adminUser.ID, 3)).To(Succeed()) - kept, err := repo.Get(ctx, keep.ID) Expect(err).ToNot(HaveOccurred()) Expect(kept.UserEpoch).To(Equal(3)) - _, err = repo.Get(ctx, drop.ID) - Expect(err).To(MatchError(model.ErrNotFound)) + stayed, _ := repo.Get(ctx, stay.ID) + Expect(stayed.UserEpoch).To(Equal(0)) Expect(repo.SetEpoch(ctx, adminUser.ID, 3, 4, "")).To(Succeed()) kept, _ = repo.Get(ctx, keep.ID) @@ -131,16 +129,29 @@ var _ = Describe("GrantRepository", func() { Expect(got.UserEpoch).To(Equal(3)) }) - It("deletes by epoch only while the row is still on it", func() { - g := newGrant(adminUser.ID, "h-cond") - g.UserEpoch = 5 - Expect(repo.Put(ctx, g)).To(Succeed()) - Expect(repo.DeleteIfEpoch(ctx, g.ID, 4)).To(Succeed()) - _, err := repo.Get(ctx, g.ID) - Expect(err).ToNot(HaveOccurred()) - Expect(repo.DeleteIfEpoch(ctx, g.ID, 5)).To(Succeed()) - _, err = repo.Get(ctx, g.ID) - Expect(err).To(MatchError(model.ErrNotFound)) + It("deletes only the user's grants on an epoch before the current one", func() { + older := newGrant(adminUser.ID, "h-older") + older.UserEpoch = 1 + previous := newGrant(adminUser.ID, "h-previous") + previous.UserEpoch = 4 + current := newGrant(adminUser.ID, "h-current") + current.UserEpoch = 5 + otherUser := newGrant(regularUser.ID, "h-other-user") + otherUser.UserEpoch = 1 + for _, g := range []*model.Grant{older, previous, current, otherUser} { + Expect(repo.Put(ctx, g)).To(Succeed()) + } + + Expect(repo.DeleteStaleEpochs(ctx, adminUser.ID, 5)).To(Succeed()) + + for _, g := range []*model.Grant{older, previous} { + _, err := repo.Get(ctx, g.ID) + Expect(err).To(MatchError(model.ErrNotFound)) + } + for _, g := range []*model.Grant{current, otherUser} { + _, err := repo.Get(ctx, g.ID) + Expect(err).ToNot(HaveOccurred()) + } }) It("touches a never-used grant, then throttles until notSince passes", func() { diff --git a/persistence/persistence_suite_test.go b/persistence/persistence_suite_test.go index ee2794454..dc6134793 100644 --- a/persistence/persistence_suite_test.go +++ b/persistence/persistence_suite_test.go @@ -1,7 +1,9 @@ package persistence import ( + "bytes" "context" + "os" "path/filepath" "testing" "time" @@ -348,6 +350,18 @@ var _ = BeforeSuite(func() { } }) +// captureTraceLogs sends trace logs, SQL included, to a buffer for the rest of the spec. +func captureTraceLogs() *bytes.Buffer { + buf := &bytes.Buffer{} + log.SetOutput(buf) + log.SetLevel(log.LevelTrace) + DeferCleanup(func() { + log.SetOutput(os.Stderr) + log.SetLevel(log.LevelFatal) + }) + return buf +} + func GetDBXBuilder() *dbx.DB { return dbx.NewFromDB(db.Db(), db.Dialect) } diff --git a/persistence/property_repository.go b/persistence/property_repository.go index 87d97491c..916b16430 100644 --- a/persistence/property_repository.go +++ b/persistence/property_repository.go @@ -21,6 +21,7 @@ func NewPropertyRepository(db dbx.Builder) model.PropertyRepository { } func (r propertyRepository) Put(ctx context.Context, id string, value string) error { + ctx = withSecretArgs(ctx) update := Update(r.tableName).Set("value", value).Where(Eq{"id": id}) count, err := r.executeSQL(ctx, update) if err != nil { @@ -35,6 +36,7 @@ func (r propertyRepository) Put(ctx context.Context, id string, value string) er } func (r propertyRepository) PutIfAbsent(ctx context.Context, id string, value string) error { + ctx = withSecretArgs(ctx) insert := Insert(r.tableName).Columns("id", "value").Values(id, value).Options("OR IGNORE") _, err := r.executeSQL(ctx, insert) return err diff --git a/persistence/property_repository_test.go b/persistence/property_repository_test.go index 0f4f9ebe5..f80fa0b36 100644 --- a/persistence/property_repository_test.go +++ b/persistence/property_repository_test.go @@ -39,4 +39,17 @@ var _ = Describe("Property Repository", func() { Expect(pr.PutIfAbsent(ctx, "pia", "second")).To(Succeed()) Expect(pr.Get(ctx, "pia")).To(Equal("first")) }) + + It("never logs the values it writes, but still logs the SQL", func() { + logs := captureTraceLogs() + Expect(pr.Put(ctx, "secret-prop", "inserted-secret")).To(Succeed()) + Expect(pr.Put(ctx, "secret-prop", "updated-secret")).To(Succeed()) + Expect(pr.PutIfAbsent(ctx, "secret-prop-2", "absent-secret")).To(Succeed()) + + Expect(logs.String()).To(ContainSubstring("INSERT INTO property")) + Expect(logs.String()).To(ContainSubstring("UPDATE property")) + Expect(logs.String()).ToNot(ContainSubstring("inserted-secret")) + Expect(logs.String()).ToNot(ContainSubstring("updated-secret")) + Expect(logs.String()).ToNot(ContainSubstring("absent-secret")) + }) }) diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index 03cc6a01b..d929e7ef4 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -658,9 +658,28 @@ func (r sqlRepository) deleteByID(ctx context.Context, id string) error { return nil } +type secretArgsKey struct{} + +// withSecretArgs hides a statement's args from the SQL log: stored secrets are encrypted with a key that may be the public default. +func withSecretArgs(ctx context.Context) context.Context { + return context.WithValue(ctx, secretArgsKey{}, true) +} + +func hasSecretArgs(ctx context.Context) bool { + if ctx == nil { + return false + } + secret, _ := ctx.Value(secretArgsKey{}).(bool) + return secret +} + func (r sqlRepository) logSQL(ctx context.Context, sql string, args dbx.Params, err error, rowsAffected int64, start time.Time) { elapsed := time.Since(start) - fields := []any{ctx, "SQL: `" + sql + "`", "args", args, "rowsAffected", rowsAffected, "elapsedTime", elapsed} + var logArgs any = args + if hasSecretArgs(ctx) { + logArgs = "[REDACTED]" + } + fields := []any{ctx, "SQL: `" + sql + "`", "args", logArgs, "rowsAffected", rowsAffected, "elapsedTime", elapsed} if err == nil || errors.Is(err, context.Canceled) { log.Trace(append(fields, err)...) return diff --git a/persistence/user_repository.go b/persistence/user_repository.go index 20b4e5125..a750bbef4 100644 --- a/persistence/user_repository.go +++ b/persistence/user_repository.go @@ -118,6 +118,7 @@ func (r *userRepository) Put(ctx context.Context, u *model.User) error { } u.UpdatedAt = time.Now() if u.NewPassword != "" { + ctx = withSecretArgs(ctx) _ = r.encryptPassword(ctx, u) } values, err := toSQLArgs(*u) @@ -432,7 +433,7 @@ func (r *userRepository) initPasswordEncryptionKey(ctx context.Context) error { u.NewPassword = u.Password if err := r.encryptPassword(ctx, &u); err == nil { upd := Update(r.tableName).Set("password", u.NewPassword).Where(Eq{"id": u.ID}) - _, err = r.executeSQL(ctx, upd) + _, err = r.executeSQL(withSecretArgs(ctx), upd) if err != nil { log.Error("Password NOT encrypted! This may cause problems!", "user", u.UserName, "id", u.ID, err) } else { diff --git a/persistence/user_repository_test.go b/persistence/user_repository_test.go index 0e776fc3a..4f907b447 100644 --- a/persistence/user_repository_test.go +++ b/persistence/user_repository_test.go @@ -17,6 +17,7 @@ import ( "github.com/navidrome/navidrome/utils/slice" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/pocketbase/dbx" ) var _ = Describe("UserRepository", func() { @@ -74,6 +75,27 @@ var _ = Describe("UserRepository", func() { Expect(err).ToNot(HaveOccurred()) Expect(actual.Password).To(Equal("newpass")) }) + It("never logs the stored password, on insert or update, but still logs the SQL", func() { + logs := captureTraceLogs() + storedPassword := func(id string) string { + var enc string + Expect(GetDBXBuilder().NewQuery("select password from user where id = {:id}"). + Bind(dbx.Params{"id": id}).Row(&enc)).To(Succeed()) + return enc + } + u := model.User{ID: "u-logged", UserName: "u-logged", NewPassword: "first-secret"} + Expect(repo.Put(ctx, &u)).To(Succeed()) + inserted := storedPassword(u.ID) + u.NewPassword = "second-secret" + Expect(repo.Put(ctx, &u)).To(Succeed()) + updated := storedPassword(u.ID) + + Expect(logs.String()).To(ContainSubstring("INSERT INTO user")) + Expect(logs.String()).To(ContainSubstring("UPDATE user")) + for _, secret := range []string{inserted, updated, "first-secret", "second-secret"} { + Expect(logs.String()).ToNot(ContainSubstring(secret)) + } + }) It("persists and reads back the scrobble filter", func() { usr := model.User{ID: "u-filter", UserName: "u-filter", Name: "Filter User", ScrobbleFilter: `{"all":[{"contains":{"title":"????"}}]}`} diff --git a/server/apiv1/api_gen.go b/server/apiv1/api_gen.go index ccca0efc0..e174b0b43 100644 --- a/server/apiv1/api_gen.go +++ b/server/apiv1/api_gen.go @@ -1046,15 +1046,25 @@ type LoginResponseObject interface { VisitLoginResponse(w http.ResponseWriter) error } -type Login200JSONResponse GrantCreated +type Login200ResponseHeaders struct { + CacheControl *string +} + +type Login200JSONResponse struct { + Body GrantCreated + Headers Login200ResponseHeaders +} func (response Login200JSONResponse) VisitLoginResponse(w http.ResponseWriter) error { var buf bytes.Buffer - if err := json.NewEncoder(&buf).Encode(response); err != nil { + if err := json.NewEncoder(&buf).Encode(response.Body); err != nil { return err } w.Header().Set("Content-Type", "application/json") + if response.Headers.CacheControl != nil { + w.Header().Set("Cache-Control", fmt.Sprint(*response.Headers.CacheControl)) + } w.WriteHeader(200) _, err := buf.WriteTo(w) return err @@ -1366,15 +1376,25 @@ type SetupFirstAdminResponseObject interface { VisitSetupFirstAdminResponse(w http.ResponseWriter) error } -type SetupFirstAdmin201JSONResponse GrantCreated +type SetupFirstAdmin201ResponseHeaders struct { + CacheControl *string +} + +type SetupFirstAdmin201JSONResponse struct { + Body GrantCreated + Headers SetupFirstAdmin201ResponseHeaders +} func (response SetupFirstAdmin201JSONResponse) VisitSetupFirstAdminResponse(w http.ResponseWriter) error { var buf bytes.Buffer - if err := json.NewEncoder(&buf).Encode(response); err != nil { + if err := json.NewEncoder(&buf).Encode(response.Body); err != nil { return err } w.Header().Set("Content-Type", "application/json") + if response.Headers.CacheControl != nil { + w.Header().Set("Cache-Control", fmt.Sprint(*response.Headers.CacheControl)) + } w.WriteHeader(201) _, err := buf.WriteTo(w) return err @@ -1471,15 +1491,25 @@ type CreateAccessTokenResponseObject interface { VisitCreateAccessTokenResponse(w http.ResponseWriter) error } -type CreateAccessToken200JSONResponse AccessToken +type CreateAccessToken200ResponseHeaders struct { + CacheControl *string +} + +type CreateAccessToken200JSONResponse struct { + Body AccessToken + Headers CreateAccessToken200ResponseHeaders +} func (response CreateAccessToken200JSONResponse) VisitCreateAccessTokenResponse(w http.ResponseWriter) error { var buf bytes.Buffer - if err := json.NewEncoder(&buf).Encode(response); err != nil { + if err := json.NewEncoder(&buf).Encode(response.Body); err != nil { return err } w.Header().Set("Content-Type", "application/json") + if response.Headers.CacheControl != nil { + w.Header().Set("Cache-Control", fmt.Sprint(*response.Headers.CacheControl)) + } w.WriteHeader(200) _, err := buf.WriteTo(w) return err diff --git a/server/apiv1/api_test.go b/server/apiv1/api_test.go index 9e38672d1..3851db3d1 100644 --- a/server/apiv1/api_test.go +++ b/server/apiv1/api_test.go @@ -3,6 +3,7 @@ package apiv1 import ( "net/http" "net/http/httptest" + "strings" "github.com/getkin/kin-openapi/openapi3" "github.com/go-chi/chi/v5" @@ -30,6 +31,29 @@ var _ = Describe("Router", func() { } }) + It("declares Cache-Control no-store on the success responses of every no-store operation", func() { + doc, err := openapi3.NewLoader().LoadFromData(api.SpecJSON()) + Expect(err).ToNot(HaveOccurred()) + checked := map[string]bool{} + for _, item := range doc.Paths.Map() { + for _, op := range item.Operations() { + if !gateRulesV1.noStore[op.OperationID] { + continue + } + for code, resp := range op.Responses.Map() { + if !strings.HasPrefix(code, "2") { + continue + } + h := resp.Value.Headers["Cache-Control"] + Expect(h).ToNot(BeNil(), op.OperationID+" "+code) + Expect(h.Value.Schema.Value.Enum).To(ConsistOf("no-store"), op.OperationID+" "+code) + checked[op.OperationID] = true + } + } + } + Expect(checked).To(HaveLen(len(gateRulesV1.noStore))) + }) + It("returns a 404 problem for unknown paths", func() { w := serve(router, httptest.NewRequest(http.MethodGet, "/api/v1/nope", nil)) Expect(w.Code).To(Equal(http.StatusNotFound)) diff --git a/server/apiv1/apiv1_suite_test.go b/server/apiv1/apiv1_suite_test.go index 69416c924..6743951de 100644 --- a/server/apiv1/apiv1_suite_test.go +++ b/server/apiv1/apiv1_suite_test.go @@ -9,6 +9,7 @@ import ( "net/http" "net/http/httptest" "path/filepath" + "strings" "testing" "github.com/getkin/kin-openapi/openapi3" @@ -66,10 +67,18 @@ type testClient struct { } func (c testClient) call(method, path, bearer string, body any) *httptest.ResponseRecorder { + if body == nil { + return c.callRaw(method, path, bearer, "") + } + b, _ := json.Marshal(body) + return c.callRaw(method, path, bearer, string(b)) +} + +// callRaw sends body verbatim, for JSON a map cannot express, like keys differing only in case. +func (c testClient) callRaw(method, path, bearer, body string) *httptest.ResponseRecorder { var req *http.Request - if body != nil { - b, _ := json.Marshal(body) - req = httptest.NewRequestWithContext(c.ctx, method, path, bytes.NewReader(b)) + if body != "" { + req = httptest.NewRequestWithContext(c.ctx, method, path, strings.NewReader(body)) req.Header.Set("Content-Type", "application/json") } else { req = httptest.NewRequestWithContext(c.ctx, method, path, nil) diff --git a/server/apiv1/auth_handlers.go b/server/apiv1/auth_handlers.go index 1ddfaa33b..100a0e19a 100644 --- a/server/apiv1/auth_handlers.go +++ b/server/apiv1/auth_handlers.go @@ -22,12 +22,12 @@ func (rt *Router) CreateAccessToken(ctx context.Context, req CreateAccessTokenRe if err != nil { return nil, err } - return CreateAccessToken200JSONResponse{ + return CreateAccessToken200JSONResponse{Body: AccessToken{ AccessToken: tok.Token, TokenType: AccessTokenTokenTypeBearer, ExpiresIn: int(tok.ExpiresIn.Seconds()), Scopes: toScopes(tok.Scopes), - }, nil + }}, nil } func (rt *Router) ListGrants(ctx context.Context, req ListGrantsRequestObject) (ListGrantsResponseObject, error) { diff --git a/server/apiv1/auth_test.go b/server/apiv1/auth_test.go index 884865d16..e04766e54 100644 --- a/server/apiv1/auth_test.go +++ b/server/apiv1/auth_test.go @@ -131,7 +131,7 @@ var _ = Describe("auth endpoints", func() { gc := api.setup() at := api.mint(gc.Secret, nil) Expect(api.call(http.MethodGet, "/api/v1/auth/grants", at.AccessToken, nil).Code).To(Equal(http.StatusOK)) // caches the grant - Expect(realDS.Grant().Delete(ctx, gc.Grant.Id)).To(Succeed()) + Expect(realDS.Grant().DeleteForUser(ctx, gc.User.Id, gc.Grant.Id)).To(Succeed()) w := api.call(http.MethodPost, "/api/v1/auth/logout", at.AccessToken, nil) Expect(w.Code).To(Equal(http.StatusOK), w.Body.String()) @@ -142,13 +142,57 @@ var _ = Describe("auth endpoints", func() { gc := api.setup() at := api.mint(gc.Secret, nil) Expect(api.call(http.MethodGet, "/api/v1/auth/grants", at.AccessToken, nil).Code).To(Equal(http.StatusOK)) // caches the grant - Expect(realDS.Grant().Delete(ctx, gc.Grant.Id)).To(Succeed()) + Expect(realDS.Grant().DeleteForUser(ctx, gc.User.Id, gc.Grant.Id)).To(Succeed()) w := api.call(http.MethodPost, "/api/v1/auth/password", at.AccessToken, map[string]any{"currentPassword": "pw", "newPassword": "pw2"}) Expect(w.Code).To(Equal(http.StatusUnauthorized), w.Body.String()) Expect(w.Header().Get("WWW-Authenticate")).To(Equal(`Bearer error="invalid_token"`)) }) + It("rejects a case-variant scopes key that would widen an explicit empty subset", func() { + gc := api.setup() + w := api.callRaw(http.MethodPost, "/api/v1/auth/token", gc.Secret, `{"scopes":[],"Scopes":null}`) + Expect(w.Code).To(Equal(http.StatusBadRequest), w.Body.String()) + p := decodeProblem(w) + Expect(p.Code).To(Equal(ProblemCodeValidation)) + Expect(*p.Errors).To(ConsistOf(ValidationError{Field: "Scopes", Message: "must match the field name exactly"})) + }) + + It("rejects a case-variant client key that would skip its length limit", func() { + api.setup() + body := `{"username":"admin","password":"pw","client":"ok","Client":"` + strings.Repeat("x", 60_000) + `"}` + w := api.callRaw(http.MethodPost, "/api/v1/auth/login", "", body) + Expect(w.Code).To(Equal(http.StatusBadRequest), w.Body.String()) + Expect(*decodeProblem(w).Errors).To(ConsistOf(ValidationError{Field: "Client", Message: "must match the field name exactly"})) + }) + + DescribeTable("rejects a body with data after its JSON value, without echoing it", + func(path string, needsSecret bool, body string) { + secret := "" + if gc := api.setup(); needsSecret { + secret = gc.Secret + } + w := api.callRaw(http.MethodPost, path, secret, body) + Expect(w.Code).To(Equal(http.StatusBadRequest), w.Body.String()) + p := decodeProblem(w) + Expect(p.Code).To(Equal(ProblemCodeValidation)) + Expect(*p.Errors).To(ConsistOf(ValidationError{Field: "", Message: "must be a single JSON value"})) + Expect(w.Body.String()).ToNot(ContainSubstring("hunter2")) + }, + Entry("token with a trailing byte", "/api/v1/auth/token", true, `{"scopes":[],"Scopes":null}x`), + Entry("login with a second value", "/api/v1/auth/login", false, `{"username":"a","password":"hunter2","client":"c"} {}`), + ) + + It("checks the token body even when Content-Type has a repeated parameter", func() { + gc := api.setup() + req := httptest.NewRequestWithContext(ctx, http.MethodPost, "/api/v1/auth/token", strings.NewReader(`{"scopes":[],"Scopes":null}`)) + req.Header.Set("Content-Type", "application/json; a=1; a=2") + req.Header.Set("Authorization", "Bearer "+gc.Secret) + w := serve(api.router, req) + Expect(w.Code).To(Equal(http.StatusBadRequest), w.Body.String()) + Expect(*decodeProblem(w).Errors).To(ConsistOf(ValidationError{Field: "Scopes", Message: "must match the field name exactly"})) + }) + It("marks grant and token responses no-store", func() { w := api.call(http.MethodPost, "/api/v1/auth/setup", "", creds("admin", "pw")) Expect(w.Code).To(Equal(http.StatusCreated)) diff --git a/server/apiv1/gate.go b/server/apiv1/gate.go index ea4e2b2c9..ee25b15a2 100644 --- a/server/apiv1/gate.go +++ b/server/apiv1/gate.go @@ -1,13 +1,18 @@ package apiv1 import ( + "bytes" "cmp" "context" + "encoding/json" "errors" "fmt" + "io" + "maps" "net/http" "regexp" "slices" + "strconv" "strings" "github.com/getkin/kin-openapi/openapi3" @@ -265,19 +270,88 @@ func (g *gate) validate(w http.ResponseWriter, r *http.Request, op *gateOp, rctx err := openapi3filter.ValidateRequest(r.Context(), &openapi3filter.RequestValidationInput{ Request: r, PathParams: params, Route: op.route, Options: validationOptions, }) - if err == nil { - return true - } if tooLarge(err) { writeProblem(w, r, ClientError(err, tooLargeDetail)) return false } - fields := sanitizeValidation(err) + var fields []ValidationError + if err != nil { + fields = sanitizeValidation(err) + } else if fields = jsonBodyFields(r, op.route.Operation); len(fields) == 0 { + return true + } log.Debug(r.Context(), "API v1: request failed validation", "operation", op.id(), "errors", fields) writeProblemStatus(w, r, http.StatusBadRequest, ProblemCodeValidation, "the request does not match the API schema", fields...) return false } +// jsonBodyFields checks what kin-openapi misses in a JSON body: data after the first value, which Go's decoder +// ignores, and keys that only case-fold to a declared property, which encoding/json decodes into that property. +func jsonBodyFields(r *http.Request, op *openapi3.Operation) []ValidationError { + if op.RequestBody == nil || op.RequestBody.Value == nil || r.Body == nil { + return nil + } + // Keyed on the spec, not the request's Content-Type: the handlers decode JSON whatever the header says. + media := op.RequestBody.Value.Content.Get("application/json") + if media == nil || media.Schema == nil { + return nil + } + data, err := io.ReadAll(r.Body) + r.Body = io.NopCloser(bytes.NewReader(data)) + if err != nil || len(bytes.TrimSpace(data)) == 0 { + return nil + } + dec := json.NewDecoder(bytes.NewReader(data)) + var body any + if err := dec.Decode(&body); err != nil { + return []ValidationError{{Field: "", Message: "must be a single JSON value"}} + } + if _, err := dec.Token(); !errors.Is(err, io.EOF) { + return []ValidationError{{Field: "", Message: "must be a single JSON value"}} + } + var out []ValidationError + collectCaseAliases(body, media.Schema.Value, "", &out) + return out +} + +func collectCaseAliases(v any, schema *openapi3.Schema, path string, out *[]ValidationError) { + if schema == nil { + return + } + switch v := v.(type) { + case map[string]any: + for _, key := range slices.Sorted(maps.Keys(v)) { + field := joinField(path, key) + if prop, ok := schema.Properties[key]; ok { + if prop != nil { + collectCaseAliases(v[key], prop.Value, field, out) + } + continue + } + for name := range schema.Properties { + if strings.EqualFold(key, name) { + *out = append(*out, ValidationError{Field: field, Message: "must match the field name exactly"}) + break + } + } + } + case []any: + if schema.Items == nil { + return + } + for i, item := range v { + collectCaseAliases(item, schema.Items.Value, joinField(path, strconv.Itoa(i)), out) + } + } +} + +func joinField(path, name string) string { + if path == "" { + return name + } + return path + "." + name +} + var missingProperty = regexp.MustCompile(`property "([^"]+)" is missing`) // sanitizeValidation keeps only field paths and fixed messages: kin-openapi errors can embed the submitted value. diff --git a/server/apiv1/gate_test.go b/server/apiv1/gate_test.go index dc8f8aeb6..37cc67448 100644 --- a/server/apiv1/gate_test.go +++ b/server/apiv1/gate_test.go @@ -47,7 +47,9 @@ paths: schema: type: object required: [name] - properties: {name: {type: string, maxLength: 5}} + properties: + name: {type: string, maxLength: 5} + tags: {type: array, items: {type: object, properties: {label: {type: string, maxLength: 5}}}} responses: {'200': {description: ok}} /caps: get: {operationId: caps, x-module: core, security: [{bearerAuth: []}], responses: {'200': {description: ok}}} @@ -287,6 +289,64 @@ var _ = Describe("spec gate", func() { Expect(*decodeProblem(w).Errors).To(ConsistOf(ValidationError{Field: "name", Message: "is required"})) }) + DescribeTable("rejects a case variant of a declared body field, which Go would decode into it", + func(body, field string) { + fa.principal.Scopes = []string{"password"} + w := do(http.MethodPost, "/things", "Bearer x", body) + Expect(w.Code).To(Equal(http.StatusBadRequest), w.Body.String()) + p := decodeProblem(w) + Expect(p.Code).To(Equal(ProblemCodeValidation)) + Expect(*p.Errors).To(ConsistOf(ValidationError{Field: field, Message: "must match the field name exactly"})) + Expect(reached).To(BeEmpty()) + }, + Entry("top level", `{"name":"ok","NAME":"much-too-long"}`, "NAME"), + Entry("inside array items", `{"name":"ok","tags":[{"label":"a"},{"label":"b","Label":"much-too-long"}]}`, "tags.1.Label"), + Entry("Unicode case folding", "{\"name\":\"ok\",\"tag\u017f\":null}", "tag\u017f"), + ) + + DescribeTable("rejects data after the first JSON value, which Go's decoder would ignore", + func(body string) { + fa.principal.Scopes = []string{"password"} + w := do(http.MethodPost, "/things", "Bearer x", body) + Expect(w.Code).To(Equal(http.StatusBadRequest), w.Body.String()) + p := decodeProblem(w) + Expect(p.Code).To(Equal(ProblemCodeValidation)) + Expect(*p.Errors).To(ConsistOf(ValidationError{Field: "", Message: "must be a single JSON value"})) + Expect(reached).To(BeEmpty()) + }, + Entry("garbage", `{"name":"ok","NAME":"much-too-long"}x`), + Entry("a second value", `{"name":"ok"} {"NAME":"much-too-long"}`), + Entry("a stray bracket", `{"name":"ok"}]`), + ) + + DescribeTable("checks the body whatever parameters the Content-Type carries", + func(contentType string) { + fa.principal.Scopes = []string{"password"} + req := httptest.NewRequestWithContext(ctx, http.MethodPost, "/things", strings.NewReader(`{"name":"ok","NAME":"much-too-long"}`)) + req.Header.Set("Content-Type", contentType) + req.Header.Set("Authorization", "Bearer x") + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + Expect(w.Code).To(Equal(http.StatusBadRequest), w.Body.String()) + Expect(*decodeProblem(w).Errors).To(ConsistOf(ValidationError{Field: "NAME", Message: "must match the field name exactly"})) + Expect(reached).To(BeEmpty()) + }, + Entry("repeated parameter", "application/json; a=1; a=2"), + Entry("repeated charset", "application/json; charset=utf-8; CHARSET=latin1"), + ) + + It("accepts trailing whitespace after the JSON value", func() { + fa.principal.Scopes = []string{"password"} + Expect(do(http.MethodPost, "/things", "Bearer x", "{\"name\":\"ok\"}\n \t").Code).To(Equal(http.StatusOK)) + }) + + It("allows unknown body fields that do not collide with a declared one", func() { + fa.principal.Scopes = []string{"password"} + w := do(http.MethodPost, "/things", "Bearer x", `{"name":"ok","extra":{"Name":"x"},"tags":[{"label":"a","other":1}]}`) + Expect(w.Code).To(Equal(http.StatusOK), w.Body.String()) + Expect(reached).To(Equal("createThing")) + }) + It("validates path parameters", func() { w := do(http.MethodGet, "/things/toolong", "Bearer x", "") Expect(w.Code).To(Equal(http.StatusBadRequest)) diff --git a/server/apiv1/password_handlers.go b/server/apiv1/password_handlers.go index 5eef55090..32cf09e87 100644 --- a/server/apiv1/password_handlers.go +++ b/server/apiv1/password_handlers.go @@ -14,7 +14,7 @@ func (rt *Router) Login(ctx context.Context, req LoginRequestObject) (LoginRespo if err != nil { return nil, err } - return Login200JSONResponse(toGrantCreated(issued)), nil + return Login200JSONResponse{Body: toGrantCreated(issued)}, nil } func (rt *Router) SetupFirstAdmin(ctx context.Context, req SetupFirstAdminRequestObject) (SetupFirstAdminResponseObject, error) { @@ -23,7 +23,7 @@ func (rt *Router) SetupFirstAdmin(ctx context.Context, req SetupFirstAdminReques if err != nil { return nil, err } - return SetupFirstAdmin201JSONResponse(toGrantCreated(issued)), nil + return SetupFirstAdmin201JSONResponse{Body: toGrantCreated(issued)}, nil } func (rt *Router) ChangePassword(ctx context.Context, req ChangePasswordRequestObject) (ChangePasswordResponseObject, error) {