Reject CORS request with 403 if Origin header is malformed
When assessing if a request is a CORS request, both mvc and reactive `DefaultCorsProcessor` now catch `IllegalArgumentException` and turn this into a 403 rejection rather than letting the exception propagate into a 500 response. Closes gh-33688
This commit is contained in:
		
							parent
							
								
									f991c19b30
								
							
						
					
					
						commit
						8da31e1db7
					
				|  | @ -83,9 +83,16 @@ public class DefaultCorsProcessor implements CorsProcessor { | ||||||
| 			response.addHeader(HttpHeaders.VARY, HttpHeaders.ACCESS_CONTROL_REQUEST_HEADERS); | 			response.addHeader(HttpHeaders.VARY, HttpHeaders.ACCESS_CONTROL_REQUEST_HEADERS); | ||||||
| 		} | 		} | ||||||
| 
 | 
 | ||||||
|  | 		try { | ||||||
| 			if (!CorsUtils.isCorsRequest(request)) { | 			if (!CorsUtils.isCorsRequest(request)) { | ||||||
| 				return true; | 				return true; | ||||||
| 			} | 			} | ||||||
|  | 		} | ||||||
|  | 		catch (IllegalArgumentException ex) { | ||||||
|  | 			logger.debug("Reject: origin is malformed"); | ||||||
|  | 			rejectRequest(new ServletServerHttpResponse(response)); | ||||||
|  | 			return false; | ||||||
|  | 		} | ||||||
| 
 | 
 | ||||||
| 		if (response.getHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN) != null) { | 		if (response.getHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN) != null) { | ||||||
| 			logger.trace("Skip: response already contains \"Access-Control-Allow-Origin\""); | 			logger.trace("Skip: response already contains \"Access-Control-Allow-Origin\""); | ||||||
|  |  | ||||||
|  | @ -83,9 +83,16 @@ public class DefaultCorsProcessor implements CorsProcessor { | ||||||
| 			} | 			} | ||||||
| 		} | 		} | ||||||
| 
 | 
 | ||||||
|  | 		try { | ||||||
| 			if (!CorsUtils.isCorsRequest(request)) { | 			if (!CorsUtils.isCorsRequest(request)) { | ||||||
| 				return true; | 				return true; | ||||||
| 			} | 			} | ||||||
|  | 		} | ||||||
|  | 		catch (IllegalArgumentException ex) { | ||||||
|  | 			logger.debug("Reject: origin is malformed"); | ||||||
|  | 			rejectRequest(response); | ||||||
|  | 			return false; | ||||||
|  | 		} | ||||||
| 
 | 
 | ||||||
| 		if (responseHeaders.getFirst(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN) != null) { | 		if (responseHeaders.getFirst(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN) != null) { | ||||||
| 			logger.trace("Skip: response already contains \"Access-Control-Allow-Origin\""); | 			logger.trace("Skip: response already contains \"Access-Control-Allow-Origin\""); | ||||||
|  |  | ||||||
|  | @ -184,6 +184,20 @@ class DefaultCorsProcessorTests { | ||||||
| 		assertThat(this.response.containsHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN)).isTrue(); | 		assertThat(this.response.containsHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN)).isTrue(); | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
|  | 	@Test //gh-33682 | ||||||
|  | 	public void actualRequestMalformedOriginRejected() throws Exception { | ||||||
|  | 		this.request.setMethod(HttpMethod.GET.name()); | ||||||
|  | 		this.request.addHeader(HttpHeaders.ORIGIN, "http://*@:;"); | ||||||
|  | 		this.conf.addAllowedOrigin("https://domain2.com"); | ||||||
|  | 
 | ||||||
|  | 		boolean result = this.processor.processRequest(this.conf, this.request, this.response); | ||||||
|  | 		assertThat(result).isFalse(); | ||||||
|  | 		assertThat(this.response.containsHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN)).isFalse(); | ||||||
|  | 		assertThat(this.response.getHeaders(HttpHeaders.VARY)).contains(HttpHeaders.ORIGIN, | ||||||
|  | 				HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, HttpHeaders.ACCESS_CONTROL_REQUEST_HEADERS); | ||||||
|  | 		assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_FORBIDDEN); | ||||||
|  | 	} | ||||||
|  | 
 | ||||||
| 	@Test | 	@Test | ||||||
| 	void actualRequestExposedHeaders() throws Exception { | 	void actualRequestExposedHeaders() throws Exception { | ||||||
| 		this.request.setMethod(HttpMethod.GET.name()); | 		this.request.setMethod(HttpMethod.GET.name()); | ||||||
|  |  | ||||||
|  | @ -190,6 +190,21 @@ class DefaultCorsProcessorTests { | ||||||
| 		assertThat(response.getHeaders().containsKey(ACCESS_CONTROL_ALLOW_ORIGIN)).isTrue(); | 		assertThat(response.getHeaders().containsKey(ACCESS_CONTROL_ALLOW_ORIGIN)).isTrue(); | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
|  | 	@Test // gh-33682 | ||||||
|  | 	public void actualRequestMalformedOriginRejected() { | ||||||
|  | 		ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest | ||||||
|  | 				.method(HttpMethod.GET, "http://localhost/test.html") | ||||||
|  | 				.header(HttpHeaders.ORIGIN, "http://*@:;")); | ||||||
|  | 
 | ||||||
|  | 		this.conf.addAllowedOrigin("https://domain2.com"); | ||||||
|  | 		boolean result = this.processor.process(this.conf, exchange); | ||||||
|  | 		ServerHttpResponse response = exchange.getResponse(); | ||||||
|  | 
 | ||||||
|  | 		assertThat(result).isFalse(); | ||||||
|  | 		assertThat(response.getStatusCode()).isEqualTo(HttpStatus.FORBIDDEN); | ||||||
|  | 		assertThat(response.getHeaders().containsKey(ACCESS_CONTROL_ALLOW_ORIGIN)).isFalse(); | ||||||
|  | 	} | ||||||
|  | 
 | ||||||
| 	@Test | 	@Test | ||||||
| 	void actualRequestExposedHeaders() { | 	void actualRequestExposedHeaders() { | ||||||
| 		ServerWebExchange exchange = actualRequest(); | 		ServerWebExchange exchange = actualRequest(); | ||||||
|  |  | ||||||
		Loading…
	
		Reference in New Issue