Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send a 405 Method Not Allowed error when POST to a static asset #1818

Merged
merged 9 commits into from Mar 20, 2019

Conversation

3 participants
@Ronolibert
Copy link
Contributor

Ronolibert commented Mar 5, 2019

Description

When someone makes a POST request to a static asset, send back a 405 error since it is a POST request hitting a GET endpoint

Reviewer Notes

  • I opted to go with a 405 after checking how other sites do this. 405 is method not allowed which I think is what we want to tell those using the GET endpoint in this way
  • I'm curious if there's a better way to handle this for all these endpoints. A way for the server to return a 405 if a GET endpoint is being hit with a POST request

Setup

Instructions in card

I tested this in Postman making a POST request to http://officelocal:8080/static/css/foobar when make server_run is running

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@Ronolibert Ronolibert requested review from jim , chrisgilmerproj and kimallen Mar 5, 2019

@@ -930,6 +943,7 @@ func main() {
site.Use(httpsComplianceMiddleware)
site.Use(securityHeadersMiddleware)
site.Use(limitBodySizeMiddleware)
site.Use(validMethodForStaticMiddleware)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 5, 2019

Contributor

Instead of adding this everywhere could we just add it to the routes that server static files? Then we don't have to do a path check, we can just ensure that only GETs are being used.

Similarly, could we remove the CSRF milddleware from the routes for static files?

This comment has been minimized.

@Ronolibert

Ronolibert Mar 6, 2019

Author Contributor

Good idea! I looked into both. I couldn't find anything about being able to remove middleware from specific subroutes. All I saw were suggestions to use subrouting and use specific middlewares there but since our csrf check is intentionally on all requests it doesn't meet our usecase. I ended up going with creating a subroute for static and checking if it is not a get request than send back a 405

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 7, 2019

Contributor

I guess what I'm getting at is that the CSRF middleware shouldn't be on all requests, just the ones that require POST/PATCH/PUT/DELETE. Or rather, it shouldn't do any enforcing on routes that do GET even though I'm ok with it setting the cookie on GET.

So yes, we should error on POST requests for static assets, but the real problem is that CSRF middleware is being triggered on static assets in the first place. Solving both of these problems would be good to do in this PR.

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

Questions in line.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #1818 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1818   +/-   ##
=======================================
  Coverage   50.31%   50.31%           
=======================================
  Files         441      441           
  Lines       19077    19077           
  Branches     1641     1641           
=======================================
  Hits         9599     9599           
  Misses       8637     8637           
  Partials      841      841

@Ronolibert Ronolibert requested a review from chrisgilmerproj Mar 6, 2019

// Allow public content through without any auth or app checks
site.Handle(pat.Get("/static/*"), clientHandler)
site.Handle(pat.New("/static/*"), staticMux)
site.Handle(pat.Get("/swagger-ui/*"), clientHandler)
site.Handle(pat.Get("/downloads/*"), clientHandler)
site.Handle(pat.Get("/favicon.ico"), clientHandler)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 7, 2019

Contributor

You may want to use the staticMux on the downloads and favicon as well. If the swagger-ui is always just static files as well then I'd include it too.

This comment has been minimized.

@Ronolibert

Ronolibert Mar 8, 2019

Author Contributor

Good catch, I have now used it for swagger-ui, downloads and the favicon. Swagger-ui is always just a static html file

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

I think this is pretty close. I'd love to see a test that shows the 405 is returned from a static route, which should confirm that its not being intercepted by the CSRF middleware.

@Ronolibert Ronolibert force-pushed the roc-#164177839-404-invalid-requests branch from 0a9e378 to 38de60b Mar 8, 2019

@Ronolibert

This comment has been minimized.

Copy link
Contributor Author

Ronolibert commented Mar 8, 2019

I think this is pretty close. I'd love to see a test that shows the 405 is returned from a static route, which should confirm that its not being intercepted by the CSRF middleware.

Do you know of any files/examples I could use a template for a test like this? I'm not sure how the mock server setup and making a request to it would look like. Looking through the code I see uses of httptest.NewRecorder and things like that but unclear how to use that in this situation since we're not hitting any specific handlers

@Ronolibert Ronolibert force-pushed the roc-#164177839-404-invalid-requests branch from 38de60b to b3b0e98 Mar 8, 2019

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

chrisgilmerproj commented Mar 14, 2019

Do you know of any files/examples I could use a template for a test like this? I'm not sure how the mock server setup and making a request to it would look like. Looking through the code I see uses of httptest.NewRecorder and things like that but unclear how to use that in this situation since we're not hitting any specific handlers

I think you could probably do something like this middleware test: https://github.com/transcom/mymove/blob/master/pkg/auth/cookie_test.go#L53-L78

I'll also ping @stangah to see if he has any questions about this.

@stangah

This comment has been minimized.

Copy link
Contributor

stangah commented Mar 19, 2019

@chrisgilmerproj I wrote an API test using Cypress since we are really testing the muxing instead of the middleware, let me know if that works for you

@@ -154,6 +154,18 @@ func httpsComplianceMiddleware(inner http.Handler) http.Handler {
return http.HandlerFunc(mw)
}

func validMethodForStaticMiddleware(inner http.Handler) http.Handler {
mw := func(w http.ResponseWriter, r *http.Request) {
if r.Method != "GET" {

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 19, 2019

Contributor

Thinking about this, we should probably allow HEAD as well.

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

I honestly think using Cypress to test out the api is brilliant. I think you're right that it gives us a better indicator of whether or not its working. That being said, I'd still like to see a unit test against the middleware.


// Explicitly disable swagger.json route
site.Handle(pat.Get("/swagger.json"), http.NotFoundHandler())
if v.GetBool(serveSwaggerUIFlag) {
logger.Info("Swagger UI static file serving is enabled")
site.Handle(pat.Get("/swagger-ui/*"), clientHandler)
site.Handle(pat.Get("/swagger-ui/*"), staticMux)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 20, 2019

Contributor

nice find!

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

chrisgilmerproj commented Mar 20, 2019

This looks good to me!

@stangah stangah merged commit e15f86c into master Mar 20, 2019

20 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: client_test_coverage Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 884f84f...7c7b489
Details
codecov/project 50.31% remains the same compared to 884f84f
Details

@stangah stangah deleted the roc-#164177839-404-invalid-requests branch Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.