fix: CORS: security routes and origin handling #1478
Merged
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
|
Now that the configured origin list check works we have another problem: if you configure any CORS origin and don't explicitly configure the one where Admin UI is being used this will block admin UI save functionalities. How to deal with this? At the moment my idea is to add something to the UI that will include the host where admin UI is being used by default but optionally in the list of allowed CORS origins, like |
tkurki
changed the title
|
Also some font and style requests from the Admin UI get blocked, because they have origin explicitly for some reason. |
|
This seeems a reasonable approach. |
CORS handling was not working in any of the paths related to authentication, because cors middleware was added after security path handlers were added. This changes the order so that CORS works with these paths also.
CORS origin check was broken in two ways: - the first configured allowed origin was not allowed - any other origin would be allowed access This fixes the check and changes logging denied requests to debug only.
Ignore whitespace and trailing slash.
Add special handling for Admin UI CORS origin and a brief explanation of the mechanism in the security configuration form.
tkurki
changed the title
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.
CORS was previously not enabled for any of the security related routes, specifically `/signalk/v1/auth/login. This PR changes the order in server setup so that CORS middleware is activated before security routes. Fixes #1479.
Without any origins configured the server allows CORS requests from any host.
However when configured the origin list was not working correctly:
undefinedthat is same hostThis PR fixes the origin checking logic so that it actually works.
In addition this PR adds special handling for the Admin UI origin: the origin that the security configuration form that is used to set CORS origin field to non empty value is automatically added to the list of allowed CORS origins. Otherwise Admin UI itself will stop working, blocked by CORS.
Now the security configuration form also includes a short description of how CORS works.