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

code-server 4.0.0 behind nginx reports wrong port #4607

Closed
fritterhoff opened this issue Dec 11, 2021 · 15 comments · Fixed by #4631
Closed

code-server 4.0.0 behind nginx reports wrong port #4607

fritterhoff opened this issue Dec 11, 2021 · 15 comments · Fixed by #4631
Assignees
Labels
bug Something isn't working

Comments

@fritterhoff
Copy link
Contributor

Using the following nginx configuration (fragment) in combination the SSL and code server reports port 80 in code-server instead of 443/https?

    location / {
      proxy_pass http://localhost:8080/;
      proxy_set_header Host $host;
      proxy_set_header Upgrade $http_upgrade;
      proxy_set_header Connection upgrade;
      proxy_set_header Accept-Encoding gzip;
    }
@benz0li
Copy link
Contributor

benz0li commented Dec 11, 2021

Same problem with my setup. I'm running code-server via JupyterHub as part of a JupyterLab image with Traefik as a reverse proxy.

@code-asher @jsjoeio @bpmct You may check at https://vscode-r.jupyter.b-data.ch.
ℹ️ I have whitelisted your GitHub accounts for this deployment back in June.

@fritterhoff
Copy link
Contributor Author

Propably completely independent from the proxy. Can reproduce it with Caddy and Apache ;).

@jsjoeio
Copy link
Contributor

jsjoeio commented Dec 13, 2021

@fritterhoff do you mind posting repro steps for Caddy? I have experience with that so it'd be easier for me to investigate

@jsjoeio jsjoeio added the needs-investigation This issue needs to be further investigated label Dec 13, 2021
@fritterhoff
Copy link
Contributor Author

Well pretty simple and without any magic:

  1. Configure Caddy:
mydomain.com {
  reverse_proxy 127.0.0.1:8080
}
  1. Restart caddy
  2. Start code-server
  3. Open code-server in your browser
  4. Recognize that the port in lower left corner is :80 and service-workers don't work

@jsjoeio jsjoeio self-assigned this Dec 13, 2021
@code-asher code-asher assigned code-asher and unassigned jsjoeio Dec 13, 2021
@code-asher code-asher added bug Something isn't working and removed needs-investigation This issue needs to be further investigated labels Dec 13, 2021
@fritterhoff
Copy link
Contributor Author

@code-asher you mention in your last commit "does not work behind reverse proxies unless they send the right headers containing information about the proxied source" which headers would be required?

code-asher added a commit to coder/vscode that referenced this issue Dec 14, 2021
Trying to determine the remote authority on the backend is brittle
because it does not work behind reverse proxies unless they send the
right headers containing information about the proxied source.

We could require users add the relevant configuration or provide the
remote authority via a flag but neither are user-friendly options.

We can make it work out of the box by changing the frontend to make
requests to its current address (which is what we try to set the remote
authority to anyway).  This actually already happens for the most part
except in some UI and logs although recent issues suggest there might be
other problems which should be entirely resolved by setting this on the
frontend.

In other words, the remote authority we set on the backend should never
be used so we set it to something invalid to ensure we notice (the
alternative is to rip it out but that is probably a bigger patch thus
generating more conflicts).

One scenario where we might want to set the remote authority from the
backend is if the frontend is served from a different location than the
backend but that is not supported behavior at the moment.  Even if we
did support this we still cannot determine the authority from the
backend (even for non-proxy scenarios in this case) and would need to
add a flag for it so this change would still be necessary.

coder/code-server#4604
coder/code-server#4607
coder/code-server#4608
@code-asher
Copy link
Member

Forwarded is the best one (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded) followed by X-Forwarded-Proto combined with either X-Forwarded-Host or Host.

@code-asher
Copy link
Member

code-asher commented Dec 14, 2021

There might still be weirdness caused by the code explicitly adding an 80 or 443 to the authority but I think it will work if forwarding headers are set.

@fritterhoff
Copy link
Contributor Author

fritterhoff commented Dec 14, 2021

Mhm okay than the current implementation may also have a bug as caddy sets some headers by default:

By default, Caddy passes thru incoming headers to the backend-including the Host header—without modifications, with two exceptions:

    It adds or augments the X-Forwarded-For header field.
    It sets the X-Forwarded-Proto header field.

So my upper example for caddy should work as expected and provide me an :443 as long as i access it via https.

@benz0li
Copy link
Contributor

benz0li commented Dec 14, 2021

@code-asher Træfik sets X-Forwarded-Proto and X-Forwarded-Host headers (among others) at https://vscode-r.jupyter.b-data.ch.
ℹ️ See https://whoami.b-data.ch for all headers set by default by Træfik.

@code-asher
Copy link
Member

Huh, interesting. There must be something else wrong with the code. I am curious as to the problem but I ended up just removing the logic entirely in favor of just setting it to location.host on the frontend so it should just work regardless of headers now. I might look into it later just to see what went wrong.

@code-asher
Copy link
Member

code-asher commented Dec 14, 2021

I poked into it anyway but it seems like the forwarded headers should work:

> uri = getRemoteAuthority({ headers: { "X-Forwarded-Proto": "https", "X-Forwarded-Host": "vscode-r.jupyter.b-data.ch" }})
> `${uri.hostname}:${uri.port || (uri.protocol === 'https:' ? '443' : '80')}`
'vscode-r.jupyter.b-data.ch:443'

And yet it is actually setting the remote authority to "vscode-r.jupyter.b-data.ch:80".

I guess it is academic at this point but quite confusing. 😕

@code-asher
Copy link
Member

code-asher commented Dec 14, 2021

Ah...the headers appear to be lowercased so they do not match. Not sure if they always come that way or if Express lowercases them. 🤦

So I guess forwarded should work since we have it lowercase but not the X- variants. But as mentioned next version will get rid of all this.

@voxsoftware
Copy link

still same problem

@tjpalanca
Copy link

tjpalanca commented Dec 15, 2021

As a temporary solution just in case this helps anyone, I've just made sure that X-Forwarded-Host returned the https://<code-server-url>:443 (hardcoded the 443 port) and everything works perfectly.

code-asher added a commit to code-asher/code-server that referenced this issue Dec 15, 2021
Fixes coder#3410
Fixes coder#4604
Fixes coder#4607
Fixes coder#4608
Fixes coder#4609

Also has the foundation for
coder#4619.
jsjoeio pushed a commit that referenced this issue Dec 15, 2021
Fixes #3410
Fixes #4604
Fixes #4607
Fixes #4608
Fixes #4609

Also has the foundation for
#4619.
@rrgeorge
Copy link

I fixed this issue in Apache by adding a Forwarded header to my apache config as follows:
RequestHeader set Forwarded "for=%{REMOTE_ADDR}s;host=%{SERVER_NAME}s;proto=https"
Not sure exactly what the syntax is for nginx, but a similar approach should solve it.

ZauberNerd pushed a commit to ZauberNerd/vscode that referenced this issue Dec 23, 2021
Trying to determine the remote authority on the backend is brittle
because it does not work behind reverse proxies unless they send the
right headers containing information about the proxied source.

We could require users add the relevant configuration or provide the
remote authority via a flag but neither are user-friendly options.

We can make it work out of the box by changing the frontend to make
requests to its current address (which is what we try to set the remote
authority to anyway).  This actually already happens for the most part
except in some UI and logs although recent issues suggest there might be
other problems which should be entirely resolved by setting this on the
frontend.

In other words, the remote authority we set on the backend should never
be used so we set it to something invalid to ensure we notice (the
alternative is to rip it out but that is probably a bigger patch thus
generating more conflicts).

One scenario where we might want to set the remote authority from the
backend is if the frontend is served from a different location than the
backend but that is not supported behavior at the moment.  Even if we
did support this we still cannot determine the authority from the
backend (even for non-proxy scenarios in this case) and would need to
add a flag for it so this change would still be necessary.

coder/code-server#4604
coder/code-server#4607
coder/code-server#4608
ZauberNerd pushed a commit to ZauberNerd/vscode that referenced this issue Dec 23, 2021
Trying to determine the remote authority on the backend is brittle
because it does not work behind reverse proxies unless they send the
right headers containing information about the proxied source.

We could require users add the relevant configuration or provide the
remote authority via a flag but neither are user-friendly options.

We can make it work out of the box by changing the frontend to make
requests to its current address (which is what we try to set the remote
authority to anyway).  This actually already happens for the most part
except in some UI and logs although recent issues suggest there might be
other problems which should be entirely resolved by setting this on the
frontend.

In other words, the remote authority we set on the backend should never
be used so we set it to something invalid to ensure we notice (the
alternative is to rip it out but that is probably a bigger patch thus
generating more conflicts).

One scenario where we might want to set the remote authority from the
backend is if the frontend is served from a different location than the
backend but that is not supported behavior at the moment.  Even if we
did support this we still cannot determine the authority from the
backend (even for non-proxy scenarios in this case) and would need to
add a flag for it so this change would still be necessary.

coder/code-server#4604
coder/code-server#4607
coder/code-server#4608
ZauberNerd pushed a commit to ZauberNerd/vscode that referenced this issue Dec 23, 2021
Trying to determine the remote authority on the backend is brittle
because it does not work behind reverse proxies unless they send the
right headers containing information about the proxied source.

We could require users add the relevant configuration or provide the
remote authority via a flag but neither are user-friendly options.

We can make it work out of the box by changing the frontend to make
requests to its current address (which is what we try to set the remote
authority to anyway).  This actually already happens for the most part
except in some UI and logs although recent issues suggest there might be
other problems which should be entirely resolved by setting this on the
frontend.

In other words, the remote authority we set on the backend should never
be used so we set it to something invalid to ensure we notice (the
alternative is to rip it out but that is probably a bigger patch thus
generating more conflicts).

One scenario where we might want to set the remote authority from the
backend is if the frontend is served from a different location than the
backend but that is not supported behavior at the moment.  Even if we
did support this we still cannot determine the authority from the
backend (even for non-proxy scenarios in this case) and would need to
add a flag for it so this change would still be necessary.

coder/code-server#4604
coder/code-server#4607
coder/code-server#4608
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants