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

http: Fix WebSockets with Firefox #1550

Merged
merged 1 commit into from
Feb 10, 2025
Merged

http: Fix WebSockets with Firefox #1550

merged 1 commit into from
Feb 10, 2025

Conversation

ac000
Copy link
Member

@ac000 ac000 commented Jan 27, 2025

    http: Fix WebSockets with Firefox
    
    Firefox (going back a couple of years at least) was unable to open a
    WebSocket connection to Unit due to it sending a Connection header of
    
      Connection: keep-alive, Upgrade
    
    However in Unit we were expecting only a single value in the header.
    
    Fix the 'Connection' parsing in nxt_h1p_connection() to address this.
    
    Closes: https://github.com/nginx/unit/issues/772
    Signed-off-by: Andrew Clayton <[email protected]>

@ac000 ac000 marked this pull request as ready for review January 27, 2025 18:01
@ac000 ac000 requested review from hongzhidao and avahahn January 27, 2025 18:02
@hongzhidao
Copy link
Contributor

Hi @ac000,
It seems it only works for WS, then do we need to check if the header “Upgrader: websocket” exists?

@ac000
Copy link
Member Author

ac000 commented Feb 6, 2025

True, as the code currently stands we are assuming that we only upgrade to websockets.

I guess strictly speaking we should check that's the case... but I would probably do that as a separate patch to this as this patch simply fixes the problem of determining if we are upgrading the connection.

@ac000
Copy link
Member Author

ac000 commented Feb 6, 2025

@hongzhidao

Actually we do already check the Upgrade header for websocket in nxt_h1p_upgrade() where we set r->proto.h1->upgrade_websocket = 1 if it's found.

If we find upgrade in the Connection header we set r->proto.h1->connection_upgrade = 1

This combination is then checked for in nxt_h1p_header_process()

if (h1p->connection_upgrade && h1p->upgrade_websocket) {

So I think we're all good, right?

@hongzhidao
Copy link
Contributor

hongzhidao commented Feb 7, 2025

Correct.
Btw, is there any other case like “upgrade, close”?
And I feel it's necessary to check the value strictly. I mean we need to check the value like Connection: xkeep-alive, xUpgrade.

@ac000
Copy link
Member Author

ac000 commented Feb 7, 2025

Not sure it makes a lot of sense, but sure, we can do that...

@ac000
Copy link
Member Author

ac000 commented Feb 7, 2025

  • Check for 'close' the same way we do for the others...
$ git range-diff b844e40a...aede3ed6
1:  b844e40a ! 1:  aede3ed6 http: Fix WebSockets with Firefox
    @@ Metadata
      ## Commit message ##
         http: Fix WebSockets with Firefox
     
    -    Firefox (134 at least) was unable to open a WebSocket connection to Unit
    -    due to it sending a Connection header of
    +    Firefox (going back a couple of years at least) was unable to open a
    +    WebSocket connection to Unit due to it sending a Connection header of
     
           Connection: keep-alive, Upgrade
     
    @@ Commit message
     
         Fix the 'Connection' parsing in nxt_h1p_connection() to address this.
     
    -    We keep the check for 'close' basically the same as that really should
    -    be the only value.
    -
    -    Then we check for both of 'keep-alive' and 'upgrade'.
    -
         Closes: https://github.com/nginx/unit/issues/772
         Signed-off-by: Andrew Clayton <[email protected]>
     
    @@ src/nxt_h1proto.c: nxt_h1p_header_buffer_test(nxt_task_t *task, nxt_h1proto_t *h
          nxt_http_request_t  *r;
      
          r = ctx;
    -@@ src/nxt_h1proto.c: nxt_h1p_connection(void *ctx, nxt_http_field_t *field, uintptr_t data)
    -         && nxt_memcasecmp(field->value, "close", 5) == 0)
    -     {
    +     field->hopbyhop = 1;
    + 
    +-    if (field->value_length == 5
    +-        && nxt_memcasecmp(field->value, "close", 5) == 0)
    +-    {
    ++    end = field->value + field->value_length;
    ++
    ++    if (nxt_memcasestrn(field->value, end, "close", 5) != NULL) {
              r->proto.h1->keepalive = 0;
    -+        return NXT_OK;
     +    }
      
     -    } else if (field->value_length == 10
     -               && nxt_memcasecmp(field->value, "keep-alive", 10) == 0)
     -    {
    -+    end = field->value + field->value_length;
    -+
     +    if (nxt_memcasestrn(field->value, end, "keep-alive", 10) != NULL) {
              r->proto.h1->keepalive = 1;
     +    }

Firefox (going back a couple of years at least) was unable to open a
WebSocket connection to Unit due to it sending a Connection header of

  Connection: keep-alive, Upgrade

However in Unit we were expecting only a single value in the header.

Fix the 'Connection' parsing in nxt_h1p_connection() to address this.

Closes: nginx#772
Signed-off-by: Andrew Clayton <[email protected]>
@ac000
Copy link
Member Author

ac000 commented Feb 10, 2025

Rebased with master

$ git range-diff aede3ed6...df62a2a2
-:  -------- > 1:  7b7b29fc ruby: Fix build failures with Ruby 3.4
-:  -------- > 2:  15037822 Fix build with GCC 15
-:  -------- > 3:  c2279c0d python: Fix Litestar WebSockets compatibility
-:  -------- > 4:  b2866409 otel, tools/unitctl: bump the openssl crate from 0.10.68 to 0.10.70
1:  aede3ed6 = 5:  df62a2a2 http: Fix WebSockets with Firefox

@ac000 ac000 merged commit df62a2a into nginx:master Feb 10, 2025
23 of 24 checks passed
@ac000 ac000 deleted the firefox-ws-fix branch February 10, 2025 23:22
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.

2 participants