mkopylec / charon-spring-boot-starter

Reverse proxy implementation in form of a Spring Boot starter.
Apache License 2.0
248 stars 55 forks source link

existing "X-Forwarded-For" causes crash #102

Closed tlzxsun closed 4 years ago

tlzxsun commented 4 years ago

if there already has a "X-Forwarded-For" header, then in

CommonRequestProxyHeadersRewriter
void rewriteHeaders(HttpHeaders headers, URI uri, Consumer<HttpHeaders> headersSetter) {
        HttpHeaders rewrittenHeaders = copyHeaders(headers);
        List<String> forwardedFor = rewrittenHeaders.get(X_FORWARDED_FOR);
        if (isEmpty(forwardedFor)) {
            forwardedFor = new ArrayList<>(1);
        }
        forwardedFor.add(uri.getAuthority());
        rewrittenHeaders.put(X_FORWARDED_FOR, forwardedFor);
        rewrittenHeaders.set(X_FORWARDED_PROTO, uri.getScheme());
        rewrittenHeaders.set(X_FORWARDED_HOST, uri.getHost());
        rewrittenHeaders.set(X_FORWARDED_PORT, resolvePort(uri));
        headersSetter.accept(rewrittenHeaders);
        log.debug("Request headers rewritten from {} to {}", headers, rewrittenHeaders);
    }

it will add "uri.getAuthority()" for a Collections.UnmodifiableRandomAccessList which will cause unsupportedOperationException

Collections.UnmodifiableCollection
public boolean add(E e) {
            throw new UnsupportedOperationException();
        }
tlzxsun commented 4 years ago

the full route is from ReverseProxyFilter.doFilterInternal->HttpRequestMapper.map->RequestEntity(T body, MultiValueMap<String, String> headers, HttpMethod method, URI url, Type type)->

public HttpEntity(T body, MultiValueMap<String, String> headers) {
        this.body = body;
        HttpHeaders tempHeaders = new HttpHeaders();
        if (headers != null) {
            tempHeaders.putAll(headers);
        }
        this.headers = HttpHeaders.readOnlyHttpHeaders(tempHeaders);
    }
mkopylec commented 4 years ago

Hi, thanks for spotting the bug and fixing it! The build has failed thus, I'll investigate what's going on

mkopylec commented 4 years ago

There is already a test that checks this use case. The test is passing in both, webmvc and webflux modules. The list is not unmodifiable, it is a java.util.LinkedList. What Spring version are you using? Paste your Charon configuration.

tlzxsun commented 4 years ago
<parent>
        <groupId>org.springframework.boot</groupId>
        <artifactId>spring-boot-starter-parent</artifactId>
        <version>1.4.3.RELEASE</version>
</parent>

SpringBoot I am using is 1.4.3, after check the git history I found the issue. In newer versions, RestTemplate copies existing requestHeaders values instead of using the original collections. https://github.com/spring-projects/spring-framework/commit/73a81f98d40eb6f5faa91aceb868db53fae2a94b The detail can be found in this commit. Thanks for your time, this library is great~

mkopylec commented 4 years ago

Fixed in 4.6.0. BTW I thought Charon won't work with Spring Boot 1+ but you're saying that you use Charon with Spring Boot 1.4.3. Well, that's good I guess.

tlzxsun commented 4 years ago

Fixed in 4.6.0. BTW I thought Charon won't work with Spring Boot 1+ but you're saying that you use Charon with Spring Boot 1.4.3. Well, that's good I guess.

It's better than good😁,it's great~