FileSystemFinder harsh regarding its input on Windows

[Marc Perrin]

It seems to me that FileSystemFinder.find_location code is unnecessarily harsh re os.sep.

(context: I got an issue about finding (React) static files on Windows with whitenoise lib > django)

See this code ​https://github.com/django/django/blob/main/django/contrib/staticfiles/finders.py#L137

i.e.

def find_location(self, root, path, prefix=None):
    """
    Find a requested static file in a location and return the found
    absolute path (or ``None`` if no match).
    """
    if prefix:
        prefix = '%s%s' % (prefix, os.sep)
        if not path.startswith(prefix):
            return None
        path = path[len(prefix):]
    path = safe_join(root, path)
    if os.path.exists(path):
        return path

In my case, when debugging I go into the return None a lot because os.sep is backslash, and the args of find_location are for example:

root = r'C:\some\folder\for\react\files'
path = 'bundled_react/some_file.hot-update.js'
prefix = 'bundled_react'

Right now, this basically forces to call finders.find with os-adapted paths (using e.g. url2pathname), but that seems unnecessarily harsh, considering that:

• other finders (e.g. AppDirectoriesFinder) seem much more lenient
• even within FileSystemFinder.find_location, that does not seem justified, as the safe_join called below is perfectly capable to do for example:

  assert safe_join(r'C:\some\folder\for\react\files', 'some/path/for/a/file.js') == r'C:\some\folder\for\react\files\some\path\for\a\file.js'
  

NB: an old, related issue might be: https://code.djangoproject.com/ticket/19046

Also see ​https://github.com/evansd/whitenoise/issues/472 - at first I thought it called for a change on whitenoise side, but now I'm thinking it would make sense on django side.

[Mohamed Nabil Rady]

I believe this is a potential fix, although os.path.commonpatah throws errors in some scenarios(​https://docs.python.org/3/library/os.path.html#os.path.commonpath), but I believe these scenarios will never happen.

if prefix:
            if os.path.commonpath([path, prefix]) == '':
                return None

[Marc Perrin]

Yeah that paragraph looks pretty good to me.

We'd have to beware about potential edge cases though, e.g. if path = prefix (for some reason...)
In that case the return None that currently happens is not a matter of os.sep mismatch, so maybe we want to keep that behavior.
(but likely, that change of behavior about that edge case would be just fine)

Replying to Mohamed Nabil Rady:

I believe this is a potential fix, although os.path.commonpatah throws errors in some scenarios(​https://docs.python.org/3/library/os.path.html#os.path.commonpath), but I believe these scenarios will never happen.

if prefix:
            if not os.path.commonpath([path, prefix]):
                return None
            path = os.path.relpath(path, prefix)

[Mariusz Felisiak]

other finders (e.g. AppDirectoriesFinder) seem much more lenient

AppDirectoriesFinder had exactly the same code before removing AppStaticStorage, see f56c88a8eed91f68f6845bbf730f7b7361c5cfb1 and #21867.

In my case, when debugging I go into the return None a lot because os.sep is backslash, and the args of find_location are for example:

root = r'C:\some\folder\for\react\files'
path = 'bundled_react/some_file.hot-update.js'
prefix = 'bundled_react'

I'm not sure how you reached find_location() with a path from another OS. It seems that you're using it in a non-intended way and if so you need to standardize paths before passing them to FileSystemFinder on your own. Also, using commonpath() is not a proper change as it breaks many tests. I hope that makes sense.

[Marc Perrin]

Re how I reach find_location() (well, finders.find()) with a path from another OS (well, an url path), see:
​https://github.com/evansd/whitenoise/issues/472
​https://github.com/evansd/whitenoise/blob/main/src/whitenoise/middleware.py#L158

But if you tell me the spec is that finders.find() must be called with an OS-standardized path, it's a valid answer to my concern, and it means the fix has to happen on whitenoise side (and we can close the present issue).

After all, finders.find() is in django.contrib.staticfiles indeed, and even though I found it surprising that it would not be more flexible, maybe it makes sense.

Replying to Mariusz Felisiak:

other finders (e.g. AppDirectoriesFinder) seem much more lenient

AppDirectoriesFinder had exactly the same code before removing AppStaticStorage, see f56c88a8eed91f68f6845bbf730f7b7361c5cfb1 and #21867.

In my case, when debugging I go into the return None a lot because os.sep is backslash, and the args of find_location are for example:

root = r'C:\some\folder\for\react\files'
path = 'bundled_react/some_file.hot-update.js'
prefix = 'bundled_react'

I'm not sure how you reached find_location() with a path from another OS. It seems that you're using it in a non-intended way and if so you need to standardize paths before passing them to FileSystemFinder on your own. Also, using commonpath() is not a proper change as it breaks many tests. I hope that makes sense.