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) {