7

In the CSRF implementation of Spring Security (https://github.com/spring-projects/spring-security/blob/master/web/src/main/java/org/springframework/security/web/csrf/CsrfAuthenticationStrategy.java#L57) they first "delete" the XSRF-TOKEN-Cookie (Line 58) and directly after that they set a new one (Line 60).

This leads to two Set-Cookie Header on the same requests:

Set-Cookie: XSRF-TOKEN=; Max-Age=0; Expires=Thu, 01-Jan-1970 00:00:10 GMT; Path=/abc
Set-Cookie: XSRF-TOKEN=35ed383f-4ea1-4e00-8e5d-9bc9a3740498; Path=/abc
  • Can someone explain why this is done in this way?
  • Is this a common pattern?
  • Are there any problems with browsers as they got two Headers for the same Cookie?
waXve
  • 173
  • 4

2 Answers2

6

It looks like it's just a bug.

By looking at the history of the function onAuthentication, we can see that the first implementation only had the line:

this.csrfTokenRepository.saveToken(null, request, response);

Then, it was refactored by adding:

CsrfToken newToken = this.csrfTokenRepository.generateToken(request);
this.csrfTokenRepository.saveToken(newToken, request, response);

The first line with saveToken(null, ...) was never removed, despite serving no purpose.

In a comment, AlphaD pointed to a comment on a github issue explaining why the first line was kept:

it is first invalidating the cookie and then setting the cookie. That is expected if you log out and then try and use a token

Which should not be needed because the second Set-Cookie: should overwrite the previous cookie, thus invalidating it. In fact, this "obvious" behavior is not specified, so browsers are also free to discard the second declaration of a cookie with a conflicting name, which would break the expected behavior by the Spring developers.

Indeed, RFC6265 asks not ot use to Set-Cookie: with the same cookie-name (here XSRF-TOKEN), so this behavior is a pattern that is explicitly discouraged:

Servers SHOULD NOT include more than one Set-Cookie header field in the same response with the same cookie-name. (See Section 5.2 for how user agents handle this case.)

Since this pattern should not happen and the RFC does not provide further recommendations, browsers are free to interpret it how they wish. Compatibility breaks with some browsers could happen at any time if they update their algorithm.

Community
  • 1
A. Hersean
  • 10,046
  • 3
  • 28
  • 42
  • I disagree with the last paragraph. Just because it is discouraged does not necessitate that it is undefined behavior the spec could still say servers should not send cookies with the same name, but if they do, the first encountered instance should be used. –  Dec 14 '20 at 11:28
  • @MechMK1 But it is indeed undefined behavior. I've read the RFC and the behavior is both discouraged and undefined. If you disagree, please quote the the part of the RFC that defines the behavior, maybe I have misread it. The RFC does not suggest or mandate to use the first or the last duplicated entry. – A. Hersean Dec 14 '20 at 12:52
  • No no, I don't doubt that in this instance, it is undefined behavior. My gripes were with the phrase "Since this should not happen, browsers are free to interpret it however they wish", as to me, it implies a more general statement than really is the case. Maybe I'm reading too much into this - nevermind :D –  Dec 14 '20 at 12:56
  • @MechMK1 Oh, yeah, I see what you mean. You are right in the general case, but in this case there's no further indication so browser are indeed free to do as they wish. I'll try to rewrite it. – A. Hersean Dec 14 '20 at 13:14
  • Also great answer, much better than mine –  Dec 14 '20 at 13:44
3

So, since nobody is coming up with a better answer, I did some digging. Note that this is not an authoritative answer, so take everything with a grain of salt.

Why is it done this way?

I assume that it is either Cargo Cult or a case of "If it ain't broke, don't fix it". The line that clears the CSRF token was present since the commit that added CSRF support back in 2013.

In March this year, GitHub user AnastasiaBlack noticed that two CSRF-TOKEN cookies were being saved, to which the code author rwinch helpfully replied that she should ask on StackOverflow instead.

Is this a common pattern?

No. Browsers are designed to overwrite cookies when a new cookie with the same name is received. Explicit "clearing" is not necessary.

While this is more of a personal anecdote than some official guideline, I can say that I have tested my fair share of web applications and receiving two cookies with the same name, one with an empty value and one with a token, is certainly not something I have encountered outside Spring applications.

The OWASP Cheatsheet for Cross-Site Request Forgery Prevention also does not explicitly mention to do that, although they do recommend using Spring Security.

Are there problems with browsers when they get the same cookie twice?

It depends™

Browsers behave differently from each other. This answer from 2014 references RFC 6265, which states that browsers SHOULD sort the cookie list by some order and that servers SHOULD NOT rely upon the order of cookies being sent.

So it means technically a browser could pick one of them randomly and it would conform to RFC 6265, though of course no browser would do this unless it was written to be maliciously compliant.

Since it "seems to work", there is no reason for the maintainers of Spring to change the code that is already working. However, if browsers decided to react differently to doubly-set cookies, that could break compatibility, and would need them to change the code.

Community
  • 1
  • Just as a reference, the code author gave a more direct reply to the two cookie headers [here](https://github.com/spring-projects/spring-security/issues/5673#issuecomment-413878407) – AlphaD Dec 03 '20 at 03:55