Ignore empty domain string in ResponseCookie

[34875634567]

Affects v5.2.4.RELEASE, though I think response cookie validation was added in v5.2.x.

We're calling a 3rd party service that returns a Set-Cookie header with a domain="" value. This is non-compliant with rfc6265 so is technically wrong.

However, Spring's now throwing an exception when encountering this header, so the entire request fails and the application is unable to consume the response.

Is this intended behaviour? I think in this case Spring should "be conservative in what it does, be liberal in what it accepts from others".

[rstoyanchev]

What exactly is it returning? validateDomain starts with this which should ignore "":

if (!StringUtils.hasLength(domain)) {
	return;
}

However, Spring's now throwing an exception when encountering this header, so the entire request fails and the application is unable to consume the response.

To my knowledge we are not automatically parsing the cookies. It has to be a call from the application to access the cookies, so whether to parse the cookies and how to treat errors should be in the application's control.

[34875634567]

Set-Cookie: xxx=xxx; Domain=""; Expires=Wed, 10-Mar-2021 16:08:18 GMT; Path=/
So it's parsing "\"\"" I think.

[34875634567]

I'll get the full stacktrace but as far as I know we're not explicitly accessing the cookies.

[rstoyanchev]

So it's parsing "\"\"" I think

This doesn't look right. What client library is this with Reactor Netty or Jetty?

as far as I know we're not explicitly accessing the cookies

Yes please if you could find where it's failing. There might some response copying somewhere (e.g. through a filter) but by default the cookies are only parsed via ClientResponse#cookies.

Ultimately if a server is sending invalid cookies, I don't think we can ignore that either. However the "" looks more like a parsing issue to me. It should result in an empty string probably.

[34875634567]

Could be opentracing reading the cookies:

ERROR 2020-03-10 17:00:24,628 [] [reactor-http-nio-2] r.c.p.Operators: Operator called default onErrorDropped
java.lang.IllegalArgumentException: "": invalid cookie domain char '34'
	at org.springframework.http.ResponseCookie$Rfc6265Utils.validateDomain(ResponseCookie.java:384)
	at org.springframework.http.ResponseCookie.<init>(ResponseCookie.java:72)
	at org.springframework.http.ResponseCookie.<init>(ResponseCookie.java:36)
	at org.springframework.http.ResponseCookie$1.build(ResponseCookie.java:253)
	at org.springframework.http.client.reactive.ReactorClientHttpResponse.lambda$getCookies$4(ReactorClientHttpResponse.java:110)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.HashMap$KeySpliterator.forEachRemaining(HashMap.java:1600)
	at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:658)
	at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:274)
	at java.base/java.util.HashMap$ValueSpliterator.forEachRemaining(HashMap.java:1672)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497)
	at org.springframework.http.client.reactive.ReactorClientHttpResponse.getCookies(ReactorClientHttpResponse.java:103)
	at org.springframework.web.reactive.function.client.DefaultClientResponse.cookies(DefaultClientResponse.java:104)
	at org.springframework.web.reactive.function.client.DefaultClientResponseBuilder.lambda$new$1(DefaultClientResponseBuilder.java:92)
	at org.springframework.web.reactive.function.client.DefaultClientResponseBuilder.cookies(DefaultClientResponseBuilder.java:138)
	at org.springframework.web.reactive.function.client.DefaultClientResponseBuilder.<init>(DefaultClientResponseBuilder.java:92)
	at org.springframework.web.reactive.function.client.ClientResponse.from(ClientResponse.java:227)
	at io.opentracing.contrib.spring.web.client.TracingClientResponseSubscriber.onNext(TracingClientResponseSubscriber.java:81)
	at io.opentracing.contrib.spring.web.client.TracingClientResponseSubscriber.onNext(TracingClientResponseSubscriber.java:35)
	at io.opentracing.contrib.reactor.TracedSubscriber.lambda$onNext$2(TracedSubscriber.java:69)
	at io.opentracing.contrib.reactor.TracedSubscriber.withActiveSpan(TracedSubscriber.java:95)
	at io.opentracing.contrib.reactor.TracedSubscriber.onNext(TracedSubscriber.java:69)
	at reactor.core.publisher.FluxMap$MapSubscriber.onNext(FluxMap.java:114)
	at io.opentracing.contrib.reactor.TracedSubscriber.lambda$onNext$2(TracedSubscriber.java:69)
	at io.opentracing.contrib.reactor.TracedSubscriber.withActiveSpan(TracedSubscriber.java:95)
	at io.opentracing.contrib.reactor.TracedSubscriber.onNext(TracedSubscriber.java:69)
	at reactor.core.publisher.FluxPeek$PeekSubscriber.onNext(FluxPeek.java:192)
	at io.opentracing.contrib.reactor.TracedSubscriber.lambda$onNext$2(TracedSubscriber.java:69)
	at io.opentracing.contrib.reactor.TracedSubscriber.withActiveSpan(TracedSubscriber.java:95)
	at io.opentracing.contrib.reactor.TracedSubscriber.onNext(TracedSubscriber.java:69)
	at reactor.core.publisher.FluxPeek$PeekSubscriber.onNext(FluxPeek.java:192)
	at io.opentracing.contrib.reactor.TracedSubscriber.lambda$onNext$2(TracedSubscriber.java:69)
	at io.opentracing.contrib.reactor.TracedSubscriber.withActiveSpan(TracedSubscriber.java:95)
	at io.opentracing.contrib.reactor.TracedSubscriber.onNext(TracedSubscriber.java:69)
	at reactor.core.publisher.MonoNext$NextSubscriber.onNext(MonoNext.java:76)
	at io.opentracing.contrib.reactor.TracedSubscriber.lambda$onNext$2(TracedSubscriber.java:69)
	at io.opentracing.contrib.reactor.TracedSubscriber.withActiveSpan(TracedSubscriber.java:95)
	at io.opentracing.contrib.reactor.TracedSubscriber.onNext(TracedSubscriber.java:69)
	at reactor.core.publisher.MonoFlatMapMany$FlatMapManyInner.onNext(MonoFlatMapMany.java:242)
	at reactor.core.publisher.Operators$ScalarSubscription.request(Operators.java:2267)
	at reactor.core.publisher.MonoFlatMapMany$FlatMapManyMain.onSubscribeInner(MonoFlatMapMany.java:143)
	at reactor.core.publisher.MonoFlatMapMany$FlatMapManyInner.onSubscribe(MonoFlatMapMany.java:237)
	at reactor.core.publisher.MonoJust.subscribe(MonoJust.java:54)
	at reactor.core.publisher.Flux.subscribe(Flux.java:8186)
	at reactor.core.publisher.MonoFlatMapMany$FlatMapManyMain.onNext(MonoFlatMapMany.java:188)
	at io.opentracing.contrib.reactor.TracedSubscriber.lambda$onNext$2(TracedSubscriber.java:69)
	at io.opentracing.contrib.reactor.TracedSubscriber.withActiveSpan(TracedSubscriber.java:95)
	at io.opentracing.contrib.reactor.TracedSubscriber.onNext(TracedSubscriber.java:69)
	at reactor.core.publisher.FluxRetryPredicate$RetryPredicateSubscriber.onNext(FluxRetryPredicate.java:82)
	at io.opentracing.contrib.reactor.TracedSubscriber.lambda$onNext$2(TracedSubscriber.java:69)
	at io.opentracing.contrib.reactor.TracedSubscriber.withActiveSpan(TracedSubscriber.java:95)
	at io.opentracing.contrib.reactor.TracedSubscriber.onNext(TracedSubscriber.java:69)
	at reactor.core.publisher.MonoCreate$DefaultMonoSink.success(MonoCreate.java:156)
	at reactor.netty.http.client.HttpClientConnect$HttpObserver.onStateChange(HttpClientConnect.java:398)
	at reactor.netty.ReactorNetty$CompositeConnectionObserver.onStateChange(ReactorNetty.java:514)
	at reactor.netty.resources.PooledConnectionProvider$DisposableAcquire.onStateChange(PooledConnectionProvider.java:501)
	at reactor.netty.resources.PooledConnectionProvider$PooledConnection.onStateChange(PooledConnectionProvider.java:413)
	at reactor.netty.http.client.HttpClientOperations.onInboundNext(HttpClientOperations.java:550)
	at reactor.netty.channel.ChannelOperationsHandler.channelRead(ChannelOperationsHandler.java:90)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:377)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:363)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:355)
	at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:102)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:377)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:363)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:355)
	at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:436)
	at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:321)
	at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:308)
	at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:422)
	at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276)
	at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:251)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:377)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:363)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:355)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:377)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:363)
	at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
	at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:163)
	at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:714)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:650)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:576)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:493)
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989)
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:835)

[rstoyanchev]

Yes, OpenTracing is causing cookies to be parsed early. The OpenTracing implementation, which came from Sleuth I think, has room for improvement. I will create tickets for that and link here.

We can also improve how ClientResponse mutation works. I have created #24680 but because it is a more significant change it's set for 5.3.

In the mean time here I think we can check for and ignore "" to avoid this issue.

[34875634567]

Great, thanks for the quick responses.