Add x-forwarded-for to PDS-proxied requests to entryway #3553
Conversation
| const result: T | HeadersParam = params ?? { headers: {} } | ||
| if (ip) { | ||
| result.headers['x-forwarded-for'] = ip | ||
| } |
There was a problem hiding this comment.
Wouldn't it be more accurate (in terms of semantics of that header), and easier/efficient, to simply add the req.socket.remoteAddress to the already provided list of x-forwarded-for ? (req.ip is a getter which we could avoid calling by doing this)
Another option would be to use req.ips.join(', ') in order to reveal all the (untrusted) proxies the request went through. Again, just as a matter of respecting the semantics of that header.
The reason it might be relevant to keep all the forwarded IPs is in case of a bad actor using a public proxy service. We would log only the public proxy's ip, and discard the IP of the bad actor (assuming the public proxy properly generates the forwarded for header).
| await ctx | ||
| .serviceAuthHeaders( | ||
| recipientDid, | ||
| ctx.cfg.entryway.did, | ||
| ids.ComAtprotoAdminSendEmail, | ||
| ) | ||
| .then((x) => forwardIp(req, x)), |
There was a problem hiding this comment.
Would it make sense to add dedicated methods that do this?
ctx.entrywayServiceAuthHeaders(req, did, lxm)ctx.entrywayPassthruAuthHeaders(req)
It makes it less likely, in the future, that someone forgets to forward the ip when proxying towards entryway, don't you think ?
There was a problem hiding this comment.
It looks like you missed one/two spot:
atproto/packages/pds/tests/entryway.test.ts
Lines 120 to 124 in 75c05d5
As advertised: adding x-forwarded-for to PDS-proxied requests to entryway.
Can get rid of some minor noise in review by ignoring whitespace changes.