fix: don't serve directories as static files

[caalador]

Add check to the resource to see
if it is a directory and do not serve
if this is the case.

Fixe #11047

[denis-anisimov]

Does getPath() always ends with "/" for a directory ?
In the test the URL may be "new URL("file:///C:/frontend" and not the "new URL("file:///C:/frontend/".
The path is different but the semantic is the same : it's still a directory.

So the assumption looks quite fragile.

[caalador]

the getResource(path) url returns always (as far as I've found out) directories ending with '/' it's the same expectation as we have for the request file name up top which also fails if the request is only /frontend also /frontend might be a file named only frontend and not a directory.

I couldn't find a better way to make certain it's actually a directory as most of the folders are inside jars and you can't make a File for those to check isDirectory. One will get java.nio.file.FileSystemNotFoundException for example when using Files.isDirectory(Paths.get(resource.toUri())) if the file is inside a jar as we can't generate a Path for it.

[caalador]

Another way would be to have something like:

if (resource != null && resource.getProtocol().equals("jar")) {
            FileSystem fileSystem = null;
            try {
                URI resourceUri = resource.toURI();
                fileSystem = FileSystems
                    .newFileSystem(resourceUri, Collections.emptyMap());
                final Path path = fileSystem.getPath(resource.getPath()
                    .substring(resource.getPath().lastIndexOf("!") + 1));
                if (Files.isDirectory(path)) {
                    // Directory resources should not be served.
                    return false;
                }
            } catch (URISyntaxException | IOException e) {
                e.printStackTrace();
            } finally {
                if (fileSystem != null) {
                    try {
                        fileSystem.close();
                    } catch (IOException e) {
                        e.printStackTrace();
                    }
                }
            }
        }

But for the use it feels a bit heavy to have as it will be called quite often.

[denis-anisimov]

getResource(path)

I cannot find this method.

There is call resource = getStaticResource(requestFilename);.

The getStaticResource method :

• may be overriden anyhow
• its default impl delegates to vaadinService.getStaticResource(path); which also some internal API and may return anything

So this code protects the directory listing only in some cases with some assumptions.

I'm not sure how the code should be written but the fix in the PR doesn't fix the issue generally for me.

[denis-anisimov]

The related question is:
why the very first check

        String requestFilename = getRequestFilename(request);
        if (requestFilename.endsWith("/")) {
            // Directories are not static resources although
            // servletContext.getResource will return a URL for them, at
            // least with Jetty
            return false;
        }

doesn't help here ?

There is some logic which make a redirect for some requested paths and the redirect goes with / in the end.
So getRequestFilename(request) may be checked against / in the end (and this is also not a generic case).

Apparently requestFilename doesn't end with / in the considered case and in fact this is exactly the same situation with getStaticResource(requestFilename) : one more unreliable check.

[denis-anisimov]

The request goes to the servlerContext.getResource(path)

No it doesn't.

It goes via

• StaticFileServer::getStaticResource
• VaadinService::getStaticResource
• and only as an impl detail of VaadinServletService it goes to getServlet().getServletContext().getResource.

so this fix assumes two things:

• StaticFileServer::getStaticResource is not reimplemented or its implementation behaves in the certain way.
• VaadinService::getStaticResource is in fact VaadinServletService::getStaticResource which is not reimplemented or if it's reimplemented then it behaves in a certain way.

I think it's just incorrect to solve this issue in the StaticFileSever at all. At least in the isStaticResourceRequest.

It should be considered as a static resource request but it should not render the directory.
The logic which needs to be changed is rendering the directory listing: not sure which place it is.
ResponseWriter may be.

[caalador]

I don't see how the ResponseWriter could differentiate between a directory listing and a normal file content.
Also one point with not handling a directory request as a StaticFile request is that you can't go around guessing what folders may exist in a project.

[denis-anisimov]

There should be some logic which shows the file content.

Technically the directory is also a file in UNIX but I think the directory is handled via a specific way.
The logic is most likely checks whether this is a regular file or not and if it's a directory then it renders a listing (like directory is listed by pure Web server).
So the result of the rendering is a specific HTML which contains elements like <li> or whatever for the files inside the directory.
The real directory on the FS doesn't contain any HTML tags of course. So there should be something which decides how to show the URL : as a listing of files or the content of the file.

I don't know how exactly the list of files is shown since the original ticket doesn't say that. But I assume this is a listing done using HTML markup. Then there is some code which does this.
This code should not do this for the directory and that's it.
This is theoretically. It might be not that easy in practice.

But the changes in the current PR doesn't protect from the dir listing generally. This can be used only as a lightweight preliminary check : if this true then deny. But if it's not true (doesn't end with slash) still try to check whether this is a directory or not.

[caalador]

I updated it to check for actual directory, with test for dir in file system and dir inside a zip.

[denis-anisimov]

OK, I've checked how the response is written and apparently it's delegated URLConnection subclasses like that
DirectoryURLConnection.
So there is no code which controls the response :(

[denis-anisimov]

This can be extracted to a several methods to avoid having everything here ilined.
The current code contains too many nested if-try-else-if.

[caalador]

Cleaned up code and fixed the test.

[vaadin-bot]

SonarQube analysis reported 5 issues

1. StaticFileServer.java#L152: Replace this with a call to the "toFile().isDirectory()" method
2. StaticFileServer.java#L160: Replace this with a call to the "toFile().isDirectory()" method
3. StaticFileServer.java#L145: Move the "jar" string literal on the left side of this string comparison.
4. StaticFileServer.java#L150: Put single-quotes around '!' to use the faster "lastIndexOf(char)" method.
5. StaticFileServer.java#L159: Move the "file" string literal on the left side of this string comparison.

[vaadin-bot]

Hi @caalador , this commit cannot be picked to 2.7 by this bot, can you take a look and pick it manually?
Error Message: Error: Command failed: git cherry-pick 7bf140b
error: could not apply 7bf140b... fix: don't serve directories as static files (#11072)
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]

Hi @caalador , this commit cannot be picked to 2.6 by this bot, can you take a look and pick it manually?
Error Message: Error: Command failed: git cherry-pick 7bf140b
error: could not apply 7bf140b... fix: don't serve directories as static files (#11072)
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]

Hi @caalador , this commit cannot be picked to 1.0 by this bot, can you take a look and pick it manually?
Error Message: Error: Command failed: git cherry-pick 7bf140b
error: could not apply 7bf140b... fix: don't serve directories as static files (#11072)
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 21.0.0.alpha3. For prerelease versions, it will be included in its final version.

[vaadin-bot]

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