fix(ip): use RFC 7239 for forwarded header

[wooorm-arcjet]

@arcjet/ip reads from a forwarded header. That header is the standard alternative to x-forwarded-for, for passing several IPs from different services through to an application. However, @arcjet/ip does not support any valid values of that header. Values are a list of elements, each element consisting of key-value pairs.

This PR implements RFC 7239 to support valid values. As this header is intended to include several IPs, of which the right-most IPs could be trusted IPs from proxies, this PR supports that too.

While this adds a bit of code to parse things correctly, I believe it to be worth it, because finding the correct IP is tough, trusted proxies are a bottleneck, and standards are important.

An alternative is to stop reading from forwarded altogether. I think standards are too important though.

[trunk-io]

Merging to main in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

[blaine-arcjet]

I think we should figure out what platforms support this. It may be better to just drop it.

[blaine-arcjet]

What platforms support this? I found a Caddy issue from 5 years ago and they have indicated it is not a high priority.

[wooorm-arcjet]

I saw that vercel supports it correctly.
Yes, removing support for forwarded is also an option.
But it is a standard, and I can see CDNs that folks put in front of where they deploy, supporting these standards

[blaine-arcjet]

Supposedly HAProxy supports the forwarded header. I couldn't find anything conclusive for nginx.

[wooorm-arcjet]

(see this at some point recommended nginx regex: https://redirect.github.com/golang/go/issues/30963#issuecomment-1085057745)

2 reasons not to do forwarded:

• it is unknown what is good if some proxies do x-forwarded-for and others do forwarded; the only good solution is to do both or neither, but that’s not the reality. As nearly everyone does x-forwarded-for, that’s the better choice.
• forwarded has syntax errors, that means a malicious user is able to discard the entire header (https://redirect.github.com/lpinca/forwarded-parse/issues/6, https://adam-p.ca/blog/2022/03/forwarded-header-sabotage/); this can be solved by parsing right-to-left tho (and proxies should discard syntax errors, but probably don’t)

[blaine-arcjet]

These are great points (thanks for including links!). I still have to read about the header sabotage, but I'd like to know your opinion based on this information. Should we remove the forwarded header detection?

[wooorm-arcjet]

I am quite okay with either but have a slight preference for being a good web citizen and supporting standards. On the other hand, it has no practical use yet (vercel also sets x-forwarded-for)

At some points in various conversations @davidmytton mentions that things can go in blog posts.
A bit esoteric but publishing a right-to-left forwarded parser may be a good candidate for that!

[blaine-arcjet]

A bit esoteric but publishing a right-to-left forwarded parser may be a good candidate for that!

💪 Is the parser currently implemented a left-to-right parser with the problems surfaced in the header sabotage blog? What is the effort level of a right-to-left parser?

[wooorm-arcjet]

Currently left-to-right parsing, so the whole thing stops on a syntax error.
I think it’s possible the other way around, it’s still a very strict grammar.
API has to be switched to a generator-like thing though, but that’s not difficult.

[blaine-arcjet]

I propose we make a new (breaking) PR to remove forwarded since it is not handled correctly right now. I'll draft this PR until we have time to implement the right-to-left parser and write the blog post.

[qw-in]

I'm going to close this for now. We may come back to it if there is a customer request or we make a decision to do so and implement it across the sdks.