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

Fix cookies on multiple page #290

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

j0k3r
Copy link
Owner

@j0k3r j0k3r commented Mar 1, 2022

It was the case for golem.de, the cookie wasn't properly send to the next page (might be a bug in the cookie jar not properly retrieving previous defined cookies).

Fix j0k3r/graby-site-config#48

@coveralls
Copy link

coveralls commented Mar 1, 2022

Coverage Status

Coverage increased (+0.5%) to 95.611% when pulling e3504a5 on fix/cookie-multiple-pages into a7aecce on master.

@j0k3r j0k3r force-pushed the fix/cookie-multiple-pages branch from 786621f to f803fd9 Compare March 1, 2022 20:35
@Kdecherf
Copy link
Collaborator

Kdecherf commented Mar 2, 2022

Didn't check in depth but we may need to add a test to ensure that we don't leak cookies if the next page is not on the same domain. What do you think?

@j0k3r
Copy link
Owner Author

j0k3r commented Mar 2, 2022

That's a good question, this shouldn't happen so much.
Should we add the cookie in the cookie jar instead so it'll be checked later in the foreach?

It was the case for golem.de, the cookie wasn't properly send to the next page (might be a bug in the cookie jar not properly retrieving previous defined cookies).
@j0k3r
Copy link
Owner Author

j0k3r commented Jun 30, 2022

I've checked and cookies we are re-injecting are only those which are defined in site config, so there shouldn't really contains sensitive data?
Otherwise I don't know how to fix the leak because at the point where we should check it, we don't know the host associated to cookies which are in the headers.

@j0k3r j0k3r force-pushed the fix/cookie-multiple-pages branch from f803fd9 to e3504a5 Compare June 30, 2022 18:07
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.

multipage articles do not use defined cookie?
3 participants