feat: add Forwarded header parsing and real IP extraction with tests #2744
Conversation
|
cc : @aldas |
|
@aldas fixed lint error |
|
cc: @aldas |
1 similar comment
|
cc: @aldas |
| @@ -40,6 +40,8 @@ type Context interface { | |||
| // Scheme returns the HTTP protocol scheme, `http` or `https`. | |||
| Scheme() string | |||
|
|
|||
| SchemeForwarded() *Forwarded | |||
There was a problem hiding this comment.
Adding new SchemeForwarded method to Context interface is a backwards incompatible change. It has to be some utility function or specific IPExtractor implementation
| func (c *context) RealIP() string { | ||
| if c.echo != nil && c.echo.IPExtractor != nil { | ||
| return c.echo.IPExtractor(c.request) | ||
| } | ||
| // Check if the "Forwarded" header is present in the request. |
There was a problem hiding this comment.
After seeing how complex or how less understood Forwarded header is - I think it is better not to add it directly to RealIP method. It is better suited as separate IPExtractor implementation
| func (c *context) SchemeForwarded() *Forwarded { | ||
| // Parse and get "Forwarded" header. | ||
| // See : https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded | ||
| if scheme := c.request.Header.Get(HeaderForwarded); scheme != "" { |
There was a problem hiding this comment.
This only parses single Header. From mozilla article I understand that you can have actually multiple Forwarded Headers in request.
RFC says https://datatracker.ietf.org/doc/html/rfc7239
A proxy server that wants to add a new "Forwarded" header field value
can either append it to the last existing "Forwarded" header field
after a comma separator or add a new field at the end of the header
block.
Mozilla rephrases this as
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded
If there are multiple proxy servers between the client and server, they may each specify their own forwarding information. This can be done by adding a new Forwarded header to the end of the header block, or by appending the information to the end of the last Forwarded header in a comma-separated list.
There is Nginx issue https://trac.nginx.org/nginx/ticket/1316 that has example for multiple headers in request
|
|
||
| func (c *context) parseForwarded(input string) (Forwarded, error) { | ||
| forwarded := Forwarded{} | ||
| entries := strings.Split(input, ",") |
There was a problem hiding this comment.
To be honest - I do not understand when is semicolon used and when is colon used.
Mozilla documentation says that Directives are key=value pairs, separated by a semicolon. and This can be done by adding a new Forwarded header to the end of the header block, or by appending the information to the end of the last Forwarded header in a comma-separated list.
Can semicolon and comma be mixed?
There was a problem hiding this comment.
Additionally - what about quoted strings that contain comma or semicolon?
RFC says that field value can be value = token / quoted-string where
These 2 are defined as
token = <Defined in [RFC7230], Section 3.2.6>
quoted-string = <Defined in [RFC7230], Section 3.2.6>
I think rfc7230 section-3.2.6 allows unescaped commas in quoted field value
| } | ||
|
|
||
| key := strings.TrimSpace(parts[0]) | ||
| value, err := url.QueryUnescape(strings.TrimSpace(parts[1])) |
There was a problem hiding this comment.
Is url.QueryUnescape correct by rfc7230 section-3.2.6?
RFC says that value is defined as:
forwarded-pair = token "=" value
value = token / quoted-string
token = <Defined in [RFC7230], Section 3.2.6>
quoted-string = <Defined in [RFC7230], Section 3.2.6>
|
This probably be better off as separate library |
For issue #2694