Answer a 400 when the ajax base url port is not a number

[17krishna8]

Url.parse() throws NumberFormatException for a port that is not a number, e.g. for http://localhost:80x/index.html. In an ajax request that exception escaped ServletWebRequest.getClientUrl() and ended as an error page with status 500.

Url.parse() itself keeps failing fast: it is public API that applications use on their own strings, and for a URL a developer wrote the exception is the more useful behaviour.

The targeted fix is in getClientUrl(): answer a base url that cannot be parsed the same way a missing one is already answered a few lines above, with AbortWithHttpErrorCodeException and status code 400. The port read from the base url is not used anyway, setParameters() overwrites host, port and protocol with the values the container reports.

Also document that Url.parse() throws NumberFormatException for a port that is not a number.

[reiern70]

Thanks for the PR. The NumberFormatException is real, I reproduced it on master, but I don't think Url.parse() is the place to fix it.

Url.parse() is public API that applications use on their own strings. With this change a malformed port no longer fails but becomes the protocol's default port, so Url.parse("http://example.com:8o8o/x") renders as http://example.com/x, a different endpoint, without any signal. For a URL a developer wrote, failing fast is the more useful behaviour.

For the case the PR is about, the guessed port doesn't matter at all: ServletWebRequest.getClientUrl() hands the parsed ajax base URL to setParameters(), which overwrites host, port and protocol with the values the container reports. The targeted fix is in getClientUrl(): answer a base URL that cannot be parsed the way a missing one is answered a few lines above, with a 400, instead of letting the exception escape as a 500:

try
{
	return setParameters(Url.parse(base, getCharset()));
}
catch (NumberFormatException e)
{
	throw new AbortWithHttpErrorCodeException(HttpServletResponse.SC_BAD_REQUEST,
		"Current ajax request has a malformed base url");
}

with a test next to getClientUrlAjaxWithoutBaseUrl in ServletWebRequestTest:

@Test
void getClientUrlAjaxWithMalformedBaseUrl()
{
	MockHttpServletRequest httpRequest = new MockHttpServletRequest(null, null, null);
	httpRequest.setHeader(ServletWebRequest.HEADER_AJAX, "true");
	httpRequest.setHeader(ServletWebRequest.HEADER_AJAX_BASE_URL, "http://localhost:80x/page");
	ServletWebRequest webRequest = new ServletWebRequest(httpRequest, "");

	AbortWithHttpErrorCodeException e = assertThrows(AbortWithHttpErrorCodeException.class,
		webRequest::getClientUrl);
	assertEquals(HttpServletResponse.SC_BAD_REQUEST, e.getErrorCode());
}

If anything changes in Url itself, I'd rather document that parse() throws NumberFormatException for a port that is not a number than make it guess.

[reiern70]

This only catches ports that aren't an int at all. :-1 and :70000 are still taken as they are, so whether a bad port is replaced depends on whether it happens to fit an int; parse23 locks in 99999999999 becoming 443 while 70000 stays 70000.

[mattrpav]

Why fallback to a default value vs error? I'm concerned about having a default fallback on bad input could mask a broader issue.

[17krishna8]

@mattrpav agreed, the fallback was the wrong call - Url.parse() keeps failing fast, and the request now gets a 400 instead.

@reiern70 thanks for the detailed review and the sketch. I have reworked the PR: Url.parse() is unchanged and now documents that it throws NumberFormatException for a port that is not a number, and ServletWebRequest.getClientUrl() answers a base url that cannot be parsed with the same 400 a missing one already gets, with your getClientUrlAjaxWithMalformedBaseUrl test next to getClientUrlAjaxWithoutBaseUrl.