Revise cookies support with Apache HTTP Components in WebClient and WebTestClient

[MFAshby]

Steps to reproduce

• Setup Spring WebTestClient using apache httpcomponents as the underlying HTTP client library.
• Send a request with some cookies
• Read cookies from the response

Expected bevaviour:

only cookies which were returned from a 'Set-Cookie' header in the HTTP response are found when calling org.springframework.test.web.reactive.server.ExchangeResult#getResponseCookies.

Actual behaviour:

cookies set on the request are also present in getResponseCookies, with spurious 'Path' metadata, even though no such cookies were returned from the server.

It seems like the problem is in org.springframework.http.client.reactive.HttpComponentsClientHttpRequest#applyCookies: this is creating entries in the cookieStore with 'path' and 'domain' metadata set to whatever we are currently requesting.

	@Override
	protected void applyCookies() {
		if (getCookies().isEmpty()) {
			return;
		}

		CookieStore cookieStore = this.context.getCookieStore();

		getCookies().values()
				.stream()
				.flatMap(Collection::stream)
				.forEach(cookie -> {
					BasicClientCookie clientCookie = new BasicClientCookie(cookie.getName(), cookie.getValue());
					clientCookie.setDomain(getURI().getHost());
					clientCookie.setPath(getURI().getPath());
					cookieStore.addCookie(clientCookie);
				});
	}

Then this same cookieStore then used to store response cookies; see org.springframework.http.client.reactive.HttpComponentsClientHttpResponse#getCookies:

	@Override
	public MultiValueMap<String, ResponseCookie> getCookies() {
		LinkedMultiValueMap<String, ResponseCookie> result = new LinkedMultiValueMap<>();
		this.context.getCookieStore().getCookies().forEach(cookie ->
				result.add(cookie.getName(),
						ResponseCookie.fromClientResponse(cookie.getName(), cookie.getValue())
								.domain(cookie.getDomain())
								.path(cookie.getPath())
								.maxAge(getMaxAgeSeconds(cookie))
								.secure(cookie.isSecure())
								.httpOnly(cookie.containsAttribute("httponly"))
								.sameSite(cookie.getAttribute("samesite"))
								.build()));
		return result;
	}

I can prepare a simple example project shortly.

[MFAshby]

Furthermore the WebTestClient API WebTestClient.RequestHeadersSpec#cookies doesn't expose the cookies that are supplied from apache httpcomponents default cookie store, contrary to what the documentation comment suggests. This prevents the user from having full control over cookies:

		/**
		 * Manipulate this request's cookies with the given consumer. The
		 * map provided to the consumer is "live", so that the consumer can be used to
		 * {@linkplain MultiValueMap#set(Object, Object) overwrite} existing header values,
		 * {@linkplain MultiValueMap#remove(Object) remove} values, or use any of the other
		 * {@link MultiValueMap} methods.
		 * @param cookiesConsumer a function that consumes the cookies map
		 * @return this builder
		 */
		S cookies(Consumer<MultiValueMap<String, String>> cookiesConsumer);

[rstoyanchev]

You raise a good point that is more general to our Apache HttpClient support for WebClient an WebTestClient.

For client side cookies we only aim to facilitate setting the request, and reading the response, but not to store them especially across requests or paths and domains. In that sense, I'm not sure that our use of the CookieStore is a good fit, and can lead to surprises and side effects.

HttpComponentsClientConnector obtains an HttpClientContext with the CookieStore per request in . If that is a new instance per request, then request cookies work as expected, but getting the response cookies from the store is surprising if the expectation is the actual SET_COOKIE header on the response. If HttpClientContext (and the CookieStore it holds) is re-used across requests, then the store contains cookies for different paths and domains, and having its full content returned from the client response is plainly wrong.

For comparison, we recently added support for cookies to RestClient #33697 where simply serialize cookies to request headers. We can adjust to do something similar in the reactive package, which would be a breaking change.

[MFAshby]

Thanks @rstoyanchev ! This is great stuff.

I'm not sure that our use of the CookieStore is a good fit, and can lead to surprises and side effects.

I'd note that without the user explicitly disabling cookie management in the underlying HttpClient (see HttpClientBuilder#disableCookieManagement) that the cookiestore is still used by default and therefore cookies received in previous requests may be sent in a new one without explicitly adding them. Should WebClient enforce disabling cookie management, or log a warning if cookie management is enabled?

I left a couple of comments on the implementation also;

37243f4#r148896291

37243f4#r148896413

[rstoyanchev]

Re-opening to address the feedback.

[rstoyanchev]

About the CookieStore, I don't think we need to get in the way of someone wanting to use that vs passing them on every request. I see, however, our HttpComponentsClientHttpConnector sets up a BasicCookieStore by default, and that should not longer be necessary. I will remove it.

I also responded on the commit comments.