Repository navigation
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reproduction
SupressNotFoundresponds 404 for requests the router would have served, becauseits preflight
Matchcall usesr.Methodandr.URL.Pathwhile the routerresolves the request differently when it dispatches.
Encoded slashes, with
r.Get("/files/{id}", handler)andSupressNotFoundon thesame router:
A method override from middleware earlier in the chain behaves the same way.
GetHeadsetsRouteMethodtoGETfor HEAD requests, so withGetHeadregistered before
SupressNotFound:Root cause
Dispatch in
Mux.routeHTTPprefersr.URL.RawPathover the decodedr.URL.Path, and usesrctx.RouteMethodwhen it is set. The preflight inSupressNotFoundused neither, so it matched a different request than the onethe 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:
rctx.RouteMethod, falling back tor.Methodr.URL.RawPath, falling back tor.URL.Path, falling back to/The probe still runs on a throwaway route context, as #1220 established.
Not addressed
rctx.RoutePathis deliberately not used. When this middleware is mounted on asubrouter,
rctx.Routesstill points at the root router whileRoutePathholdsthe sub-path, so matching
RoutePathagainstrctx.Routesreturns 404 forrequests the subrouter serves. A request rewritten by
StripSlashesafter thismiddleware 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
TestSupressNotFoundResolvesRequestLikeRoutercovers both cases above plus acontrol asserting that an unknown path is still suppressed. On the previous
implementation:
TestSupressNotFoundMountandTestSupressNotFoundRoutePatternfrom #1220 stillpass unchanged.
Commands run: