Origins doesn't anticipate an Origin header with a path [SPR-13478]

[spring-projects-issues]

George opened SPR-13478 and commented

1. Transformation of the origin into uri components used to fail with a NumberFormatException when origin string contained a character after the port definition like:
   • http://domain1.org:8080/
   • http://domain1.org:80/path/file
2. This also affected DefaultCorsProcessor.processRequest, due to WebUtils.isSameOrigin call.
3. Origin validity check was considering http://domain1.com and http://domain1.com/ to be different. The same applies to allowed origins comparison.

Everything above doesn't comply to RFC 6454 standard:

3.2.1. Examples
All of the following resources have the same origin:
http://example.com/
http://example.com:80/
http://example.com/path/file

I'm not totally sure about UriComponentsBuilder.fromHttpRequest but based on the code it may fail the same way as UriComponentsBuilder fromOriginHeader did. It depends what value can be stored in X-Forwarded-Host.

---

Affects: 4.2 GA, 4.2.1

Reference URL: facebookincubator/SocketRocket#256

Referenced from: pull request #875, and commits 9c66dfa

[spring-projects-issues]

Sébastien Deleuze commented

GeorgeCGV I think we have not the same reading of RFC 6454.

In the RFC example you quote, http://example.com/, http://example.com:80/ and http://example.com/path/file are resource examples that have the same origin, not examples of valid origins. These 3 resources have the same origin because they have the same scheme (http), host (example.com) and port (80).

Section 6 and 7.1 of the RFC seems pretty clear about Origin header value format. If I reuse your example, the 2 possible valid values are http://example.com and http://example.com:80, and our implementation already compares these 2 variants correctly. Trailing slashes or any kind of path after the host/port are not allowed in an Origin header value.

Any thoughts?

[spring-projects-issues]

George commented

Sébastien Deleuze, I think that the Server should consider http://domain1.org/ and http://domain1.org:80/ as http://domain1.org and http://domain1.org:80 to be valid origins if http://domain1.org is allowed. After all this is just a slash and some libraries and applications may terminate origin url with it.

Regarding http://domain1.org/path/file and http://domain1.org:80/path/file, I think you are right and from security perspective they must be invalid.

I just pushed some changes which will consider origins with the ending / to be valid if such origins are allowed. I didn't modify anything related to allowedOrigins collection, so Spring users will need to specify allowed origin as follows http://domain1.org and then http://domain1.org and http://domain1.org/ will be considered as valid.

[spring-projects-issues]

Sébastien Deleuze commented

GeorgeCGV The RFC clearly specifies that the Origin header value should have not trailing path, and I see no reason for accepting this. After reading the discussion on facebookincubator/SocketRocket#256, the fix initially proposed is the right one. Every client I am aware of implements the RFC correctly and set Origin without a trailing path, so this is from my point of view a SocketRocket bug that should be fixed.

On Spring side, I see 2 improvements we could consider (even if they will not fix directly your problem):

• Consider removing the trailing path (if any) from the allowed origins configured by the user
• Catch the NumberFormatException in UriComponentsBuilder.fromOriginHeader() in order to return a meaningful error message instead of a stacktrace

Any thoughts?

[spring-projects-issues]

George commented

Sébastien Deleuze, just committed last changes.

After all,

• isSameOrigin() behaves by RFC specification (initial push)

Restored functionality of isValidOrigin() as it used to be.
Keep in mind that when allowedOrigins is empty then isValidOrigin() behaves as isSameOrigin().

Consider removing the trailing path (if any) from the allowed origins configured by the user

I think it is better to leave this as it is and let the users full control of allowed origins.

[spring-projects-issues]

Sébastien Deleuze commented

GeorgeCGV When I wrote "the fix initially proposed is the right one", if you follow the link, you will see I was referring to the SocketRocket fix, not to your initial push, sorry if it was not clear.

I really appreciate your contribution on this PR, but to the best of my understanding, the current implementation is already RFC 6454 compliant. As a consequence, based on your latest feedback, I just plan to improve exception handling when a malformed Origin header value is parsed by UriComponentsBuilder.fromOriginHeader().

[spring-projects-issues]

George commented

Sébastien Deleuze,I know what you meant with the fix.

As a consequence, based on your latest feedback, I just plan to improve exception handling when a malformed Origin header value is parsed by UriComponentsBuilder.fromOriginHeader().

My last commit does exactly this.

[spring-projects-issues]

Sébastien Deleuze commented

GeorgeCGV I really appreciate your contribution, but your commit introduces too much changes, and I would like to minimize the risk of regressions. I have fixed this issue with this commit that changes less things.

[spring-projects-issues]

George commented

Sébastien Deleuze, my commit reused what was already introduced and used in the class. Anyway, thank you.