Skip to content

Conversation

@rwinch
Copy link
Member

@rwinch rwinch commented Apr 25, 2016

Previously MockWebResponseBuilder would use the cookie domain of the cookie
received from MockMvc response. This caused problems because the cookie
domain can be null, but HtmlUnit forbids a null domain.

This commit defaults the domain of the cookie to the domain of the
WebRequest.

Issues SPR-14169

@rwinch
Copy link
Member Author

rwinch commented Apr 25, 2016

cc @rstoyanchev @sbrannen

@sbrannen
Copy link
Member

@rwinch,

Thanks for the quick work!

I haven't cloned your PR locally to try it out, but the implementation looks solid, and the test reflects what I think one would expect (i.e., that the cookie domain defaults to the request domain).

So, from my perspective, feel free to merge it.

@rstoyanchev
Copy link
Contributor

I agree but either way I'll do that before RC2. Thanks a lot Rob!

@rwinch
Copy link
Member Author

rwinch commented Apr 25, 2016

@sbrannen @rstoyanchev

Thanks for the feedback. I decided to improve the test by only setting required fields (i.e. fields populated by the constructor) on the Cookie. This will help to ensure we don't have problems for any other fields. I have pushed those changes to the branch.

@chrylis
Copy link

chrylis commented Apr 25, 2016

Is the current-request-host guaranteed to be just localhost for requests on this path? If not, a cookie returned from request to foo.example.com without a domain set will be valid for *.example.com.

Previously MockWebResponseBuilder would use the cookie domain of the cookie
received from MockMvc response. This caused problems because the cookie
domain can be null, but HtmlUnit forbids a null domain.

This commit defaults the domain of the cookie to the domain of the
WebRequest.

Issues SPR-14169
@rwinch
Copy link
Member Author

rwinch commented Apr 25, 2016

Is the current-request-host guaranteed to be just localhost for requests on this path?

The host is the value that the user passed into MockMvc and can be any value.

If not, a cookie returned from request to foo.example.com without a domain set will be valid for *.example.com.

Previously I didn't see a way around this. I found a way to work around the problem. See the updated PR.

@rstoyanchev
Copy link
Contributor

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants