Handle filenames being too long in `File.info?`

[jgaskins]

I was originally going to solve #15901 a different way, but on a whim I decided to check the implementation of File.info? (invoked here). If a filename is too long for the filesystem, it's a safe assumption that the file does not exist, which is what the line I modified appears to be checking for.

Fixes #15901

[jgaskins]

I assume we may need to handle this for win32, as well, but I'm not sure how. I imagine it would be this constant needing to be added to this list?

[straight-shoota]

On Windows, there is also WSAENAMETOOLONG.

However, I think we should approach this from the perspective of the File API, not the subordinate use case of StaticFileHandler. Here, the question is whether a filename that is too long should map to File::NotFoundError, or stay as less specific File::Error. The assumption that a filename that is too long means that the file does not exist, seems reasonable. But it might not be foolproof. Could be worth looking at prior art on this.

Another aspect is that unspecific File::Error can happen in other circumstances, and StaticFileHandler should probably do a better job at handling them. It doesn't really matter whether the reason is that the filename is too long, or some other kind of odd error state that is currently not handled gracefully.

So IMO we should split into two paths here:

• Fix https://github.com/crystal-lang/crystal/issues/15901 by making StaticFileHandler more fault-tolerant for any File::Error.
• Consider making ENAMETOOLONG a condition for File::NotFoundError.

[jgaskins]

On Windows, there is also WSAENAMETOOLONG.

Do you mean that that constant should also be added to the NOT_FOUND_ERRORS list? Or that that's more correct than ERROR_FILENAME_EXCED_RANGE? Or a secret third thing?

However, I think we should approach this from the perspective of the File API, not the subordinate use case of StaticFileHandler.

Hmm, any chance you saw the PR title or the commit message?

Here, the question is whether a filename that is too long should map to File::NotFoundError, or stay as less specific File::Error.

I seem to be missing context here. File.info? shouldn't raise either one if the file doesn't exist, right?

The assumption that a filename that is too long means that the file does not exist, seems reasonable. But it might not be foolproof. Could be worth looking at prior art on this.

ENAMETOOLONG is returned before looking up the file on disk. Even if it were possible to somehow have stored the entry into the filesystem (which can't hold it because it's too many bytes), you wouldn't be able to read that file because the ENAMETOOLONG check will return before it interacts with the filesystem on disk in any way. ENAMETOOLONG is effectively a special case of ENOENT — it exists only for the purpose of clearer error messages.

Rust, Go, and Ruby all have distinct errors for each Errno value. Rust and Go return them, Ruby raises Errno::* as exceptions. So, unfortunately, they're not helpful as prior art because their structure is different.

NGINX handles ENOENT, ENOTDIR, and ENAMETOOLONG identically in many areas in the codebase. A few relevant examples:

• ngx_http_static_module.c (serves static files, like StaticFileHandler)
• ngx_http_try_files_module.c (often used to check whether the requested resource is a file before delegating to a dynamic endpoint, similar to how StaticFileHandler functions as middleware)

Another aspect is that unspecific File::Error can happen in other circumstances, and StaticFileHandler should probably do a better job at handling them. It doesn't really matter whether the reason is that the filename is too long, or some other kind of odd error state that is currently not handled gracefully.

...

• Fix HTTP::StaticFileHandler raises an exception when given a long filename #15901 by making StaticFileHandler more fault-tolerant for any File::Error.

I disagree with this. I opened #15901 not because StaticFileHandler can raise an exception on server-side file errors, but because malicious client inputs can raise server-side errors that don't make sense. Malicious client inputs aren't exceptional cases and should result in HTTP 4xx responses, not 5xx.

If there's an actual file error (for example, filesystem permissions, disk failure, etc), those shouldn't be ignored. It makes perfect sense for those scenarios to raise an exception because those are actual server-side issues and should be reported to an error-tracking service.

This PR was wasn't intended to be a catch-all for things that can crop up with File.info?. It was just intended to treat missing filesystem entries the same way in a higher-level method.

[straight-shoota]

I just mentioned WSAENAMETOOLONG as another error to consider.

I seem to be missing context here. File.info? shouldn't raise either one if the file doesn't exist, right?

The context is the File API in general. We should not just look at File.info? in isolation.

For example, the close sibling method File.info does raise an exception on error. If File.info? interprets ENAMETOOLONG as the file does not exist, File.info should probably raise File::NotExist accordingly.

ENAMETOOLONG is effectively a special case of ENOENT — it exists only for the purpose of clearer error messages.

That sounds good. Thanks for digging up existing uses!

• Fix HTTP::StaticFileHandler raises an exception when given a long filename #15901 by making StaticFileHandler more fault-tolerant for any File::Error.

I disagree with this. I opened #15901 not because StaticFileHandler can raise an exception on server-side file errors, but because malicious client inputs can raise server-side errors that don't make sense. Malicious client inputs aren't exceptional cases and should result in HTTP 4xx responses, not 5xx.

I believe we actually do agree because I fully support your argument: Malicious or erroneous client input should result in a client error, not a server error.

For example, if a requested file exists but is not readable to the process, that's currently also an error 500. It should rather be a 404. (Theoretically 401, though that would leak the existence of the file. So 404 is probably better in terms of security)

[HertzDevil]

ERROR_FILENAME_EXCED_RANGE only occurs if the path exceeds even the long limit of 32767 WCHARs (including the null terminator). Anything shorter that still exceeds MAX_PATH would produce ERROR_PATH_NOT_FOUND.

[straight-shoota]

The original problem #15901 is fixed by #16025. The issue about error code consequences is tracked in #15905.