fix(api): consolidate grant deletes and harden API v1 auth after review

- GrantRepository keeps three deletes: DeleteForUser, DeleteStaleEpochs
  (replaces DeleteOtherEpochs and DeleteIfEpoch) and DeleteIdle. Delete(id)
  is gone; the idle path in ResolveGrant now calls DeleteIdle, so a grant
  renewed by another node between the read and the delete survives.
  settleEpoch deletes the user's grants below the snapshot's epoch, which
  is safe outside the transaction because epochs only move forward.
- The "dead grants on an older epoch are only deleted when presented"
  policy note moves from the repository to core ListGrants.
- SQL trace logging no longer prints the args of property writes (signing
  keys) or user password writes, both encrypted with a key that may be the
  public default. The SQL statement is still logged.
- The spec gate rejects JSON body keys that differ from a declared property
  only in case. kin-openapi validates exact names while encoding/json
  decodes case-insensitively, so {"scopes":[],"Scopes":null} minted a
  token with every scope and a "Client" key skipped maxLength.
  It also rejects data after the first JSON value, which the handlers'
  decoder ignores and which let a body skip the alias check.
- The liveness cache trims its eviction log on evict, not only on put, so
  evict-only traffic stays bounded; the floor still drops stale fills.
- createAccessToken, login and setupFirstAdmin declare Cache-Control:
  no-store on their success responses.
This commit is contained in:
Deluan 2026-09-26 12:52:20 -04:00
commit 8789561b60
26 changed files with 493 additions and 72 deletions

View file

@ -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"
]
}
}
}
}

View file

@ -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

View file

@ -0,0 +1,4 @@
description: Always `no-store`, because the response carries a secret or a token.
schema:
type: string
enum: [no-store]

View file

@ -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:

View file

@ -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)

View file

@ -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))

View file

@ -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)
})
}

View file

@ -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)

View file

@ -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))
})
})

View file

@ -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)
}

View file

@ -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()}}})

View file

@ -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() {

View file

@ -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)
}

View file

@ -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

View file

@ -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"))
})
})

View file

@ -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

View file

@ -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 {

View file

@ -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":"????"}}]}`}

View file

@ -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

View file

@ -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))

View file

@ -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)

View file

@ -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) {

View file

@ -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))

View file

@ -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.

View file

@ -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))

View file

@ -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) {