fix(graph): address Copilot review feedback on path lookup middleware

- Drop double-decoding of URL path components. r.URL.Path is already decoded by net/http; calling url.PathUnescape again would let crafted inputs like "%252F" become "/", changing path semantics. Path and anchor-id strings now go straight to utils.MakeRelativePath / storagespace.ParseID. - Distinguish operational errors from "not found". Gateway selection failure, RPC transport errors, and unexpected CS3 status codes now surface as 500 (don't mask outages); only NOT_FOUND and PERMISSION_DENIED collapse to 404 (no existence disclosure). Implemented via a sentinel errPathNotFound + a 500 fall-through. - Refactor the two regex-handling branches in rewriteColonPath to share a resolution path via a small colonMatch struct + matchInto/extract helpers. Removes the SonarCloud duplication finding without changing behavior. - Update the docstring on ResolveGraphPath to reflect that the rewrite preserves the requested API version (/{version}/...) rather than hard-coding /v1beta1/... - Test cleanup: register t.Cleanup to remove the per-subtest selector entry from pool's global selectors map after the subtest, and update the "unexpected status" case to expect 500 (matches the new error semantics).
2026-07-13 17:12:05 -04:00 · 2026-04-27 22:53:51 +02:00
parent 0b373817a8
commit f31e50381f
2 changed files with 131 additions and 90 deletions
--- a/services/graph/pkg/middleware/path_lookup.go
+++ b/services/graph/pkg/middleware/path_lookup.go
@@ -2,8 +2,9 @@ package middleware

 import (
 	"context"
+	"errors"
+	"fmt"
 	"net/http"
-	"net/url"
 	"regexp"
 	"strings"

@@ -24,15 +25,15 @@ import (
 // where {version} is "v1.0" or "v1beta1" — preserves the requested version
 // so it lands on a registered route.
 //   group 1: prefix up to and including the driveID
-//   group 2: driveID (URL-encoded)
+//   group 2: driveID
 //   group 3: path (with leading slash)
 //   group 4: suffix (with leading slash) — empty for direct item lookup
 var rootColonRe = regexp.MustCompile(`^(.*?/v1(?:\.0|beta1)/drives/([^/]+))/root:(/.+?)(?::(/[^:]+))?:?$`)

 // {HTTP.Root}/{version}/drives/{driveID}/items/{itemID}:/<path>[:/<suffix>][:]
 //   group 1: prefix up to and including the driveID
-//   group 2: driveID (URL-encoded)
-//   group 3: anchor itemID (URL-encoded)
+//   group 2: driveID
+//   group 3: anchor itemID
 //   group 4: path (with leading slash)
 //   group 5: suffix (with leading slash)
 var itemColonRe = regexp.MustCompile(`^(.*?/v1(?:\.0|beta1)/drives/([^/]+))/items/([^/]+):(/.+?)(?::(/[^:]+))?:?$`)
@@ -43,44 +44,57 @@ type contextKey string
 // tracing/logging consumers.
 const OriginalPathContextKey contextKey = "graph.original_path"

+// errPathNotFound is the sentinel returned when a colon-syntax URL matches
+// the pattern but the path either doesn't exist or the user lacks permission
+// to see it. Both cases collapse to a single 404 — never disclose existence
+// to unauthorized callers. Distinct from operational errors (gateway,
+// transport, unexpected status), which surface as 5xx.
+var errPathNotFound = errors.New("path not found")
+
 // ResolveGraphPath returns middleware that detects MS Graph colon-syntax
 // path lookup URLs and rewrites them to the canonical
-// /v1beta1/drives/{driveID}/items/{resolvedItemID}/{suffix} form before
-// chi performs route matching.
+// /{version}/drives/{driveID}/items/{resolvedItemID}{suffix} form before
+// chi performs route matching. The requested API version is preserved
+// (for example, "v1.0" or "v1beta1").
 //
 // Two URL shapes are recognized:
 //
-//	/v1beta1/drives/{driveID}/root:/<path>[:/<suffix>][:]
-//	/v1beta1/drives/{driveID}/items/{itemID}:/<path>[:/<suffix>][:]
+//	/{version}/drives/{driveID}/root:/<path>[:/<suffix>][:]
+//	/{version}/drives/{driveID}/items/{itemID}:/<path>[:/<suffix>][:]
 //
-// Path resolution runs as the request user via CS3 Stat. Both NOT_FOUND
-// and PERMISSION_DENIED collapse to a 404 response so existence isn't
-// disclosed to unauthorized callers.
+// Path resolution runs as the request user via CS3 Stat. NOT_FOUND and
+// PERMISSION_DENIED collapse to 404 (no existence disclosure); operational
+// failures (gateway selection, RPC transport, unexpected status) surface
+// as 5xx so outages aren't masked.
 //
-// URLs without colon syntax fast-path through the middleware with a
-// single substring check.
+// URLs without colon syntax fast-path through with a single substring check.
 func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log.Logger) func(http.Handler) http.Handler {
 	l := logger.With().Str("middleware", "graphPathLookup").Logger()
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			// Fast-path: skip URLs that can't be colon-syntax
+			// Fast-path: skip URLs that can't be colon-syntax.
 			if !strings.Contains(r.URL.Path, ":") {
 				next.ServeHTTP(w, r)
 				return
 			}

 			original := r.URL.Path
-			rewritten, matched := rewriteColonPath(r.Context(), gws, l, original)
-			if !matched {
-				next.ServeHTTP(w, r)
-				return
-			}
-			if rewritten == "" {
-				// Resolution failed: not found, permission denied, or invalid input.
-				// Collapse to 404 to avoid disclosing existence.
-				l.Debug().Str("original", original).Msg("colon-path resolution failed; returning 404")
+			rewritten, err := rewriteColonPath(r.Context(), gws, l, original)
+			switch {
+			case errors.Is(err, errPathNotFound):
+				l.Debug().Str("original", original).Msg("colon-path resolution: not found")
 				errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "item not found")
 				return
+			case err != nil:
+				l.Error().Err(err).Str("original", original).Msg("colon-path resolution: internal error")
+				errorcode.GeneralException.Render(
+					w, r, http.StatusInternalServerError, "internal error resolving path",
+				)
+				return
+			case rewritten == "":
+				// No colon-syntax match — pass through untouched.
+				next.ServeHTTP(w, r)
+				return
 			}

 			l.Debug().
@@ -88,7 +102,6 @@ func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log.
 				Str("rewritten", rewritten).
 				Msg("rewrote MS Graph colon-syntax path")

-			// Stash original URL for downstream logging/tracing.
 			ctx := context.WithValue(r.Context(), OriginalPathContextKey, original)
 			r = r.WithContext(ctx)
 			r.URL.Path = rewritten
@@ -98,94 +111,117 @@ func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log.
 	}
 }

-// rewriteColonPath returns:
-//   - rewritten URL + true  when the URL matched a colon-syntax pattern and resolution succeeded
-//   - "" + true             when the URL matched a colon-syntax pattern but resolution failed (caller should 404)
-//   - "" + false            when the URL did not match any colon-syntax pattern
-func rewriteColonPath(ctx context.Context, gws pool.Selectable[gateway.GatewayAPIClient], logger zerolog.Logger, urlPath string) (string, bool) {
-	if m := rootColonRe.FindStringSubmatch(urlPath); m != nil {
-		prefix, driveIDStr, relPath, suffix := m[1], m[2], m[3], m[4]
-		driveID, err := decodeAndParseID(driveIDStr)
-		if err != nil {
-			logger.Debug().Err(err).Str("driveID", driveIDStr).Msg("invalid driveID in colon path")
-			return "", true
-		}
-		itemID, ok := resolvePath(ctx, gws, logger, &driveID, relPath)
-		if !ok {
-			return "", true
-		}
-		return buildCanonicalPath(prefix, itemID, suffix), true
-	}
-
-	if m := itemColonRe.FindStringSubmatch(urlPath); m != nil {
-		prefix, _, anchorIDStr, relPath, suffix := m[1], m[2], m[3], m[4], m[5]
-		anchorID, err := decodeAndParseID(anchorIDStr)
-		if err != nil {
-			logger.Debug().Err(err).Str("itemID", anchorIDStr).Msg("invalid item anchor in colon path")
-			return "", true
-		}
-		itemID, ok := resolvePath(ctx, gws, logger, &anchorID, relPath)
-		if !ok {
-			return "", true
-		}
-		return buildCanonicalPath(prefix, itemID, suffix), true
-	}
-
-	return "", false
+// colonMatch is the normalized result of matching either the root- or
+// item-anchored colon-syntax regex. The two regexes capture different
+// groups; this struct erases that difference for downstream resolution.
+type colonMatch struct {
+	prefix      string // canonical prefix up to and including /drives/{driveID}
+	anchorIDStr string // resource id to anchor path resolution (driveID for root, itemID for item-anchored)
+	relPath     string // relative path with leading slash
+	suffix      string // suffix with leading slash (e.g. "/children"); may be empty
 }

-func resolvePath(ctx context.Context, gws pool.Selectable[gateway.GatewayAPIClient], logger zerolog.Logger, anchor *storageprovider.ResourceId, rawPath string) (string, bool) {
+// rewriteColonPath returns:
+//   - ""        + nil               — no colon-syntax pattern matched (passthrough)
+//   - rewritten + nil               — matched and resolved to a canonical URL
+//   - ""        + errPathNotFound   — matched but path doesn't exist or user lacks permission (caller renders 404)
+//   - ""        + other error       — matched but operational/internal error (caller renders 5xx)
+func rewriteColonPath(
+	ctx context.Context,
+	gws pool.Selectable[gateway.GatewayAPIClient],
+	logger zerolog.Logger,
+	urlPath string,
+) (string, error) {
+	var match colonMatch
+	switch {
+	case matchInto(rootColonRe, urlPath, &match, rootMatchExtract):
+	case matchInto(itemColonRe, urlPath, &match, itemMatchExtract):
+	default:
+		return "", nil
+	}
+
+	// r.URL.Path is already URL-decoded by net/http; do NOT call url.PathUnescape
+	// here, that would double-decode and let crafted inputs like "%252F" become
+	// "/", changing path semantics.
+	anchor, err := storagespace.ParseID(match.anchorIDStr)
+	if err != nil {
+		// An unparseable anchor ID can't reference a real resource — collapse
+		// to "not found" rather than leaking parser internals via 5xx.
+		logger.Debug().Err(err).Str("anchor", match.anchorIDStr).Msg("invalid anchor id in colon path")
+		return "", errPathNotFound
+	}
+
+	itemID, err := resolvePath(ctx, gws, logger, &anchor, match.relPath)
+	if err != nil {
+		return "", err
+	}
+	return buildCanonicalPath(match.prefix, itemID, match.suffix), nil
+}
+
+// matchInto runs the regex; on a hit, populates *out via extract and returns true.
+// A small indirection that lets the switch in rewriteColonPath stay flat.
+func matchInto(re *regexp.Regexp, s string, out *colonMatch, extract func([]string) colonMatch) bool {
+	m := re.FindStringSubmatch(s)
+	if m == nil {
+		return false
+	}
+	*out = extract(m)
+	return true
+}
+
+func rootMatchExtract(m []string) colonMatch {
+	return colonMatch{prefix: m[1], anchorIDStr: m[2], relPath: m[3], suffix: m[4]}
+}
+
+func itemMatchExtract(m []string) colonMatch {
+	return colonMatch{prefix: m[1], anchorIDStr: m[3], relPath: m[4], suffix: m[5]}
+}
+
+// resolvePath translates a relative filesystem path (anchored at the given
+// CS3 resource id) into the resolved item's id, running with the request
+// user's permissions via CS3 Stat.
+func resolvePath(
+	ctx context.Context,
+	gws pool.Selectable[gateway.GatewayAPIClient],
+	logger zerolog.Logger,
+	anchor *storageprovider.ResourceId,
+	rawPath string,
+) (string, error) {
 	gw, err := gws.Next()
 	if err != nil {
-		logger.Error().Err(err).Msg("could not select gateway client")
-		return "", false
-	}
-
-	decoded, err := url.PathUnescape(rawPath)
-	if err != nil {
-		logger.Debug().Err(err).Str("path", rawPath).Msg("failed to URL-decode path segment")
-		return "", false
+		return "", fmt.Errorf("gateway selector: %w", err)
 	}

+	// rawPath comes from r.URL.Path which is already decoded by net/http —
+	// no extra unescape (would double-decode crafted "%25xx" inputs).
 	statRes, err := gw.Stat(ctx, &storageprovider.StatRequest{
 		Ref: &storageprovider.Reference{
 			ResourceId: anchor,
-			Path:       utils.MakeRelativePath(decoded),
+			Path:       utils.MakeRelativePath(rawPath),
 		},
 	})
 	if err != nil {
-		logger.Error().Err(err).Msg("Stat call failed during colon-path resolution")
-		return "", false
+		return "", fmt.Errorf("CS3 Stat: %w", err)
 	}

 	switch statRes.GetStatus().GetCode() {
 	case cs3rpc.Code_CODE_OK:
 		// fall through
 	case cs3rpc.Code_CODE_NOT_FOUND, cs3rpc.Code_CODE_PERMISSION_DENIED:
-		// Both collapse to "not found" — never tell unauthorized callers
-		// that the resource exists.
-		return "", false
+		return "", errPathNotFound
 	default:
-		logger.Debug().
-			Str("code", statRes.GetStatus().GetCode().String()).
-			Str("message", statRes.GetStatus().GetMessage()).
-			Msg("unexpected Stat status during colon-path resolution")
-		return "", false
+		return "", fmt.Errorf(
+			"CS3 Stat returned %s: %s",
+			statRes.GetStatus().GetCode(),
+			statRes.GetStatus().GetMessage(),
+		)
 	}

 	id := statRes.GetInfo().GetId()
 	if id == nil {
-		return "", false
+		return "", fmt.Errorf("CS3 Stat returned OK but missing Info.Id")
 	}
-	return storagespace.FormatResourceID(id), true
-}
-
-func decodeAndParseID(s string) (storageprovider.ResourceId, error) {
-	decoded, err := url.PathUnescape(s)
-	if err != nil {
-		return storageprovider.ResourceId{}, err
-	}
-	return storagespace.ParseID(decoded)
+	return storagespace.FormatResourceID(id), nil
 }

 func buildCanonicalPath(prefix, itemID, suffix string) string {
--- a/services/graph/pkg/middleware/path_lookup_test.go
+++ b/services/graph/pkg/middleware/path_lookup_test.go
@@ -26,9 +26,12 @@ const (
 func newTestSelector(t *testing.T, gw *cs3mocks.GatewayAPIClient) pool.Selectable[gateway.GatewayAPIClient] {
 	t.Helper()
 	// Unique key per test so pool's selector cache doesn't hand back a stale gw.
+	// t.Cleanup removes the entry from pool's global selectors map after the
+	// subtest, so global state doesn't grow across the suite.
 	svcName := "TestGatewaySelector"
 	key := "test.gateway." + t.Name()
 	pool.RemoveSelector(svcName + key)
+	t.Cleanup(func() { pool.RemoveSelector(svcName + key) })
 	return pool.GetSelector[gateway.GatewayAPIClient](
 		svcName,
 		key,
@@ -154,11 +157,13 @@ func TestResolveGraphPath(t *testing.T) {
 			expectURLPath:    "",
 		},
 		{
-			name:             "Stat unexpected status returns 404",
+			// Operational/unexpected CS3 statuses must NOT be collapsed to 404 —
+			// that would mask outages. Surface as 500 like other graph handlers do.
+			name:             "Stat unexpected status returns 500 (not 404 — don't mask outages)",
 			urlPath:          "/graph/v1.0/drives/" + testDriveID + "/root:/Anything:",
 			statCode:         cs3rpc.Code_CODE_INTERNAL,
 			expectStatCalled: true,
-			expectStatus:     http.StatusNotFound,
+			expectStatus:     http.StatusInternalServerError,
 			expectURLPath:    "",
 		},
 	}