Skip to content

Fix cookies on multiple page#290

Open
j0k3r wants to merge 2 commits into
masterfrom
fix/cookie-multiple-pages
Open

Fix cookies on multiple page#290
j0k3r wants to merge 2 commits into
masterfrom
fix/cookie-multiple-pages

Conversation

@j0k3r

@j0k3r j0k3r commented Mar 1, 2022

Copy link
Copy Markdown
Owner

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

coveralls commented Mar 1, 2022

Copy link
Copy Markdown

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

Kdecherf commented Mar 2, 2022

Copy link
Copy Markdown
Collaborator

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

j0k3r commented Mar 2, 2022

Copy link
Copy Markdown
Owner Author

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

j0k3r commented Jun 30, 2022

Copy link
Copy Markdown
Owner Author

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