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

feat: add new allowedHosts option #13278

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

ematipico
Copy link
Member

Changes

Closes #13060

This PR adds a new allowedHosts and --allowed-hosts options.

This options isn't passed to the node adapter yet, we will have to figure out how to use it in the preview server.

Testing

I don't know how to test this functionality, help would be appreciated.

Docs

Will send a PR

Copy link

changeset-bot bot commented Feb 20, 2025

🦋 Changeset detected

Latest commit: 4e03d68

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) docs pr semver: minor Change triggers a `minor` release labels Feb 20, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@ematipico ematipico force-pushed the feat/add-allowed-hosts branch from 7e674ad to 84d8fcf Compare February 20, 2025 14:32
Copy link

codspeed-hq bot commented Feb 20, 2025

CodSpeed Performance Report

Merging #13278 will not alter performance

Comparing feat/add-allowed-hosts (4e03d68) with main (2e1321e)

Summary

✅ 6 untouched benchmarks

@ematipico ematipico added the pr preview This PR has a preview release label Feb 21, 2025
Copy link

pkg-pr-new bot commented Feb 21, 2025

astro

npm i https://pkg.pr.new/astro@13278

@astrojs/cloudflare

npm i https://pkg.pr.new/@astrojs/cloudflare@13278

@astrojs/netlify

npm i https://pkg.pr.new/@astrojs/netlify@13278

@astrojs/node

npm i https://pkg.pr.new/@astrojs/node@13278

@astrojs/vercel

npm i https://pkg.pr.new/@astrojs/vercel@13278

commit: 727c386

Copy link
Member

@florian-lefebvre florian-lefebvre left a comment

Choose a reason for hiding this comment

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

I think my concern is that we are a bit conflicting with the vite option. What happens to it? Is it not used at all anymore? Should we warn if used?

Copy link
Member

@ArmandPhilippot ArmandPhilippot left a comment

Choose a reason for hiding this comment

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

LGTM, I just left some nit related to the doc.

Co-authored-by: Armand Philippot <[email protected]>
@florian-lefebvre florian-lefebvre added this to the 5.4.0 milestone Feb 21, 2025
@ematipico ematipico added - P1: chore Doesn't change code behavior (priority) and removed pr preview This PR has a preview release labels Feb 21, 2025
@ematipico ematipico added - P1: chore Doesn't change code behavior (priority) and removed - P1: chore Doesn't change code behavior (priority) labels Feb 21, 2025
@ematipico ematipico added - P1: chore Doesn't change code behavior (priority) pr preview This PR has a preview release and removed - P1: chore Doesn't change code behavior (priority) pr preview This PR has a preview release labels Feb 21, 2025
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Looks great here, just some small thoughts!

Fun fact, I literally just configured vite.server.allowHosts myself yesterday to allow my dev server to work in Gitpod 😄

Also noting that I didn't notice any new error messages added along with this feature.

* @version 5.4.0
* @description
*
* A list of hostnames that Astro is allowed to respond to. When the value is set to `true`, any
Copy link
Member

Choose a reason for hiding this comment

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

Description looks great!

I noticed that Vite's documentation includes multiple warnings (which hosts can I trust? using true is dangerous and we suggest you don't do it...) Do we feel the need for any of that here?

Otherwise assuming it renders fine in preview, looks good to me!

Copy link
Member Author

Choose a reason for hiding this comment

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

I am a bit conflicted, so I leave the decision to you. My idea was to defer the more "in-depth" description to vite, but happy to change it. Let me know


Adds a new configuration option `server.allowedHosts` and CLI option `--allowed-hosts`.

Now you can specify the hostnames that the dev and preview servers are allowed to respond to. This is useful for...
Copy link
Member

Choose a reason for hiding this comment

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

You're pulling a Matthew and not filling in the blank, @ematipico !! 😂

I think we can put the extra note here in the changelog. If you can fill in the blank, I think it would be a useful addition:

This is useful for/when...

(I just made up a couple of things that might not be the right way of saying it. I just know that I needed to add Gitpod to my allowed list in order to run the dev server this weekend, so that's for sure one use case.)

Suggested change
Now you can specify the hostnames that the dev and preview servers are allowed to respond to. This is useful for...
Now you can specify the hostnames that the dev and preview servers are allowed to respond to. This is useful for allowing additional subdomains, or running the dev server in a web container.
`allowedHosts` checks the Host header on HTTP requests from browsers and if it doesn't match, it will reject the request to prevent CSRF and XSS attacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P1: chore Doesn't change code behavior (priority) docs pr pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aastro preview doesn't respect vite preview allowedHostnames in astro config
6 participants