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

Switch to checking if the conenction header contains 'upgrade' rather than equals 'upgrade' #519

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

Conversation

tomfin46
Copy link

A potential solution to the issue firefox has where it sends both 'keep-alive' and 'upgrade' in the connection header (see #497)

I don't have much experience with C++ or the project so if there's any other changes required or tests that need to be updated/created I'm happy to look into that with some direction

@NoelAbrahams
Copy link

The fix seems to only affect passing through the keep-alive value. I think the underlying problem is that when FireFox sends Connection: keep-alive, Upgrade, IISNode only relays Connection: keep-alive to Node.

@tomfin46
Copy link
Author

I haven't used C++ in quite a while but from what I gathered the reason IISNode is only relaying the keep-alive connection header is because of this line. I believe the check was saying if the connection doesn't exactly equal upgrade then send just keep-alive so if it's changed to the check of just the connection header including upgrade it should pass along both keep-alive and upgrade from firefox.

@ferk6a
Copy link

ferk6a commented Jul 4, 2016

There is one problem with this commit: the original checked for upgrade ignoring case (by using stricmp), and with strstr, you lose the ignore case capability, and there is no stristr in the standard library.

Because I didn't want to introduce a stristr function in my patch, I did this instead.

The reason for it not working for Firefox is not that IISNode relays only "keep-alive", the reason is that firefox sends Upgrade with an uppercase U.

@tomfin46
Copy link
Author

tomfin46 commented Jul 5, 2016

Cheers @Fer22f for the comments. Totally didn't twig about the case insensitive stuff.

Updated based on your idea about splitting the header on the comma and checking each one.

Obviously it's not perfect yet and doesn't seem to follow the styling/conventions of the rest of the project but maybe we can build upon this.

@RayzRazko
Copy link

Hey guys. Is this pull request going to be merged?

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.

5 participants