Handle unauthorized errors plus return safe codes #472
Conversation
This file contains 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
https://eaflood.atlassian.net/browse/WATER-4085 https://eaflood.atlassian.net/browse/WATER-4155 The final step of our pass through authentication story is dealing with unauthorized requests. This is where Hapi will compare the scope defined in the credentials our `AuthPlugin` sets to what is in the route config. If they don't match Hapi will through an error with a [403 Forbidden](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/403) code. When this happens our `ErrorsPlugin` will pick up the response and using `ErrorPagesService` determine if it should be redirected to an error page. Prior to this change our logic would have deduced its a [500 Internal Server Error](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/500) and show the page for that. Clearly that's wrong but the solution is a little more complex. If you assume we don't render links to things a user cannot access, then the request must have been manufactured i.e. directly entered into the browser. If we respond with a 403 and a nice 'You are not authorised' page we would be confirming the thing someone is trying to access exists, they just don't have authority too. Similar, to handling [user enumeration attacks](https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/03-Identity_Management_Testing/04-Testing_for_Account_Enumeration_and_Guessable_User_Account) we don't want to do that. Instead, it is [recommended to return a 404](https://www.rfc-editor.org/rfc/rfc9110.html#name-403-forbidden). So, we need to amend our error handling logic to check for `403` errors but ensure we respond with a `404` instead. Finally, whilst we are messing with the logic we are also going to deal with the fact returning a `500` means the user never sees our error page. This is because the service is behind a [F5 Silverline Managed Web Application Firewall (WAF)](https://aws.amazon.com/marketplace/pp/prodview-7ge77z55kayes) which blocks `5xx` responses and returns its own error page. (It does this to prevent apps leaking info because of unhandled errors.)
These are the changes we thought would get the tests passing but they don't! :-) The reason is we'd like to log that an unauthorised request has been made, but ensure we return `404` as the response code. But we've overlooked we determine what status code to send back first, then whether to log anything based on that calculated code. So, we're going to need to rethink things to achieve all the things we want.
So, all fixed and we are doing what we wanted when a 403 is raised - always log 'Not authorised' - return 404 if we are going to render a page - return 403 if this was an API (internal service) request We did get ahead of ourselves though and have now broken the tests for `5xx` errors. So, next we'll fix that.
Just removing some of the inconsistencies between the tests.
Again we update our tests to reflect the outcome we want. Realised there was little point on creating routes with handlers that generate a scenario, if we then stub `ErrorPagesService` to return a specific result. What `ErrorPagesService` is doing is very light weight. So, in this case we're happy to remove the stubbing altogether and let the scenarios (route setup in each test) dictate what the plugin needs to handle.
Cruikshanks
force-pushed
the
handle-unauthorized-error-responses
branch
from
29631f0 to
3e38313
Compare
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.
https://eaflood.atlassian.net/browse/WATER-4085
https://eaflood.atlassian.net/browse/WATER-4155
The final step of our pass-through authentication story is dealing with unauthorized requests. This is where Hapi will compare the scope defined in the credentials our
AuthPluginsets to what is in the route config. If they don't match Hapi will through an error with a 403 Forbidden code.When this happens our
ErrorsPluginwill pick up the response and usingErrorPagesServicedetermine if it should be redirected to an error page. Before this change, our logic would have deduced a 500 Internal Server Error and show the page for that.Clearly, that's wrong but the solution is a little more complex. If you assume we don't render links to things a user cannot access, the request must have been manufactured i.e. directly entered into the browser. If we respond with a
403and a nice 'You are not authorised' page we would be confirming the thing someone is trying to access exists, they just don't have the authority to. Similar to handling user enumeration attacks we don't want to do that. Instead, it is recommended to return a 404.So, we need to amend our error handling logic to check for
403errors but ensure we respond with404instead.Finally, whilst we are messing with the logic, we will also deal with the fact that returning a
500means the user never sees our error page. This is because the service is behind a F5 Silverline Managed Web Application Firewall (WAF) which blocks5xxresponses and returns its own error page. (It does this to prevent apps from leaking info because of unhandled errors.)