Opened 2 years ago

Closed 2 weeks ago

#34497 closed Bug (invalid)

ManifestStaticFilesStorage skips import and export of javascript modules with absolute paths

Reported by: Hielke Walinga Owned by: Hielke Walinga
Component: contrib.staticfiles Version: 4.2
Severity: Normal Keywords: ManifestStaticFilesStorage ES modules
Cc: blighj Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The recently added support for finding the ES modules does not work for absolute paths. As this is skipped.

It is not uncommon to import from the absolute path.

import function from "/appname/file.js"

The current implementation just skips this.

A change in the code can be made to find these correctly.

Current:

            # Ignore absolute/protocol-relative and data-uri URLs.
            if re.match(r"^[a-z]+:", url):
                return matched

            # Ignore absolute URLs that don't point to a static file (dynamic
            # CSS / JS?). Note that STATIC_URL cannot be empty.
            if url.startswith("/") and not url.startswith(settings.STATIC_URL):
                return matched

            # Strip off the fragment so a path-like fragment won't interfere.
            url_path, fragment = urldefrag(url)

            # Ignore URLs without a path
            if not url_path:
                return matched

            if url_path.startswith("/"):
                # Otherwise the condition above would have returned prematurely.
                assert url_path.startswith(settings.STATIC_URL)
                target_name = url_path[len(settings.STATIC_URL) :]
            else:
                # We're using the posixpath module to mix paths and URLs conveniently.
                source_name = name if os.sep == "/" else name.replace(os.sep, "/")
                target_name = posixpath.join(posixpath.dirname(source_name), url_path)

            # Determine the hashed name of the target file with the storage backend.
            hashed_url = self._url(
                self._stored_name,
                unquote(target_name),
                force=True,
                hashed_files=hashed_files,
            )

            transformed_url = "/".join(
                url_path.split("/")[:-1] + hashed_url.split("/")[-1:]
            )

Proposed change:

            # Ignore absolute/protocol-relative and data-uri URLs.
            if re.match(r"^[a-z]+:", url):
                return matched

            # Strip off the fragment so a path-like fragment won't interfere.
            url_path, fragment = urldefrag(url)

            # Ignore URLs without a path
            if not url_path:
                return matched

            if url_path.startswith('/'):
                if url_path.startswith(settings.STATIC_URL):
                    target_name = url_path[len(settings.STATIC_URL):]
                else:
                    target_name = url_path[1:]
            else:
                # We're using the posixpath module to mix paths and URLs conveniently.
                source_name = name if os.sep == '/' else name.replace(os.sep, '/')
                target_name = posixpath.join(posixpath.dirname(source_name), url_path)

            try:
                # Determine the hashed name of the target file with the storage backend.
                hashed_url = self._url(
                    self._stored_name, unquote(target_name),
                    force=True, hashed_files=hashed_files,
                )
            except ValueError:
                # Ignore absolute URLs that don't point to a static file (dynamic
                # CSS / JS?). Note that STATIC_URL cannot be empty.
                if url.startswith("/") and not url.startswith(settings.STATIC_URL):
                    return matched
                else:
                    raise

            transformed_url = "/".join(
                url_path.split("/")[:-1] + hashed_url.split("/")[-1:]
            )

PR

I could make a PR if this seems good. Thank you.

Change History (6)

comment:1 by Claude Paroz, 2 years ago

Triage Stage: UnreviewedAccepted

PR welcome, of course!

comment:3 by David Sanders, 2 years ago

Has patch: set
Owner: changed from nobody to Hielke Walinga
Status: newassigned

comment:4 by Mariusz Felisiak, 2 years ago

Needs tests: set

comment:5 by blighj, 3 months ago

Cc: blighj added

I think this is working as intended for the current way ManifestStaticFilesStorage works.
If you use an absolute url that is outside /static like the example given

import function from "/appname/file.js"

I think it is correct that this is not ManifestStaticFilesStorage's responsibility. It's not part of the django project's static file, if it was it would be in a static folder and we'd expect it to be served with /static.

importmaps blur this line, but they aren't widly used yet, and it's not clear to me how the code should account for them at this point.

Based on that understanding I'd suggest to close this ticket.

comment:6 by blighj, 2 weeks ago

Resolution: invalid
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top