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

Support standard Forwarded header in Node adapter output #11148

Open
Conduitry opened this issue Nov 30, 2023 · 2 comments
Open

Support standard Forwarded header in Node adapter output #11148

Conduitry opened this issue Nov 30, 2023 · 2 comments
Labels
feature request New feature or request p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc. pkg:adapter-node

Comments

@Conduitry
Copy link
Member

Describe the problem

I've recently learned about the existence of the Forwarded header, which is a standard I'd never heard of before, but it apparently replaces the several other X-Forwaded-* headers that have been de facto standards for a while. The Node adapter does not support pulling its protocol, host, and client IP address information from this header.

Describe the proposed solution

This should be something that the user should be able to opt in to via runtime environment variables passed to the built Node application. Determining the API here is probably going to be half the battle.

We should probably have this work in the same "count from the right" way that the X-Forwarded-For header does. At that point, we can take the for, host, and proto values from that segment of the header. I don't know what we should do if the header appears to be malformed. Is that an instant 500 before the request even reaches the rest of the app?

I don't know whether we want to allow Forwarded to be used simultaneously with the other PROTOCOL_HEADER, HOST_HEADER, ADDRESS_HEADER env vars. Probably not?

I don't know how we want parsing the Forwarded header to be enabled. A single FORWARDED_DEPTH env var maybe? Since it's a standard, I don't know how valuable it is to allow people to use a different header name.

Alternatives considered

No response

Importance

nice to have

Additional Information

No response

@Conduitry Conduitry added feature request New feature or request pkg:adapter-node p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc. labels Nov 30, 2023
@benmccann
Copy link
Member

I don't know whether we want to allow Forwarded to be used simultaneously with the other PROTOCOL_HEADER, HOST_HEADER, ADDRESS_HEADER env vars. Probably not?

I think I'd allow that. While unlikely, a server may have multiple load balancers in front of it - some of whom produce Forwarded and some of whom produce X-Forwarded-For, etc. and so it could be valuable to understand both headers. I'd probably ignore the X- variants if Forwarded is present though.

@Conduitry
Copy link
Member Author

I actually no longer need this imminently at work, so I might not be a great champion for this feature now.


For ADDRESS_HEADER (optionally with XFF_DEPTH set), we currently make it a runtime error when the user calls getClientAddress if the header specified in the env var isn't set. We don't silently fall back to the connection's client address, because that's likely to be wrong. Similarly, I don't think we want to fall back from Forwarded to whatever's specified in ADDRESS_HEADER.

For HOST_HEADER and PROTOCOL_HEADER, the situation is somewhat different, because the presence of the headers pointed at by those isn't checked at runtime, as far as I can tell. The protocol just falls back to https, and the host is, I guess, going to fall back to the string undefined.

Also, what really matters is here whether the proxy server layer whose values you want to trust has support for Forwarded or not. If so, you'll then need to know how many load balancers after it also support Forwarded, so you know how many items to count over from the right in the header value. If the proxy server whose values you want to trust does not support Forwarded, then you're never going to want to use the Forwarded value, and are going to want to use the other values instead. I don't think we want to pretend we have a way to deal with nondeterministic things happening on different requests making it to the final server, which is the only situation I can imagine needing to fallback from one thing to another.


Separately: When I was looking at this earlier, I found https://github.com/lpinca/forwarded-parse as a potential library to parse this header, if we're looking for inspiration. Alternatively, we might be able to split it on commas and then parse the relevant piece as though it were a cookie header, which we already have runtime code for doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc. pkg:adapter-node
Projects
None yet
Development

No branches or pull requests

2 participants