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 Astro.url.protocol when using the @astrojs/node SSR adapter with HTTPS #5992

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

HiDeoo
Copy link
Member

@HiDeoo HiDeoo commented Jan 26, 2023

Changes

This PR fixes #5890.

When using Astro with the @astrojs/node SSR adapter, Astro.url.protocol is always http: even when the server is running with HTTPS.

When creating the request used during rendering based on the Node.js request, the protocol is currently hardcoded to http:.

This pull request fixes this issue by checking if the Node.js request is using HTTPS and updating the protocol accordingly. To do so, we check if the request socket is either an instance of tls.TLSSocket or if the X-Forwarded-Proto header is set to https.

While testing this, I also realised that when starting the server, the console is always logging a URL with the HTTP protocol, e.g. Server listening on http://127.0.0.1:3000. I decided to include a small change in this PR to log the proper protocol by checking if the server is an instance of https.Server or not. I thought this was not worth a different PR or even mentioning it in the changeset but maybe I was wrong?

Testing

I added new @astrojs/node tests in a url-protocol suite using a fixture rendering a page with Astro.url.protocol and checking the rendered HTML based on the protocol of the Node.js request.

I also manually tested locally this change in a local repro using the file: pnpm protocol and confirmed that logging Astro.url.protocol correctly return https: when running with SERVER_KEY_PATH=./key.pem SERVER_CERT_PATH=./cert.pem node ./dist/server/entry.mjs

Docs

I don't think this change needs to be documented.

@changeset-bot
Copy link

changeset-bot bot commented Jan 26, 2023

🦋 Changeset detected

Latest commit: 299ad49

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) pkg: integration Related to any renderer integration (scope) labels Jan 26, 2023
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

This is a fantastic PR. thank you!

@matthewp matthewp merged commit 60b32d5 into withastro:main Jan 26, 2023
@astrobot-houston astrobot-houston mentioned this pull request Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Astro.url.protocol returning http event when in https
2 participants