Skip to content
This repository has been archived by the owner on Apr 18, 2021. It is now read-only.

Skip websocket? #18

Closed
Equim-chan opened this issue Aug 8, 2017 · 10 comments · Fixed by #43
Closed

Skip websocket? #18

Equim-chan opened this issue Aug 8, 2017 · 10 comments · Fixed by #43

Comments

@Equim-chan
Copy link

Equim-chan commented Aug 8, 2017

Cache is not appliable to websocket.

I just tested and it responsed with 502

08/Aug/2017:23:27:41 +0800 [ERROR 502 /ws] *cache.Response is not a hijacker

Maybe apply a filter to detect websocket and then skip it?

@nicolasazrak
Copy link
Owner

Mmm I haven't thought about that. Probably detecting and ignoring them will be the best option, I'll look at it the next few days.

nicolasazrak added a commit that referenced this issue Aug 11, 2017
…When a request is a socket handshake, the proxy middleware tries to hijack the connection, but the http.ResponseWritter is not the original TCP connection

anymore.
@nicolasazrak
Copy link
Owner

@Equim-chan I have deployed a fix that should solve your issue. Could you try it and tell if that worked?

@SimJoSt
Copy link

SimJoSt commented Mar 8, 2018

I just ran into this issue again.
I'm using caddy 0.10.11 as a reverse proxy with Nextcloud and Collabora. The latter uses websockets.
For some reason Chrome worked like a charm but Firefox users could not properly use Collabora to edit files due to 502 errors when accessing the websocket. Appended to the error message, was *cache.Response is not a hijacker.

@SimJoSt
Copy link

SimJoSt commented Mar 8, 2018

I ran into the same issue just now.
Running Caddy 0.10.11 as a reverse proxy with the current Nextcloud and Collabora, Firefox users had issue loading more then the editor ui. The document was missing. Chrome worked fine.
In the log I got 500 errors which had *cache.Response is not a hijackerappended.

@nicolasazrak
Copy link
Owner

@SimJoSt it seems strange, there is a fix for websockets that checks the request headers (https://github.com/nicolasazrak/caddy-cache/blob/master/handler.go#L102), so if there is a connection: upgrade the cache is bypassed. Could you tell me what headers is caddy receiving? Are you using other plugins?

@dstull
Copy link

dstull commented Apr 24, 2019

getting the same when implementing cache directive like so:

cache {
  match_path /assets
  path /tmp
}

websockets only fail when using websockets from firefox (chrome/safari are fine)

24/Apr/2019:17:10:33 +0000 [ERROR 502 /cable] *cache.Response is not a hijacker
10.1.117.27 - - [24/Apr/2019:17:10:33 +0000] "GET /cable HTTP/1.1" 502 56 "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:66.0) Gecko/20100101 Firefox/66.0" cache=miss

removing the cache directive resolves, but would like to dig more and figure out a real fix here

@dstull
Copy link

dstull commented Apr 25, 2019

so I found in this line:

if strings.ToLower(req.Header.Get("Connection")) == "upgrade" && strings.ToLower(req.Header.Get("Upgrade")) == "websocket" {
you are doing an exact match on the 'Connection' header for 'upgrade', which is fine for chrome.

However, in firefox the request header for 'Connection' is actually this
Connection: keep-alive, Upgrade

so perhaps we just need to change this matcher to be a little more relaxed?

fyi - other projects seem to have stumbled onto the same issue: nodejs/http-parser#99

@nicolasazrak
Copy link
Owner

@dstull seems right, I don't have much time right now. Do you want to make a PR?

@dstull
Copy link

dstull commented Apr 25, 2019

hey @nicolasazrak, I believe my colleague @jumanjiman is going to give a PR a shot on this - thanks for the quick response!

jumanjiman added a commit to jumanjiman/caddy-cache that referenced this issue Apr 26, 2019
Resolves nicolasazrak#18.

`Header.Get("Connection")` only returns
the first value of a multi-value header.

Chrome and Safari send:

    Connection: Upgrade

But Firefox sends:

    Connection: keep-alive, Upgrade

Therefore add logic and tests to access the Header map directly
and parse the Connection header for all known cases as described
in go's net/http source code for Request and Header.
@jumanjiman
Copy link
Contributor

hi @nicolasazrak

I opened #43 to resolve this.
I commented on the PR about recent build and test requirements.

jumanjiman added a commit to jumanjiman/caddy-cache that referenced this issue Apr 29, 2019
Resolves nicolasazrak#18.

`Header.Get("Connection")` only returns
the first value of a multi-value header.

Chrome and Safari send:

    Connection: Upgrade

But Firefox sends:

    Connection: keep-alive, Upgrade

Therefore add logic and tests to access the Header map directly
and parse the Connection header for all known cases as described
in go's net/http source code for Request and Header.

New tests show on pass:

    === RUN   TestWebSocketDetection
    --- PASS: TestWebSocketDetection (0.00s)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants