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

Warn for ws:// and wss:// site and upstream addresses #5755

Closed
francislavoie opened this issue Aug 16, 2023 · 10 comments · Fixed by #5757
Closed

Warn for ws:// and wss:// site and upstream addresses #5755

francislavoie opened this issue Aug 16, 2023 · 10 comments · Fixed by #5757
Labels
feature ⚙️ New feature or request good first issue 🐤 Good for newcomers
Milestone

Comments

@francislavoie
Copy link
Member

I've seen that commonly, users try to use ws:// or wss:// either as site addresses or proxy upstream addresses. Neither of these work in Caddy, they're specific to browsers for use with the new WebSocket(url) JS API.

We should warn/error when those are used in the config, and suggest using http:// or https:// (or omitting the scheme) respectively.

@francislavoie francislavoie added feature ⚙️ New feature or request good first issue 🐤 Good for newcomers labels Aug 16, 2023
@francislavoie francislavoie added this to the 2.7.5 milestone Aug 16, 2023
@singhalkarun
Copy link
Contributor

singhalkarun commented Aug 16, 2023

Hey @francislavoie I would like to take this up. Can you please assign this to me?

@francislavoie
Copy link
Member Author

You can just open up a PR when you have something 👍

@singhalkarun
Copy link
Contributor

I went through the code and was able to understand that an adapter is being used to adapt the config provided and convert it to json. I found there are multiple layers of abstractions which is making it slightly challenging to understand the flow of adaption. Would you like to point out some specific things that I should look for?

@francislavoie @mholt

@mholt
Copy link
Member

mholt commented Aug 16, 2023

@singhalkarun Maybe somewhere around here?

commonScheme = pa.scheme

I wonder if we should just return an error if it's not http or https. Are there reasons to allow other schemes here?

@singhalkarun
Copy link
Contributor

singhalkarun commented Aug 16, 2023

I wonder if we should just return an error if it's not http or https. Are there reasons to allow other schemes here?

I think we should throw an error. If we try to use wss as proxy url prefix in nginx, it also throws error (saying invalid url prefix). Also, I believe we should only allow 3 vaues as scheme, http, https and empty scheme.

Raising a PR as per this.

@francislavoie
Copy link
Member Author

francislavoie commented Aug 16, 2023

We also support h2c://, the docs explain, https://caddyserver.com/docs/caddyfile/directives/reverse_proxy#upstream-addresses

I think what we want is to specifically check for ws and wss and suggest http and https respectively if they use those. The solution for what they're trying to do is obvious, so the error message should tell them exactly what to do.

This is also about the site addresses which is parsed elsewhere in the Caddyfile adapter. I'm on mobile right now so I can't get a link but I'm sure you can find where that's parsed.

@mholt
Copy link
Member

mholt commented Aug 16, 2023

Oh yeah, I forgot about h2c (another preset).

I think what we want is to specifically check for ws and wss and suggest http and https respectively if they use those. The solution for what they're trying to do is obvious, so the error message should tell them exactly what to do.

That sounds good to me.

@singhalkarun
Copy link
Contributor

We also support h2c://, the docs explain, https://caddyserver.com/docs/caddyfile/directives/reverse_proxy#upstream-addresses

I think what we want is to specifically check for ws and wss and suggest http and https respectively if they use those. The solution for what they're trying to do is obvious, so the error message should tell them exactly what to do.

This is also about the site addresses which is parsed elsewhere in the Caddyfile adapter. I'm on mobile right now so I can't get a link but I'm sure you can find where that's parsed.

Got it. As per my understanding, the following changes have to be made,

  1. If someone uses wss or ws specifically, we should send error saying them to use https or http respectively
  2. We should allow https/http/h2c/empty as schemes and if someone uses anything apart from this we should send them error message with invalid prefix (this can be optional, but preferred as someone might have a typo in script in prefix, like if someone uses htps instead of https) <- @francislavoie please let me know your views on this?
  3. Need to add same checks for the site adresses

Please let me know if my understanding is correct before I take this up further.

@mholt
Copy link
Member

mholt commented Aug 16, 2023

@singhalkarun I believe that's correct.

If the scheme is ws or wss, a helpful error message specific to that is preferred.

For every other unrecognized scheme, a general error message saying that scheme isn't recognized.

Yeah, site blocks in the Caddyfile should probably be checked similarly as well. (I don't know if the h2c shortcut works there, though, I'd have to check.)

@francislavoie
Copy link
Member Author

(I don't know if the h2c shortcut works there, though, I'd have to check.)

It does not. Currently only http and https for site addresses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request good first issue 🐤 Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants