From 425fe862baa5930f307a16e2c7e58cc8f1fcbb03 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 13 Feb 2026 15:47:51 -0500 Subject: [PATCH] refactor(plugins): enhance user authorization handling and streamline form data processing Signed-off-by: Deluan --- plugins/host_subsonicapi.go | 7 ++--- plugins/http_endpoint.go | 28 ++++++++----------- plugins/manager_call.go | 51 +++++++++++++++++++--------------- server/subsonic/middlewares.go | 39 +++++++++++++------------- 4 files changed, 62 insertions(+), 63 deletions(-) diff --git a/plugins/host_subsonicapi.go b/plugins/host_subsonicapi.go index ed44d139c..ea9523a61 100644 --- a/plugins/host_subsonicapi.go +++ b/plugins/host_subsonicapi.go @@ -86,11 +86,8 @@ func (s *subsonicAPIServiceImpl) executeRequest(ctx context.Context, uri string, RawQuery: query.Encode(), } - // Create HTTP request with a fresh context to avoid Chi RouteContext pollution. - // Using http.NewRequest (instead of http.NewRequestWithContext) ensures the internal - // SubsonicAPI call doesn't inherit routing information from the parent handler, - // which would cause Chi to invoke the wrong handler. Authentication context is - // explicitly added in the next step via request.WithInternalAuth. + // Use http.NewRequest (not WithContext) to avoid inheriting Chi RouteContext; + // auth context is set explicitly below via request.WithInternalAuth. httpReq, err := http.NewRequest("GET", finalURL.String(), nil) if err != nil { return nil, fmt.Errorf("failed to create HTTP request: %w", err) diff --git a/plugins/http_endpoint.go b/plugins/http_endpoint.go index d0fc96c49..68d1017bc 100644 --- a/plugins/http_endpoint.go +++ b/plugins/http_endpoint.go @@ -104,7 +104,8 @@ func (h *endpointHandler) serveWithNativeAuth(w http.ResponseWriter, r *http.Req func (h *endpointHandler) dispatch(w http.ResponseWriter, r *http.Request, p *plugin) { ctx := r.Context() - // Check user authorization (skip for auth:"none") + // Check user authorization and extract user info (skip for auth:"none") + var httpUser *capabilities.HTTPUser if p.manifest.Permissions.Endpoints.Auth != EndpointsPermissionAuthNone { user, ok := request.UserFrom(ctx) if !ok { @@ -116,6 +117,12 @@ func (h *endpointHandler) dispatch(w http.ResponseWriter, r *http.Request, p *pl http.Error(w, "Forbidden", http.StatusForbidden) return } + httpUser = &capabilities.HTTPUser{ + ID: user.ID, + Username: user.UserName, + Name: user.Name, + IsAdmin: user.IsAdmin, + } } // Read request body with size limit @@ -128,21 +135,10 @@ func (h *endpointHandler) dispatch(w http.ResponseWriter, r *http.Request, p *pl // Build the plugin request // Normalize path: both /ext/plugin and /ext/plugin/ map to "" - relPath := "/" + chi.URLParam(r, "*") - if relPath == "/" || relPath == "" { - relPath = "" - } - - var httpUser *capabilities.HTTPUser - if p.manifest.Permissions.Endpoints.Auth != EndpointsPermissionAuthNone { - if user, ok := request.UserFrom(ctx); ok { - httpUser = &capabilities.HTTPUser{ - ID: user.ID, - Username: user.UserName, - Name: user.Name, - IsAdmin: user.IsAdmin, - } - } + rawPath := chi.URLParam(r, "*") + relPath := "" + if rawPath != "" { + relPath = "/" + rawPath } pluginReq := capabilities.HTTPHandleRequest{ diff --git a/plugins/manager_call.go b/plugins/manager_call.go index 12729767c..9ed8ee94c 100644 --- a/plugins/manager_call.go +++ b/plugins/manager_call.go @@ -61,39 +61,44 @@ func callPluginFunction[I any, O any](ctx context.Context, plugin *plugin, funcN startCall := time.Now() exit, output, err := p.CallWithContext(ctx, funcName, inputBytes) elapsed := time.Since(startCall) + + success := false + skipMetrics := false + defer func() { + if !skipMetrics { + plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, success, elapsed.Milliseconds()) + } + }() + if err != nil { // If context was cancelled, return that error instead of the plugin error if ctx.Err() != nil { + skipMetrics = true log.Debug(ctx, "Plugin call cancelled", "plugin", plugin.name, "function", funcName, "pluginDuration", elapsed) return result, ctx.Err() } - plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, false, elapsed.Milliseconds()) log.Trace(ctx, "Plugin call failed", "plugin", plugin.name, "function", funcName, "pluginDuration", elapsed, "navidromeDuration", startCall.Sub(start), err) return result, fmt.Errorf("plugin call failed: %w", err) } if exit != 0 { if exit == notImplementedCode { + skipMetrics = true log.Trace(ctx, "Plugin function not implemented", "plugin", plugin.name, "function", funcName, "pluginDuration", elapsed, "navidromeDuration", startCall.Sub(start)) - // TODO Should we record metrics for not implemented calls? - //plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, true, elapsed.Milliseconds()) return result, fmt.Errorf("%w: %s", errNotImplemented, funcName) } - plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, false, elapsed.Milliseconds()) return result, fmt.Errorf("plugin call exited with code %d", exit) } if len(output) > 0 { - err = json.Unmarshal(output, &result) - if err != nil { + if err = json.Unmarshal(output, &result); err != nil { log.Trace(ctx, "Plugin call failed", "plugin", plugin.name, "function", funcName, "pluginDuration", elapsed, "navidromeDuration", startCall.Sub(start), err) + return result, err } } - // Record metrics for successful calls (or JSON unmarshal failures) - plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, err == nil, elapsed.Milliseconds()) - + success = true log.Trace(ctx, "Plugin call succeeded", "plugin", plugin.name, "function", funcName, "pluginDuration", time.Since(startCall), "navidromeDuration", startCall.Sub(start)) - return result, err + return result, nil } // callPluginFunctionRaw calls a plugin function using binary framing for []byte fields. @@ -133,64 +138,64 @@ func callPluginFunctionRaw[I any, O any]( startCall := time.Now() exit, output, err := p.CallWithContext(ctx, funcName, frame) elapsed := time.Since(startCall) + + success := false + skipMetrics := false + defer func() { + if !skipMetrics { + plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, success, elapsed.Milliseconds()) + } + }() + if err != nil { if ctx.Err() != nil { + skipMetrics = true log.Debug(ctx, "Plugin call cancelled", "plugin", plugin.name, "function", funcName, "pluginDuration", elapsed) return result, ctx.Err() } - plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, false, elapsed.Milliseconds()) log.Trace(ctx, "Plugin call failed", "plugin", plugin.name, "function", funcName, "pluginDuration", elapsed, "navidromeDuration", startCall.Sub(start), err) return result, fmt.Errorf("plugin call failed: %w", err) } if exit != 0 { if exit == notImplementedCode { + skipMetrics = true log.Trace(ctx, "Plugin function not implemented", "plugin", plugin.name, "function", funcName, "pluginDuration", elapsed, "navidromeDuration", startCall.Sub(start)) return result, fmt.Errorf("%w: %s", errNotImplemented, funcName) } - plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, false, elapsed.Milliseconds()) return result, fmt.Errorf("plugin call exited with code %d", exit) } // Parse output frame if len(output) < 1 { - plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, false, elapsed.Milliseconds()) return result, fmt.Errorf("empty response from plugin") } statusByte := output[0] if statusByte == 0x01 { - // Error frame: [0x01][UTF-8 error message] - plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, false, elapsed.Milliseconds()) - errMsg := string(output[1:]) - return result, fmt.Errorf("plugin error: %s", errMsg) + return result, fmt.Errorf("plugin error: %s", string(output[1:])) } - if statusByte != 0x00 { - plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, false, elapsed.Milliseconds()) return result, fmt.Errorf("unknown response status byte: 0x%02x", statusByte) } // Success frame: [0x00][json_len:4B][JSON][raw bytes] if len(output) < 5 { - plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, false, elapsed.Milliseconds()) return result, fmt.Errorf("malformed success response from plugin") } jsonLen := binary.BigEndian.Uint32(output[1:5]) if uint32(len(output)-5) < jsonLen { - plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, false, elapsed.Milliseconds()) return result, fmt.Errorf("invalid json length in response frame: %d exceeds available %d bytes", jsonLen, len(output)-5) } jsonData := output[5 : 5+jsonLen] rawData := output[5+jsonLen:] if err := json.Unmarshal(jsonData, &result); err != nil { - plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, false, elapsed.Milliseconds()) return result, fmt.Errorf("failed to unmarshal response: %w", err) } setRawOutput(&result, rawData) - plugin.metrics.RecordPluginRequest(ctx, plugin.name, funcName, true, elapsed.Milliseconds()) + success = true log.Trace(ctx, "Plugin call succeeded", "plugin", plugin.name, "function", funcName, "pluginDuration", time.Since(startCall), "navidromeDuration", startCall.Sub(start)) return result, nil } diff --git a/server/subsonic/middlewares.go b/server/subsonic/middlewares.go index 97fc29fe5..cf9896f10 100644 --- a/server/subsonic/middlewares.go +++ b/server/subsonic/middlewares.go @@ -28,20 +28,29 @@ import ( "github.com/navidrome/navidrome/utils/req" ) +// mergeFormIntoQuery parses form data (both URL query params and POST body) +// and writes all values back into r.URL.RawQuery. This is needed because +// some Subsonic clients send parameters as form fields instead of query params. +// This support the OpenSubsonic `formPost` extension +func mergeFormIntoQuery(r *http.Request) error { + if err := r.ParseForm(); err != nil { + return err + } + var parts []string + for key, values := range r.Form { + for _, v := range values { + parts = append(parts, url.QueryEscape(key)+"="+url.QueryEscape(v)) + } + } + r.URL.RawQuery = strings.Join(parts, "&") + return nil +} + func postFormToQueryParams(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - err := r.ParseForm() - if err != nil { + if err := mergeFormIntoQuery(r); err != nil { sendError(w, r, newError(responses.ErrorGeneric, err.Error())) } - var parts []string - for key, values := range r.Form { - for _, v := range values { - parts = append(parts, url.QueryEscape(key)+"="+url.QueryEscape(v)) - } - } - r.URL.RawQuery = strings.Join(parts, "&") - next.ServeHTTP(w, r) }) } @@ -169,17 +178,9 @@ func authenticate(ds model.DataStore) func(next http.Handler) http.Handler { func ValidateAuth(ds model.DataStore, r *http.Request) (*model.User, error) { // Parse form data into query params (same as postFormToQueryParams middleware, // which is not in the call chain when ValidateAuth is used directly) - if err := r.ParseForm(); err != nil { + if err := mergeFormIntoQuery(r); err != nil { return nil, fmt.Errorf("parsing form: %w", err) } - var parts []string - for key, values := range r.Form { - for _, v := range values { - parts = append(parts, url.QueryEscape(key)+"="+url.QueryEscape(v)) - } - } - r.URL.RawQuery = strings.Join(parts, "&") - return authenticateRequest(ds, r) }