Fix CORS redirects in noms serve #3617
Conversation
The httprouter was HTTP-redirecting paths like graphql to graphql/ which isn't CORS-compatible. This patch just hard-codes the trailing and non-trailing slash versions of each handler path.
|
@rafael-atticlabs ptal I looked at httprouter and I didn't see a better way of doing this. |
|
I.e. before patch:
|
|
(adding the trailing / i.e. |
ghost
requested review from
|
I think @arv is a better person to review. |
ghost
closed this
ghost
reopened this
| router.POST(strings.TrimSuffix(path, "/"), s.corsHandle(s.makeHandle(hndlr))) | ||
| } | ||
| } | ||
| post(constants.BasePath, HandleBaseGet) |
There was a problem hiding this comment.
Why did HandleBaseGet change to POST?
|
Maybe disable RedirectTrailingSlash instead? https://godoc.org/github.com/julienschmidt/httprouter#Router.RedirectTrailingSlash |
|
This patch does also disable RedirectTrailingSlash. But without adding it back manually, that would be backwards incompatible. |
|
Do we really want to support both? |
| router.OPTIONS(constants.GraphQLPath, s.corsHandle(noopHandle)) | ||
| // Note: we implement trailing trailing-slash removal ourselves because | ||
| // httprouter doesn't handle the redirects properly with CORS. | ||
| router := httprouter.Router{} |
There was a problem hiding this comment.
This changes more things than RedirectTrailingSlash. Maybe use New and then set RedirectTrailingSlash instead
There was a problem hiding this comment.
I checked the options and having them all seems correct to me.
There was a problem hiding this comment.
All false, I mean.
See https://github.com/julienschmidt/httprouter/blob/master/router.go#L170
- We don't want any of the Redirect options (they're all CORS incompatible).
- HandleMethodNotAllowed is weird
- HandleOPTIONS is irrelevant since we implement our own
Yes I believe so. |
The httprouter was HTTP-redirecting paths like graphql to graphql/ which
isn't CORS-compatible. This patch just hard-codes the trailing and
non-trailing slash versions of each handler path.