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.

Sign up for GitHub

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

[breaking] rename xForwardedForIndex to XFF_DEPTH #4332

Merged
merged 10 commits into from
Mar 16, 2022
Merged

[breaking] rename xForwardedForIndex to XFF_DEPTH #4332

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8e1e1ae
[breaking] rename xForwardedForIndex to xForwardedForNumProxies
benmccann Mar 15, 2022
fd06024
add back link to blog post
benmccann Mar 15, 2022
2b12a16
changeset
benmccann Mar 15, 2022
6b584f3
update based on code review
benmccann Mar 16, 2022
d79b35c
update changeset
benmccann Mar 16, 2022
09b1f45
XFF_DEPTH_ENV (#4357)
Rich-Harris Mar 16, 2022
5f595df
Update packages/adapter-node/README.md
Rich-Harris Mar 16, 2022
7af0096
Update packages/adapter-node/README.md
Rich-Harris Mar 16, 2022
4892a18
Update README.md
Rich-Harris Mar 16, 2022
fb3e63e
overhaul readme
Rich-Harris Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/sweet-parents-sell.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/adapter-node': patch
---

[breaking] rename `xForwardedForIndex` to `xForwardedForNumProxies`
14 changes: 4 additions & 10 deletions packages/adapter-node/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,23 +94,17 @@ MY_ORIGINURL=https://my.site \
node build
```

### xForwardedForIndex
### xForwardedForNumProxies
benmccann marked this conversation as resolved.
Show resolved Hide resolved

If the `ADDRESS_HEADER` is `X-Forwarded-For`, the header value will contain a comma-separated list of IP addresses. For example, if there are three proxies between your server and the client, proxy 3 will forward the addresses of the client and the first two proxies:
If the `ADDRESS_HEADER` is `X-Forwarded-For`, the header value will contain a comma-separated list of IP addresses. `xForwardedForNumProxies` should specify how many trusted proxies sit between your server and the client. E.g. if there are three trusted proxies, proxy 3 will forward the addresses of the client and the first two proxies:

```
<client address>, <proxy 1 address>, <proxy 2 address>
```

To get the client address we could use `xForwardedFor: 0` or `xForwardedFor: -3`, which counts back from the number of addresses.
To get the client address we could use `xForwardedFor: 3`.
benmccann marked this conversation as resolved.
Show resolved Hide resolved

**X-Forwarded-For is [trivial to spoof](https://adam-p.ca/blog/2022/03/x-forwarded-for/), howevever**:

```
<spoofed address>, <client address>, <proxy 1 address>, <proxy 2 address>
```

For that reason you should always use a negative number (depending on the number of proxies) if you need to trust `event.clientAddress`. In the above example, `0` would yield the spoofed address while `-3` would continue to work.
If there may be a variable number of proxies, you would have to read the `X-Forwarded-For` header yourself and read the IP address from the left, but be very careful that you do not use the result for any applications with possible security implications since `X-Forwarded-For` is [trivial to spoof](https://adam-p.ca/blog/2022/03/x-forwarded-for/).
Copy link
Member

Choose a reason for hiding this comment

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

Is this a realistic scenario? Surely there's going to be a fixed number of proxies unless you changed your network architecture?

I think it might be worth preserving the previous illustration, because most people aren't going to know the implications of 'read from the left' or 'read from the right' (at least, I wouldn't have known before this week)

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 was thinking that there could be proxies that are not controlled by your own server infrastructure. E.g. some users may have the traffic flow through a corporate proxy at the user's workplace while others wouldn't, so there could always be variable number of proxies. This could probably be worded more clearly

Copy link
Member

Choose a reason for hiding this comment

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

Surely the relevant number isn't the proxies between the server and the client, but the (trusted) proxies between the server and the public internet?

Copy link
Member Author

@benmccann benmccann Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why would that be? Any proxy could be added to the X-Forwarded-For

Sometimes you could have:

<client address>, <trusted proxy address>

And sometimes you could have:

<client address>, <untrusted proxy address>, <trusted proxy address>

In this case it doesn't work to go a set number from the right. And actually now I'm remembering @mrkishi suggesting that we would need to not just use an index, but check the addresses in the list and I'm wondering if we should just remove the ability to lookup by index as it actually does not seem very safe to me.

Copy link
Member

Choose a reason for hiding this comment

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

Using an index is fine, you just need to be careful do it from the right (as we're already doing). If you have an untrusted proxy after a trusted one, you can't trust the header at all, so in that case you shouldn't set the header in order to get remoteAddress instead.

A list of trusted proxy "addresses" is a valid option, but since it was considered too troublesome, we can just let it up to the user since they already have access to the header. Even with a list of trusted proxies, you should still read from the right: keep discarding trusted addresses from the right and use the first untrusted one as the client address.


## Custom server

Expand Down
2 changes: 1 addition & 1 deletion packages/adapter-node/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ interface AdapterOptions {
host?: string;
};
};
xForwardedForIndex?: number;
xForwardedForNumProxies?: number;
}

declare function plugin(options?: AdapterOptions): Adapter;
Expand Down
8 changes: 6 additions & 2 deletions packages/adapter-node/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,16 @@ export default function ({
host: host_header_env = 'HOST_HEADER'
} = {}
} = {},
xForwardedForIndex = -1
xForwardedForNumProxies = 1
} = {}) {
return {
name: '@sveltejs/adapter-node',

async adapt(builder) {
if (xForwardedForNumProxies < 1) {
throw new Error('xForwardedForNumProxies cannot be less than 1');
}

builder.rimraf(out);

builder.log.minor('Copying assets');
Expand All @@ -56,7 +60,7 @@ export default function ({
PROTOCOL_HEADER: JSON.stringify(protocol_header_env),
HOST_HEADER: JSON.stringify(host_header_env),
ADDRESS_HEADER: JSON.stringify(address_header_env),
X_FORWARDED_FOR_INDEX: JSON.stringify(xForwardedForIndex)
X_FORWARDED_FOR_PROXIES: JSON.stringify(xForwardedForNumProxies)
}
});

Expand Down
2 changes: 1 addition & 1 deletion packages/adapter-node/src/handler.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ declare global {
const ADDRESS_HEADER: string;
const HOST_HEADER: string;
const PROTOCOL_HEADER: string;
const X_FORWARDED_FOR_INDEX: number;
const X_FORWARDED_FOR_PROXIES: number;
}

export const handler: Handle;
9 changes: 7 additions & 2 deletions packages/adapter-node/src/handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { getRequest, setResponse } from '@sveltejs/kit/node';
import { Server } from 'SERVER';
import { manifest } from 'MANIFEST';

/* global ORIGIN, ADDRESS_HEADER, PROTOCOL_HEADER, HOST_HEADER, X_FORWARDED_FOR_INDEX */
/* global ORIGIN, ADDRESS_HEADER, PROTOCOL_HEADER, HOST_HEADER, X_FORWARDED_FOR_PROXIES */

const server = new Server(manifest);
const origin = ORIGIN;
Expand Down Expand Up @@ -62,7 +62,12 @@ const ssr = async (req, res) => {

if (address_header === 'x-forwarded-for') {
const addresses = value.split(',');
return addresses[(addresses.length + X_FORWARDED_FOR_INDEX) % addresses.length].trim();
if (X_FORWARDED_FOR_PROXIES > addresses.length) {
throw new Error(
`Received xForwardedForNumProxies of ${X_FORWARDED_FOR_PROXIES}, but only found ${addresses.length} addresses`
);
}
return addresses[addresses.length - X_FORWARDED_FOR_PROXIES].trim();
}

return value;
Expand Down