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

Make Origin scheme-aware #388

Conversation

wgonczaronek
Copy link
Contributor

Regarding #379
I've added checks against schemes so that different schemes are not allowed. If scheme is not provided, request is processed, but deprecation warning is raised.
I am not sure whether checking on request processing is best idea, an alternative would be to check it on project setup, I am not sure how this would work with CORS defined in model.
I have also skipped checking regex, since user might deliberately avoid passing scheme.
I am open for suggestions.

@vitaliimelnychuk
Copy link

Great, I was facing the same issue.
I think the is no reasons to check on request processing.
One more idea is to process passed host on project setup and create all available variants
eventually, we will receive something like this

input:

    CORS_ORIGIN_WHITELIST = (
         'google.com'
     )		 

array to use in each response:

    [
       'http://google.com', 'https://google.com'
    ]

Also, a warning message might be in the project set up.

Waiting for merge

@wgonczaronek
Copy link
Contributor Author

Since this is a security feature I would like to avoid any behavior that is not immediately obvious and might be subject to some sort of missclicks. I'd rather make user explicitly define schemes they are OK with than risk having a situation when they wanted to allow only one of them, but forgot to change some settings and got loosened security policy.

I'll implement checks on startup though.

@vitaliimelnychuk
Copy link

Yes, you are right. I mean to apply this flow for old defined configs.
I this case will be warning that this is deprecated and won't work in feature releases. but this host will be apllied for two schemas.

@adamchainz
Copy link
Owner

Hi @wgonczaronek ,

This is a great start, thank you very much. Sorry I've been slow to respond, it's been a busy season with Djagnocon Europe.

I've pushed a few cosmetic changes, but I still have some more things to think through. Parsing the list every time seems unnecessary, and the model is adding a lot of confusion to the code. @melnychukvitaliy 's suggestion to automatically update old configuration to new is a good one as it would reduce the amount of edge cases in the code but it's impossible to know all the schemes that users were previously working with, there will be edge cases such as onion:// and ftp://.

The model is also making things awkward here. I've been tempted to drop the model for a long time, since I doubt many users have used it (no one had any problems after making it abstract in 2.0.0). It was added in #27 without any discussion and has been in many users' databases since.

Also the code has some weird branches for supporting scheme-less origins. parsed_origin.netloc or parsed_origin.path is especially confusing until you understand that urlparse without a scheme puts everything in path. This makes me tempted to go ahead without a deprecation perio, and instead force a major version release to make everyone add schemes in their settings. In my experience deprecation warnings are ignored 80% of the time, and we want to help users be secure here.

I'm still forming a plan. I'll probably merge this as-is as part of it.

Thanks for pushing on this!

Adam

@adamchainz adamchainz force-pushed the issue-379-origin-should-include-scheme branch from 21d32d4 to a2703bf Compare May 10, 2019 09:07
@adamchainz adamchainz changed the title Issue 379 origin should include scheme Make Origin scheme-aware May 10, 2019
@adamchainz adamchainz merged commit 3a1c92d into adamchainz:master May 10, 2019
@wgonczaronek
Copy link
Contributor Author

Hi, I have missed the notice that you commented on the issue.

I agree with the problem with schemes that could be used. There are plenty of them and I can imagine a situation where some custom ones are used.

I also agree with the issues with model. This was a tricky part for me to check it. Same applies to urlparse. Unfortunately handling both urls with and without schema was problematic. Forcing users to use schema would greatly simplify this code.

Let me know if you'd like me to implement some further changes. I am happy to help.

@adamchainz
Copy link
Owner

Oh, thanks for the reply. I merged it because I'm working on it right now, first removing the model, then moving to non-deprecation path. Don't worry about it, thanks for your contribution!

adamchainz added a commit that referenced this pull request May 10, 2019
As per history note, and comment on #388.
adamchainz added a commit that referenced this pull request May 10, 2019
As per history note, and comment on #388.
adamchainz added a commit that referenced this pull request May 10, 2019
As per history note, and comment on #388.
adamchainz added a commit that referenced this pull request May 10, 2019
This "upgrades" the change in #388 to *require* scheme, rather than going through a deprecation period. It simplifies the code greatly.

I added checks as well to ensure that `CORS_ORIGIN_WHITELIST` is well-formed, so that users upgrading will see some obvious errors.
adamchainz added a commit that referenced this pull request May 10, 2019
This "upgrades" the change in #388 to *require* scheme, rather than going through a deprecation period. It simplifies the code greatly.

I added checks as well to ensure that `CORS_ORIGIN_WHITELIST` is well-formed, so that users upgrading will see some obvious errors.
adamchainz added a commit that referenced this pull request May 10, 2019
This "upgrades" the change in #388 to *require* scheme, rather than going through a deprecation period. It simplifies the code greatly.

I added checks as well to ensure that `CORS_ORIGIN_WHITELIST` is well-formed, so that users upgrading will see some obvious errors.
adamchainz added a commit that referenced this pull request May 10, 2019
This "upgrades" the change in #388 to *require* scheme, rather than going through a deprecation period. It simplifies the code greatly.

I added checks as well to ensure that `CORS_ORIGIN_WHITELIST` is well-formed, so that users upgrading will see some obvious errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants