fix: Strip servlet path from path when serving static files (#14239)

[archiecobbs]

Description

When the Vaadin servlet tries to serve static files from classpath resources, it needs to strip off the servlet path, if any, associated with the request before trying to find them, because the servlet is located relative to the servlet path in the HTTP URI request space. But the StaticFileServer is not doing that.

When the Vaadin servlet is mapped to /* then this bug has no impact, because the servlet path is the empty string.

But in any other scenario, e.g., servlet mapped to /myapp, then classpath resources will not be found because Vaadin will try to find classpath resource /foo/bar.gif at classpath location /myapp/foo/bar.gif, which doesn't exist.

For example, this bug causes any add-on that includes JS or CSS files (which is most of them) to fail.

Fixes #14239

Type of change

• [X] Bugfix
• [ ] Feature

Checklist

• [X] I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
• [X] I have added a description following the guideline.
• [X] The issue is created in the corresponding repository and I have referenced it.
• [X] I have added tests to ensure my change is effective and works as intended.
• [X] New and existing tests are passing locally with my change.
• [X] I have performed self-review and corrected misspellings.

NOTE: A couple of existing unit tests were wrong (in my opinion) and had to be corrected. They now serve as unit tests for this fix.

Obviously, I'm unsure about "correcting" an existing unit test. But basically they seemed obviously wrong to me because they were trying to access a static resource from the servlet, but without using the servlet's servlet path, which makes no sense.

[CLAassistant]

All committers have signed the CLA.

[Legioth]

I don't think this is a proper solution. The purpose of StaticFileServer is to emulate the servlet container's "default" servlet for serving static resources in the cases where the Vaadin servlet mapping "gets in the way".

Without any specific servlet mapped to a path, a request to contextPath/foo/bar.txt would be served by the file from src/main/webapp/foo/bar.txt if built as a WAR file with standard Maven conventions, from src/main/resources/META-INF/resources/foo/bar.txt from a JAR file on the classpath based on the Web Fragment feature defined by Servlet 3, and some additional locations with Spring Boot conventions.

This shouldn't change just because a Vaadin servlet happens to be mapped to /foo/* (as long as there isn't a @Route that matches bar.txt). StaticFileServer should therefore retain the servlet path when looking up static resources so that a request to contextPath/foo/bar.txt serves the file from src/main/webapp/foo/bar.txt regardless of how Vaadin servlets are deployed.

[archiecobbs]

Hi @Legioth,

Without any specific servlet mapped to a path, a request to contextPath/foo/bar.txt would be served by the file from src/main/webapp/foo/bar.txt if built as a WAR file with standard Maven conventions

This shouldn't change just because a Vaadin servlet happens to be mapped to /foo/*

Well the Tomcat developers disagree with you. Tomcat's default servlet strips off the servlet path. See for example https://bz.apache.org/bugzilla/show_bug.cgi?id=42411

The root cause here is the current functionality that was changed by the "fix" for #13769.

That bug is just wrongheaded. It completely breaks the modularity of servlet mapping.

The bug reports states:

In all cases, the static files (JS bundle files, vaadinPush script etc) should be loaded from VAADIN inside the context root.

What?? According to whom?? There is no justification given here, and this completely breaks a bunch of stuff.

Thus if you have multiple servlet mappings, you still load e.g. web components only once from /VAADIN and not from both /myapp/VAADIN and /admin/VAADIN.

So at best, this is all for a completely trivial optimization. Not worth it!!

Did you read the part where almost every add-on is broken now?

[Legioth]

If you explicitly deploy the default servlet to some specific path, then it's very clear that you do that explicitly for the purpose of making static resources available below that path instead instead of the default.

When it comes to Vaadin's servlet path, it does so much more than just serve static resources, thus putting it in a different situation than in the Tomcat case. The path also affects static resources not related to Vaadin, and the path served by Vaadin would be in addition to the default one (unless there's also some other servlet that overrides the default path). Can we for instance assume that an application with a custom access control setting for the static resource foo.txt would realize that when they deploy a Vaadin servlet to /bar, then they would need to apply the same security configuration not only for /foo.txt but also for /bar/foo.txt?

I realize that https://github.com/vaadin/flow/issues/13769 causes problems with add-ons, but the reason it was fixed in the first place was that the other way also caused problems. Another potential fix might be to change the default for resource annotations such as @StyleSheet to interpret relative URLs relative to the context root rather than the current default that is relative to the base URI (which is effectively the same as the servlet path). It's also always possible to change add-ons to explicitly use that mode by prefixing resource annotation values with context://, but that does of course require a separate fix to each affected add-on.

[Legioth]

Or maybe the proper solution would simply be to revert https://github.com/vaadin/flow/issues/13769?

[archiecobbs]

I realize that https://github.com/vaadin/flow/issues/13769 causes problems with add-ons, but the reason it was fixed in the first place was that the other way also caused problems.

I would like to better understand this (the motivation for 13769)... can you describe the problems that 13769 was supposed to fix? No proper motivation was provided in the bug description itself (as I've already pointed out).

The path also affects static resources not related to Vaadin, and the path served by Vaadin would be in addition to the default one

Good point! This bug is a big security hole because it causes every resource to be unexpectedly exposed at two different URL paths, when most security setups are designed to protect secure resources at the exactly one URL path - the one (and only one) they are expected to be accessible at.

Or maybe the proper solution would simply be to revert https://github.com/vaadin/flow/issues/13769?

Yes! I believe this is the correct answer here... at least until someone can provide a valid counter-argument.

The change in #13769 has created a giant mess, and I've yet to hear any valid explanation of what was the supposed upside. Whatever it was, it's hard to imagine that it was worth the mess.

[Legioth]

https://github.com/vaadin/flow/issues/13769 was introduced to deal with issues related to the Vite integration when using a custom servlet path.

Based on an internal discussion earlier today, we are leaning towards reverting and finding another solution to the Vite issues.

[mshabarov]

We decided to revert the change for serving static resources from context root https://github.com/vaadin/flow/pull/14357 and fix Vite-related issue https://github.com/vaadin/flow/issues/13190 in another way. Thus, not sure this PR would be needed after revert. @archiecobbs

[archiecobbs]

We decided to revert the change for serving static resources from context root https://github.com/vaadin/flow/pull/14357 and fix Vite-related issue https://github.com/vaadin/flow/issues/13190 in another way. Thus, not sure this PR would be needed after revert.

Great! Thanks for being willing to reconsider.

I'll be looking out for that commit...