Opened 10 years ago

Closed 8 years ago

#22972 closed Bug (duplicate)

HashedFilesMixin.patterns should limit URL matches to their respective filetypes

Reported by: alex.ehlke@… Owned by: aehlke
Component: contrib.staticfiles Version: dev
Severity: Normal Keywords: HashedFilesMixin CachedFilesMixin staticfiles
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

HashedFilesMixin contains a patterns property which maps file extensions to regex patterns. This works great for the default case that Django ships with "*.css" as the only filetype, but results in surprising behavior once extended with multiple filetypes. I would expect the regex patterns to only apply to the filetype they're categorized under, but once I added "*.js" to patterns (via subclassing), the CSS rules also applied to my JS files.

My use-case of extending this is to add URL rewriting to JS source map references, e.g. //# sourceMappingURL=foo.js.map which can appear at the end of JS files. I need to be able to rewrite these URLs in the same way that Django's staticfiles can rewrite URLs in CSS, for e.g. adding hashes to filenames.

Once I tried extending patterns with this:

    patterns = HashedFilesMixin.patterns + (
        ("*.js", (
            (r"""(//# sourceMappingURL=(\s*))""", """//# sourceMappingURL=%s"""),
        )),
    )

The surprising and broken behavior was that Django tried to rewrite "URLs" for this JS (from Backbone.js): this.loadUrl(window.location.hash), which matched the CSS pattern for rewriting url(foo) because the regex patterns are case-insensitive.

This also applies to the previous CachedFilesMixin from before 1.7.

Please let me know if a fix for this would be accepted, and I'll put together a test case and patch.

Change History (14)

comment:1 by anonymous, 10 years ago

Summary: HashedFilesMixinHashedFilesMixin.patterns should limit URL matches to their respective filetypes

comment:2 by aehlke, 10 years ago

Owner: changed from nobody to aehlke
Status: newassigned

comment:3 by aehlke, 10 years ago

I'm preparing a test case and patch.

comment:4 by aehlke, 10 years ago

I've submitted a pull request with test coverage (a test which fails before applying the patch) and patch for the fix. Please let me know if anything needs changing.

https://github.com/django/django/pull/2895

Last edited 10 years ago by aehlke (previous) (diff)

comment:5 by aehlke, 10 years ago

Has patch: set

comment:6 by aehlke, 10 years ago

Type: UncategorizedBug

comment:7 by aehlke, 10 years ago

Version: 1.7-rc-1master

comment:8 by aehlke, 10 years ago

Resolution: fixed
Status: assignedclosed

comment:9 by aehlke, 10 years ago

This patterns property is also documented in the Django docs, which I think adds priority to getting this fixed as it makes it sound like something you can extend (where in current versions you cannot actually extend without subtly broken behavior.)

comment:10 by Tim Graham, 10 years ago

Resolution: fixed
Status: closednew

A ticket shouldn't be closed until the fix is committed to Django.

comment:11 by aehlke, 10 years ago

timo: Ah ok, sorry about that. Thanks.

comment:12 by Tim Graham, 10 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

I left comments for improvement on the PR. Please uncheck "Patch needs improvement" when you update it, thanks.

comment:13 by James Aylett, 8 years ago

Is this the same as #19670?

in reply to:  13 comment:14 by Claude Paroz, 8 years ago

Resolution: duplicate
Status: newclosed

Replying to jaylett:

Is this the same as #19670?

I think so.

Note: See TracTickets for help on using tickets.
Back to Top