Skip to content

Conversation

@ari-party
Copy link

@ari-party ari-party commented Mar 29, 2025

Description

Updates the code from #19325 to allow multiple hosts in the __VITE_ADDITIONAL_SERVER_ALLOWED_HOSTS environment variable, and trims the http or https in case it is an FQDN. (e.g. https://example.com)

May be better to use the URL API to extract the hostname instead, please let me know if this is preferred! ("The 'URL.canParse' is still an experimental feature and is not supported until Node.js 19.9.0.")

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Would you describe why you need this feature and why setting server.allowedHosts: process.env.MY_ALLOWED_HOSTS.split(',') is not an option for you?

@ari-party
Copy link
Author

@sapphi-red I'm using a Docker container, I don't have access to the source to make that change.
The existing setup of the env var only allowed for one hostname. (even though the env var was named plural)
My changes effectively make it the same as using the config file. Though with the addition of trimming the http(s) making it compatible with platforms like Coolify.

@sapphi-red
Copy link
Member

I'm using a Docker container, I don't have access to the source to make that change.

Would you expand on why you don't have access to the source? Is the image built by others and you don't have contact with the people who built it?

Though with the addition of trimming the http(s) making it compatible with platforms like Coolify.

server.allowedHosts / preview.allowedHosts are only needed to be configured in development. You don't need to configure it when you are deploying it.

@ari-party
Copy link
Author

ari-party commented Mar 31, 2025

Would you expand on why you don't have access to the source? Is the image built by others and you don't have contact with the people who built it?

Exactly that

server.allowedHosts / preview.allowedHosts are only needed to be configured in development. You don't need to configure it when you are deploying it.

The image in question deploys using a development build

@sapphi-red sapphi-red added the p2-to-be-discussed Enhancement under consideration (priority) label Apr 1, 2025
@github-project-automation github-project-automation bot moved this to Discussing in Team Board Apr 1, 2025
@sapphi-red sapphi-red moved this from Discussing to Later in Team Board Apr 16, 2025
@sapphi-red
Copy link
Member

We discussed this feature during our team meeting.

  • Regarding the .map((i) => i.replace(/^https?:\/\//, '')) part: we'd prefer not to include that, as it would cause the environment variable to behave differently from the corresponding option.
  • For the .split(',') part: we think a more robust parsing approach is needed, since hostnames can contain commas. Also, if we're going to introduce this feature, we'd like to make sure the behavior is consistent across similar environment variables — including future ones. That said, we're unsure what the syntax should look like.

@ari-party
Copy link
Author

Could you expand upon the "hostnames can include commas"?

According to RFC 952, (host) names can only contain A-Z, 0-9, minus - and period .

@sapphi-red
Copy link
Member

The URL Standard allows , to be included in the host (you can check it by new URL("http://a,b").host). Also, browsers does not necessarily rely on DNS to resolve the host (it is an implementation-defined operation). So technically, using a comma in the host name of a URL is allowed and may work.

That said, this isn't a major point. There are other options where commas are valid, and we'd like to take those into account as well, to ensure consistent behavior in case we add more environment varibables.

@ari-party
Copy link
Author

Commas are the common seperator for environment variables
Would it be better to support enclosing hostnames in quotes, like: "a,b",example?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p2-to-be-discussed Enhancement under consideration (priority)

Projects

Status: Later

Development

Successfully merging this pull request may close these issues.

2 participants