diff --git a/services/graph/pkg/middleware/path_lookup.go b/services/graph/pkg/middleware/path_lookup.go index ff634ca81a..79d4b3e3ca 100644 --- 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}:/[:/][:] // 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:/[:/][:] -// /v1beta1/drives/{driveID}/items/{itemID}:/[:/][:] +// /{version}/drives/{driveID}/root:/[:/][:] +// /{version}/drives/{driveID}/items/{itemID}:/[:/][:] // -// 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 { diff --git a/services/graph/pkg/middleware/path_lookup_test.go b/services/graph/pkg/middleware/path_lookup_test.go index 11ffe87801..e12df34c5b 100644 --- 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: "", }, }