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

fix: omit proxy path prefix from socket.io base path #13054

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

bvenreply
Copy link
Contributor

@bvenreply bvenreply commented Nov 5, 2023

What it does

Fixes an issue causing socket.io to use the wrong basepath when accessing the backend from a reverse proxy. #13055

How to test

This can be tested by running the browser example and accessing via a local nginx proxy:

  • Build/run the browser example with yarn && yarn browser build && yarn browser start (Theia will run on port 3000)
  • Run nginx using the following nginx.conf
    http {
        server { 
            listen 3300;
    
            location ~* /proxy/(.*)? {
                proxy_set_header Host $host;
                proxy_set_header X-Real-IP $remote_addr;
                proxy_set_header X-Forwarded-For $remote_addr;
                proxy_set_header Upgrade $http_upgrade;
                proxy_set_header Connection "Upgrade";
                proxy_ssl_verify off;
    
                if ($args) {
                    proxy_pass http://127.0.0.1:3000/$1?$args;
                }
    
                proxy_pass http://127.0.0.1:3000/$1;
            }
        }
    }
    events {}
  • Access theia via http://localhost:3300/proxy/

Follow-ups

Nothing major that i can see.

The first path argument here is now redundant; probably not really worth worrying about considering the severity of the issue.

Review checklist

Reminder for reviewers

@bvenreply bvenreply marked this pull request as ready for review November 5, 2023 19:38
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me. Seems like my recent changes in #12618 accidentally broke this.

@msujew msujew added the websocket issues related to websockets/messaging label Nov 5, 2023
@msujew
Copy link
Member

msujew commented Nov 5, 2023

Before we can merge, you'll need to sign the Eclipse Contributor Agreement (ECA) though. Please do that with the same email that was used to create the commit.

@bvenreply
Copy link
Contributor Author

Before we can merge, you'll need to sign the Eclipse Contributor Agreement (ECA) though. Please do that with the same email that was used to create the commit.

Done @msujew

@msujew
Copy link
Member

msujew commented Nov 5, 2023

@bvenreply Seems like the ECA validation still doesn't go through. Have you signed the ECA with [email protected]? That is the email associated with the commit.

@bvenreply
Copy link
Contributor Author

@msujew I have:

image

Is the revalidation usually instant?

@msujew
Copy link
Member

msujew commented Nov 5, 2023

Not instant, but it's weird it's taking that long. We'll probably see tomorrow ¯\_(ツ)_/¯

@msujew
Copy link
Member

msujew commented Nov 6, 2023

I force pushed on your branch to fix the ECA check. For some reason it didn't work without a new commit. I'll merge this once the CI passes.

@msujew msujew merged commit f33547e into eclipse-theia:master Nov 6, 2023
13 checks passed
@bvenreply bvenreply deleted the fix-socketio-base-path branch November 6, 2023 14:23
vince-fugnitto pushed a commit that referenced this pull request Nov 7, 2023
@vince-fugnitto vince-fugnitto added this to the 1.44.0 milestone Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
websocket issues related to websockets/messaging
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants