Skip to content

middleware: resolve SupressNotFound preflight like the router does - #1222

Open
phaethix wants to merge 1 commit into
go-chi:masterfrom
phaethix:fix/supress-notfound-preflight
Open

phaethix wants to merge 1 commit into
go-chi:masterfrom
phaethix:fix/supress-notfound-preflight

Conversation

@phaethix

Copy link
Copy Markdown

Reproduction

SupressNotFound responds 404 for requests the router would have served, because
its preflight Match call uses r.Method and r.URL.Path while the router
resolves the request differently when it dispatches.

Encoded slashes, with r.Get("/files/{id}", handler) and SupressNotFound on the
same router:

GET /files/a%2Fb
  URL.Path  = /files/a/b
  URL.RawPath = /files/a%2Fb

without SupressNotFound: 200, id=a%2Fb
with SupressNotFound:    404

A method override from middleware earlier in the chain behaves the same way.
GetHead sets RouteMethod to GET for HEAD requests, so with GetHead
registered before SupressNotFound:

HEAD /x   (route: GET /x)
without SupressNotFound: 200
with SupressNotFound:    404

Root cause

Dispatch in Mux.routeHTTP prefers r.URL.RawPath over the decoded
r.URL.Path, and uses rctx.RouteMethod when it is set. The preflight in
SupressNotFound used neither, so it matched a different request than the one
the router later dispatched.

This is independent of the route context fix in #1220; that change stopped the
preflight from mutating the live context, but did not change which method and
path it matches.

Solution

Resolve the method and path the same way the router does before probing:

  • method: rctx.RouteMethod, falling back to r.Method
  • path: r.URL.RawPath, falling back to r.URL.Path, falling back to /

The probe still runs on a throwaway route context, as #1220 established.

Not addressed

rctx.RoutePath is deliberately not used. When this middleware is mounted on a
subrouter, rctx.Routes still points at the root router while RoutePath holds
the sub-path, so matching RoutePath against rctx.Routes returns 404 for
requests the subrouter serves. A request rewritten by StripSlashes after this
middleware therefore still reports 404. Fixing that needs a decision about which
router the preflight should match against, so it is out of scope here.

Tests

TestSupressNotFoundResolvesRequestLikeRouter covers both cases above plus a
control asserting that an unknown path is still suppressed. On the previous
implementation:

--- FAIL: TestSupressNotFoundResolvesRequestLikeRouter/raw_path
    status: got 404, want 200
--- FAIL: TestSupressNotFoundResolvesRequestLikeRouter/method_overridden_upstream
    status: got 404, want 200

TestSupressNotFoundMount and TestSupressNotFoundRoutePattern from #1220 still
pass unchanged.

Commands run:

go test ./middleware -run '^TestSupressNotFound'
go test -race ./...
gofmt -l .
go vet ./...

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant