Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate cookies in FollowRedirect client middleware. #2623

Merged
merged 8 commits into from Jul 30, 2019

Conversation

@rpiaggio
Copy link
Contributor

commented Jun 5, 2019

Some websites set cookies when issuing a redirect, which are expected to be present when invoking the new URL. This PR takes care of these cases.

@rossabaker
Copy link
Member

left a comment

Thanks, @rpiaggio. Is your redirect going across servers? For security, we have that stripSensitiveHeaders call in there, which removes the Cookie (among a couple others) if being redirected to a different authority. If it's safe in your environment to propagate cookies, you can change the sensitiveHeaderFilter argument. Does that help?

@rpiaggio

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Hi @rossabaker ! Thank you for you review.

The idea here is to propagate into the Cookie of the next request the Set-Cookie received in last response, not cookies present in the original request.

My redirect wasn't even going across servers so the sensitive header logic wasn't the issue, it was not kicking in.

However, due to the sensitive nature of cookies, I did add now logic to prevent the propagation of cookies across servers.

Maybe stripSensitiveHeaders and propagateCookies could be combined into a single fixHeaders (or similarly named) method?

Merge branch 'master' of github.com:http4s/http4s
# Conflicts:
#	client/src/main/scala/org/http4s/client/middleware/FollowRedirect.scala
@rossabaker
Copy link
Member

left a comment

This feels like mixing concerns, as it would be desirable to have a cookie jar middleware that submits relevant cookies for all requests. (I've imagined one based on a Ref. @ChristopherDavenport said something about writing one.) But that middleware would need to be inside FollowRedirect to achieve this functionality: order matters.

What you've created here looks like a more intuitive behavior in the absence of such a cookie jar. But then if there were also a cookie jar, we'd risk creating the cookies twice. Your fixHeaders idea is interesting, and would provide a way to configure this middleware so it would work with or without a cookie jar. I think it would need access to the previous response. It's also maybe less intuitive than separate functions, though they could be composed into one.

I don't have a strong opinion yet, but maybe my rambling makes something click for you. I do agree that the problem you're solving is right.

Merge branch 'master' of github.com:http4s/http4s
# Conflicts:
#	client/src/main/scala/org/http4s/client/middleware/FollowRedirect.scala
@rpiaggio

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

I didn't know about the existing CookieJar functionality. In light of this, I admit my implementation to be a bit naïve.

One possible way to go forward: If there was a CookieJarClient which maintained a mutable CookieJar, we could require the client passed to FollowRedirect to be a CookieJarClient and use its jar's cookies for requests and submit to it new cookies from responses. In other words, a FollowRedirect middleware would always have to be paired to a CookieJarClient.

In the meantime, I've updated the PR. It does solve the problem for me (and it seems it would for other people too) in the absence of a CookieJarClient.

@rossabaker

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

So yes, if we merge this, I think it fixes the problem. There are compile errors after the last merge to address first. I'm inclined to merge this and sort out the issues below once we have a working cookie jar middleware, which @ChristopherDavenport gisted today.

  1. If the cookie jar middleware is outside follow redirect, it won't see cookies set on intermediate redirect responses followed by FollowRedirect. We'd want to be sure to propagate those cookies to the cookie jar for subsequent requests. To do that, FollowRedirect would need to be able to add to the cookie jar, or we'd need to put the cookie jar middleware inside follow redirect.

  2. If the cookie jar middleware is inside follow redirect, the logic added here risks duplicating the cookie. We would either need to assume that there is a cookie jar on the outside, or add deduplication logic to request.addCookie.

rpiaggio added some commits Jul 29, 2019

Merge branch 'master' of github.com:http4s/http4s
# Conflicts:
#	client/src/main/scala/org/http4s/client/middleware/FollowRedirect.scala
Merge branch 'master' of github.com:http4s/http4s
# Conflicts:
#	client/src/main/scala/org/http4s/client/middleware/FollowRedirect.scala
@rpiaggio

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Compile error fixed.

@ChristopherDavenport

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

I worry about adding this, as if we do get a middleware and this isn't behind a flag on the middleware we will see the results you listed above, any way we could make this a param whether to enable this behavior or not?

@rossabaker

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

@rpiaggio had earlier proposed

Maybe stripSensitiveHeaders and propagateCookies could be combined into a single fixHeaders (or similarly named) method?

stripSensitiveHeaders exists elsewhere, and propagateCookies is translating response headers to a request, so I think it makes sense to keep them separate. I think an Option[CookieJar] would be an appropriate config as soon as we have that PR?

I am in favor of this because it fixes a problem we don't have another solution for, with the understanding that a breaking change is likely once the more general solution arrives.

@ChristopherDavenport
Copy link
Member

left a comment

Convinced.

@rossabaker rossabaker merged commit 9adcb54 into http4s:master Jul 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.