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(remix-express): respect expressjs hostname property when behind reverse proxies #7323

Merged
merged 6 commits into from
Oct 23, 2023
Merged

fix(remix-express): respect expressjs hostname property when behind reverse proxies #7323

merged 6 commits into from
Oct 23, 2023

Conversation

rossipedia
Copy link
Contributor

@rossipedia rossipedia commented Aug 31, 2023

@remix-run/express creates request.url directly from the Host HTTP header, but when behind a reverse proxy with app.enable('trust proxy') set, the actual hostname is provided via Express's req.hostname property, which is a "proxy aware" property that parses either Host or X-Forwarded-Host headers depending on the app configuration.

This can cause unexpected behavior when a user requests (for example) https://remix-app.org.com/page, but some condition causes a redirect to be thrown that uses request.url for a returnTo parameter (for instance: auth failure due to a protected resource).

If the app is behind a reverse proxy, and utilizing an internal DNS name (like an AWS internal host name), then that host will be used to create request.url, and you'll get a response with a bad Location header, and will also be leaking internal hostnames externally.

For example:

// server.js
const app = express();
app.enable('trust proxy');
// ... etc ...

// ~/routes/page.tsx
export function loader({request}: LoaderArgs) {
  await authenticator.isAuthenticated(request, {
    failureRedirect: `/login?returnTo=${encodeURIComponent(request.url)}`
  });
}

Request:

GET /page HTTP/1.1
Host: remix-app.internal.us-east.aws.io
X-Forwarded-Host: remix-app.org.com

Response:

HTTP/1.1 302 Found
Location /login?returnTo=http%3A%2F%2Fremix-app.internal.us-east.aws.io%2F <- INCORRECT

Closes: #

  • Docs
  • Tests

Testing Strategy:

Made the change to node_modules/@remix-run/express/dist/server.js and verified that request.url reflected the X-Forwarded-Host header when app.enable('trust proxy') was set in my Express app.

@changeset-bot
Copy link

changeset-bot bot commented Aug 31, 2023

🦋 Changeset detected

Latest commit: a3c2c0e

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

This PR includes changesets to release 16 packages
Name Type
@remix-run/express Patch
@remix-run/serve Patch
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

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

@MichaelDeBoey MichaelDeBoey changed the title fix: respect expressjs hostname property when behind reverse proxies fix(remix-express): respect expressjs hostname property when behind reverse proxies Aug 31, 2023
@brophdawg11
Copy link
Contributor

Oops, sorry this must have fallen off my radar. Do you want to add a changeset and we can get this merged?

@brophdawg11 brophdawg11 self-assigned this Oct 18, 2023
@brophdawg11 brophdawg11 merged commit ef67e12 into remix-run:dev Oct 23, 2023
5 checks passed
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.2.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 2.2.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@rossipedia rossipedia deleted the fix/respect-hostname-when-behind-proxy branch October 31, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants