Conversation
|
Looks good to me. @xiaoyijun Please take a look, thx |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for applications running behind a reverse proxy by introducing a WithTrustForwardedHeader option that allows the Logto client to trust X-Forwarded-* headers when constructing callback URLs.
Changes:
- Added
WithTrustForwardedHeaderfunctional option to configure whether to trust forwarded headers - Implemented utility functions to extract host, URI, and protocol from forwarded headers
- Modified
HandleSignInCallbackto use forwarded headers when the option is enabled
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| client/client.go | Adds WithTrustForwardedHeader option and trustForwardedHeader field to LogtoClient struct |
| client/util.go | Implements helper functions for extracting URL components from X-Forwarded-* headers |
| client/handle_sign_in_callback.go | Conditionally uses forwarded headers to construct callback URI based on configuration |
| client/util_test.go | Adds unit tests for the new forwarded header utility functions |
Comments suppressed due to low confidence (1)
client/util.go:45
- Security concern: The
getRequestProtocolfunction always trusts theX-Forwarded-Protoheader regardless of thetrustForwardedHeadersetting. This means that even whentrustForwardedHeaderis false (the default), the protocol can be controlled by the client through headers, which could be a security issue.
The function should be refactored to accept a parameter indicating whether to trust forwarded headers, or there should be two separate functions: one that trusts forwarded headers and one that doesn't. When trustForwardedHeader is false, only request.TLS should be checked to determine if HTTPS is being used.
func getRequestProtocol(request *http.Request) string {
if request.TLS != nil {
return "https"
}
proto := request.Header.Get("X-Forwarded-Proto")
if proto != "" {
extractedProto := strings.Split(proto, ",")[0]
return strings.ToLower(strings.Trim(extractedProto, " "))
}
return "http"
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func getForwaredRequestUrl(request *http.Request) string { | ||
| proto := getRequestProtocol(request) | ||
| host := getForwaredRequestHost(request) | ||
| uri := getForwaredRequestRequestUri(request) | ||
| return proto + "://" + host + uri | ||
| } | ||
| func getForwaredRequestHost(request *http.Request) string { | ||
| host := request.Header.Get("X-Forwarded-Host") | ||
| if host != "" { | ||
| return host | ||
| } | ||
| return request.Host | ||
| } | ||
| func getForwaredRequestRequestUri(request *http.Request) string { | ||
| uri := request.Header.Get("X-Forwarded-Url") | ||
| if uri != "" { | ||
| return uri | ||
| } | ||
| return request.RequestURI | ||
| } |
There was a problem hiding this comment.
There's a spelling error in the function names throughout the code. "Forwared" should be spelled "Forwarded" (with two 'd's). This affects:
getForwaredRequestUrlgetForwaredRequestHostgetForwaredRequestRequestUri- Test function names:
TestGetForwaredRequestUrlShouldReturnXForwardedIfPresent,TestGetForwaredRequestHostShouldFallbackToRequestHost, andTestGetForwaredRequestRequestUriShouldFallbackToRequestURI
| func TestGetForwaredRequestUrlShouldReturnXForwardedIfPresent(t *testing.T) { | ||
| req, err := http.NewRequest("GET", "http://example.com/path?query=1", nil) | ||
| assert.Nil(t, err) | ||
| // Ensure RequestURI is set like in real servers | ||
| req.RequestURI = "/path?query=1" | ||
|
|
||
| req.Header.Add("X-Forwarded-Host", "forwarded.example.com") | ||
| req.Header.Add("X-Forwarded-Url", "/forwarded-path?query=2") | ||
| req.Header.Add("X-Forwarded-Proto", "https") | ||
|
|
||
| url := getForwaredRequestUrl(req) | ||
|
|
||
| assert.Equal(t, "https://forwarded.example.com/forwarded-path?query=2", url) | ||
| } | ||
|
|
||
| func TestGetForwaredRequestHostShouldFallbackToRequestHost(t *testing.T) { | ||
| req, _ := http.NewRequest("GET", "http://example.com/", nil) | ||
| req.RequestURI = "/" | ||
|
|
||
| host := getForwaredRequestHost(req) | ||
|
|
||
| assert.Equal(t, "example.com", host) | ||
|
|
||
| req.Header.Add("X-Forwarded-Host", "proxied.example.com") | ||
| host = getForwaredRequestHost(req) | ||
| assert.Equal(t, "proxied.example.com", host) | ||
| } | ||
|
|
||
| func TestGetForwaredRequestRequestUriShouldFallbackToRequestURI(t *testing.T) { | ||
| req, _ := http.NewRequest("GET", "http://example.com/api", nil) | ||
| req.RequestURI = "/api" | ||
|
|
||
| uri := getForwaredRequestRequestUri(req) | ||
| assert.Equal(t, "/api", uri) | ||
|
|
||
| req.Header.Add("X-Forwarded-Url", "/proxied/api?x=1") | ||
| uri = getForwaredRequestRequestUri(req) | ||
| assert.Equal(t, "/proxied/api?x=1", uri) | ||
| } |
There was a problem hiding this comment.
There's a spelling error in the test function names. "Forwared" should be spelled "Forwarded" (with two 'd's). This affects:
TestGetForwaredRequestUrlShouldReturnXForwardedIfPresentTestGetForwaredRequestHostShouldFallbackToRequestHostTestGetForwaredRequestRequestUriShouldFallbackToRequestURI
| func getForwaredRequestUrl(request *http.Request) string { | ||
| proto := getRequestProtocol(request) | ||
| host := getForwaredRequestHost(request) | ||
| uri := getForwaredRequestRequestUri(request) | ||
| return proto + "://" + host + uri | ||
| } |
There was a problem hiding this comment.
The getForwaredRequestUrl function calls getRequestProtocol, which unconditionally trusts the X-Forwarded-Proto header. This means the protocol portion is determined by forwarded headers regardless of the trustForwardedHeader setting.
For consistency and security, when constructing URLs for forwarded requests, the protocol should also respect the forwarded headers setting. Consider creating a separate protocol detection function that accepts a flag to control whether to trust forwarded headers.
| callbackUri := GetOriginRequestUrl(request) | ||
| if logtoClient.trustForwardedHeader { | ||
| callbackUri = getForwaredRequestUrl(request) | ||
| } |
There was a problem hiding this comment.
Missing test coverage: There are no test cases that verify the HandleSignInCallback function correctly uses forwarded headers when trustForwardedHeader is enabled. The existing tests only cover the unit-level utility functions.
Consider adding an integration test that:
- Creates a LogtoClient with
WithTrustForwardedHeader(true) - Calls
HandleSignInCallbackwith a request containingX-Forwarded-Host,X-Forwarded-Url, andX-Forwarded-Protoheaders - Verifies that the callback URI passed to
core.VerifyAndParseCodeFromCallbackUricontains the forwarded values rather than the original request values
| uri := request.Header.Get("X-Forwarded-Url") | ||
| if uri != "" { | ||
| return uri | ||
| } |
There was a problem hiding this comment.
Non-standard header usage: The implementation uses X-Forwarded-Url header, which is not a standard forwarded header. The standard headers typically used by reverse proxies are:
X-Forwarded-Protofor protocolX-Forwarded-Hostfor hostX-Forwarded-PathorX-Original-URIfor the path/URI
X-Forwarded-Url is not widely supported by standard reverse proxies like nginx, Apache, or load balancers. Consider using X-Forwarded-Path or X-Original-URI instead, or document clearly that this requires custom proxy configuration. Alternatively, if the path should remain unchanged, consider using just the Host and Proto headers.
| uri := request.Header.Get("X-Forwarded-Url") | |
| if uri != "" { | |
| return uri | |
| } | |
| uri := request.Header.Get("X-Original-URI") | |
| if uri != "" { | |
| return uri | |
| } | |
| uri = request.Header.Get("X-Forwarded-Path") | |
| if uri != "" { | |
| return uri | |
| } |
| // WithTrustForwardedHeader sets whether to trust X-Forwarded-* headers for checking the request's origin, useful when behind a reverse proxy. | ||
| func WithTrustForwardedHeader(b bool) LogtoClientOption { | ||
| return func(c *LogtoClient) { | ||
| c.trustForwardedHeader = b | ||
| } | ||
| } |
There was a problem hiding this comment.
Security concern: When trustForwardedHeader is enabled, the forwarded headers are trusted without any validation. This could be exploited if the application is not actually behind a properly configured reverse proxy, or if the proxy doesn't strip client-provided X-Forwarded-* headers.
Consider adding:
- Documentation warning that this option should only be enabled when the application is behind a trusted reverse proxy that properly sets and strips forwarded headers
- Optional validation that the forwarded values are well-formed URLs/hosts
- A comment in the code explaining the security implications
Summary
fixed #177
This pull request introduces the
WithTrustForwardedHeaderoption to theNewLogtoClientconstructor. When this option is set totrue, the Logto client will trustX-Forwarded-*headers (e.g.,X-Forwarded-Host,X-Forwarded-Proto) to correctly construct the request's origin URL.This is particularly useful for applications running behind a reverse proxy or load balancer, ensuring that redirect URIs for the sign-in and sign-out processes are generated with the correct public-facing scheme and host.
Usage example
Testing
added unittest cases.
Also I have been tested it in firebase hosting with cloud run environment that described in #177 .
Checklist
[ ].changeset[ ] necessary TSDoc comments