From 0b373817a89726cb9bd5d21353dbe48cbd1c0f6d Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Mon, 27 Apr 2026 21:22:25 +0200 Subject: [PATCH 01/13] feat(graph): add MS Graph colon-syntax path lookup middleware Adds a chi middleware that detects MS Graph colon-syntax URLs and rewrites them to the canonical /items/{itemID}/... form before chi performs route matching. Existing handlers, routes, and GetDriveAndItemIDParam stay unchanged. Two URL shapes are recognized at both /v1.0 and /v1beta1: /drives/{driveID}/root:/[:/][:] /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. URLs without colon syntax fast-path through with a single substring check. The original URL is stashed in request context under OriginalPathContextKey for downstream tracing/logging. The middleware is registered as a top-level mux.Use so it runs before any route matching: chi middleware on a sub-router runs after the prefix is matched but cannot redirect to a different leaf route. Top-level middleware lets URL rewriting actually re-route the request. Tests cover regex matching across versions, all rewrite variants (root/items anchored, with/without suffix, with/without trailing colon, deep paths), NOT_FOUND -> 404, PERMISSION_DENIED -> 404 (security: no existence disclosure), and original-URL preservation in request context. --- services/graph/pkg/middleware/path_lookup.go | 197 ++++++++++++++++ .../graph/pkg/middleware/path_lookup_test.go | 219 ++++++++++++++++++ services/graph/pkg/service/v0/service.go | 6 + 3 files changed, 422 insertions(+) create mode 100644 services/graph/pkg/middleware/path_lookup.go create mode 100644 services/graph/pkg/middleware/path_lookup_test.go diff --git a/services/graph/pkg/middleware/path_lookup.go b/services/graph/pkg/middleware/path_lookup.go new file mode 100644 index 0000000000..ff634ca81a --- /dev/null +++ b/services/graph/pkg/middleware/path_lookup.go @@ -0,0 +1,197 @@ +package middleware + +import ( + "context" + "net/http" + "net/url" + "regexp" + "strings" + + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + cs3rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" + storageprovider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + + "github.com/rs/zerolog" + + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" + "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" + "github.com/opencloud-eu/reva/v2/pkg/storagespace" + "github.com/opencloud-eu/reva/v2/pkg/utils" +) + +// {HTTP.Root}/{version}/drives/{driveID}/root:/[:/][:] +// 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 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 4: path (with leading slash) +// group 5: suffix (with leading slash) +var itemColonRe = regexp.MustCompile(`^(.*?/v1(?:\.0|beta1)/drives/([^/]+))/items/([^/]+):(/.+?)(?::(/[^:]+))?:?$`) + +type contextKey string + +// OriginalPathContextKey holds the pre-rewrite request path for downstream +// tracing/logging consumers. +const OriginalPathContextKey contextKey = "graph.original_path" + +// 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. +// +// Two URL shapes are recognized: +// +// /v1beta1/drives/{driveID}/root:/[:/][:] +// /v1beta1/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. +// +// URLs without colon syntax fast-path through the middleware 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 + 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") + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "item not found") + return + } + + l.Debug(). + Str("original", original). + 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 + r.URL.RawPath = "" + next.ServeHTTP(w, r) + }) + } +} + +// 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 +} + +func resolvePath(ctx context.Context, gws pool.Selectable[gateway.GatewayAPIClient], logger zerolog.Logger, anchor *storageprovider.ResourceId, rawPath string) (string, bool) { + 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 + } + + statRes, err := gw.Stat(ctx, &storageprovider.StatRequest{ + Ref: &storageprovider.Reference{ + ResourceId: anchor, + Path: utils.MakeRelativePath(decoded), + }, + }) + if err != nil { + logger.Error().Err(err).Msg("Stat call failed during colon-path resolution") + return "", false + } + + 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 + default: + logger.Debug(). + Str("code", statRes.GetStatus().GetCode().String()). + Str("message", statRes.GetStatus().GetMessage()). + Msg("unexpected Stat status during colon-path resolution") + return "", false + } + + id := statRes.GetInfo().GetId() + if id == nil { + return "", false + } + 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) +} + +func buildCanonicalPath(prefix, itemID, suffix string) string { + // r.URL.Path is the decoded form per Go's net/http convention; chi tree + // matching reads it directly. Insert the raw itemID without escaping so + // chi's {itemID} param captures the same string the handler will see + // after parseIDParam unescapes (no-op for already-decoded chars). + return prefix + "/items/" + itemID + suffix +} diff --git a/services/graph/pkg/middleware/path_lookup_test.go b/services/graph/pkg/middleware/path_lookup_test.go new file mode 100644 index 0000000000..11ffe87801 --- /dev/null +++ b/services/graph/pkg/middleware/path_lookup_test.go @@ -0,0 +1,219 @@ +package middleware_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + cs3rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" + storageprovider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "google.golang.org/grpc" + + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/services/graph/pkg/middleware" + "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" + cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" +) + +const ( + testDriveID = "storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd" + testItemID = "storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d" +) + +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. + svcName := "TestGatewaySelector" + key := "test.gateway." + t.Name() + pool.RemoveSelector(svcName + key) + return pool.GetSelector[gateway.GatewayAPIClient]( + svcName, + key, + func(cc grpc.ClientConnInterface) gateway.GatewayAPIClient { return gw }, + ) +} + +// statResponse builds a CS3 StatResponse with the given status code, optionally +// returning a resource info populated with testItemID for OK responses. +func statResponse(code cs3rpc.Code, withInfo bool) *storageprovider.StatResponse { + res := &storageprovider.StatResponse{Status: &cs3rpc.Status{Code: code}} + if withInfo { + res.Info = &storageprovider.ResourceInfo{ + Id: &storageprovider.ResourceId{ + StorageId: "storage-users-1", + SpaceId: "f503f6fe-2656-4b0f-8289-fb3184962dfd", + OpaqueId: "f0e20017-9cba-498a-87e5-3467b976604d", + }, + } + } + return res +} + +// recordingHandler captures the request URL path it receives so tests can assert +// the middleware rewrote (or passed through) correctly. +func recordingHandler() (http.Handler, *string) { + var seen string + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + seen = r.URL.Path + w.WriteHeader(http.StatusOK) + }), &seen +} + +func TestResolveGraphPath(t *testing.T) { + tests := []struct { + name string + urlPath string + statCode cs3rpc.Code + statErr error + expectStatCalled bool + expectStatus int + expectURLPath string // empty = handler not invoked + }{ + { + name: "non-colon URL passes through unchanged", + urlPath: "/graph/v1.0/me/drives", + expectStatCalled: false, + expectStatus: http.StatusOK, + expectURLPath: "/graph/v1.0/me/drives", + }, + { + name: "URL with colon but not matching pattern passes through", + urlPath: "/graph/v1.0/some/other/route:/foo", + expectStatCalled: false, + expectStatus: http.StatusOK, + expectURLPath: "/graph/v1.0/some/other/route:/foo", + }, + { + name: "v1.0 root-anchored with /children rewrites", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Documents:/children", + statCode: cs3rpc.Code_CODE_OK, + expectStatCalled: true, + expectStatus: http.StatusOK, + expectURLPath: "/graph/v1.0/drives/" + testDriveID + "/items/storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d/children", + }, + { + name: "v1beta1 root-anchored with /createLink rewrites", + urlPath: "/graph/v1beta1/drives/" + testDriveID + "/root:/Documents:/createLink", + statCode: cs3rpc.Code_CODE_OK, + expectStatCalled: true, + expectStatus: http.StatusOK, + expectURLPath: "/graph/v1beta1/drives/" + testDriveID + "/items/storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d/createLink", + }, + { + name: "trailing colon (no suffix) rewrites to bare item URL", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Documents:", + statCode: cs3rpc.Code_CODE_OK, + expectStatCalled: true, + expectStatus: http.StatusOK, + expectURLPath: "/graph/v1.0/drives/" + testDriveID + "/items/storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d", + }, + { + name: "no trailing colon, no suffix rewrites to bare item URL", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Documents", + statCode: cs3rpc.Code_CODE_OK, + expectStatCalled: true, + expectStatus: http.StatusOK, + expectURLPath: "/graph/v1.0/drives/" + testDriveID + "/items/storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d", + }, + { + name: "deep path rewrites correctly", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Documents/Reports:/children", + statCode: cs3rpc.Code_CODE_OK, + expectStatCalled: true, + expectStatus: http.StatusOK, + expectURLPath: "/graph/v1.0/drives/" + testDriveID + "/items/storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d/children", + }, + { + name: "item-anchored colon syntax rewrites", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/items/" + testItemID + ":/notes.txt:/children", + statCode: cs3rpc.Code_CODE_OK, + expectStatCalled: true, + expectStatus: http.StatusOK, + expectURLPath: "/graph/v1.0/drives/" + testDriveID + "/items/storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d/children", + }, + { + name: "Stat NOT_FOUND returns 404 without invoking handler", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Missing:", + statCode: cs3rpc.Code_CODE_NOT_FOUND, + expectStatCalled: true, + expectStatus: http.StatusNotFound, + expectURLPath: "", // handler must NOT be called + }, + { + // CRITICAL security test: PERMISSION_DENIED must not leak existence. + // We collapse it to 404, identical to NOT_FOUND, so an unauthorized + // caller can't distinguish "doesn't exist" from "exists but you can't see it". + name: "Stat PERMISSION_DENIED returns 404 (not 403) — don't disclose existence", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Restricted:", + statCode: cs3rpc.Code_CODE_PERMISSION_DENIED, + expectStatCalled: true, + expectStatus: http.StatusNotFound, + expectURLPath: "", + }, + { + name: "Stat unexpected status returns 404", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Anything:", + statCode: cs3rpc.Code_CODE_INTERNAL, + expectStatCalled: true, + expectStatus: http.StatusNotFound, + expectURLPath: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gw := &cs3mocks.GatewayAPIClient{} + if tt.expectStatCalled { + gw.On("Stat", mock.Anything, mock.Anything). + Return(statResponse(tt.statCode, tt.statCode == cs3rpc.Code_CODE_OK), tt.statErr) + } + + selector := newTestSelector(t, gw) + handler, seen := recordingHandler() + mw := middleware.ResolveGraphPath(selector, log.NewLogger()) + + req := httptest.NewRequest(http.MethodGet, "http://localhost"+tt.urlPath, nil) + rr := httptest.NewRecorder() + + mw(handler).ServeHTTP(rr, req) + + assert.Equal(t, tt.expectStatus, rr.Code, "status code") + assert.Equal(t, tt.expectURLPath, *seen, "URL seen by next handler") + + if tt.expectStatCalled { + gw.AssertCalled(t, "Stat", mock.Anything, mock.Anything) + } else { + gw.AssertNotCalled(t, "Stat", mock.Anything, mock.Anything) + } + }) + } +} + +// TestResolveGraphPath_OriginalPathContext verifies the rewrite preserves the +// original URL in request context for downstream tracing/logging. +func TestResolveGraphPath_OriginalPathContext(t *testing.T) { + gw := &cs3mocks.GatewayAPIClient{} + gw.On("Stat", mock.Anything, mock.Anything). + Return(statResponse(cs3rpc.Code_CODE_OK, true), nil) + + selector := newTestSelector(t, gw) + original := "/graph/v1.0/drives/" + testDriveID + "/root:/Documents:/children" + + var capturedOriginal interface{} + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedOriginal = r.Context().Value(middleware.OriginalPathContextKey) + w.WriteHeader(http.StatusOK) + }) + + mw := middleware.ResolveGraphPath(selector, log.NewLogger()) + req := httptest.NewRequest(http.MethodGet, "http://localhost"+original, nil) + rr := httptest.NewRecorder() + + mw(handler).ServeHTTP(rr, req) + + assert.Equal(t, http.StatusOK, rr.Code) + assert.Equal(t, original, capturedOriginal, "original URL must be available via OriginalPathContextKey") +} diff --git a/services/graph/pkg/service/v0/service.go b/services/graph/pkg/service/v0/service.go index ecce7f69e1..16c811c97a 100644 --- a/services/graph/pkg/service/v0/service.go +++ b/services/graph/pkg/service/v0/service.go @@ -132,6 +132,12 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx m := chi.NewMux() m.Use(options.Middleware...) + // Must be top-level mux.Use, not r.Use inside a Route block: chi + // sub-router middleware runs after the prefix has already been matched, + // so URL rewriting at that point can't redirect to a different leaf + // route. Top-level middleware runs before any matching, so the rewritten + // URL is what chi walks the radix tree against. + m.Use(graphm.ResolveGraphPath(options.GatewaySelector, options.Logger)) m.Use( otelchi.Middleware( "graph", From f31e50381ff831b890475bbb2d4472d779743079 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Mon, 27 Apr 2026 22:53:51 +0200 Subject: [PATCH 02/13] 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). --- services/graph/pkg/middleware/path_lookup.go | 212 ++++++++++-------- .../graph/pkg/middleware/path_lookup_test.go | 9 +- 2 files changed, 131 insertions(+), 90 deletions(-) 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: "", }, } From e49959b833cf57755e77ad13d07d556b8caa3d6e Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Tue, 28 Apr 2026 10:30:16 +0200 Subject: [PATCH 03/13] fix(graph): preserve RawPath workaround + use NopLogger in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-up Copilot review nits on the colon-syntax middleware: - Don't blank r.URL.RawPath after the rewrite. Graph.ServeHTTP sets RawPath = EscapedPath() as a workaround for chi's parameter-binding quirks with `$`/`!` in IDs (see go-chi/chi#641). Clearing RawPath for rewritten requests negates that workaround. Re-establish the same invariant after the rewrite. - Tests previously used log.NewLogger(), which mutates global zerolog state. Switch to log.NopLogger() — order-independent, no global side effects. --- services/graph/pkg/middleware/path_lookup.go | 13 ++++++++++++- services/graph/pkg/middleware/path_lookup_test.go | 4 ++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/services/graph/pkg/middleware/path_lookup.go b/services/graph/pkg/middleware/path_lookup.go index 79d4b3e3ca..0c11ddc411 100644 --- a/services/graph/pkg/middleware/path_lookup.go +++ b/services/graph/pkg/middleware/path_lookup.go @@ -105,7 +105,18 @@ func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log. ctx := context.WithValue(r.Context(), OriginalPathContextKey, original) r = r.WithContext(ctx) r.URL.Path = rewritten - r.URL.RawPath = "" + // Match the chi-escaping workaround in Graph.ServeHTTP — RawPath + // must be a properly-escaped form of Path so chi's parameter + // binding works for rewritten requests too. Drive/item IDs + // contain `$` and `!`, both of which need percent-encoding for + // chi to round-trip them through its tree match. + // (See services/graph/pkg/service/v0/graph.go ServeHTTP and + // https://github.com/go-chi/chi/issues/641#issuecomment-883156692.) + // + // EscapedPath() re-encodes Path because the existing RawPath + // (set by Graph.ServeHTTP for the original URL) no longer + // unescapes to our rewritten Path. + r.URL.RawPath = r.URL.EscapedPath() next.ServeHTTP(w, r) }) } diff --git a/services/graph/pkg/middleware/path_lookup_test.go b/services/graph/pkg/middleware/path_lookup_test.go index e12df34c5b..bc1093ed57 100644 --- a/services/graph/pkg/middleware/path_lookup_test.go +++ b/services/graph/pkg/middleware/path_lookup_test.go @@ -178,7 +178,7 @@ func TestResolveGraphPath(t *testing.T) { selector := newTestSelector(t, gw) handler, seen := recordingHandler() - mw := middleware.ResolveGraphPath(selector, log.NewLogger()) + mw := middleware.ResolveGraphPath(selector, log.NopLogger()) req := httptest.NewRequest(http.MethodGet, "http://localhost"+tt.urlPath, nil) rr := httptest.NewRecorder() @@ -213,7 +213,7 @@ func TestResolveGraphPath_OriginalPathContext(t *testing.T) { w.WriteHeader(http.StatusOK) }) - mw := middleware.ResolveGraphPath(selector, log.NewLogger()) + mw := middleware.ResolveGraphPath(selector, log.NopLogger()) req := httptest.NewRequest(http.MethodGet, "http://localhost"+original, nil) rr := httptest.NewRecorder() From 7498e8f848ec7ebb4924902c6bb54e6f3788e53b Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Tue, 28 Apr 2026 10:55:39 +0200 Subject: [PATCH 04/13] fix(graph): map UNAUTHENTICATED to 401, validate drive/item id pair MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three more Copilot review nits on the colon-syntax middleware: - CS3 Stat returning UNAUTHENTICATED now surfaces as HTTP 401 (was 500 via the default-case fallback). Distinct sentinel + handler branch. - Item-anchored form (/drives/{driveID}/items/{itemID}:/...) now validates that driveID's storage/space prefix matches the itemID's, short-circuiting to 400 InvalidRequest on mismatch instead of doing a CS3 Stat that would only fail at the handler layer. - ParseID failures on the anchor id now surface as 400 (was 404). In practice this branch is defensive — storagespace.ParseID is lenient and only errors on empty input, which the regex already filters out — but the right semantic for malformed client input is 400, not 404. Tests: added two new cases (UNAUTHENTICATED → 401, drive/item id mismatch → 400). The "malformed drive id" case Copilot suggested isn't reachable in practice given ParseID's leniency, so it's not covered. --- services/graph/pkg/middleware/path_lookup.go | 71 +++++++++++++++---- .../graph/pkg/middleware/path_lookup_test.go | 20 ++++++ 2 files changed, 77 insertions(+), 14 deletions(-) diff --git a/services/graph/pkg/middleware/path_lookup.go b/services/graph/pkg/middleware/path_lookup.go index 0c11ddc411..a13c099d3e 100644 --- a/services/graph/pkg/middleware/path_lookup.go +++ b/services/graph/pkg/middleware/path_lookup.go @@ -44,12 +44,20 @@ 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") +// Sentinels distinguishing the resolution outcomes that map to specific HTTP +// statuses. Anything else surfaces as 500. +// +// errPathNotFound — path doesn't exist or the user lacks permission to +// see it. Both collapse to 404 (no existence disclosure). +// errInvalidRequest — client sent a malformed input (unparseable drive/item +// id, drive/item mismatch in item-anchored form). 400. +// errUnauthenticated — the gateway said the caller isn't authenticated for +// the lookup (token expired, cross-storage auth, etc.). 401. +var ( + errPathNotFound = errors.New("path not found") + errInvalidRequest = errors.New("invalid request") + errUnauthenticated = errors.New("unauthenticated") +) // ResolveGraphPath returns middleware that detects MS Graph colon-syntax // path lookup URLs and rewrites them to the canonical @@ -85,6 +93,14 @@ func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log. l.Debug().Str("original", original).Msg("colon-path resolution: not found") errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "item not found") return + case errors.Is(err, errInvalidRequest): + l.Debug().Str("original", original).Msg("colon-path resolution: invalid request") + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid request") + return + case errors.Is(err, errUnauthenticated): + l.Debug().Str("original", original).Msg("colon-path resolution: unauthenticated") + errorcode.Unauthenticated.Render(w, r, http.StatusUnauthorized, "unauthenticated") + return case err != nil: l.Error().Err(err).Str("original", original).Msg("colon-path resolution: internal error") errorcode.GeneralException.Render( @@ -127,16 +143,19 @@ func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log. // groups; this struct erases that difference for downstream resolution. type colonMatch struct { prefix string // canonical prefix up to and including /drives/{driveID} + driveIDStr string // captured driveID for item-anchored form (empty for root-anchored, where anchorIDStr already IS the 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 } // 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) +// - "" + nil — no colon-syntax pattern matched (passthrough) +// - rewritten + nil — matched and resolved to a canonical URL +// - "" + errPathNotFound — path doesn't exist or user lacks permission (404) +// - "" + errInvalidRequest — malformed input (400) +// - "" + errUnauthenticated — gateway said caller isn't authenticated (401) +// - "" + other error — operational / internal failure (5xx) func rewriteColonPath( ctx context.Context, gws pool.Selectable[gateway.GatewayAPIClient], @@ -156,10 +175,28 @@ func rewriteColonPath( // "/", 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. + // Unparseable input is malformed by the client, not "not found". logger.Debug().Err(err).Str("anchor", match.anchorIDStr).Msg("invalid anchor id in colon path") - return "", errPathNotFound + return "", errInvalidRequest + } + + // Item-anchored form ("/drives/{driveID}/items/{itemID}:/...") captures + // driveID separately. Validate the itemID belongs to the given driveID + // (storage + space prefix) — otherwise the request is malformed and we + // short-circuit instead of doing an unnecessary CS3 Stat. + if match.driveIDStr != "" { + drive, err := storagespace.ParseID(match.driveIDStr) + if err != nil { + logger.Debug().Err(err).Str("driveID", match.driveIDStr).Msg("invalid drive id in colon path") + return "", errInvalidRequest + } + if drive.GetStorageId() != anchor.GetStorageId() || drive.GetSpaceId() != anchor.GetSpaceId() { + logger.Debug(). + Str("driveID", match.driveIDStr). + Str("itemID", match.anchorIDStr). + Msg("drive id does not match item id storage/space") + return "", errInvalidRequest + } } itemID, err := resolvePath(ctx, gws, logger, &anchor, match.relPath) @@ -181,11 +218,15 @@ func matchInto(re *regexp.Regexp, s string, out *colonMatch, extract func([]stri } func rootMatchExtract(m []string) colonMatch { + // Root-anchored: anchorIDStr IS the driveID, so leave driveIDStr empty + // to skip the drive/item match check (it would compare driveID to itself). 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]} + // Item-anchored: capture driveID (m[2]) so the resolver can validate it + // matches the itemID's storage/space. + return colonMatch{prefix: m[1], driveIDStr: m[2], anchorIDStr: m[3], relPath: m[4], suffix: m[5]} } // resolvePath translates a relative filesystem path (anchored at the given @@ -220,6 +261,8 @@ func resolvePath( // fall through case cs3rpc.Code_CODE_NOT_FOUND, cs3rpc.Code_CODE_PERMISSION_DENIED: return "", errPathNotFound + case cs3rpc.Code_CODE_UNAUTHENTICATED: + return "", errUnauthenticated default: return "", fmt.Errorf( "CS3 Stat returned %s: %s", diff --git a/services/graph/pkg/middleware/path_lookup_test.go b/services/graph/pkg/middleware/path_lookup_test.go index bc1093ed57..768bbccd52 100644 --- a/services/graph/pkg/middleware/path_lookup_test.go +++ b/services/graph/pkg/middleware/path_lookup_test.go @@ -166,6 +166,26 @@ func TestResolveGraphPath(t *testing.T) { expectStatus: http.StatusInternalServerError, expectURLPath: "", }, + { + // UNAUTHENTICATED is its own distinct outcome — must surface as 401, + // not 500, so clients can detect "your token is bad" vs "server error". + name: "Stat UNAUTHENTICATED returns 401", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Documents:", + statCode: cs3rpc.Code_CODE_UNAUTHENTICATED, + expectStatCalled: true, + expectStatus: http.StatusUnauthorized, + expectURLPath: "", + }, + { + // Item-anchored form with a driveID that doesn't match the itemID's + // storage/space — the request is malformed; short-circuit to 400 + // instead of doing a Stat that would only fail downstream. + name: "drive id and item id storage/space mismatch returns 400", + urlPath: "/graph/v1.0/drives/storage-users-2$other-space-id/items/" + testItemID + ":/notes.txt:/children", + expectStatCalled: false, + expectStatus: http.StatusBadRequest, + expectURLPath: "", + }, } for _, tt := range tests { From 6343f7e8615f4dc8a6e8757bf088287866535f38 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Tue, 5 May 2026 20:10:48 +0200 Subject: [PATCH 05/13] refactor(graph): drop MS Graph framing from colon-path middleware logs --- services/graph/pkg/middleware/path_lookup.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/graph/pkg/middleware/path_lookup.go b/services/graph/pkg/middleware/path_lookup.go index a13c099d3e..dab46fee9d 100644 --- a/services/graph/pkg/middleware/path_lookup.go +++ b/services/graph/pkg/middleware/path_lookup.go @@ -59,8 +59,8 @@ var ( errUnauthenticated = errors.New("unauthenticated") ) -// ResolveGraphPath returns middleware that detects MS Graph colon-syntax -// path lookup URLs and rewrites them to the canonical +// ResolveGraphPath returns middleware that detects colon-syntax path +// lookup URLs and rewrites them to the canonical // /{version}/drives/{driveID}/items/{resolvedItemID}{suffix} form before // chi performs route matching. The requested API version is preserved // (for example, "v1.0" or "v1beta1"). @@ -116,7 +116,7 @@ func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log. l.Debug(). Str("original", original). Str("rewritten", rewritten). - Msg("rewrote MS Graph colon-syntax path") + Msg("colon-path resolution: rewrote") ctx := context.WithValue(r.Context(), OriginalPathContextKey, original) r = r.WithContext(ctx) From 3b42c6250dbbcbc114b17eeb902b84334ed856ca Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 6 May 2026 12:46:42 +0200 Subject: [PATCH 06/13] test(graph): acceptance tests for MS Graph colon-syntax path lookup Cover the rewrite shapes the middleware handles end-to-end against a real OpenCloud server: root-anchored, item-anchored, deep paths, trailing colon, and the "/:/" sub-route form. Also assert that NOT_FOUND and PERMISSION_DENIED both collapse to 404. The /permissions sub-route is registered only at /v1beta1, and the v1beta1 GetDriveItem handler is share-jail-only, so the v1beta1 mount of the middleware is exercised through the permissions scenario, since there is no other v1beta1 endpoint that works for regular personal-drive items. --- tests/acceptance/bootstrap/GraphContext.php | 165 ++++++++++++++++++ .../apiGraph/colonSyntaxPathLookup.feature | 114 ++++++++++++ 2 files changed, 279 insertions(+) create mode 100644 tests/acceptance/features/apiGraph/colonSyntaxPathLookup.feature diff --git a/tests/acceptance/bootstrap/GraphContext.php b/tests/acceptance/bootstrap/GraphContext.php index cf350be6be..70254354c0 100644 --- a/tests/acceptance/bootstrap/GraphContext.php +++ b/tests/acceptance/bootstrap/GraphContext.php @@ -3334,4 +3334,169 @@ class GraphContext implements Context { $response = $this->unmarkFavorite($user, $itemId); $this->featureContext->theHTTPStatusCodeShouldBe(204, '', $response); } + + /** + * Encode a colon-syntax path segment so slashes survive but the + * structural ":" delimiters of the Graph URL remain literal. + * + * @param string $path + * + * @return string + */ + private function encodeColonPathSegment(string $path): string { + $path = \ltrim($path, '/'); + $parts = \explode('/', $path); + $encoded = \array_map('rawurlencode', $parts); + return \implode('/', $encoded); + } + + /** + * Send a Graph API request and capture the response on FeatureContext. + * + * @param string $user + * @param string $method + * @param string $relativeUrl + * + * @return void + */ + private function sendGraphRequestAndCaptureResponse( + string $user, + string $method, + string $relativeUrl + ): void { + $response = $this->featureContext->sendingToWithDirectUrl($user, $method, $relativeUrl); + $this->featureContext->setResponse($response); + } + + /** + * @When /^user "([^"]*)" gets the drive item with colon path "([^"]*)" of space "([^"]*)" using the Graph API version "(v1\.0|v1beta1)"$/ + * + * Hits /graph/{version}/drives/{driveID}/root:/{path}, exercising the + * colon-syntax path lookup middleware (root-anchored, no suffix). + * + * @param string $user + * @param string $path + * @param string $spaceName + * @param string $apiVersion + * + * @return void + */ + public function userGetsDriveItemWithColonPathOfSpace( + string $user, + string $path, + string $spaceName, + string $apiVersion + ): void { + $driveId = $this->spacesContext->getSpaceIdByName($user, $spaceName); + $encoded = $this->encodeColonPathSegment($path); + $url = "/graph/$apiVersion/drives/$driveId/root:/$encoded"; + $this->sendGraphRequestAndCaptureResponse($user, "GET", $url); + } + + /** + * @When /^user "([^"]*)" gets the drive item with colon path "([^"]*)" of space "([^"]*)" with trailing colon using the Graph API version "(v1\.0|v1beta1)"$/ + * + * Hits /graph/{version}/drives/{driveID}/root:/{path}: with the optional + * trailing ":" — verifies the middleware accepts both forms. + * + * @param string $user + * @param string $path + * @param string $spaceName + * @param string $apiVersion + * + * @return void + */ + public function userGetsDriveItemWithColonPathOfSpaceWithTrailingColon( + string $user, + string $path, + string $spaceName, + string $apiVersion + ): void { + $driveId = $this->spacesContext->getSpaceIdByName($user, $spaceName); + $encoded = $this->encodeColonPathSegment($path); + $url = "/graph/$apiVersion/drives/$driveId/root:/$encoded:"; + $this->sendGraphRequestAndCaptureResponse($user, "GET", $url); + } + + /** + * @When /^user "([^"]*)" gets the drive item with colon path "([^"]*)" relative to folder "([^"]*)" of space "([^"]*)" using the Graph API version "(v1\.0|v1beta1)"$/ + * + * Hits /graph/{version}/drives/{driveID}/items/{itemID}:/{relPath}, the + * item-anchored colon-syntax form. + * + * @param string $user + * @param string $relPath + * @param string $folderName + * @param string $spaceName + * @param string $apiVersion + * + * @return void + */ + public function userGetsDriveItemWithColonPathRelativeToFolderOfSpace( + string $user, + string $relPath, + string $folderName, + string $spaceName, + string $apiVersion + ): void { + $driveId = $this->spacesContext->getSpaceIdByName($user, $spaceName); + $folderId = $this->spacesContext->getResourceId($user, $spaceName, $folderName); + $encoded = $this->encodeColonPathSegment($relPath); + $url = "/graph/$apiVersion/drives/$driveId/items/$folderId:/$encoded"; + $this->sendGraphRequestAndCaptureResponse($user, "GET", $url); + } + + /** + * @When /^user "([^"]*)" lists permissions of the drive item with colon path "([^"]*)" of space "([^"]*)" using the Graph API version "(v1\.0|v1beta1)"$/ + * + * Hits /graph/{version}/drives/{driveID}/root:/{path}:/permissions, the + * "colon path with suffix" form (root-anchored colon path + canonical + * sub-route). + * + * @param string $user + * @param string $path + * @param string $spaceName + * @param string $apiVersion + * + * @return void + */ + public function userListsPermissionsOfDriveItemWithColonPathOfSpace( + string $user, + string $path, + string $spaceName, + string $apiVersion + ): void { + $driveId = $this->spacesContext->getSpaceIdByName($user, $spaceName); + $encoded = $this->encodeColonPathSegment($path); + $url = "/graph/$apiVersion/drives/$driveId/root:/$encoded:/permissions"; + $this->sendGraphRequestAndCaptureResponse($user, "GET", $url); + } + + /** + * @When /^user "([^"]*)" gets the drive item with colon path "([^"]*)" of the personal space of "([^"]*)" using the Graph API version "(v1\.0|v1beta1)"$/ + * + * Same as userGetsDriveItemWithColonPathOfSpace, but the request is + * issued by :user against the personal space drive ID of :owner. Used + * for security tests where one user attempts to reach another user's + * resource via colon syntax — should be indistinguishable from a + * not-found result. + * + * @param string $user + * @param string $path + * @param string $owner + * @param string $apiVersion + * + * @return void + */ + public function userGetsDriveItemWithColonPathOfPersonalSpaceOf( + string $user, + string $path, + string $owner, + string $apiVersion + ): void { + $driveId = $this->spacesContext->getSpaceIdByName($owner, "Personal"); + $encoded = $this->encodeColonPathSegment($path); + $url = "/graph/$apiVersion/drives/$driveId/root:/$encoded"; + $this->sendGraphRequestAndCaptureResponse($user, "GET", $url); + } } diff --git a/tests/acceptance/features/apiGraph/colonSyntaxPathLookup.feature b/tests/acceptance/features/apiGraph/colonSyntaxPathLookup.feature new file mode 100644 index 0000000000..d8e445f769 --- /dev/null +++ b/tests/acceptance/features/apiGraph/colonSyntaxPathLookup.feature @@ -0,0 +1,114 @@ +Feature: colon-syntax path lookup on the Graph API + As a client + I want to address drive items by path using colon-syntax URLs on the Graph API + So that I do not have to walk the path with successive lookups before issuing a request + + The colon-syntax shapes recognised by the path-lookup middleware are: + + /graph/{version}/drives/{driveID}/root:/[:/][:] + /graph/{version}/drives/{driveID}/items/{itemID}:/[:/][:] + + Both /v1.0 and /v1beta1 are supported. NOT_FOUND and PERMISSION_DENIED + collapse to 404 so the middleware never discloses the existence of + resources the caller is not allowed to see. + + Background: + Given user "Alice" has been created with default attributes + And user "Alice" has created folder "folder1" + And user "Alice" has created folder "folder1/sub" + And user "Alice" has uploaded file with content "hello" to "folder1/file.txt" + And user "Alice" has uploaded file with content "deep" to "folder1/sub/deep.txt" + + + Scenario: get a drive item by root-anchored colon path + When user "Alice" gets the drive item with colon path "folder1/file.txt" of space "Personal" using the Graph API version "v1.0" + Then the HTTP status code should be "200" + And the JSON data of the response should match + """ + { + "type": "object", + "required": ["id", "name", "parentReference"], + "properties": { + "id": { + "type": "string", + "pattern": "^%file_id_pattern%$" + }, + "name": { + "const": "file.txt" + } + } + } + """ + + + Scenario: get a drive item by root-anchored colon path with a deep path + When user "Alice" gets the drive item with colon path "folder1/sub/deep.txt" of space "Personal" using the Graph API version "v1.0" + Then the HTTP status code should be "200" + And the JSON data of the response should match + """ + { + "type": "object", + "required": ["id", "name"], + "properties": { + "name": { + "const": "deep.txt" + } + } + } + """ + + + Scenario: get a drive item by root-anchored colon path with a trailing colon + When user "Alice" gets the drive item with colon path "folder1/file.txt" of space "Personal" with trailing colon using the Graph API version "v1.0" + Then the HTTP status code should be "200" + And the JSON data of the response should match + """ + { + "type": "object", + "required": ["id", "name"], + "properties": { + "name": { + "const": "file.txt" + } + } + } + """ + + + Scenario: get a drive item by item-anchored colon path + When user "Alice" gets the drive item with colon path "file.txt" relative to folder "folder1" of space "Personal" using the Graph API version "v1.0" + Then the HTTP status code should be "200" + And the JSON data of the response should match + """ + { + "type": "object", + "required": ["id", "name"], + "properties": { + "name": { + "const": "file.txt" + } + } + } + """ + + + Scenario: list permissions of a drive item via colon path with a sub-route suffix on v1beta1 + # Exercises the "/:/" rewrite shape and, at the same time, + # the v1beta1 mount of the middleware. The /permissions sub-route is only + # registered at /v1beta1/, and the canonical /v1beta1 GetDriveItem + # handler is share-jail-only, so this is the cleanest way to assert that + # the v1beta1 colon-syntax path actually reaches a working handler for + # regular drive items. + When user "Alice" lists permissions of the drive item with colon path "folder1" of space "Personal" using the Graph API version "v1beta1" + Then the HTTP status code should be "200" + + + Scenario: non-existent colon path returns 404 + When user "Alice" gets the drive item with colon path "folder1/does-not-exist.txt" of space "Personal" using the Graph API version "v1.0" + Then the HTTP status code should be "404" + + + Scenario: another user cannot disclose existence of a resource via colon path + Given user "Brian" has been created with default attributes + When user "Brian" gets the drive item with colon path "folder1/file.txt" of the personal space of "Alice" using the Graph API version "v1.0" + Then the HTTP status code should be "404" From 8f51a4318a2acfaaf53b0e58bfa4196cb107af7d Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Tue, 12 May 2026 09:16:46 +0200 Subject: [PATCH 07/13] test(graph): pin chi URLParam round-trip for IDs with `!` sub-delim Codacy flagged the colon-path middleware comment claiming both `$` and `!` need percent-encoding for chi's tree match, while the implementation only calls r.URL.RawPath = r.URL.EscapedPath() which does not encode either character per the suggestion's reading. In practice EscapedPath does encode `!` as `%21` (only `?` is the hardcoded escape, `!` is escaped because Go's net/url treats it as needing encoding outside specific contexts). It leaves `$` literal, which chi handles fine. chi.URLParam returns the encoded segment verbatim, and downstream OpenCloud handlers (parseIDParam, GetDriveAndItemIDParam) already PathUnescape before parsing IDs, so the round-trip works end-to-end. The acceptance tests on this branch already exercise this with real `$`/`!`-containing IDs. Add a focused unit test that mounts the middleware behind chi, sends a colon URL, and asserts the actual contract: - driveID (only `$`): chi.URLParam returns it literal - itemID (with `!`): PathUnescape(chi.URLParam(...)) == original Update the misleading comment so future readers (and reviewers) see what the encoding actually does and which downstream contract it relies on. --- services/graph/pkg/middleware/path_lookup.go | 24 ++++---- .../graph/pkg/middleware/path_lookup_test.go | 59 +++++++++++++++++++ 2 files changed, 73 insertions(+), 10 deletions(-) diff --git a/services/graph/pkg/middleware/path_lookup.go b/services/graph/pkg/middleware/path_lookup.go index dab46fee9d..514a0f6402 100644 --- a/services/graph/pkg/middleware/path_lookup.go +++ b/services/graph/pkg/middleware/path_lookup.go @@ -121,17 +121,21 @@ func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log. ctx := context.WithValue(r.Context(), OriginalPathContextKey, original) r = r.WithContext(ctx) r.URL.Path = rewritten - // Match the chi-escaping workaround in Graph.ServeHTTP — RawPath - // must be a properly-escaped form of Path so chi's parameter - // binding works for rewritten requests too. Drive/item IDs - // contain `$` and `!`, both of which need percent-encoding for - // chi to round-trip them through its tree match. - // (See services/graph/pkg/service/v0/graph.go ServeHTTP and - // https://github.com/go-chi/chi/issues/641#issuecomment-883156692.) + // Match the chi-escaping workaround in Graph.ServeHTTP: RawPath + // must be a valid encoded form of Path so chi's tree match (which + // reads RawPath when non-empty) routes the rewritten URL the same + // way it routes the original. The previous RawPath (set by + // Graph.ServeHTTP) was for the pre-rewrite Path and no longer + // matches; EscapedPath recomputes a canonical encoding for the + // new Path. // - // EscapedPath() re-encodes Path because the existing RawPath - // (set by Graph.ServeHTTP for the original URL) no longer - // unescapes to our rewritten Path. + // Drive/item IDs in this codebase have the shape + // {storage}${space}!{opaque}. EscapedPath leaves `$` literal (not + // escaped in path segments per RFC 3986) but encodes `!` as `%21`. + // chi.URLParam therefore returns the param with `%21`; downstream + // handlers (e.g. parseIDParam, GetDriveAndItemIDParam) already + // call url.PathUnescape before parsing the ID, so the round-trip + // works. See go-chi/chi#641 for the underlying chi behavior. r.URL.RawPath = r.URL.EscapedPath() next.ServeHTTP(w, r) }) diff --git a/services/graph/pkg/middleware/path_lookup_test.go b/services/graph/pkg/middleware/path_lookup_test.go index 768bbccd52..b4509550de 100644 --- a/services/graph/pkg/middleware/path_lookup_test.go +++ b/services/graph/pkg/middleware/path_lookup_test.go @@ -3,11 +3,13 @@ package middleware_test import ( "net/http" "net/http/httptest" + "net/url" "testing" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" cs3rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" storageprovider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "google.golang.org/grpc" @@ -217,6 +219,63 @@ func TestResolveGraphPath(t *testing.T) { } } +// TestResolveGraphPath_ChiParamRoundTripWithSubDelims pins down what chi +// actually returns from URLParam for rewritten URLs whose IDs contain `$` +// and `!`, and verifies the PathUnescape round-trip downstream handlers +// rely on still recovers the original ID. +// +// Specifically: r.URL.RawPath = r.URL.EscapedPath() leaves `$` literal but +// encodes `!` as `%21` (net/url's encodePath behavior). chi.URLParam returns +// the matched RawPath segment as-is, so the bound param contains `%21`. +// Existing handlers (parseIDParam, GetDriveAndItemIDParam) call +// url.PathUnescape on the param before parsing the ID, which recovers `!`. +// +// This test guards against any future change to the encoding strategy that +// would silently break that round-trip, for example, switching to +// url.PathEscape(itemID) and stuffing the result into r.URL.Path (which +// expects the decoded form) and thereby double-encoding `!` to `%2521`. +func TestResolveGraphPath_ChiParamRoundTripWithSubDelims(t *testing.T) { + gw := &cs3mocks.GatewayAPIClient{} + gw.On("Stat", mock.Anything, mock.Anything). + Return(statResponse(cs3rpc.Code_CODE_OK, true), nil) + + selector := newTestSelector(t, gw) + + var gotDriveID, gotItemID string + r := chi.NewRouter() + r.Use(middleware.ResolveGraphPath(selector, log.NopLogger())) + r.Route("/graph/v1.0/drives/{driveID}", func(r chi.Router) { + r.Get("/items/{itemID}/children", func(w http.ResponseWriter, r *http.Request) { + gotDriveID = chi.URLParam(r, "driveID") + gotItemID = chi.URLParam(r, "itemID") + w.WriteHeader(http.StatusOK) + }) + }) + + req := httptest.NewRequest( + http.MethodGet, + "http://localhost/graph/v1.0/drives/"+testDriveID+"/root:/Documents:/children", + nil, + ) + rr := httptest.NewRecorder() + r.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusOK, rr.Code, "chi must route the rewritten URL to the handler") + + // driveID has only `$` (a sub-delim left literal by EscapedPath), so chi + // returns it without any percent-encoding. + assert.Equal(t, testDriveID, gotDriveID, + "chi.URLParam should return driveID unchanged: only `$` sub-delim, left literal by EscapedPath") + + // itemID has both `$` and `!`. EscapedPath encodes `!` as `%21`, so chi + // returns the param with `%21` literal; PathUnescape recovers the + // original ID. This is the round-trip downstream handlers rely on. + unescaped, err := url.PathUnescape(gotItemID) + assert.NoError(t, err, "URLParam value must be a valid percent-encoded string") + assert.Equal(t, testItemID, unescaped, + "PathUnescape(chi.URLParam(itemID)) must recover the original ID") +} + // TestResolveGraphPath_OriginalPathContext verifies the rewrite preserves the // original URL in request context for downstream tracing/logging. func TestResolveGraphPath_OriginalPathContext(t *testing.T) { From 1f51bad9d1c15305e164ce54e755b70b20d69fe7 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Tue, 30 Jun 2026 19:10:01 +0200 Subject: [PATCH 08/13] refactor(graph): scope colon-path middleware to /drives/{driveID} Addresses review feedback: a sub-router middleware can re-route after all, as long as it rewrites chi.RouteContext().RoutePath instead of r.URL.Path. Once chi has descended into a sub-router its routeHTTP matches against rctx.RoutePath and ignores r.URL.Path, which is why the earlier top-level registration was thought to be required. Move ResolveGraphPath off the top-level mux.Use and attach it to the /drives/{driveID} sub-routers (v1.0 + v1beta1). It now reads driveID from chi.URLParam and matches against RoutePath (the part below the drive), so the regexes drop the version + drive prefix entirely. RoutePath carries the percent-encoded wire form (Graph.ServeHTTP sets RawPath), so the captured driveID/itemID/path are PathUnescape'd exactly once - reproducing the decoded r.URL.Path a normal handler would see, without the previous RawPath/EscapedPath workaround. r.URL.Path is now left untouched; only chi's internal RoutePath is rewritten. Tests are reworked to drive requests through a chi router mirroring the production nesting (including the Graph.ServeHTTP RawPath behavior), so chi's sub-router middleware ordering, RoutePath encoding and param round-trip are all covered indirectly: a chi upgrade that changes any of them fails these tests instead of silently breaking colon-path lookups. Adds explicit coverage for percent-decoding (%20, %252F) and the `$`/`!` sub-delimiter id round-trip. --- services/graph/pkg/middleware/path_lookup.go | 234 +++++++----- .../graph/pkg/middleware/path_lookup_test.go | 351 ++++++++++++------ services/graph/pkg/service/v0/service.go | 18 +- 3 files changed, 391 insertions(+), 212 deletions(-) diff --git a/services/graph/pkg/middleware/path_lookup.go b/services/graph/pkg/middleware/path_lookup.go index 514a0f6402..8fe38bd6c7 100644 --- a/services/graph/pkg/middleware/path_lookup.go +++ b/services/graph/pkg/middleware/path_lookup.go @@ -5,12 +5,14 @@ import ( "errors" "fmt" "net/http" + "net/url" "regexp" "strings" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" cs3rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" storageprovider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/go-chi/chi/v5" "github.com/rs/zerolog" @@ -21,22 +23,26 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/utils" ) -// {HTTP.Root}/{version}/drives/{driveID}/root:/[:/][:] -// 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 -// 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:(/.+?)(?::(/[^:]+))?:?$`) +// The middleware is attached to the /drives/{driveID} sub-routers, so by the +// time it runs chi has already consumed the {version}/drives/{driveID} prefix +// and exposes the remainder via chi.RouteContext().RoutePath. The regexes +// therefore only need to match the part below the drive: +// +// root-anchored: /root:/[:/][:] +// item-anchored: /items/{itemID}:/[:/][:] -// {HTTP.Root}/{version}/drives/{driveID}/items/{itemID}:/[:/][:] -// group 1: prefix up to and including the driveID -// 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/([^/]+):(/.+?)(?::(/[^:]+))?:?$`) +// /root:/[:/][:] +// +// group 1: path (with leading slash) +// group 2: suffix (with leading slash) - empty for direct item lookup +var rootColonRe = regexp.MustCompile(`^/root:(/.+?)(?::(/[^:]+))?:?$`) + +// /items/{itemID}:/[:/][:] +// +// group 1: anchor itemID +// group 2: path (with leading slash) +// group 3: suffix (with leading slash) +var itemColonRe = regexp.MustCompile(`^/items/([^/]+):(/.+?)(?::(/[^:]+))?:?$`) type contextKey string @@ -47,47 +53,59 @@ const OriginalPathContextKey contextKey = "graph.original_path" // Sentinels distinguishing the resolution outcomes that map to specific HTTP // statuses. Anything else surfaces as 500. // -// errPathNotFound — path doesn't exist or the user lacks permission to -// see it. Both collapse to 404 (no existence disclosure). -// errInvalidRequest — client sent a malformed input (unparseable drive/item -// id, drive/item mismatch in item-anchored form). 400. -// errUnauthenticated — the gateway said the caller isn't authenticated for -// the lookup (token expired, cross-storage auth, etc.). 401. +// errPathNotFound - path doesn't exist or the user lacks permission to +// see it. Both collapse to 404 (no existence disclosure). +// errInvalidRequest - client sent a malformed input (unparseable drive/item +// id, drive/item mismatch in item-anchored form). 400. +// errUnauthenticated - the gateway said the caller isn't authenticated for +// the lookup (token expired, cross-storage auth, etc.). 401. var ( errPathNotFound = errors.New("path not found") errInvalidRequest = errors.New("invalid request") errUnauthenticated = errors.New("unauthenticated") ) -// ResolveGraphPath returns middleware that detects colon-syntax path -// lookup URLs and rewrites them to the canonical -// /{version}/drives/{driveID}/items/{resolvedItemID}{suffix} form before -// chi performs route matching. The requested API version is preserved -// (for example, "v1.0" or "v1beta1"). +// ResolveGraphPath returns middleware that detects MS Graph colon-syntax path +// lookup URLs and rewrites chi's internal route path to the canonical +// /items/{resolvedItemID}{suffix} form so the request lands on the existing +// /drives/{driveID}/items/{itemID}... routes. +// +// It must be attached to the /drives/{driveID} sub-routers (it reads driveID +// from chi.URLParam and matches against chi.RouteContext().RoutePath). chi runs +// a sub-router's middleware chain before that sub-router performs its own route +// matching, so rewriting RoutePath here re-routes the request to a different +// leaf. (Rewriting r.URL.Path would NOT work at this level: once chi has +// descended into a sub-router, routeHTTP matches against rctx.RoutePath and +// ignores r.URL.Path.) // // Two URL shapes are recognized: // -// /{version}/drives/{driveID}/root:/[:/][:] -// /{version}/drives/{driveID}/items/{itemID}:/[:/][:] +// /drives/{driveID}/root:/[:/][:] +// /drives/{driveID}/items/{itemID}:/[:/][:] // // 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 with a single substring check. +// Requests whose RoutePath contains no colon fast-path through untouched. 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. - if !strings.Contains(r.URL.Path, ":") { + rctx := chi.RouteContext(r.Context()) + + // Fast-path: skip anything that can't be colon-syntax. RoutePath + // is the part below /drives/{driveID}, e.g. "/items/{id}/children" + // for a normal request - no colon, so this returns immediately. + if rctx == nil || !strings.Contains(rctx.RoutePath, ":") { next.ServeHTTP(w, r) return } + driveID := chi.URLParam(r, "driveID") original := r.URL.Path - rewritten, err := rewriteColonPath(r.Context(), gws, l, original) + rewritten, err := rewriteColonPath(r.Context(), gws, l, driveID, rctx.RoutePath) switch { case errors.Is(err, errPathNotFound): l.Debug().Str("original", original).Msg("colon-path resolution: not found") @@ -108,7 +126,7 @@ func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log. ) return case rewritten == "": - // No colon-syntax match — pass through untouched. + // No colon-syntax match - pass through untouched. next.ServeHTTP(w, r) return } @@ -118,25 +136,11 @@ func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log. Str("rewritten", rewritten). Msg("colon-path resolution: rewrote") - ctx := context.WithValue(r.Context(), OriginalPathContextKey, original) - r = r.WithContext(ctx) - r.URL.Path = rewritten - // Match the chi-escaping workaround in Graph.ServeHTTP: RawPath - // must be a valid encoded form of Path so chi's tree match (which - // reads RawPath when non-empty) routes the rewritten URL the same - // way it routes the original. The previous RawPath (set by - // Graph.ServeHTTP) was for the pre-rewrite Path and no longer - // matches; EscapedPath recomputes a canonical encoding for the - // new Path. - // - // Drive/item IDs in this codebase have the shape - // {storage}${space}!{opaque}. EscapedPath leaves `$` literal (not - // escaped in path segments per RFC 3986) but encodes `!` as `%21`. - // chi.URLParam therefore returns the param with `%21`; downstream - // handlers (e.g. parseIDParam, GetDriveAndItemIDParam) already - // call url.PathUnescape before parsing the ID, so the round-trip - // works. See go-chi/chi#641 for the underlying chi behavior. - r.URL.RawPath = r.URL.EscapedPath() + // Preserve the original (unmodified) request path for downstream + // tracing/logging. r.URL.Path itself stays untouched; only chi's + // internal RoutePath is rewritten. + r = r.WithContext(context.WithValue(r.Context(), OriginalPathContextKey, original)) + rctx.RoutePath = rewritten next.ServeHTTP(w, r) }) } @@ -145,69 +149,97 @@ func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log. // 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. +// +// itemAnchorID, relPath and suffix hold the raw (still percent-encoded) +// captures from RoutePath; rewriteColonPath decodes them as needed. type colonMatch struct { - prefix string // canonical prefix up to and including /drives/{driveID} - driveIDStr string // captured driveID for item-anchored form (empty for root-anchored, where anchorIDStr already IS the 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 + isItemAnchored bool // item-anchored form: anchor is itemAnchorID, validate against driveID + itemAnchorID string // captured itemID for the item-anchored form (empty for root-anchored) + relPath string // relative path with leading slash + suffix string // suffix with leading slash (e.g. "/children"); may be empty } // rewriteColonPath returns: -// - "" + nil — no colon-syntax pattern matched (passthrough) -// - rewritten + nil — matched and resolved to a canonical URL -// - "" + errPathNotFound — path doesn't exist or user lacks permission (404) -// - "" + errInvalidRequest — malformed input (400) -// - "" + errUnauthenticated — gateway said caller isn't authenticated (401) -// - "" + other error — operational / internal failure (5xx) +// - "" + nil - no colon-syntax pattern matched (passthrough) +// - rewritten + nil - matched and resolved to a canonical RoutePath +// - "" + errPathNotFound - path doesn't exist or user lacks permission (404) +// - "" + errInvalidRequest - malformed input (400) +// - "" + errUnauthenticated - gateway said caller isn't authenticated (401) +// - "" + other error - operational / internal failure (5xx) +// +// driveIDParam is the {driveID} route param (raw chi.URLParam value); routePath +// is chi.RouteContext().RoutePath (the part below /drives/{driveID}). func rewriteColonPath( ctx context.Context, gws pool.Selectable[gateway.GatewayAPIClient], logger zerolog.Logger, - urlPath string, + driveIDParam string, + routePath string, ) (string, error) { var match colonMatch switch { - case matchInto(rootColonRe, urlPath, &match, rootMatchExtract): - case matchInto(itemColonRe, urlPath, &match, itemMatchExtract): + case matchInto(rootColonRe, routePath, &match, rootMatchExtract): + case matchInto(itemColonRe, routePath, &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) + // RoutePath follows chi's RawPath, i.e. the percent-encoded wire form + // (e.g. "/Documents/My%20File"). A single PathUnescape reproduces exactly + // what net/http put in r.URL.Path; it is NOT a double-decode (a crafted + // "%252F" decodes once to "%2F", matching the decoded r.URL.Path, not "/"). + driveID, err := url.PathUnescape(driveIDParam) if err != nil { - // Unparseable input is malformed by the client, not "not found". - logger.Debug().Err(err).Str("anchor", match.anchorIDStr).Msg("invalid anchor id in colon path") + logger.Debug().Err(err).Str("driveID", driveIDParam).Msg("undecodable drive id in colon path") return "", errInvalidRequest } - // Item-anchored form ("/drives/{driveID}/items/{itemID}:/...") captures - // driveID separately. Validate the itemID belongs to the given driveID - // (storage + space prefix) — otherwise the request is malformed and we - // short-circuit instead of doing an unnecessary CS3 Stat. - if match.driveIDStr != "" { - drive, err := storagespace.ParseID(match.driveIDStr) + anchorIDStr := driveID + if match.isItemAnchored { + anchorIDStr, err = url.PathUnescape(match.itemAnchorID) if err != nil { - logger.Debug().Err(err).Str("driveID", match.driveIDStr).Msg("invalid drive id in colon path") + logger.Debug().Err(err).Str("itemID", match.itemAnchorID).Msg("undecodable item id in colon path") + return "", errInvalidRequest + } + } + + anchor, err := storagespace.ParseID(anchorIDStr) + if err != nil { + // Unparseable input is malformed by the client, not "not found". + logger.Debug().Err(err).Str("anchor", anchorIDStr).Msg("invalid anchor id in colon path") + return "", errInvalidRequest + } + + // Item-anchored form captures driveID and itemID separately. Validate the + // itemID belongs to the given driveID (storage + space prefix) - otherwise + // the request is malformed and we short-circuit instead of doing an + // unnecessary CS3 Stat. + if match.isItemAnchored { + drive, err := storagespace.ParseID(driveID) + if err != nil { + logger.Debug().Err(err).Str("driveID", driveID).Msg("invalid drive id in colon path") return "", errInvalidRequest } if drive.GetStorageId() != anchor.GetStorageId() || drive.GetSpaceId() != anchor.GetSpaceId() { logger.Debug(). - Str("driveID", match.driveIDStr). - Str("itemID", match.anchorIDStr). + Str("driveID", driveID). + Str("itemID", anchorIDStr). Msg("drive id does not match item id storage/space") return "", errInvalidRequest } } - itemID, err := resolvePath(ctx, gws, logger, &anchor, match.relPath) + relPath, err := url.PathUnescape(match.relPath) + if err != nil { + logger.Debug().Err(err).Str("relPath", match.relPath).Msg("undecodable path in colon path") + return "", errInvalidRequest + } + + itemID, err := resolvePath(ctx, gws, &anchor, relPath) if err != nil { return "", err } - return buildCanonicalPath(match.prefix, itemID, match.suffix), nil + return buildCanonicalRoutePath(itemID, match.suffix), nil } // matchInto runs the regex; on a hit, populates *out via extract and returns true. @@ -222,15 +254,15 @@ func matchInto(re *regexp.Regexp, s string, out *colonMatch, extract func([]stri } func rootMatchExtract(m []string) colonMatch { - // Root-anchored: anchorIDStr IS the driveID, so leave driveIDStr empty - // to skip the drive/item match check (it would compare driveID to itself). - return colonMatch{prefix: m[1], anchorIDStr: m[2], relPath: m[3], suffix: m[4]} + // Root-anchored: the anchor is the {driveID} route param, supplied by the + // caller - nothing item-specific to capture here. + return colonMatch{relPath: m[1], suffix: m[2]} } func itemMatchExtract(m []string) colonMatch { - // Item-anchored: capture driveID (m[2]) so the resolver can validate it - // matches the itemID's storage/space. - return colonMatch{prefix: m[1], driveIDStr: m[2], anchorIDStr: m[3], relPath: m[4], suffix: m[5]} + // Item-anchored: capture itemID so the resolver anchors on it and validates + // it against the {driveID} route param's storage/space. + return colonMatch{isItemAnchored: true, itemAnchorID: m[1], relPath: m[2], suffix: m[3]} } // resolvePath translates a relative filesystem path (anchored at the given @@ -239,21 +271,20 @@ func itemMatchExtract(m []string) colonMatch { func resolvePath( ctx context.Context, gws pool.Selectable[gateway.GatewayAPIClient], - logger zerolog.Logger, anchor *storageprovider.ResourceId, - rawPath string, + relPath string, ) (string, error) { gw, err := gws.Next() if err != nil { 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). + // relPath is already decoded (PathUnescape'd once by the caller), matching + // the form a normal handler would receive from r.URL.Path. statRes, err := gw.Stat(ctx, &storageprovider.StatRequest{ Ref: &storageprovider.Reference{ ResourceId: anchor, - Path: utils.MakeRelativePath(rawPath), + Path: utils.MakeRelativePath(relPath), }, }) if err != nil { @@ -282,10 +313,13 @@ func resolvePath( return storagespace.FormatResourceID(id), nil } -func buildCanonicalPath(prefix, itemID, suffix string) string { - // r.URL.Path is the decoded form per Go's net/http convention; chi tree - // matching reads it directly. Insert the raw itemID without escaping so - // chi's {itemID} param captures the same string the handler will see - // after parseIDParam unescapes (no-op for already-decoded chars). - return prefix + "/items/" + itemID + suffix +// buildCanonicalRoutePath produces the RoutePath chi should match against after +// the rewrite, relative to /drives/{driveID}: /items/{itemID}{suffix}. +// +// itemID is inserted literally (the FormatResourceID output, e.g. with `$` and +// `!` sub-delims). chi binds it verbatim into the {itemID} param; downstream +// handlers (parseIDParam, GetDriveAndItemIDParam) call url.PathUnescape on the +// param, which is a no-op for these literal sub-delims, so the id round-trips. +func buildCanonicalRoutePath(itemID, suffix string) string { + return "/items/" + itemID + suffix } diff --git a/services/graph/pkg/middleware/path_lookup_test.go b/services/graph/pkg/middleware/path_lookup_test.go index b4509550de..531de52f8c 100644 --- a/services/graph/pkg/middleware/path_lookup_test.go +++ b/services/graph/pkg/middleware/path_lookup_test.go @@ -57,55 +57,174 @@ func statResponse(code cs3rpc.Code, withInfo bool) *storageprovider.StatResponse return res } -// recordingHandler captures the request URL path it receives so tests can assert -// the middleware rewrote (or passed through) correctly. -func recordingHandler() (http.Handler, *string) { - var seen string - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - seen = r.URL.Path - w.WriteHeader(http.StatusOK) - }), &seen +// leafCapture records what the matched leaf handler saw, so tests can assert +// the middleware rewrote chi's route path correctly and the request reached the +// intended /items/{itemID}... handler with the resolved id bound as a param. +type leafCapture struct { + hit string // which leaf was reached ("" = none) + urlPath string // r.URL.Path as seen by the handler (must stay the original) + driveID string // chi.URLParam(driveID) + itemID string // resolved item id, decoded via PathUnescape + original any // OriginalPathContextKey value +} + +// newGraphTestRouter wires ResolveGraphPath into a chi router that mirrors the +// production /drives/{driveID} nesting for both API versions, including the +// RawPath workaround Graph.ServeHTTP applies. Driving requests through real chi +// routing is deliberate: it exercises chi's actual behavior (sub-router +// middleware ordering, RoutePath encoding, param round-trip) indirectly, so a +// chi upgrade that changes any of it fails these tests instead of silently +// breaking colon-path lookups in production. +func newGraphTestRouter(t *testing.T, gw *cs3mocks.GatewayAPIClient) (http.Handler, *leafCapture) { + t.Helper() + cap := &leafCapture{} + selector := newTestSelector(t, gw) + + leaf := func(name string) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + cap.hit = name + cap.urlPath = r.URL.Path + cap.driveID = chi.URLParam(r, "driveID") + // v1.0 binds the item param as {driveItemID}, v1beta1 as {itemID}. + raw := chi.URLParam(r, "itemID") + if raw == "" { + raw = chi.URLParam(r, "driveItemID") + } + // Downstream handlers PathUnescape the param before parsing the id; + // mirror that here so we assert on the recovered id. + cap.itemID, _ = url.PathUnescape(raw) + cap.original = r.Context().Value(middleware.OriginalPathContextKey) + w.WriteHeader(http.StatusOK) + } + } + + m := chi.NewMux() + // Mirror Graph.ServeHTTP: RawPath drives chi's tree walk, which is what + // makes RoutePath carry the percent-encoded wire form. + m.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r.URL.RawPath = r.URL.EscapedPath() + next.ServeHTTP(w, r) + }) + }) + // Leaf routes mirror production exactly (service.go), per version, so the + // tests assert reachability that production actually reproduces: a rewrite + // to a suffix that doesn't exist for that version must 404 here too. + m.Route("/graph", func(r chi.Router) { + r.Route("/v1beta1/drives/{driveID}", func(r chi.Router) { + r.Use(middleware.ResolveGraphPath(selector, log.NopLogger())) + r.Route("/items/{itemID}", func(r chi.Router) { + r.Get("/", leaf("item")) + r.Post("/createLink", leaf("createLink")) + r.Route("/permissions", func(r chi.Router) { + r.Get("/", leaf("permissions")) + r.Post("/{permissionID}/setPassword", leaf("setPassword")) + }) + }) + }) + r.Route("/v1.0/drives/{driveID}", func(r chi.Router) { + r.Use(middleware.ResolveGraphPath(selector, log.NopLogger())) + r.Route("/items/{driveItemID}", func(r chi.Router) { + r.Get("/", leaf("item")) + r.Get("/children", leaf("children")) + }) + }) + }) + return m, cap } func TestResolveGraphPath(t *testing.T) { tests := []struct { name string + method string // defaults to GET when empty urlPath string statCode cs3rpc.Code - statErr error expectStatCalled bool expectStatus int - expectURLPath string // empty = handler not invoked + expectHit string // leaf the request must land on ("" = none reached) + expectItemID string // resolved item id the leaf must see (after PathUnescape) }{ { - name: "non-colon URL passes through unchanged", - urlPath: "/graph/v1.0/me/drives", + name: "non-colon URL routes normally without a Stat", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/items/" + testItemID + "/children", expectStatCalled: false, expectStatus: http.StatusOK, - expectURLPath: "/graph/v1.0/me/drives", + expectHit: "children", + expectItemID: testItemID, }, { - name: "URL with colon but not matching pattern passes through", - urlPath: "/graph/v1.0/some/other/route:/foo", + name: "colon URL not matching the lookup pattern passes through (404, no Stat)", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/items/" + testItemID + "/foo:bar", expectStatCalled: false, - expectStatus: http.StatusOK, - expectURLPath: "/graph/v1.0/some/other/route:/foo", + expectStatus: http.StatusNotFound, + expectHit: "", }, { - name: "v1.0 root-anchored with /children rewrites", + // Anchoring: the colon-syntax shape appearing under a junk prefix + // (i.e. NOT at the configured HTTP root) must not trigger a rewrite + // or a CS3 Stat. Because the middleware is mounted under the + // /graph root, chi never routes such a path into it (404 first) — + // no /foo/.../v1.0/drives/...:/... can over-match. + name: "colon-syntax under a junk prefix does not match (anchored on HTTP root)", + urlPath: "/foo/graph/v1.0/drives/" + testDriveID + "/root:/Documents:/children", + expectStatCalled: false, + expectStatus: http.StatusNotFound, + expectHit: "", + }, + { + name: "v1.0 root-anchored with /children rewrites and routes", urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Documents:/children", statCode: cs3rpc.Code_CODE_OK, expectStatCalled: true, expectStatus: http.StatusOK, - expectURLPath: "/graph/v1.0/drives/" + testDriveID + "/items/storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d/children", + expectHit: "children", + expectItemID: testItemID, }, { - name: "v1beta1 root-anchored with /createLink rewrites", - urlPath: "/graph/v1beta1/drives/" + testDriveID + "/root:/Documents:/createLink", + // Mirrors acceptance scenario 5: v1beta1 root-anchored with a + // /permissions suffix (a real v1beta1-only route, GET). + name: "v1beta1 root-anchored with /permissions rewrites and routes", + urlPath: "/graph/v1beta1/drives/" + testDriveID + "/root:/folder1:/permissions", statCode: cs3rpc.Code_CODE_OK, expectStatCalled: true, expectStatus: http.StatusOK, - expectURLPath: "/graph/v1beta1/drives/" + testDriveID + "/items/storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d/createLink", + expectHit: "permissions", + expectItemID: testItemID, + }, + { + // Non-GET colon request: createLink is POST on v1beta1. + name: "v1beta1 root-anchored with /createLink (POST) rewrites and routes", + method: http.MethodPost, + urlPath: "/graph/v1beta1/drives/" + testDriveID + "/root:/folder1:/createLink", + statCode: cs3rpc.Code_CODE_OK, + expectStatCalled: true, + expectStatus: http.StatusOK, + expectHit: "createLink", + expectItemID: testItemID, + }, + { + // Multi-segment suffix: /permissions/{permissionID}/setPassword on + // v1beta1 (POST). Pins that the suffix regex carries embedded slashes + // through the rewrite into a real nested route. + name: "v1beta1 root-anchored with multi-segment suffix rewrites and routes", + method: http.MethodPost, + urlPath: "/graph/v1beta1/drives/" + testDriveID + "/root:/folder1:/permissions/perm-1/setPassword", + statCode: cs3rpc.Code_CODE_OK, + expectStatCalled: true, + expectStatus: http.StatusOK, + expectHit: "setPassword", + expectItemID: testItemID, + }, + { + // Item-anchored colon syntax on v1beta1 (POST createLink). + name: "v1beta1 item-anchored with /createLink (POST) rewrites and routes", + method: http.MethodPost, + urlPath: "/graph/v1beta1/drives/" + testDriveID + "/items/" + testItemID + ":/notes.txt:/createLink", + statCode: cs3rpc.Code_CODE_OK, + expectStatCalled: true, + expectStatus: http.StatusOK, + expectHit: "createLink", + expectItemID: testItemID, }, { name: "trailing colon (no suffix) rewrites to bare item URL", @@ -113,7 +232,8 @@ func TestResolveGraphPath(t *testing.T) { statCode: cs3rpc.Code_CODE_OK, expectStatCalled: true, expectStatus: http.StatusOK, - expectURLPath: "/graph/v1.0/drives/" + testDriveID + "/items/storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d", + expectHit: "item", + expectItemID: testItemID, }, { name: "no trailing colon, no suffix rewrites to bare item URL", @@ -121,7 +241,8 @@ func TestResolveGraphPath(t *testing.T) { statCode: cs3rpc.Code_CODE_OK, expectStatCalled: true, expectStatus: http.StatusOK, - expectURLPath: "/graph/v1.0/drives/" + testDriveID + "/items/storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d", + expectHit: "item", + expectItemID: testItemID, }, { name: "deep path rewrites correctly", @@ -129,7 +250,8 @@ func TestResolveGraphPath(t *testing.T) { statCode: cs3rpc.Code_CODE_OK, expectStatCalled: true, expectStatus: http.StatusOK, - expectURLPath: "/graph/v1.0/drives/" + testDriveID + "/items/storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d/children", + expectHit: "children", + expectItemID: testItemID, }, { name: "item-anchored colon syntax rewrites", @@ -137,56 +259,57 @@ func TestResolveGraphPath(t *testing.T) { statCode: cs3rpc.Code_CODE_OK, expectStatCalled: true, expectStatus: http.StatusOK, - expectURLPath: "/graph/v1.0/drives/" + testDriveID + "/items/storage-users-1$f503f6fe-2656-4b0f-8289-fb3184962dfd!f0e20017-9cba-498a-87e5-3467b976604d/children", + expectHit: "children", + expectItemID: testItemID, }, { - name: "Stat NOT_FOUND returns 404 without invoking handler", + name: "Stat NOT_FOUND returns 404 without reaching a leaf", urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Missing:", statCode: cs3rpc.Code_CODE_NOT_FOUND, expectStatCalled: true, expectStatus: http.StatusNotFound, - expectURLPath: "", // handler must NOT be called + expectHit: "", }, { // CRITICAL security test: PERMISSION_DENIED must not leak existence. // We collapse it to 404, identical to NOT_FOUND, so an unauthorized - // caller can't distinguish "doesn't exist" from "exists but you can't see it". - name: "Stat PERMISSION_DENIED returns 404 (not 403) — don't disclose existence", + // caller can't distinguish "doesn't exist" from "exists but hidden". + name: "Stat PERMISSION_DENIED returns 404 (not 403) - don't disclose existence", urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Restricted:", statCode: cs3rpc.Code_CODE_PERMISSION_DENIED, expectStatCalled: true, expectStatus: http.StatusNotFound, - expectURLPath: "", + expectHit: "", }, { - // 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)", + // Operational/unexpected CS3 statuses must NOT collapse to 404 - + // that would mask outages. Surface as 500 like other graph handlers. + 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.StatusInternalServerError, - expectURLPath: "", + expectHit: "", }, { - // UNAUTHENTICATED is its own distinct outcome — must surface as 401, + // UNAUTHENTICATED is its own distinct outcome - must surface as 401, // not 500, so clients can detect "your token is bad" vs "server error". name: "Stat UNAUTHENTICATED returns 401", urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Documents:", statCode: cs3rpc.Code_CODE_UNAUTHENTICATED, expectStatCalled: true, expectStatus: http.StatusUnauthorized, - expectURLPath: "", + expectHit: "", }, { // Item-anchored form with a driveID that doesn't match the itemID's - // storage/space — the request is malformed; short-circuit to 400 + // storage/space - the request is malformed; short-circuit to 400 // instead of doing a Stat that would only fail downstream. name: "drive id and item id storage/space mismatch returns 400", urlPath: "/graph/v1.0/drives/storage-users-2$other-space-id/items/" + testItemID + ":/notes.txt:/children", expectStatCalled: false, expectStatus: http.StatusBadRequest, - expectURLPath: "", + expectHit: "", }, } @@ -195,20 +318,27 @@ func TestResolveGraphPath(t *testing.T) { gw := &cs3mocks.GatewayAPIClient{} if tt.expectStatCalled { gw.On("Stat", mock.Anything, mock.Anything). - Return(statResponse(tt.statCode, tt.statCode == cs3rpc.Code_CODE_OK), tt.statErr) + Return(statResponse(tt.statCode, tt.statCode == cs3rpc.Code_CODE_OK), nil) } - selector := newTestSelector(t, gw) - handler, seen := recordingHandler() - mw := middleware.ResolveGraphPath(selector, log.NopLogger()) - - req := httptest.NewRequest(http.MethodGet, "http://localhost"+tt.urlPath, nil) + method := tt.method + if method == "" { + method = http.MethodGet + } + router, cap := newGraphTestRouter(t, gw) + req := httptest.NewRequest(method, "http://localhost"+tt.urlPath, nil) rr := httptest.NewRecorder() - - mw(handler).ServeHTTP(rr, req) + router.ServeHTTP(rr, req) assert.Equal(t, tt.expectStatus, rr.Code, "status code") - assert.Equal(t, tt.expectURLPath, *seen, "URL seen by next handler") + assert.Equal(t, tt.expectHit, cap.hit, "leaf handler reached") + + if tt.expectHit != "" { + assert.Equal(t, testDriveID, cap.driveID, "driveID param") + assert.Equal(t, tt.expectItemID, cap.itemID, "resolved item id seen by leaf") + // r.URL.Path must stay the original; only chi's RoutePath is rewritten. + assert.Equal(t, tt.urlPath, cap.urlPath, "r.URL.Path must remain the original request path") + } if tt.expectStatCalled { gw.AssertCalled(t, "Stat", mock.Anything, mock.Anything) @@ -219,85 +349,94 @@ func TestResolveGraphPath(t *testing.T) { } } -// TestResolveGraphPath_ChiParamRoundTripWithSubDelims pins down what chi -// actually returns from URLParam for rewritten URLs whose IDs contain `$` -// and `!`, and verifies the PathUnescape round-trip downstream handlers -// rely on still recovers the original ID. -// -// Specifically: r.URL.RawPath = r.URL.EscapedPath() leaves `$` literal but -// encodes `!` as `%21` (net/url's encodePath behavior). chi.URLParam returns -// the matched RawPath segment as-is, so the bound param contains `%21`. -// Existing handlers (parseIDParam, GetDriveAndItemIDParam) call -// url.PathUnescape on the param before parsing the ID, which recovers `!`. -// -// This test guards against any future change to the encoding strategy that -// would silently break that round-trip, for example, switching to -// url.PathEscape(itemID) and stuffing the result into r.URL.Path (which -// expects the decoded form) and thereby double-encoding `!` to `%2521`. -func TestResolveGraphPath_ChiParamRoundTripWithSubDelims(t *testing.T) { +// TestResolveGraphPath_DecodesEncodedPath pins the decoding contract: chi's +// RoutePath carries the percent-encoded wire form (because Graph.ServeHTTP sets +// RawPath), and the middleware must hand CS3 Stat the decoded path - exactly +// what a normal handler reading r.URL.Path would get. Driving this through real +// chi routing means a chi change to RoutePath encoding would break this test. +func TestResolveGraphPath_DecodesEncodedPath(t *testing.T) { + tests := []struct { + name string + urlPath string + expectedStat string + }{ + { + name: "space encoded as %20 is decoded for Stat", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Documents/My%20File:/children", + expectedStat: "./Documents/My File", + }, + { + // A crafted double-encoding must be decoded exactly once (to the + // literal "%2F"), never twice into a path separator. + name: "double-encoded %252F decodes once, not twice", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/a%252Fb:/children", + expectedStat: "./a%2Fb", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var statPath string + gw := &cs3mocks.GatewayAPIClient{} + gw.On("Stat", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + req := args.Get(1).(*storageprovider.StatRequest) + statPath = req.GetRef().GetPath() + }). + Return(statResponse(cs3rpc.Code_CODE_OK, true), nil) + + router, cap := newGraphTestRouter(t, gw) + req := httptest.NewRequest(http.MethodGet, "http://localhost"+tt.urlPath, nil) + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusOK, rr.Code) + assert.Equal(t, "children", cap.hit) + assert.Equal(t, tt.expectedStat, statPath, "path passed to CS3 Stat must be decoded exactly once") + }) + } +} + +// TestResolveGraphPath_ItemIDRoundTrip guards the sub-delimiter round-trip: the +// resolved id contains `$` and `!`, and after the RoutePath rewrite chi must +// bind it to the {itemID} param such that the downstream PathUnescape recovers +// the original id. This exercises chi's param binding indirectly; a regression +// in how chi stores matched segments would surface here. +func TestResolveGraphPath_ItemIDRoundTrip(t *testing.T) { gw := &cs3mocks.GatewayAPIClient{} gw.On("Stat", mock.Anything, mock.Anything). Return(statResponse(cs3rpc.Code_CODE_OK, true), nil) - selector := newTestSelector(t, gw) - - var gotDriveID, gotItemID string - r := chi.NewRouter() - r.Use(middleware.ResolveGraphPath(selector, log.NopLogger())) - r.Route("/graph/v1.0/drives/{driveID}", func(r chi.Router) { - r.Get("/items/{itemID}/children", func(w http.ResponseWriter, r *http.Request) { - gotDriveID = chi.URLParam(r, "driveID") - gotItemID = chi.URLParam(r, "itemID") - w.WriteHeader(http.StatusOK) - }) - }) - + router, cap := newGraphTestRouter(t, gw) req := httptest.NewRequest( http.MethodGet, "http://localhost/graph/v1.0/drives/"+testDriveID+"/root:/Documents:/children", nil, ) rr := httptest.NewRecorder() - r.ServeHTTP(rr, req) + router.ServeHTTP(rr, req) - assert.Equal(t, http.StatusOK, rr.Code, "chi must route the rewritten URL to the handler") - - // driveID has only `$` (a sub-delim left literal by EscapedPath), so chi - // returns it without any percent-encoding. - assert.Equal(t, testDriveID, gotDriveID, - "chi.URLParam should return driveID unchanged: only `$` sub-delim, left literal by EscapedPath") - - // itemID has both `$` and `!`. EscapedPath encodes `!` as `%21`, so chi - // returns the param with `%21` literal; PathUnescape recovers the - // original ID. This is the round-trip downstream handlers rely on. - unescaped, err := url.PathUnescape(gotItemID) - assert.NoError(t, err, "URLParam value must be a valid percent-encoded string") - assert.Equal(t, testItemID, unescaped, - "PathUnescape(chi.URLParam(itemID)) must recover the original ID") + assert.Equal(t, http.StatusOK, rr.Code, "chi must route the rewritten path to the leaf") + assert.Equal(t, "children", cap.hit) + assert.Equal(t, testItemID, cap.itemID, + "PathUnescape(chi.URLParam(itemID)) must recover the original id (with `$` and `!`)") } -// TestResolveGraphPath_OriginalPathContext verifies the rewrite preserves the -// original URL in request context for downstream tracing/logging. +// TestResolveGraphPath_OriginalPathContext verifies the original URL is +// preserved in request context for downstream tracing/logging, and that +// r.URL.Path itself is left untouched by the rewrite. func TestResolveGraphPath_OriginalPathContext(t *testing.T) { gw := &cs3mocks.GatewayAPIClient{} gw.On("Stat", mock.Anything, mock.Anything). Return(statResponse(cs3rpc.Code_CODE_OK, true), nil) - selector := newTestSelector(t, gw) original := "/graph/v1.0/drives/" + testDriveID + "/root:/Documents:/children" - - var capturedOriginal interface{} - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - capturedOriginal = r.Context().Value(middleware.OriginalPathContextKey) - w.WriteHeader(http.StatusOK) - }) - - mw := middleware.ResolveGraphPath(selector, log.NopLogger()) + router, cap := newGraphTestRouter(t, gw) req := httptest.NewRequest(http.MethodGet, "http://localhost"+original, nil) rr := httptest.NewRecorder() - - mw(handler).ServeHTTP(rr, req) + router.ServeHTTP(rr, req) assert.Equal(t, http.StatusOK, rr.Code) - assert.Equal(t, original, capturedOriginal, "original URL must be available via OriginalPathContextKey") + assert.Equal(t, original, cap.original, "original URL must be available via OriginalPathContextKey") + assert.Equal(t, original, cap.urlPath, "r.URL.Path must remain the original request path") } diff --git a/services/graph/pkg/service/v0/service.go b/services/graph/pkg/service/v0/service.go index 16c811c97a..fb5e490851 100644 --- a/services/graph/pkg/service/v0/service.go +++ b/services/graph/pkg/service/v0/service.go @@ -132,12 +132,6 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx m := chi.NewMux() m.Use(options.Middleware...) - // Must be top-level mux.Use, not r.Use inside a Route block: chi - // sub-router middleware runs after the prefix has already been matched, - // so URL rewriting at that point can't redirect to a different leaf - // route. Top-level middleware runs before any matching, so the rewritten - // URL is what chi walks the radix tree against. - m.Use(graphm.ResolveGraphPath(options.GatewaySelector, options.Logger)) m.Use( otelchi.Middleware( "graph", @@ -271,6 +265,12 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx r.Route("/drives", func(r chi.Router) { r.Get("/", svc.GetAllDrives(APIVersion_1_Beta_1)) r.Route("/{driveID}", func(r chi.Router) { + // Rewrites MS Graph colon-syntax lookups (root:/path, + // items/{id}:/path) to /items/{resolvedID}... before chi + // matches the leaf route. Must sit here, on the + // /drives/{driveID} sub-router, so it can rewrite the + // remaining RoutePath. See graphm.ResolveGraphPath. + r.Use(graphm.ResolveGraphPath(options.GatewaySelector, options.Logger)) r.Route("/root", func(r chi.Router) { r.Post("/children", drivesDriveItemApi.CreateDriveItem) r.Post("/invite", driveItemPermissionsApi.SpaceRootInvite) @@ -373,6 +373,12 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx r.Get("/", svc.GetAllDrives(APIVersion_1)) r.Post("/", svc.CreateDrive) r.Route("/{driveID}", func(r chi.Router) { + // Rewrites MS Graph colon-syntax lookups (root:/path, + // items/{id}:/path) to /items/{resolvedID}... before chi + // matches the leaf route. Must sit here, on the + // /drives/{driveID} sub-router, so it can rewrite the + // remaining RoutePath. See graphm.ResolveGraphPath. + r.Use(graphm.ResolveGraphPath(options.GatewaySelector, options.Logger)) r.Patch("/", svc.UpdateDrive) r.Get("/", svc.GetSingleDrive) r.Delete("/", svc.DeleteDrive) From 4b9802268889151782f7f5564b574ae9a4f8988b Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 1 Jul 2026 11:42:03 +0200 Subject: [PATCH 09/13] refactor(graph): parse colon paths with string ops instead of regexes Review feedback: the two colon-syntax regexes were hard to read. Since the middleware is scoped to /drives/{driveID}, the RoutePath it inspects is just the sub-path (/root:/... or /items/{id}:/...), so a regex buys nothing. Replace rootColonRe/itemColonRe (and the matchInto/extract helpers) with a single parseColonPath that uses plain string operations (HasPrefix/TrimPrefix/TrimSuffix/Cut), one commented step at a time. Behavior is unchanged; the existing table tests (trailing colon, no suffix, deep paths, multi-segment suffixes, item-anchored, encoded paths) still pass and pin it. --- services/graph/pkg/middleware/path_lookup.go | 111 ++++++++++++------- 1 file changed, 68 insertions(+), 43 deletions(-) diff --git a/services/graph/pkg/middleware/path_lookup.go b/services/graph/pkg/middleware/path_lookup.go index 8fe38bd6c7..796d19ec8e 100644 --- a/services/graph/pkg/middleware/path_lookup.go +++ b/services/graph/pkg/middleware/path_lookup.go @@ -6,7 +6,6 @@ import ( "fmt" "net/http" "net/url" - "regexp" "strings" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" @@ -25,25 +24,12 @@ import ( // The middleware is attached to the /drives/{driveID} sub-routers, so by the // time it runs chi has already consumed the {version}/drives/{driveID} prefix -// and exposes the remainder via chi.RouteContext().RoutePath. The regexes -// therefore only need to match the part below the drive: +// and exposes the remainder via chi.RouteContext().RoutePath. parseColonPath +// therefore only needs to handle the part below the drive: // // root-anchored: /root:/[:/][:] // item-anchored: /items/{itemID}:/[:/][:] -// /root:/[:/][:] -// -// group 1: path (with leading slash) -// group 2: suffix (with leading slash) - empty for direct item lookup -var rootColonRe = regexp.MustCompile(`^/root:(/.+?)(?::(/[^:]+))?:?$`) - -// /items/{itemID}:/[:/][:] -// -// group 1: anchor itemID -// group 2: path (with leading slash) -// group 3: suffix (with leading slash) -var itemColonRe = regexp.MustCompile(`^/items/([^/]+):(/.+?)(?::(/[^:]+))?:?$`) - type contextKey string // OriginalPathContextKey holds the pre-rewrite request path for downstream @@ -146,12 +132,12 @@ func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log. } } -// 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. +// colonMatch is the normalized result of parseColonPath for either the root- +// or item-anchored colon-syntax shape. The two shapes carry different parts; +// this struct erases that difference for downstream resolution. // // itemAnchorID, relPath and suffix hold the raw (still percent-encoded) -// captures from RoutePath; rewriteColonPath decodes them as needed. +// substrings from RoutePath; rewriteColonPath decodes them as needed. type colonMatch struct { isItemAnchored bool // item-anchored form: anchor is itemAnchorID, validate against driveID itemAnchorID string // captured itemID for the item-anchored form (empty for root-anchored) @@ -176,11 +162,8 @@ func rewriteColonPath( driveIDParam string, routePath string, ) (string, error) { - var match colonMatch - switch { - case matchInto(rootColonRe, routePath, &match, rootMatchExtract): - case matchInto(itemColonRe, routePath, &match, itemMatchExtract): - default: + match, ok := parseColonPath(routePath) + if !ok { return "", nil } @@ -242,27 +225,69 @@ func rewriteColonPath( return buildCanonicalRoutePath(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 +// parseColonPath splits a colon-syntax RoutePath (the part below +// /drives/{driveID}) into its parts, or reports ok=false when the path is not +// colon-syntax and should pass through untouched. +// +// Below the drive the grammar is one of: +// +// /root: +// /items/: +// +// where is "/", optionally followed by ":/", and an +// optional trailing ":". Colons are structural delimiters, so neither +// nor contains one. For example: +// +// /root:/Documents -> path "/Documents" +// /root:/Documents: -> path "/Documents" +// /root:/Documents:/children -> path "/Documents", suffix "/children" +// /items/{id}:/notes.txt:/children -> itemID "{id}", path "/notes.txt", suffix "/children" +// +// The returned fields are the raw (still percent-encoded) substrings; the +// caller decodes them. +func parseColonPath(routePath string) (colonMatch, bool) { + var m colonMatch + + // rest is everything after the anchor and its delimiting colon, i.e. + // "/[:/][:]". + var rest string + switch { + case strings.HasPrefix(routePath, "/root:"): + // Root-anchored: the anchor is the {driveID} route param supplied by + // the caller, so there's nothing item-specific to capture here. + rest = strings.TrimPrefix(routePath, "/root:") + case strings.HasPrefix(routePath, "/items/"): + // Item-anchored: the itemID is a single segment terminated by ':'. If a + // '/' appears before any ':', this is an ordinary /items/{id}/... + // request, not colon syntax. + after := strings.TrimPrefix(routePath, "/items/") + itemID, tail, found := strings.Cut(after, ":") + if !found || strings.Contains(itemID, "/") { + return m, false + } + m.isItemAnchored = true + m.itemAnchorID = itemID + rest = tail + default: + return m, false } - *out = extract(m) - return true -} -func rootMatchExtract(m []string) colonMatch { - // Root-anchored: the anchor is the {driveID} route param, supplied by the - // caller - nothing item-specific to capture here. - return colonMatch{relPath: m[1], suffix: m[2]} -} + // Drop a single optional trailing ':' (the "...:" no-suffix shape), then + // split path from suffix on the one remaining delimiter colon, if any. + // strings.Cut leaves suffix empty when there's no delimiter. + rest = strings.TrimSuffix(rest, ":") + m.relPath, m.suffix, _ = strings.Cut(rest, ":") -func itemMatchExtract(m []string) colonMatch { - // Item-anchored: capture itemID so the resolver anchors on it and validates - // it against the {driveID} route param's storage/space. - return colonMatch{isItemAnchored: true, itemAnchorID: m[1], relPath: m[2], suffix: m[3]} + // Shape requirements (matching the previous regex): the path must be + // absolute and non-empty ("/x"), and a present suffix must be absolute and + // colon-free. + if len(m.relPath) < 2 || m.relPath[0] != '/' { + return m, false + } + if m.suffix != "" && (m.suffix[0] != '/' || strings.IndexByte(m.suffix, ':') >= 0) { + return m, false + } + return m, true } // resolvePath translates a relative filesystem path (anchored at the given From 407bb2bc70b6cde1b5452733d2e01370267f7966 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 1 Jul 2026 11:54:03 +0200 Subject: [PATCH 10/13] refactor(graph): clearer colon-path anchor parsing, pin second-colon rule parseColonPath split the anchor asymmetrically (root trimmed the delimiter colon as part of a literal "/root:" prefix, item cut on it), and the root branch's comment talked about the driveID which isn't this function's concern. Split once at the first colon into anchor + rest, then classify the anchor (exactly "/root", or "/items/{id}" with a single-segment id). Same behavior, easier to follow. Also add explicit coverage for the rule that a suffix requires a second colon: "/root:/Documents/children" (no second colon) is the path "/Documents/children", not the path "/Documents" with a "/children" suffix. Two tests pin it end-to-end - it must route to the bare item (not /items/{id}/children) and Stat must receive the full path. --- services/graph/pkg/middleware/path_lookup.go | 42 ++++++++++--------- .../graph/pkg/middleware/path_lookup_test.go | 25 ++++++++++- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/services/graph/pkg/middleware/path_lookup.go b/services/graph/pkg/middleware/path_lookup.go index 796d19ec8e..43bb25f174 100644 --- a/services/graph/pkg/middleware/path_lookup.go +++ b/services/graph/pkg/middleware/path_lookup.go @@ -248,33 +248,37 @@ func rewriteColonPath( func parseColonPath(routePath string) (colonMatch, bool) { var m colonMatch - // rest is everything after the anchor and its delimiting colon, i.e. - // "/[:/][:]". - var rest string + // The anchor is separated from the rest by the first colon; no colon at all + // means this isn't colon syntax and should pass through. + anchor, rest, found := strings.Cut(routePath, ":") + if !found { + return m, false + } + switch { - case strings.HasPrefix(routePath, "/root:"): - // Root-anchored: the anchor is the {driveID} route param supplied by - // the caller, so there's nothing item-specific to capture here. - rest = strings.TrimPrefix(routePath, "/root:") - case strings.HasPrefix(routePath, "/items/"): - // Item-anchored: the itemID is a single segment terminated by ':'. If a - // '/' appears before any ':', this is an ordinary /items/{id}/... - // request, not colon syntax. - after := strings.TrimPrefix(routePath, "/items/") - itemID, tail, found := strings.Cut(after, ":") - if !found || strings.Contains(itemID, "/") { + case anchor == "/root": + // Root-anchored: path resolution later anchors on the {driveID} route + // param, so there's no item id to capture from the URL. + case strings.HasPrefix(anchor, "/items/"): + // Item-anchored: the anchor is /items/{itemID} with a single-segment + // id. A '/' inside the id means this is an ordinary /items/{id}/... + // request that just happens to contain a colon further along. + itemID := strings.TrimPrefix(anchor, "/items/") + if itemID == "" || strings.Contains(itemID, "/") { return m, false } m.isItemAnchored = true m.itemAnchorID = itemID - rest = tail default: return m, false } - // Drop a single optional trailing ':' (the "...:" no-suffix shape), then - // split path from suffix on the one remaining delimiter colon, if any. - // strings.Cut leaves suffix empty when there's no delimiter. + // rest is "/[:/][:]". Drop a single optional trailing ':' + // (the "...:" no-suffix shape), then split path from an optional suffix on + // the one remaining delimiter colon. strings.Cut leaves the suffix empty + // when there's no delimiter: a suffix requires an explicit second colon, + // so e.g. "/root:/foo/children" is the path "/foo/children", not the path + // "/foo" with suffix "/children". rest = strings.TrimSuffix(rest, ":") m.relPath, m.suffix, _ = strings.Cut(rest, ":") @@ -284,7 +288,7 @@ func parseColonPath(routePath string) (colonMatch, bool) { if len(m.relPath) < 2 || m.relPath[0] != '/' { return m, false } - if m.suffix != "" && (m.suffix[0] != '/' || strings.IndexByte(m.suffix, ':') >= 0) { + if m.suffix != "" && (m.suffix[0] != '/' || strings.Contains(m.suffix, ":")) { return m, false } return m, true diff --git a/services/graph/pkg/middleware/path_lookup_test.go b/services/graph/pkg/middleware/path_lookup_test.go index 531de52f8c..b933c81a9d 100644 --- a/services/graph/pkg/middleware/path_lookup_test.go +++ b/services/graph/pkg/middleware/path_lookup_test.go @@ -253,6 +253,20 @@ func TestResolveGraphPath(t *testing.T) { expectHit: "children", expectItemID: testItemID, }, + { + // A trailing segment that looks like a suffix ("children") is only a + // suffix when a SECOND colon introduces it. Without it, the whole + // thing is the path, so this must resolve to the bare item (GET "/"), + // NOT to /items/{id}/children. If the parser wrongly split it, the + // request would land on the "children" leaf and this fails. + name: "trailing suffix-looking segment without a second colon stays in the path", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Documents/children", + statCode: cs3rpc.Code_CODE_OK, + expectStatCalled: true, + expectStatus: http.StatusOK, + expectHit: "item", + expectItemID: testItemID, + }, { name: "item-anchored colon syntax rewrites", urlPath: "/graph/v1.0/drives/" + testDriveID + "/items/" + testItemID + ":/notes.txt:/children", @@ -372,6 +386,14 @@ func TestResolveGraphPath_DecodesEncodedPath(t *testing.T) { urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/a%252Fb:/children", expectedStat: "./a%2Fb", }, + { + // Without a second colon the whole remainder is the path: Stat must + // receive "/Documents/children", not "/Documents" (which would mean + // "/children" was wrongly treated as a suffix). + name: "suffix-looking segment without a second colon is part of the Stat path", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Documents/children", + expectedStat: "./Documents/children", + }, } for _, tt := range tests { @@ -385,13 +407,12 @@ func TestResolveGraphPath_DecodesEncodedPath(t *testing.T) { }). Return(statResponse(cs3rpc.Code_CODE_OK, true), nil) - router, cap := newGraphTestRouter(t, gw) + router, _ := newGraphTestRouter(t, gw) req := httptest.NewRequest(http.MethodGet, "http://localhost"+tt.urlPath, nil) rr := httptest.NewRecorder() router.ServeHTTP(rr, req) assert.Equal(t, http.StatusOK, rr.Code) - assert.Equal(t, "children", cap.hit) assert.Equal(t, tt.expectedStat, statPath, "path passed to CS3 Stat must be decoded exactly once") }) } From 95544b65ef2bdba5b65d8165e7173e37b381d56d Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 1 Jul 2026 11:59:47 +0200 Subject: [PATCH 11/13] docs(graph): drop stale regex references from colon-path comments The colon-syntax parsing is plain string operations now, but a few comments still referred to "the previous regex" / "captures". Update them to match: parseColonPath extracts the itemID from the path (driveID comes from the route param), and the shape checks stand on their own. --- services/graph/pkg/middleware/path_lookup.go | 15 +++++++-------- services/graph/pkg/middleware/path_lookup_test.go | 4 ++-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/services/graph/pkg/middleware/path_lookup.go b/services/graph/pkg/middleware/path_lookup.go index 43bb25f174..7ddb0cc7c8 100644 --- a/services/graph/pkg/middleware/path_lookup.go +++ b/services/graph/pkg/middleware/path_lookup.go @@ -140,7 +140,7 @@ func ResolveGraphPath(gws pool.Selectable[gateway.GatewayAPIClient], logger log. // substrings from RoutePath; rewriteColonPath decodes them as needed. type colonMatch struct { isItemAnchored bool // item-anchored form: anchor is itemAnchorID, validate against driveID - itemAnchorID string // captured itemID for the item-anchored form (empty for root-anchored) + itemAnchorID string // itemID from the path for the item-anchored form (empty for root-anchored) relPath string // relative path with leading slash suffix string // suffix with leading slash (e.g. "/children"); may be empty } @@ -193,10 +193,10 @@ func rewriteColonPath( return "", errInvalidRequest } - // Item-anchored form captures driveID and itemID separately. Validate the - // itemID belongs to the given driveID (storage + space prefix) - otherwise - // the request is malformed and we short-circuit instead of doing an - // unnecessary CS3 Stat. + // Item-anchored form: the itemID comes from the path, driveID from the + // route param. Validate the itemID belongs to the given driveID (storage + + // space prefix) - otherwise the request is malformed and we short-circuit + // instead of doing an unnecessary CS3 Stat. if match.isItemAnchored { drive, err := storagespace.ParseID(driveID) if err != nil { @@ -282,9 +282,8 @@ func parseColonPath(routePath string) (colonMatch, bool) { rest = strings.TrimSuffix(rest, ":") m.relPath, m.suffix, _ = strings.Cut(rest, ":") - // Shape requirements (matching the previous regex): the path must be - // absolute and non-empty ("/x"), and a present suffix must be absolute and - // colon-free. + // Shape requirements: the path must be absolute and non-empty ("/x"), and a + // present suffix must be absolute and colon-free. if len(m.relPath) < 2 || m.relPath[0] != '/' { return m, false } diff --git a/services/graph/pkg/middleware/path_lookup_test.go b/services/graph/pkg/middleware/path_lookup_test.go index b933c81a9d..9347862c03 100644 --- a/services/graph/pkg/middleware/path_lookup_test.go +++ b/services/graph/pkg/middleware/path_lookup_test.go @@ -204,8 +204,8 @@ func TestResolveGraphPath(t *testing.T) { }, { // Multi-segment suffix: /permissions/{permissionID}/setPassword on - // v1beta1 (POST). Pins that the suffix regex carries embedded slashes - // through the rewrite into a real nested route. + // v1beta1 (POST). Pins that parseColonPath carries a suffix with + // embedded slashes through the rewrite into a real nested route. name: "v1beta1 root-anchored with multi-segment suffix rewrites and routes", method: http.MethodPost, urlPath: "/graph/v1beta1/drives/" + testDriveID + "/root:/folder1:/permissions/perm-1/setPassword", From b6a4a66aef0667cc8516fec32f5daa0cfa1e8a51 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 1 Jul 2026 14:24:10 +0200 Subject: [PATCH 12/13] refactor(graph): split colon paths on ":/" so colons in names work Review feedback: split the anchor/path and path/suffix on the structural delimiter ":/" instead of a bare ":". Since the path and suffix always start with "/", ":/" is the real delimiter, and a ":" *inside* a file or directory name (which OpenCloud allows but MS Graph/OneDrive forbid) is kept as part of the path instead of being mistaken for a separator. A ":" sitting at a segment boundary (e.g. a name ending in ":") stays ambiguous and must be percent-encoded as "%3A": the split works on the literal ":/", so "%3A" is never a delimiter and decodes back to ":". This is now documented in the code and the acceptance feature. Tests: colon inside a name (with and without a suffix), the Stat path carrying the colon, and the "%3A" boundary escape. --- services/graph/pkg/middleware/path_lookup.go | 56 ++++++++++++------- .../graph/pkg/middleware/path_lookup_test.go | 40 ++++++++++++- .../apiGraph/colonSyntaxPathLookup.feature | 7 +++ 3 files changed, 83 insertions(+), 20 deletions(-) diff --git a/services/graph/pkg/middleware/path_lookup.go b/services/graph/pkg/middleware/path_lookup.go index 7ddb0cc7c8..695e8b988b 100644 --- a/services/graph/pkg/middleware/path_lookup.go +++ b/services/graph/pkg/middleware/path_lookup.go @@ -231,38 +231,53 @@ func rewriteColonPath( // // Below the drive the grammar is one of: // -// /root: -// /items/: +// /root:/[:/][:] +// /items/:/[:/][:] // -// where is "/", optionally followed by ":/", and an -// optional trailing ":". Colons are structural delimiters, so neither -// nor contains one. For example: +// The structural delimiter is ":/" - a colon immediately followed by the +// leading slash of the path or suffix - not a bare ":". Splitting on ":/" +// keeps a ':' *inside* a file or directory name as data instead of mistaking +// it for a delimiter (segments are '/'-separated and never start with ':'). +// For example: // // /root:/Documents -> path "/Documents" // /root:/Documents: -> path "/Documents" // /root:/Documents:/children -> path "/Documents", suffix "/children" +// /root:/a:b.txt -> path "/a:b.txt" (colon kept in the name) // /items/{id}:/notes.txt:/children -> itemID "{id}", path "/notes.txt", suffix "/children" // +// A raw ':' at a segment boundary is ambiguous - "/root:/foo:/bar" reads as +// path "/foo" with suffix "/bar", and a trailing ':' is the no-suffix +// terminator. To address a name whose ':' sits at a boundary (e.g. a name +// ending in ':'), percent-encode it as "%3A": the split works on the literal +// ":/", so "%3A" is never a delimiter and decodes back to ':' with the rest of +// the path (e.g. "/root:/weird%3A/file.txt" -> "/weird:/file.txt"). MS Graph +// sidesteps this by forbidding ':' in names entirely; OpenCloud allows it (via +// WebDAV), so "%3A" is how such names are reached here. +// // The returned fields are the raw (still percent-encoded) substrings; the // caller decodes them. func parseColonPath(routePath string) (colonMatch, bool) { var m colonMatch - // The anchor is separated from the rest by the first colon; no colon at all + // The anchor is separated from the path by the first ":/"; no ":/" at all // means this isn't colon syntax and should pass through. - anchor, rest, found := strings.Cut(routePath, ":") + anchor, rest, found := strings.Cut(routePath, ":/") if !found { return m, false } + // Cut consumed the leading '/' of the path along with the ":/" delimiter; + // the path is absolute, so put it back. + rest = "/" + rest switch { case anchor == "/root": // Root-anchored: path resolution later anchors on the {driveID} route - // param, so there's no item id to capture from the URL. + // param, so there's no item id to read from the URL. case strings.HasPrefix(anchor, "/items/"): // Item-anchored: the anchor is /items/{itemID} with a single-segment // id. A '/' inside the id means this is an ordinary /items/{id}/... - // request that just happens to contain a colon further along. + // request that just happens to contain a ":/" further along. itemID := strings.TrimPrefix(anchor, "/items/") if itemID == "" || strings.Contains(itemID, "/") { return m, false @@ -273,21 +288,24 @@ func parseColonPath(routePath string) (colonMatch, bool) { return m, false } - // rest is "/[:/][:]". Drop a single optional trailing ':' - // (the "...:" no-suffix shape), then split path from an optional suffix on - // the one remaining delimiter colon. strings.Cut leaves the suffix empty - // when there's no delimiter: a suffix requires an explicit second colon, - // so e.g. "/root:/foo/children" is the path "/foo/children", not the path - // "/foo" with suffix "/children". - rest = strings.TrimSuffix(rest, ":") - m.relPath, m.suffix, _ = strings.Cut(rest, ":") + // rest is "/[:/][:]". Split path from an optional suffix on + // the next ":/" (the suffix regains the leading '/' Cut consumed), then + // drop a single trailing ':' - the "...:" no-suffix terminator. A suffix + // therefore requires an explicit ":/", so "/root:/foo/children" is the path + // "/foo/children", not "/foo" with suffix "/children". + m.relPath = rest + if path, suffix, found := strings.Cut(rest, ":/"); found { + m.relPath, m.suffix = path, "/"+suffix + } + m.relPath = strings.TrimSuffix(m.relPath, ":") // Shape requirements: the path must be absolute and non-empty ("/x"), and a - // present suffix must be absolute and colon-free. + // present suffix must be absolute, non-empty and colon-free (suffixes are + // fixed API routes like "/children", never file paths). if len(m.relPath) < 2 || m.relPath[0] != '/' { return m, false } - if m.suffix != "" && (m.suffix[0] != '/' || strings.Contains(m.suffix, ":")) { + if m.suffix != "" && (len(m.suffix) < 2 || strings.Contains(m.suffix, ":")) { return m, false } return m, true diff --git a/services/graph/pkg/middleware/path_lookup_test.go b/services/graph/pkg/middleware/path_lookup_test.go index 9347862c03..ee36f1d41b 100644 --- a/services/graph/pkg/middleware/path_lookup_test.go +++ b/services/graph/pkg/middleware/path_lookup_test.go @@ -163,7 +163,7 @@ func TestResolveGraphPath(t *testing.T) { // Anchoring: the colon-syntax shape appearing under a junk prefix // (i.e. NOT at the configured HTTP root) must not trigger a rewrite // or a CS3 Stat. Because the middleware is mounted under the - // /graph root, chi never routes such a path into it (404 first) — + // /graph root, chi never routes such a path into it (404 first) - // no /foo/.../v1.0/drives/...:/... can over-match. name: "colon-syntax under a junk prefix does not match (anchored on HTTP root)", urlPath: "/foo/graph/v1.0/drives/" + testDriveID + "/root:/Documents:/children", @@ -267,6 +267,29 @@ func TestResolveGraphPath(t *testing.T) { expectHit: "item", expectItemID: testItemID, }, + { + // A ':' inside a file name is data, not a delimiter (the delimiter + // is ":/"). This must resolve the whole path including the colon and + // route to the bare item. + name: "colon inside a file name is kept in the path", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/folder1/re:port.txt", + statCode: cs3rpc.Code_CODE_OK, + expectStatCalled: true, + expectStatus: http.StatusOK, + expectHit: "item", + expectItemID: testItemID, + }, + { + // Colon inside the file name AND a real ":/"-delimited suffix: the + // name keeps its colon, the suffix still routes. + name: "colon in file name with a real suffix", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/folder1/re:port.txt:/children", + statCode: cs3rpc.Code_CODE_OK, + expectStatCalled: true, + expectStatus: http.StatusOK, + expectHit: "children", + expectItemID: testItemID, + }, { name: "item-anchored colon syntax rewrites", urlPath: "/graph/v1.0/drives/" + testDriveID + "/items/" + testItemID + ":/notes.txt:/children", @@ -394,6 +417,21 @@ func TestResolveGraphPath_DecodesEncodedPath(t *testing.T) { urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/Documents/children", expectedStat: "./Documents/children", }, + { + // A ':' inside a file name (delimiter is ":/") must reach Stat as + // part of the path, not be treated as a path/suffix separator. + name: "colon inside a file name reaches Stat as part of the path", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/folder1/re:port.txt", + expectedStat: "./folder1/re:port.txt", + }, + { + // A ':' at a segment boundary is ambiguous raw, so it must be + // percent-encoded. "%3A" is never seen as the ":/" delimiter and + // decodes back to a literal ':' - here a directory named "weird:". + name: "percent-encoded colon (%3A) at a boundary reaches Stat as a literal colon", + urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/weird%3A/file.txt", + expectedStat: "./weird:/file.txt", + }, } for _, tt := range tests { diff --git a/tests/acceptance/features/apiGraph/colonSyntaxPathLookup.feature b/tests/acceptance/features/apiGraph/colonSyntaxPathLookup.feature index d8e445f769..6a725a6bb6 100644 --- a/tests/acceptance/features/apiGraph/colonSyntaxPathLookup.feature +++ b/tests/acceptance/features/apiGraph/colonSyntaxPathLookup.feature @@ -12,6 +12,13 @@ Feature: colon-syntax path lookup on the Graph API collapse to 404 so the middleware never discloses the existence of resources the caller is not allowed to see. + The delimiter is ":/", so a raw ':' inside a file or directory name is kept + as part of the path. A ':' at a segment boundary (e.g. a name ending in ':') + is ambiguous and must be percent-encoded as "%3A" to be addressed. MS Graph + and OneDrive sidestep this by forbidding ':' in names entirely; OpenCloud + allows it (via WebDAV), so the "%3A" escape is how such names are reached + here. + Background: Given user "Alice" has been created with default attributes And user "Alice" has created folder "folder1" From 11449b59437759895f1ea7f57f16dfa44093eb4c Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 1 Jul 2026 14:51:03 +0200 Subject: [PATCH 13/13] docs(graph): frame colon paths as "encode segments" instead of a raw-colon edge case Per review discussion: don't document accidental behavior. The contract is simply "percent-encode each path segment, as MS Graph requires; encode ':' as %3A" - OpenCloud allows ':' in names (OneDrive forbids it), so it's one more character in the mandatory encode set, not a special case. The parser is unchanged (split on ":/", decode once). This only rewrites the docs (code comment, acceptance feature, PR description) to state the encode contract, and drops the tests that relied on a raw, unencoded ':' in a file name - keeping the "%3A" test that reflects the actual contract. --- services/graph/pkg/middleware/path_lookup.go | 23 +++++------ .../graph/pkg/middleware/path_lookup_test.go | 38 ++----------------- .../apiGraph/colonSyntaxPathLookup.feature | 11 +++--- 3 files changed, 18 insertions(+), 54 deletions(-) diff --git a/services/graph/pkg/middleware/path_lookup.go b/services/graph/pkg/middleware/path_lookup.go index 695e8b988b..1fc89bcd6c 100644 --- a/services/graph/pkg/middleware/path_lookup.go +++ b/services/graph/pkg/middleware/path_lookup.go @@ -234,26 +234,21 @@ func rewriteColonPath( // /root:/[:/][:] // /items/:/[:/][:] // -// The structural delimiter is ":/" - a colon immediately followed by the -// leading slash of the path or suffix - not a bare ":". Splitting on ":/" -// keeps a ':' *inside* a file or directory name as data instead of mistaking -// it for a delimiter (segments are '/'-separated and never start with ':'). -// For example: +// The structural delimiter is ":/" (a colon immediately followed by the +// leading slash of the path or suffix); a trailing ":" is the no-suffix +// terminator. For example: // // /root:/Documents -> path "/Documents" // /root:/Documents: -> path "/Documents" // /root:/Documents:/children -> path "/Documents", suffix "/children" -// /root:/a:b.txt -> path "/a:b.txt" (colon kept in the name) // /items/{id}:/notes.txt:/children -> itemID "{id}", path "/notes.txt", suffix "/children" // -// A raw ':' at a segment boundary is ambiguous - "/root:/foo:/bar" reads as -// path "/foo" with suffix "/bar", and a trailing ':' is the no-suffix -// terminator. To address a name whose ':' sits at a boundary (e.g. a name -// ending in ':'), percent-encode it as "%3A": the split works on the literal -// ":/", so "%3A" is never a delimiter and decodes back to ':' with the rest of -// the path (e.g. "/root:/weird%3A/file.txt" -> "/weird:/file.txt"). MS Graph -// sidesteps this by forbidding ':' in names entirely; OpenCloud allows it (via -// WebDAV), so "%3A" is how such names are reached here. +// Clients must percent-encode each path segment, exactly as MS Graph requires +// (an unencoded path is ambiguous). The one OpenCloud-specific point: ':' must +// be encoded as "%3A". The split is on the literal ":/", so an encoded ':' is +// never a delimiter and decodes back to ':' with the rest of the path (e.g. +// "/root:/weird%3A/file.txt" -> "/weird:/file.txt"). OpenCloud allows ':' in +// names whereas MS Graph/OneDrive forbid it, hence this extra character. // // The returned fields are the raw (still percent-encoded) substrings; the // caller decodes them. diff --git a/services/graph/pkg/middleware/path_lookup_test.go b/services/graph/pkg/middleware/path_lookup_test.go index ee36f1d41b..fa866d2fed 100644 --- a/services/graph/pkg/middleware/path_lookup_test.go +++ b/services/graph/pkg/middleware/path_lookup_test.go @@ -267,29 +267,6 @@ func TestResolveGraphPath(t *testing.T) { expectHit: "item", expectItemID: testItemID, }, - { - // A ':' inside a file name is data, not a delimiter (the delimiter - // is ":/"). This must resolve the whole path including the colon and - // route to the bare item. - name: "colon inside a file name is kept in the path", - urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/folder1/re:port.txt", - statCode: cs3rpc.Code_CODE_OK, - expectStatCalled: true, - expectStatus: http.StatusOK, - expectHit: "item", - expectItemID: testItemID, - }, - { - // Colon inside the file name AND a real ":/"-delimited suffix: the - // name keeps its colon, the suffix still routes. - name: "colon in file name with a real suffix", - urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/folder1/re:port.txt:/children", - statCode: cs3rpc.Code_CODE_OK, - expectStatCalled: true, - expectStatus: http.StatusOK, - expectHit: "children", - expectItemID: testItemID, - }, { name: "item-anchored colon syntax rewrites", urlPath: "/graph/v1.0/drives/" + testDriveID + "/items/" + testItemID + ":/notes.txt:/children", @@ -418,17 +395,10 @@ func TestResolveGraphPath_DecodesEncodedPath(t *testing.T) { expectedStat: "./Documents/children", }, { - // A ':' inside a file name (delimiter is ":/") must reach Stat as - // part of the path, not be treated as a path/suffix separator. - name: "colon inside a file name reaches Stat as part of the path", - urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/folder1/re:port.txt", - expectedStat: "./folder1/re:port.txt", - }, - { - // A ':' at a segment boundary is ambiguous raw, so it must be - // percent-encoded. "%3A" is never seen as the ":/" delimiter and - // decodes back to a literal ':' - here a directory named "weird:". - name: "percent-encoded colon (%3A) at a boundary reaches Stat as a literal colon", + // Clients percent-encode ':' as "%3A" to put it in a name. "%3A" is + // never seen as the ":/" delimiter and decodes back to a literal ':' + // - here a directory named "weird:". + name: "percent-encoded colon (%3A) reaches Stat as a literal colon", urlPath: "/graph/v1.0/drives/" + testDriveID + "/root:/weird%3A/file.txt", expectedStat: "./weird:/file.txt", }, diff --git a/tests/acceptance/features/apiGraph/colonSyntaxPathLookup.feature b/tests/acceptance/features/apiGraph/colonSyntaxPathLookup.feature index 6a725a6bb6..6cd45ab8b1 100644 --- a/tests/acceptance/features/apiGraph/colonSyntaxPathLookup.feature +++ b/tests/acceptance/features/apiGraph/colonSyntaxPathLookup.feature @@ -12,12 +12,11 @@ Feature: colon-syntax path lookup on the Graph API collapse to 404 so the middleware never discloses the existence of resources the caller is not allowed to see. - The delimiter is ":/", so a raw ':' inside a file or directory name is kept - as part of the path. A ':' at a segment boundary (e.g. a name ending in ':') - is ambiguous and must be percent-encoded as "%3A" to be addressed. MS Graph - and OneDrive sidestep this by forbidding ':' in names entirely; OpenCloud - allows it (via WebDAV), so the "%3A" escape is how such names are reached - here. + Clients must percent-encode each path segment, exactly as MS Graph requires. + The only OpenCloud-specific point: ':' must be encoded as "%3A" (the split is + on the literal ":/", so an encoded ':' is never a delimiter). OpenCloud allows + ':' in names whereas MS Graph/OneDrive forbid it, so "%3A" is the one extra + character to encode. Background: Given user "Alice" has been created with default attributes