diff --git a/server/apiv1/api.go b/server/apiv1/api.go index 53076063a..70c6a7bc5 100644 --- a/server/apiv1/api.go +++ b/server/apiv1/api.go @@ -62,9 +62,8 @@ func (rt *Router) routes() http.Handler { strict := NewStrictHandlerWithOptions(rt, nil, StrictHTTPServerOptions{ RequestErrorHandlerFunc: func(w http.ResponseWriter, req *http.Request, err error) { - var tooLarge *http.MaxBytesError - if errors.As(err, &tooLarge) { - writeProblemStatus(w, req, http.StatusRequestEntityTooLarge, ProblemCodePayloadTooLarge, "request body too large") + if tooLarge(err) { + writeProblem(w, req, ClientError(err, tooLargeDetail)) return } writeProblemStatus(w, req, http.StatusBadRequest, ProblemCodeValidation, "request body is not valid JSON") @@ -120,6 +119,14 @@ func headAsGet(mux chi.Routes) func(http.Handler) http.Handler { } } +// routeMethod is the method chi dispatches on, which headAsGet sets to GET for a HEAD only GET serves. +func routeMethod(req *http.Request) string { + if rctx := chi.RouteContext(req.Context()); rctx != nil && rctx.RouteMethod != "" { + return rctx.RouteMethod + } + return req.Method +} + // routePath must pick the same path chi's routeHTTP dispatches on, or the gate could vet a different route. func routePath(req *http.Request) string { if rctx := chi.RouteContext(req.Context()); rctx != nil && rctx.RoutePath != "" { diff --git a/server/apiv1/auth_handlers.go b/server/apiv1/auth_handlers.go index 26eecd386..1ddfaa33b 100644 --- a/server/apiv1/auth_handlers.go +++ b/server/apiv1/auth_handlers.go @@ -4,14 +4,13 @@ import ( "cmp" "context" - "github.com/navidrome/navidrome/core/apiauth" "github.com/navidrome/navidrome/utils/gg" ) const defaultPageSize = 100 func (rt *Router) CreateAccessToken(ctx context.Context, req CreateAccessTokenRequestObject) (CreateAccessTokenResponseObject, error) { - p, err := principal(apiauth.PrincipalFrom(ctx)) + p, err := principalFrom(ctx) if err != nil { return nil, err } @@ -32,7 +31,7 @@ func (rt *Router) CreateAccessToken(ctx context.Context, req CreateAccessTokenRe } func (rt *Router) ListGrants(ctx context.Context, req ListGrantsRequestObject) (ListGrantsResponseObject, error) { - p, err := principal(apiauth.PrincipalFrom(ctx)) + p, err := principalFrom(ctx) if err != nil { return nil, err } @@ -50,7 +49,7 @@ func (rt *Router) ListGrants(ctx context.Context, req ListGrantsRequestObject) ( } func (rt *Router) RevokeGrant(ctx context.Context, req RevokeGrantRequestObject) (RevokeGrantResponseObject, error) { - p, err := principal(apiauth.PrincipalFrom(ctx)) + p, err := principalFrom(ctx) if err != nil { return nil, err } @@ -61,7 +60,7 @@ func (rt *Router) RevokeGrant(ctx context.Context, req RevokeGrantRequestObject) } func (rt *Router) Logout(ctx context.Context, _ LogoutRequestObject) (LogoutResponseObject, error) { - p, err := principal(apiauth.PrincipalFrom(ctx)) + p, err := principalFrom(ctx) if err != nil { return nil, err } diff --git a/server/apiv1/dto.go b/server/apiv1/dto.go index 9cab02595..c65830a6a 100644 --- a/server/apiv1/dto.go +++ b/server/apiv1/dto.go @@ -1,6 +1,8 @@ package apiv1 import ( + "context" + "github.com/navidrome/navidrome/core/apiauth" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/gg" @@ -59,8 +61,9 @@ func clientMeta(c CredentialsRequest) apiauth.ClientMeta { return apiauth.ClientMeta{Client: c.Client, Name: gg.V(c.Name), ClientVersion: gg.V(c.ClientVersion)} } -// principal fails closed if the gate did not attach a principal to the context. -func principal(p *apiauth.Principal, ok bool) (*apiauth.Principal, error) { +// principalFrom fails closed if the gate did not attach a principal to the context. +func principalFrom(ctx context.Context) (*apiauth.Principal, error) { + p, ok := apiauth.PrincipalFrom(ctx) if !ok || p == nil { return nil, model.ErrInvalidAuth } diff --git a/server/apiv1/gate.go b/server/apiv1/gate.go index 527ec2bcc..ea4e2b2c9 100644 --- a/server/apiv1/gate.go +++ b/server/apiv1/gate.go @@ -1,6 +1,7 @@ package apiv1 import ( + "cmp" "context" "errors" "fmt" @@ -17,7 +18,6 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core/apiauth" "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server" ) @@ -36,7 +36,6 @@ const ( ) type gateOp struct { - id string route *routers.Route kind authKind scope string @@ -44,9 +43,13 @@ type gateOp struct { noStore bool } +func (o *gateOp) id() string { return o.route.Operation.OperationID } + +type opKey struct{ method, path string } + type gate struct { mux chi.Routes - ops map[string]*gateOp + ops map[opKey]*gateOp auth authenticator limiter func(http.Handler) http.Handler } @@ -68,18 +71,21 @@ type gateRules struct { } func newGate(doc *openapi3.T, mux chi.Routes, auth authenticator, rules gateRules) (*gate, error) { - g := &gate{mux: mux, ops: map[string]*gateOp{}, auth: auth} + g := &gate{mux: mux, ops: map[opKey]*gateOp{}, auth: auth} + ids := map[string]bool{} for path, item := range doc.Paths.Map() { for method, op := range item.Operations() { gop, err := buildGateOp(doc, path, item, method, op, rules) if err != nil { return nil, err } - gop.limited = rules.limited[op.OperationID] - gop.noStore = rules.noStore[op.OperationID] - g.ops[method+" "+path] = gop + g.ops[opKey{method, path}] = gop + ids[op.OperationID] = true } } + if err := rules.check(ids); err != nil { + return nil, err + } if conf.Server.AuthRequestLimit > 0 { g.limiter = server.ClientIPRateLimiter(conf.Server.AuthRequestLimit, conf.Server.AuthWindowLength, httprate.WithLimitHandler(func(w http.ResponseWriter, r *http.Request) { @@ -89,10 +95,27 @@ func newGate(doc *openapi3.T, mux chi.Routes, auth authenticator, rules gateRule return g, nil } +// check fails on a rule naming an operation the spec lacks, so a typo cannot silently disable the rule. +func (rules gateRules) check(ids map[string]bool) error { + sets := map[string]map[string]bool{"limited": rules.limited, "noScope": rules.noScope, "grantOps": rules.grantOps, "noStore": rules.noStore} + for name, set := range sets { + for id := range set { + if !ids[id] { + return fmt.Errorf("gate rule %s names unknown operation %s", name, id) + } + } + } + return nil +} + // buildGateOp enforces the allowed security forms, so a spec edit cannot silently drop a requirement. func buildGateOp(doc *openapi3.T, path string, item *openapi3.PathItem, method string, op *openapi3.Operation, rules gateRules) (*gateOp, error) { id := op.OperationID - gop := &gateOp{id: id, route: &routers.Route{Spec: doc, Path: path, PathItem: item, Method: method, Operation: op}} + gop := &gateOp{ + route: &routers.Route{Spec: doc, Path: path, PathItem: item, Method: method, Operation: op}, + limited: rules.limited[id], + noStore: rules.noStore[id], + } if op.Security == nil { return nil, fmt.Errorf("operation %s must declare security explicitly", id) } @@ -119,15 +142,12 @@ func buildGateOp(doc *openapi3.T, path string, item *openapi3.PathItem, method s if gop.kind != authToken { return nil, fmt.Errorf("operation %s: x-scope needs bearerAuth", id) } - base := module - if s, ok := moduleScope[module]; ok { - base = s - } + base := cmp.Or(moduleScope[module], module) if scope != base && scope != base+":write" { - return nil, fmt.Errorf("operation %s: x-scope %q does not match module %q", op.OperationID, scope, module) + return nil, fmt.Errorf("operation %s: x-scope %q does not match module %q", id, scope, module) } - if !slices.Contains(apiauth.KnownScopes, scope) && scope != apiauth.ScopeAdmin { - return nil, fmt.Errorf("operation %s: unknown x-scope %q", op.OperationID, scope) + if !slices.Contains(apiauth.KnownScopes, scope) { + return nil, fmt.Errorf("operation %s: unknown x-scope %q", id, scope) } } gop.scope = scope @@ -143,7 +163,7 @@ func isScheme(req openapi3.SecurityRequirement, name string) bool { // checkRoutes fails when a routed pattern has no spec operation or a spec operation has no route. func (g *gate) checkRoutes() error { err := chi.Walk(g.mux, func(method, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error { - if _, ok := g.ops[method+" "+route]; !ok { + if _, ok := g.ops[opKey{method, route}]; !ok { return fmt.Errorf("route %s %s is not in the spec", method, route) } return nil @@ -153,7 +173,7 @@ func (g *gate) checkRoutes() error { } for _, op := range g.ops { if g.mux.Find(chi.NewRouteContext(), op.route.Method, op.route.Path) != op.route.Path { - return fmt.Errorf("spec operation %s (%s %s) has no route", op.id, op.route.Method, op.route.Path) + return fmt.Errorf("spec operation %s (%s %s) has no route", op.id(), op.route.Method, op.route.Path) } } return nil @@ -161,18 +181,14 @@ func (g *gate) checkRoutes() error { func (g *gate) handler(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - path := routePath(r) - method := r.Method - if method == http.MethodHead && !g.mux.Match(chi.NewRouteContext(), http.MethodHead, path) { - method = http.MethodGet - } + method := routeMethod(r) rctx := chi.NewRouteContext() - pattern := g.mux.Find(rctx, method, path) + pattern := g.mux.Find(rctx, method, routePath(r)) if pattern == "" { next.ServeHTTP(w, r) return } - op, ok := g.ops[method+" "+pattern] + op, ok := g.ops[opKey{method, pattern}] if !ok { log.Error(r.Context(), "API v1: routed pattern missing from the spec", "method", method, "pattern", pattern) writeProblemStatus(w, r, http.StatusInternalServerError, ProblemCodeInternal, "") @@ -205,7 +221,6 @@ func (g *gate) authorize(w http.ResponseWriter, r *http.Request, op *gateOp) (*h } token, ok := bearerToken(r) if !ok { - w.Header().Set("WWW-Authenticate", "Bearer") writeProblemStatus(w, r, http.StatusUnauthorized, ProblemCodeUnauthorized, "") return r, false } @@ -217,35 +232,20 @@ func (g *gate) authorize(w http.ResponseWriter, r *http.Request, op *gateOp) (*h } else { p, err = g.auth.Authenticate(r.Context(), token, ip) } - switch { - case errors.Is(err, apiauth.ErrTokenExpired), errors.Is(err, model.ErrInvalidAuth): - w.Header().Set("WWW-Authenticate", `Bearer error="invalid_token"`) - writeProblem(w, r, err) - return r, false - case errors.Is(err, apiauth.ErrInsufficientScope): - insufficientScope(w, r, op, err) - return r, false - case err != nil: - writeProblem(w, r, err) - return r, false + if err == nil && op.scope != "" && !apiauth.Satisfies(p.Scopes, op.scope) { + err = apiauth.ErrInsufficientScope } - if op.scope != "" && !apiauth.Satisfies(p.Scopes, op.scope) { - insufficientScope(w, r, op, apiauth.ErrInsufficientScope) + if errors.Is(err, apiauth.ErrInsufficientScope) { + err = &scopeError{scope: op.scope} + } + if err != nil { + writeProblem(w, r, err) return r, false } ctx := apiauth.WithPrincipal(request.WithUser(r.Context(), p.User), p) return r.WithContext(ctx), true } -func insufficientScope(w http.ResponseWriter, r *http.Request, op *gateOp, err error) { - challenge := `Bearer error="insufficient_scope"` - if op.scope != "" { - challenge += fmt.Sprintf(`, scope=%q`, op.scope) - } - w.Header().Set("WWW-Authenticate", challenge) - writeProblem(w, r, err) -} - func bearerToken(r *http.Request) (string, bool) { scheme, token, ok := strings.Cut(strings.TrimSpace(r.Header.Get("Authorization")), " ") token = strings.TrimSpace(token) @@ -255,25 +255,25 @@ func bearerToken(r *http.Request) (string, bool) { return token, true } +var validationOptions = &openapi3filter.Options{AuthenticationFunc: openapi3filter.NoopAuthenticationFunc, MultiError: true} + func (g *gate) validate(w http.ResponseWriter, r *http.Request, op *gateOp, rctx *chi.Context) bool { - params := map[string]string{} + params := make(map[string]string, len(rctx.URLParams.Keys)) for i, k := range rctx.URLParams.Keys { params[k] = rctx.URLParams.Values[i] } err := openapi3filter.ValidateRequest(r.Context(), &openapi3filter.RequestValidationInput{ - Request: r, PathParams: params, Route: op.route, - Options: &openapi3filter.Options{AuthenticationFunc: openapi3filter.NoopAuthenticationFunc, MultiError: true}, + Request: r, PathParams: params, Route: op.route, Options: validationOptions, }) if err == nil { return true } - var tooLarge *http.MaxBytesError - if errors.As(err, &tooLarge) { - writeProblemStatus(w, r, http.StatusRequestEntityTooLarge, ProblemCodePayloadTooLarge, "request body too large") + if tooLarge(err) { + writeProblem(w, r, ClientError(err, tooLargeDetail)) return false } fields := sanitizeValidation(err) - log.Debug(r.Context(), "API v1: request failed validation", "operation", op.id, "errors", fields) + 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 } diff --git a/server/apiv1/gate_test.go b/server/apiv1/gate_test.go index 3476ccd09..dc8f8aeb6 100644 --- a/server/apiv1/gate_test.go +++ b/server/apiv1/gate_test.go @@ -3,6 +3,7 @@ package apiv1 import ( "bytes" "context" + "maps" "net/http" "net/http/httptest" "os" @@ -329,6 +330,24 @@ var _ = Describe("spec gate", func() { Entry("non-empty scope list on a bearer scheme", strings.Replace(gateSpec, "operationId: caps, x-module: core, security: [{bearerAuth: []}]", "operationId: caps, x-module: core, security: [{bearerAuth: [read]}]", 1)), Entry("x-scope on a public operation", strings.Replace(gateSpec, "operationId: open, x-module: core, security: [],", "operationId: open, x-module: core, x-scope: read, security: [],", 1)), Entry("x-scope that is not a string", strings.Replace(gateSpec, "x-scope: read", "x-scope: [read]", 1)), + Entry("x-scope not in KnownScopes", strings.Replace(gateSpec, "x-module: password\n x-scope: password", "x-module: admin\n x-scope: admin", 1)), + ) + + DescribeTable("refuses rules that name an operation missing from the spec", + func(set func(*gateRules) *map[string]bool) { + doc, err := openapi3.NewLoader().LoadFromData([]byte(gateSpec)) + Expect(err).ToNot(HaveOccurred()) + rules := testGateRules + m := set(&rules) + *m = maps.Clone(*m) + (*m)["typo"] = true + _, err = newGate(doc, chi.NewRouter(), fa, rules) + Expect(err).To(MatchError(ContainSubstring("typo"))) + }, + Entry("limited", func(r *gateRules) *map[string]bool { return &r.limited }), + Entry("noScope", func(r *gateRules) *map[string]bool { return &r.noScope }), + Entry("grantOps", func(r *gateRules) *map[string]bool { return &r.grantOps }), + Entry("noStore", func(r *gateRules) *map[string]bool { return &r.noStore }), ) It("checks routes against the spec in both directions", func() { diff --git a/server/apiv1/password_handlers.go b/server/apiv1/password_handlers.go index a96d905d0..5eef55090 100644 --- a/server/apiv1/password_handlers.go +++ b/server/apiv1/password_handlers.go @@ -27,7 +27,7 @@ func (rt *Router) SetupFirstAdmin(ctx context.Context, req SetupFirstAdminReques } func (rt *Router) ChangePassword(ctx context.Context, req ChangePasswordRequestObject) (ChangePasswordResponseObject, error) { - p, err := principal(apiauth.PrincipalFrom(ctx)) + p, err := principalFrom(ctx) if err != nil { return nil, err } diff --git a/server/apiv1/problem.go b/server/apiv1/problem.go index 2c2c531a4..197a7dde6 100644 --- a/server/apiv1/problem.go +++ b/server/apiv1/problem.go @@ -3,6 +3,7 @@ package apiv1 import ( "encoding/json" "errors" + "fmt" "net/http" "github.com/navidrome/navidrome/core/apiauth" @@ -26,6 +27,20 @@ func ClientError(err error, detail string) error { return &clientError{err: err, detail: detail} } +// scopeError names the scope an operation requires, for the insufficient_scope challenge. +type scopeError struct { + scope string +} + +func (e *scopeError) Error() string { return apiauth.ErrInsufficientScope.Error() } +func (e *scopeError) Unwrap() error { return apiauth.ErrInsufficientScope } + +const tooLargeDetail = "request body too large" + +func tooLarge(err error) bool { + return errors.As(err, new(*http.MaxBytesError)) +} + type fieldErrors struct { fields []ValidationError } @@ -45,6 +60,9 @@ func writeProblem(w http.ResponseWriter, r *http.Request, err error) { return } log.Debug(r.Context(), "API v1: request failed", "path", r.URL.Path, "status", status, "code", code, err) + if code == ProblemCodeInsufficientScope { + w.Header().Set("WWW-Authenticate", scopeChallenge(err)) + } var detail string var ce *clientError if errors.As(err, &ce) { @@ -58,8 +76,19 @@ func writeProblem(w http.ResponseWriter, r *http.Request, err error) { writeProblemStatus(w, r, status, code, detail) } +func scopeChallenge(err error) string { + challenge := `Bearer error="insufficient_scope"` + var se *scopeError + if errors.As(err, &se) && se.scope != "" { + challenge += fmt.Sprintf(`, scope=%q`, se.scope) + } + return challenge +} + func classifyError(err error) (int, ProblemCode) { switch { + case tooLarge(err): + return http.StatusRequestEntityTooLarge, ProblemCodePayloadTooLarge case errors.Is(err, apiauth.ErrTokenExpired): return http.StatusUnauthorized, ProblemCodeTokenExpired case errors.Is(err, apiauth.ErrInsufficientScope):