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

Reduce http header cookie clutter #1481

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

Kiradien
Copy link
Collaborator

@Kiradien Kiradien commented Sep 10, 2024

Previous logic attempted to duplicate cookies which were being set by default within chrome's request object. Cookies at issue had specialized settings of "Session" or no_restriction on SameSite.

@gamebeaker - I'd like your opinion on the HttpClient change before pushing it through; under the current build, if you open the chrome dev console while running on a site like fanfiction.net, you can see the number of errors popping up. The errors don't actually break anything, but they do make debugging other things a tad annoying.
I'm fairly confident it covers all situations for your former fix, but I'd like to know if there's any glaring issues or if you think the filter should be handled from a different angle.

To no one in particular: SpacebattlesParser change is more of an irregularity than anything, I'm fine pushing it but the reasons for the throttle are a bit unusual: By default, cloudflare isn't actually an issue on the Xenforo forums, but when requesting 150+ chapters too quickly (Inconsistent number due to caching) they DO request throttling and eventually temp ban. I have not been able to replicate the error since setting to 50ms, however it is trial&error. It's mainly included here because I forgot to check it in when I changed it.

Now I'll get back to looking at 1454 without these errors. whistles innocently

Previous logic attempted to duplicate cookies which were being set by default within chrome's request object. Cookies at issue had specialized settings of "Session" or no_restriction on SameSite.
@gamebeaker
Copy link
Collaborator

gamebeaker commented Sep 10, 2024

@Kiradien i was able to reproduce the error on reader-hub.com, on fanfiction.net i got no error.

I don't know why your fix works but it does.
My two cents:
As the function name is setPartitionCookies() and the original idea was, that we have to set them manually because only they would not be sent with a fetch request the filter could be something like this:
cookies = cookies.filter(item => item.partitionKey != undefined);
with this only the cookies with a partitionKey get set and not the other ones which we wouldn't have to set anyway.
can you set the filter after after if/else so that it is in chrome and firefox the same?
I tested it in firefox nightly and it works.

if (!util.isFirefox()) {
    cookies = await chrome.cookies.getAll({domain: urlparts[urlparts.length-2]+"."+urlparts[urlparts.length-1],partitionKey: {}});
}else{
    cookies = await browser.cookies.getAll({domain: urlparts[urlparts.length-2]+"."+urlparts[urlparts.length-1],partitionKey: {}});
}
cookies = cookies.filter(item => item.partitionKey != undefined);

What is 1454?

@dteviot
Copy link
Owner

dteviot commented Sep 11, 2024

@gamebeaker

What is 1454?

Probably #1454

@Kiradien
Copy link
Collaborator Author

Kiradien commented Sep 11, 2024

with this only the cookies with a partitionKey get set and not the other ones which we wouldn't have to set anyway.
can you set the filter after after if/else so that it is in chrome and firefox the same?

Hmm, glad I asked. That's actually close to the original way I wanted to fix it, but I had actually completely forgotten over a few weeks and ended up digging in the chrome docs like an idiot. I'll play around with this & look for any issues for a bit, but it does seem a better solution.

The reason my fix works is because of how chrome organizes the cookies after updates in the last year; there were a whole bunch of security reasons given, but it basically amounts to "Hey, a whole bunch of cookies are automatically pulled by javascript http requests, but THESE ONES aren't.". Surprisingly, these issues haven't actually been extension exclusive, and have impacted some REST websites using Cloudflare when they came out... But yeah, I'm not exactly happy with the brute-force methodology and string matching.

As for 1454, yeah, dteviot is right... Feel free to ignore that comment, I just ended up pulling myself off that when this kept popping up.

@dteviot dteviot merged commit 07c3f21 into dteviot:ExperimentalTabMode Sep 13, 2024
1 check passed
@dteviot
Copy link
Owner

dteviot commented Sep 13, 2024

@Kiradien @gamebeaker
You don't need my approval to merge changes. If you're both happy, that's more than enough for me.

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.

3 participants