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

Projects
None yet
3 participants
@danielgustafsson
Copy link
Member

commented Sep 9, 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.

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

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

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

@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2018

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

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-nonlibpsl_fallback branch from fa167e7 to 15df541 Sep 18, 2018

@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2018

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

@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2018

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

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-nonlibpsl_fallback branch from 15df541 to 769525f Sep 19, 2018

Show resolved Hide resolved tests/data/test8

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-nonlibpsl_fallback branch from 769525f to 38d0bfd Dec 13, 2018

cookies: extend domain checks to non psl builds
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 danielgustafsson force-pushed the danielgustafsson:dg-nonlibpsl_fallback branch from 38d0bfd to a3bf73d Dec 17, 2018

@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

Red due to fuzzer build timing out

@bagder

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2018

Pushed to master, thanks for review!

@deepakhj

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.