Allow case-insensitive HTTP Upgrade header #2097

ENT8R · 2021-04-04T13:07:28Z

As far as I understand Section 11.2 of RFC 6455 ("The WebSocket Protocol"), the HTTP Upgrade header should actually be WebSocket. At least Firefox uses the header like that, which leads to a wrong detection of the underlying protocol in Sanic (e.g. timeouts after a minute because Sanic thinks the request is a normal HTTP request). This is why I propose to make the check for the Upgrade header case-insensitive in order to correctly handle the incoming request.

codecov · 2021-04-04T13:09:58Z

Codecov Report

Merging #2097 (1fa4832) into main (93a0246) will not change coverage.
The diff coverage is 100.000%.

@@            Coverage Diff            @@
##              main     #2097   +/-   ##
=========================================
  Coverage   92.253%   92.253%           
=========================================
  Files           38        38           
  Lines         3485      3485           
  Branches       583       583           
=========================================
  Hits          3215      3215           
  Misses         183       183           
  Partials        87        87

Impacted Files	Coverage Δ
sanic/request.py	`97.712% <ø> (ø)`
sanic/http.py	`77.491% <100.000%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93a0246...1fa4832. Read the comment docs.

ashleysommer

The scheme check: headers is not defined, you should use self.headers.

ashleysommer · 2021-04-04T13:36:36Z

Looks great. I think it might've been done this way (with just lowercase "websockets") because the Headers Class is a CIMultidict, meaning we don't normally need to worry about case. But of course that only applies to the key names, not the values. So when checking for specific values, we do need to convert case.

Thanks for this fix. I'm surprised we havent had reports of broken websockets in Sanic v21.3 due to this.

ashleysommer · 2021-04-04T13:42:10Z

Ah, type checking has picked up a valid error case.

headers.get('upgrade') could return None, and you can't do .lower() on None.

I know it's a pain and an extra step, but you probably need to put a and headers.get('upgrade', None) is not None in there too.

ahopkins · 2021-04-04T13:43:13Z

I agree. I am surprised as well since this has certainly been tested and out in usage already. Which kind of makes me curious as to why we haven't seen this already. Ideally I'd also like to have a test coverage for it, but that might be hard to do with our current test client.

ENT8R · 2021-04-04T15:01:20Z

Thank you for the fast response and quick review.
At least in my case, the code continued to work, but I was seeing lots of timeouts and websocket reconnections every 60 seconds. The behavior was definitely not wanted but didn't seem to cause other effects besides the reconnections. Also, if I wouldn't have checked the logs, I would have probably never seen this bug...

harshanarayana · 2021-04-04T15:42:54Z

Interesting. I did a bit of digging.

In the RFC the sample always shows

        GET /chat HTTP/1.1
        Host: server.example.com
        Upgrade: websocket
        Connection: Upgrade
        Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==
        Origin: http://example.com
        Sec-WebSocket-Protocol: chat, superchat
        Sec-WebSocket-Version: 13

So does the documentation of the Mozilla Dev page.

Running a quick inspect on Websocket using Firefox indicates the same.

This is the same behavior on chrome as well.

harshanarayana · 2021-04-04T15:46:01Z

sanic/http.py

@@ -218,7 +218,7 @@ async def http1_request_header(self):
            raise InvalidUsage("Bad Request")

        headers_instance = Header(headers)
-        self.upgrade_websocket = headers_instance.get("upgrade") == "websocket"
+        self.upgrade_websocket = "upgrade" in headers_instance and headers_instance.get("upgrade", None) is not None and headers_instance.get("upgrade").lower() == "websocket"


If we really want to do this check the easier option might be

Suggested change

self.upgrade_websocket = "upgrade" in headers_instance and headers_instance.get("upgrade", None) is not None and headers_instance.get("upgrade").lower() == "websocket"

self.upgrade_websocket = headers_instance.get("upgrade", "").lower() == "websocket"

I like this better

ashleysommer · 2021-04-05T02:18:57Z

I don't know whats going on with those travis tests. It should be passing now. I'll try to restart them.

ashleysommer · 2021-04-05T02:25:42Z

Nope, it won't let me restart it. (Error: "Oops, this Job cannot be Restarted.")

I think we can just merge this regardless.

On a different note, for @sanic-org/core-devs
I'm wondering if we should try to avoid using headers.get('key') in our codebase.
In the CIMultidict that holds our headers, each key can have multiple values. Thats why the .getone() and .getall() methods exist. From the Multidict Docs, the .get() method simply gets the first value, and has implicit None as default. Doing .get('key') is the same as .getone('key', None). I wonder if we should be more explicit with the intention in the code and change all occurrences of .get('key') to .getone('key', None).

harshanarayana · 2021-04-05T04:08:07Z

I don't know whats going on with those travis tests. It should be passing now. I'll try to restart them.

Those tests I've been really flaky lately. Especially the py3{8,9}-no-ext and like you noticed, Travis restart tests aren't working anymore. 😥

harshanarayana

Approving this change. But I do like the idea of using getone as suggested by @ashleysommer.. May be it can be a total cleanup work across the board using a different issue item?

ahopkins

Looks like there is no new line at the end of the files. It should be failing linting. Can you run make pretty to make sure that it complies with black. You can also run tox -e lint to make sure that everything is formatted properly.

ahopkins · 2021-04-05T06:43:34Z

+1 the explicit usage of getone and getall as a separate PR across the board.

ENT8R · 2021-04-05T10:35:15Z

Looks like there is no new line at the end of the files

Are you sure that this is the actual cause of the failing tests? As far as I see, there is generally no new line at the end of files in this project. I feel like this is more related to #2092 because it's only py39-no-ext which fails to build... I ran make pretty and the only complaint was related to the line length. Let's see what Travis thinks about it 🙃

ENT8R · 2021-04-05T10:42:29Z

Looks like Travis has no complaints this time 🎉

harshanarayana · 2021-04-05T11:15:57Z

+1 the explicit usage of getone and getall as a separate PR across the board.

Added a tracker item for this

ahopkins · 2021-04-05T11:18:10Z

+1 the explicit usage of getone and getall as a separate PR across the board.

Added a tracker item for this

#2098

Allow case-insensitive HTTP Upgrade header

bad35b9

ENT8R requested a review from a team as a code owner April 4, 2021 13:07

Allow case-insensitive Upgrade header when checking the scheme

bb03b22

ashleysommer reviewed Apr 4, 2021

View reviewed changes

Fix reference to headers

7218e2c

ashleysommer previously approved these changes Apr 4, 2021

View reviewed changes

Add None check

29571ad

ENT8R dismissed ashleysommer’s stale review via 29571ad April 4, 2021 14:50

harshanarayana requested changes Apr 4, 2021

View reviewed changes

ENT8R added 2 commits April 4, 2021 23:58

Simplify HTTP Upgrade checks

9368542

Fix newlines at end of file

d927e42

harshanarayana previously approved these changes Apr 5, 2021

View reviewed changes

ahopkins requested changes Apr 5, 2021

View reviewed changes

Run make pretty

1fa4832

ENT8R dismissed harshanarayana’s stale review via 1fa4832 April 5, 2021 10:32

harshanarayana approved these changes Apr 5, 2021

View reviewed changes

harshanarayana mentioned this pull request Apr 5, 2021

Cleanup Usage of CIMultidict #2098

Closed

ahopkins approved these changes Apr 5, 2021

View reviewed changes

ahopkins merged commit eba7821 into sanic-org:main Apr 5, 2021

ENT8R deleted the patch-1 branch April 5, 2021 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow case-insensitive HTTP Upgrade header #2097

Allow case-insensitive HTTP Upgrade header #2097

ENT8R commented Apr 4, 2021

codecov bot commented Apr 4, 2021 •

edited

Loading

ashleysommer left a comment

ashleysommer commented Apr 4, 2021 •

edited

Loading

ashleysommer commented Apr 4, 2021

ahopkins commented Apr 4, 2021

ENT8R commented Apr 4, 2021

harshanarayana commented Apr 4, 2021 •

edited

Loading

harshanarayana Apr 4, 2021 •

edited

Loading

ahopkins Apr 4, 2021

ashleysommer commented Apr 5, 2021

ashleysommer commented Apr 5, 2021 •

edited

Loading

harshanarayana commented Apr 5, 2021

harshanarayana left a comment

ahopkins left a comment

ahopkins commented Apr 5, 2021

ENT8R commented Apr 5, 2021

ENT8R commented Apr 5, 2021

harshanarayana commented Apr 5, 2021

ahopkins commented Apr 5, 2021

	self.upgrade_websocket = "upgrade" in headers_instance and headers_instance.get("upgrade", None) is not None and headers_instance.get("upgrade").lower() == "websocket"
	self.upgrade_websocket = headers_instance.get("upgrade", "").lower() == "websocket"

Allow case-insensitive HTTP Upgrade header #2097

Allow case-insensitive HTTP Upgrade header #2097

Conversation

ENT8R commented Apr 4, 2021

codecov bot commented Apr 4, 2021 • edited Loading

Codecov Report

ashleysommer left a comment

Choose a reason for hiding this comment

ashleysommer commented Apr 4, 2021 • edited Loading

ashleysommer commented Apr 4, 2021

ahopkins commented Apr 4, 2021

ENT8R commented Apr 4, 2021

harshanarayana commented Apr 4, 2021 • edited Loading

harshanarayana Apr 4, 2021 • edited Loading

Choose a reason for hiding this comment

ahopkins Apr 4, 2021

Choose a reason for hiding this comment

ashleysommer commented Apr 5, 2021

ashleysommer commented Apr 5, 2021 • edited Loading

harshanarayana commented Apr 5, 2021

harshanarayana left a comment

Choose a reason for hiding this comment

ahopkins left a comment

Choose a reason for hiding this comment

ahopkins commented Apr 5, 2021

ENT8R commented Apr 5, 2021

ENT8R commented Apr 5, 2021

harshanarayana commented Apr 5, 2021

ahopkins commented Apr 5, 2021

codecov bot commented Apr 4, 2021 •

edited

Loading

ashleysommer commented Apr 4, 2021 •

edited

Loading

harshanarayana commented Apr 4, 2021 •

edited

Loading

harshanarayana Apr 4, 2021 •

edited

Loading

ashleysommer commented Apr 5, 2021 •

edited

Loading