DefaultCorsProcessor's origin comparison is restrictive and inefficient [SPR-14080]

[spring-projects-issues]

Nick Verbeck opened SPR-14080 and commented

This plays in part with #18266 but is more do to the nature of how DefaultCorsProcessor does its checks.

With the DefaultCorsProcessor auto on. It will always trip the check at line 71 "WebUtils.isSameOrigin(serverRequest)" of DefaultCorsProcessor even if I've already dealt with CORS headers within my own filter beforehand.

However this is not the core issue, an issue that I'm dealing with now. Because this check uses the java.net.URI class to do its parsing its now requiring super strict Request URIs against the webapp. Which is resulting in a number of rejected requests against our app.

The other issue here isn't just the strict nature of URI but just the general use of it in the first place. Its way to overweight for a large number of things its being used for and results in slowness within the app. As well as its suffers from the Charset lock issue at high concurrency. (Nice write-up from the Evernote team on that issue here https://blog.evernote.com/tech/2011/06/21/fast-string-handling-a-frayed-knot/)

Currently the only work around to this is to wrap the HttpServletRequest and hide the Origin header from above. So that the check on line 64 "!CorsUtils.isCorsRequest(request)" will pass and stop further processing.

Ideally it would be great to be able to disable DefaultCorsProcessor at the very least. If anything for the nature of giving the developer the choice not making it for them. However the true solution would be to stop using URI for parsing just a host and port and use a simple regex or string token. As this would still let DefaultCorsProcessor be used.

---

Affects: 4.2.5

Issue Links:

• Recent ServletServerHttpRequest.getURI() change breaks CORS requests with encoded characters [SPR-14248] #18821 Recent ServletServerHttpRequest.getURI() change breaks CORS requests with encoded characters ("is duplicated by")
• Enable/Disable Spring CORS option [SPR-13691] #18266 Enable/Disable Spring CORS option
• NPE was occurred at WebUtils.isSameOrigin [SPR-14305] #18877 NPE was occurred at WebUtils.isSameOrigin

Referenced from: commits 6807bcb, 8991319, abe7345, 9a41774, a19be75, 9a52c81

[spring-projects-issues]

Juergen Hoeller commented

Would it help for your purposes if the responseHasCors check moved ahead of the isSameOrigin check? This seems to make sense in general since the former check is much quicker than the latter.

Also, could you elaborate a bit on why you're seeing requests rejected in your scenario? I might be missing something since the isSameOrigin doesn't seem to trigger the rejection of a request...

Finally, isSameOrigin goes through the UriComponentsBuilder which is essentially String-based. Where specifically do you see inefficient use of java.net.URI?

[spring-projects-issues]

Nick Verbeck commented

Moving responseHasCors ahead would help resolve my issue, since we currently have our own CORS handler, but would make it so anyone needing a less strict url parsing wouldn't be able to leverage a layer of code that's there and always running.

My requests are being rejected due to users calling our system with invalid query string params due to not encoding values.

The rejection happens due to UriComponentsBuilder.fromHttpRequest() calling request.getURI() where request is actually an instance of ServletServerHttpRequest. Which looks like

public URI getURI() {
		try {
			StringBuffer url = this.servletRequest.getRequestURL();
			String query = this.servletRequest.getQueryString();
			if (StringUtils.hasText(query)) {
				url.append('?').append(query);
			}
			return new URI(url.toString());
		}
		catch (URISyntaxException ex) {
			throw new IllegalStateException("Could not get HttpServletRequest URI: " + ex.getMessage(), ex);
		}
	}

Because of the bad query string "new URI(url.toString())" results in a URISyntaxException. To which triggers the ErrorHandler and a bad response.

Before upgrading to 4.2.5 everything was calling through just fine and we where just getting bad data within the respective param field. To which we where able to deal with within our controller.

[spring-projects-issues]

Juergen Hoeller commented

Indeed, WebUtils.isSameOrigin implicitly - and unnecessarily - triggers URI creation there. I agree, that's to be avoided even if the responseHasCors check comes first.

[spring-projects-issues]

Juergen Hoeller commented

I've moved the responseHasCors check to come first now, and also refined WebUtils.isSameOrigin to use optimized scheme+host+port retrieval from an underlying HttpServletRequest if possible, instead of attempting to build from the entire URI (which our HttpRequest abstraction otherwise enforces, unfortunately). After all, parsing the rest of the URI is in vain in any case, since all we need here is the scheme+host+port for an origin comparison.

[spring-projects-issues]

Juergen Hoeller commented

I'm handling this a bit differently between 4.3 and 4.2.6: The former gets the full revision, including more efficient (and less restrictive) parsing for the same-origin comparison. However, for 4.2.6, I'm just going to move the responseHasCors check first which restores compatibility with custom CORS handling but keeps 4.2's own CORS processing as restrictive as it initially was in that line.

[spring-projects-issues]

Nick Verbeck commented

Is there a time frame for release of 4.2.6 and 4.3? I'd like to get this into my app. If anything into a self compile for our use temporally until release.

[spring-projects-issues]

Juergen Hoeller commented

We usually keep our target dates up to date on the JIRA roadmap:
https://jira.spring.io/browse/SPR/?selectedTab=com.atlassian.jira.jira-projects-plugin:roadmap-panel

It's April 5th for both releases. I recommend trying tomorrow's 4.2.6.BUILD-SNAPSHOT (see http://projects.spring.io/spring-framework/ for Maven coodinaates) in the meantime...