-
-
Notifications
You must be signed in to change notification settings - Fork 174
[#877] Return Bad Request error if CORS failed #882
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
[#877] Return Bad Request error if CORS failed #882
Conversation
|
Fixes #877 |
aldaris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've always found it strange why the CORSService was trying to implement the whole validation. Realistically the only thing the cors service should do is to check whether the current origin is amongst the accepted Origins and set the header for that origin only. Another optimisation would be to check if there is only one accepted origin, and then just set all the headers and let the browser print all the errors it wants to the JS console. (according to the spec looking at JS console is a legitimate way to debug CORS related problems anyways.)
Of course the best would be if it would be possible to set headers/credentials/etc settings on a per origin basis, and not expose/allow unnecessary headers/etc for ALL the origins, like the current version of the filter does. Maybe a feature request for down the line?
| res.setContentType("application/json"); | ||
| res.setCharacterEncoding("UTF-8"); | ||
| res.getWriter().write(jsonValue.toString()); | ||
| res.setStatus(resourceException.getCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should set the response status before writing anything to the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed in c51d44f
| JsonValue jsonValue = resourceException.toJsonValue(); | ||
| res.setContentType("application/json"); | ||
| res.setCharacterEncoding("UTF-8"); | ||
| res.getWriter().write(jsonValue.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just keep in mind that JsonValue#toString doesn't always return actual proper JSON. If you call it on something like a Map value (result of json.get(field)), it will just print out the "value", not a valid JSON.
No description provided.