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

cookies: extend domain checks to non psl builds #2964

Closed

Conversation

danielgustafsson
Copy link
Member

Ensure to perform the checks we have to enforce a sane domain in the cookie request. The check for non-PSL enabled builds is quite basic but it's better than nothing.

The preprocessor juggling around the PSL block which ends on an else may not be too everyones liking; I'm happy to refactor in that case.

@bagder
Copy link
Member

bagder commented Sep 10, 2018

It looks like this breaks test 8 in "torture mode" ...

@danielgustafsson
Copy link
Member Author

Test 8 seems to have failed and I think I know why, will fix.

@danielgustafsson
Copy link
Member Author

Pushed a fix which makes the test pass locally, fingers crossed that Travis agrees with me.

@danielgustafsson
Copy link
Member Author

Failed again, but not because of the patch this time..

@danielgustafsson danielgustafsson added the feature-window A merge of this requires an open feature window label Nov 21, 2018
Ensure to perform the checks we have to enforce a sane domain in
the cookie request. The check for non-PSL enabled builds is quite
basic but it's better than nothing.
@danielgustafsson
Copy link
Member Author

Force pushed a rebase to avoid the redness, even though it wasn't terribly related.

@bagder do you still have doubts regarding the test?

@danielgustafsson
Copy link
Member Author

Red due to fuzzer build timing out

@bagder
Copy link
Member

bagder commented Dec 17, 2018

I'm good!

(apart from a bit concerned that we're closing in on the 50 minutes limit for the fuzzer tests more and more these days...)

@danielgustafsson
Copy link
Member Author

I'm good!

Thanks! I'll go ahead and merge this in the current feature window.

(apart from a bit concerned that we're closing in on the 50 minutes limit for the fuzzer tests more and more these days...)

Yeah.. that's not good. Is it possible to get a larger instance to run the fuzzer on, by perhaps getting sponsorship for such a node?

@danielgustafsson
Copy link
Member Author

Pushed to master, thanks for review!

@deepakhj
Copy link

deepakhj commented Feb 28, 2019

This change broke using curl to hit a local unleash container in Kubernetes. We use the pod's name without any DNS. Is there anyway to turn this off besides compiling it ourself?

* cookie 'unleash-session' dropped, domain 'unleash-http' must not set cookies for 'unleash-http'

@bagder
Copy link
Member

bagder commented Feb 28, 2019

If you want to handle this as an issue, then please a file a new one. Commenting on an old closed PR will not get much attention by anyone.

Is there anyway to turn this off besides compiling it ourself?

No. Although I would rather suggest you instead change your setup to use more proper names so that HTTP cookies can be used the way they are documented to work rather than to lower the bars for curl...

@danielgustafsson
Copy link
Member Author

danielgustafsson commented Feb 28, 2019

Is there anyway to turn this off besides compiling it ourself?

No, there are no options for this behaviour. The intention was to align non-libpsl builds with libpsl-enabled builds.

* cookie 'unleash-session' dropped, domain 'unleash-http' must not set cookies for 'unleash-http'

In your interpretation of the RFC and the follow-up draft, should this cookie have been allowed?

Also, as @bagder mentioned upthread, please raise this on the mailing-list or as an issue.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-window A merge of this requires an open feature window
Development

Successfully merging this pull request may close these issues.

None yet

3 participants