fix: do not serve UI for invalid location

[pleku]

Fixes #9443 for legacy bootstrapping mode.

IT and fix for Flow 3+ bootstrapping will come next. This commit can be picked to Flow 2.x with the IT from the other PR.

[denis-anisimov]

That fix handles only one case: open a page with location which we don't accept.
But as I understand it won't do anything if navigation happens from the correct location to the location ".." using a router link.

So the code which catch the invalid location should be somewhere inside Router handling code.
And there is already some code which handles invalid location. But it does it in a specific way.
And then there is a question how it should handle it: 503 vs 404 is a semantical difference.
".." is invalid because we are trying to prevent traversing resources via waling using parent directory. So every such attempt is disallowed and 503 is used to indicate that: it's disallowed.
On the other hand ".." is an attempt to go to the parent which doesn't exist in this specific case described in the ticket. And it should be 404 because it doesn't exist.

There is a number of issue with that:

• ".." currently is always "invalid" URL and 503 is returned,
• ".." should return 404 for "/.." according to the ticket
• if "foo" an existing route and "/" is a route then ".." should work fine if you are at the moment in the "foo" route since it becomes "foo/.." and it should be translated to "/" which is working route.

So the current 503 error handles everything semantically as: wrong URL and you should not try to use ".." ever inside the URL.
The ticket wants to distinguish ".." segment inside URL and it's significant efforts to implement.
I personally don't see serious benefit to have 404 code instead of 503.
The current approach allows to handle all this cases in a similar way.

I cannot say that the expected behavior in the ticket is really expected even though it makes sense.
The current behavior : 503 for every URL which contains ".." makes sense for me as well. So we disallow every URL which contains "..".

[pleku]

That fix handles only one case: open a page with location which we don't accept.
But as I understand it won't do anything if navigation happens from the correct location to the location ".." using a router link.

That would be another thing, which I'm not sure how relevant/important it is, but anyway it can be another ticket if necessary.

So the current 503 error handles everything semantically as: wrong URL and you should not try to use ".." ever inside the URL.

@denis-anisimov I don't understand 503 error like that, can you please elaborate how you see it like that ? To me 503 error is about service not available and in e.g. https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/503 it says

The HyperText Transfer Protocol (HTTP) 503 Service Unavailable server error response code indicates that the server is not ready to handle the request.
Common causes are a server that is down for maintenance or that is overloaded. This response should be used for temporary conditions and the Retry-After HTTP header should, if possible, contain the estimated time for the recovery of the service.

To me there is a clear distinction between server getting an invalid URL that won't work and service being down for maintenance. Thus 503 is not the right thing, and I think 400 (not 404) which e.g. Tomcat uses is something we should align with.

The HyperText Transfer Protocol (HTTP) 400 Bad Request response status code indicates that the server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing).

So to me there is a clear difference between the two that is being fixed for initial bootstrapping.

[denis-anisimov]

Update : I haven't noticed that 400 is used instead of 404 (as initially requested in the ticket). So part of my comments is irrelevant. My bad.

Well, I don't know which error code is more suitable.

This is not relevant anymore: if we use a code which indicate an error (not 404) then it's fine:
What is important is: 404 which is Page not found (without any doubt) and some error which says that there is an error on the server side. This is the only important thing.

I don't see this is as a separate issue since it's exactly the question: whether we treat ".." as invalid generally (and then it's a some error code which is not 404) or depending on request it's either valid or not.

The invalid URL is handled inside the Router logic and the fix should go there instead of boostrapping. The current fix location is just in a wrong place. So instead of covering all the cases including the router navigation it's located on the top of the stacktrace with catching only one specific case and it uses a hack: instantiate a Location which is never needed.
This is an artificial code and looks like a hack.

The fix moved into the Router on the other hand will handle every case properly and generally.

I don't know this functionality enough. @caalador knows thus much better and the code is written mostly by him.
So I suggest him as a reviewer.

[denis-anisimov]

I was referring to 404 since the original ticket description talks about 404.

But now I see 400 in the fix. I've missed that .
I though that by default 503 is used instead of 500. But it's 500 in the ticket description.

So the error code 400 is fine I think. But anyway: the logic should be somewhere inside router code which creates a Location instead having a fake Location instantiation inside boostrap.

InvalidUrlTest.invalidUrlAtInitialization_uiInitialiazesAsExpected is getting the wrong response.

[caalador]

The test probably has something to do with the strange stripQueryString functionality that has left the query from the string as part of the path.
basically it should test %3faaa instead of ?aaa, but the junit test most likely uses ? instead of %3f as it simulates the path being decoded.

[pleku]

The test probably has something to do with the strange stripQueryString functionality that has left the query from the string as part of the path.
basically it should test %3faaa instead of ?aaa, but the junit test most likely uses ? instead of %3f as it simulates the path being decoded.

The test seems broken indeed as it is using ? as part of rquest's pathInfo which is incorrect as ? is a reserved character for requests

[vaadin-bot]

SonarQube analysis reported 6 issues

1. BootstrapHandler.java#L567: Either log or rethrow this exception.
2. Location.java#L55: Remove the declaration of thrown exception 'com.vaadin.flow.router.InvalidLocationException' which is a runtime exception.
3. Location.java#L84: Remove the declaration of thrown exception 'com.vaadin.flow.router.InvalidLocationException' which is a runtime exception.
4. LocationUtil.java#L171: Replace this if-then-else statement by a single return statement.
5. LocationUtil.java#L171: Move the ".." string literal on the left side of this string comparison.
6. BootstrapHandler.java#L980: Immediately return this expression instead of assigning it to the temporary variable "resourceProvider".

[vaadin-bot]

Hi @pleku and @caalador, when i performed cherry-pick to this commit to 2.7, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 822ea46
error: could not apply 822ea46... fix: do not serve UI for invalid location (#11552)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'

[vaadin-bot]

This ticket/PR has been released with platform 22.0.0.alpha2 and is also targeting the upcoming stable 22.0.0 version.

[vaadin-bot]

This ticket/PR has been released with platform 14.7.0.rc1 and is also targeting the upcoming stable 14.7.0 version.