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

fix(cors): emptyStatusCode not being taken into account for OPTIONS requests #4024

Conversation

mfeltscher
Copy link

With #3919 the default status code for empty responses changed from 200 to 204 while still giving the user the ability to override this behaviour by using the emptyStatusCode response setting. This setting however isn't being taken into account when using CORS and OPTIONS requests. This pull request fixes this glitch.

@mfeltscher
Copy link
Author

@hueniverse I just stumbled upon this issue when upgrading to v19 - could you please have a look?

@kanongil
Copy link
Contributor

Hmm, this might not be a safe thing to do: https://stackoverflow.com/a/58794243/248062.

@mfeltscher
Copy link
Author

@kanongil I can also change this to always return 200 if that's a better solution in your eyes.

@kanongil
Copy link
Contributor

Ah, this is a bit tricky actually.

You issue pertains to the emptyStatusCode, as set on a route with a cors config, which you would expect to also apply to the OPTIONS response. But it currently doesn't.

However, when setting the emptyStatusCode as a global route config when creating the server, it does apply to all CORS OPTIONS responses. This is now by default 204.

Whether it should always return 200 will be up to @hueniverse, but there is definitely an issue here.

@hueniverse hueniverse self-assigned this Jan 14, 2020
@mfeltscher
Copy link
Author

@kanongil How can I configure this as a global route config?

@kanongil
Copy link
Contributor

kanongil commented Jan 15, 2020

new Hapi.Server({
    routes: {
        response: {
            emptyStatusCode: 200
        }
    }
});

@mfeltscher
Copy link
Author

@kanongil Thanks, that works like a charm! I'll still leave this PR open for @hueniverse to have a look at.

@hueniverse hueniverse removed their assignment Jul 16, 2020
@devinivy
Copy link
Member

It looks like kong also went through this. In short, it sounds like the spec doesn't technically care but browser implementations do care or have cared in the past. MDN docs used to recommend serving a 200, but now they recommend a 204. Kong used to serve a 204, was patched to serve a 200, and today has an open issue to move back to 204.

Kong/kong#4008
Kong/kong#4029
Kong/kong#6051
https://developer.mozilla.org/en-US/docs/Glossary/Preflight_request

I propose we stick with the 204 default, but create a separate option route.cors.preflightStatusCode to control it in hapi v21 rather than route.response.emptyStatusCode.

@devinivy
Copy link
Member

This has been hanging around for a while, and I don't think this PR is going to land as-is. But the contribution is much appreciated, and helped move the conversation about this issue forward. There are still some ideas and open questions above, so I am going to close this and move it to an issue for further discussion.

@devinivy
Copy link
Member

Here it is: #4165

@devinivy devinivy added breaking changes Change that can breaking existing code bug Bug or defect labels Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Change that can breaking existing code bug Bug or defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants