From 7498e8f848ec7ebb4924902c6bb54e6f3788e53b Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Tue, 28 Apr 2026 10:55:39 +0200 Subject: [PATCH] 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 {