Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#32304 closed Bug (fixed)

Django adds spurious "/" prefix to settings.STATIC_URL="http://server/"

Reported by: Adam Hooper Owned by: Mariusz Felisiak
Component: contrib.staticfiles Version: 3.1
Severity: Release blocker Keywords:
Cc: Florian Apolloner Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Adam Hooper)

Here's a piece of settings from a totally reasonable, sensible, okay Docker integration-test environment

STATIC_URL = "http://minio/static/"

Django 3.1 will implicitly add "/" to the URL, so my URLs look like /http://minio/static/images/app-icons/favicon.ico

The features and bugs that interact here:

  • commit c574bec, adding feature #25598, prepends SCRIPT_NAME to STATIC_URL when STATIC_URL isn't a URL.
  • bug #9202 and #25418: according to Django, "http://minio/static/" isn't a valid URL. (It is.)

Top me, the easiest fix is to address #9202 / #25418. Or to make STATIC_URL use some logic that is different from URLValidator.

Change History (12)

comment:1 Changed 7 months ago by Adam Hooper

Description: modified (diff)

comment:2 Changed 7 months ago by Adam Hooper

Description: modified (diff)

comment:3 Changed 7 months ago by Adam Hooper

My workaround was to create a phony URL and point to it in /etc/hosts.

Yes, really. https://github.com/CJWorkbench/cjworkbench/commit/6aec10f441f5392bda7df247cddc8828b52a0c84

comment:4 Changed 7 months ago by Adam Hooper

Description: modified (diff)

comment:5 Changed 7 months ago by Mariusz Felisiak

Cc: Florian Apolloner added
Component: Core (Other)contrib.staticfiles
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Thanks for the report, as a workaround you can set the FORCE_SCRIPT_NAME setting to an empty string:

FORCE_SCRIPT_NAME = ''

Ticket #9202 was rejected and will not be fixed. I think it should be fine to add http:// and https:// to recognizing absolute paths, e.g.

diff --git a/django/conf/__init__.py b/django/conf/__init__.py
index 23fee7d5b7..c2ddc942db 100644
--- a/django/conf/__init__.py
+++ b/django/conf/__init__.py
@@ -139,7 +139,7 @@ class LazySettings(LazyObject):
         except (ValidationError, AttributeError):
             pass
         # Don't apply prefix to absolute paths.
-        if value.startswith('/'):
+        if value.startswith(('http://', 'https://', '/')):
             return value
         from django.urls import get_script_prefix
         return '%s%s' % (get_script_prefix(), value)

Florian, What do you think?

comment:6 Changed 7 months ago by Florian Apolloner

Uff, yes that is certainly a bug. I think your proposed fix is okay; but I'd also remove the usage of the URLValidator completely. Maybe:

diff --git a/django/conf/__init__.py b/django/conf/__init__.py
index 23fee7d5b7..fc36b64d05 100644
--- a/django/conf/__init__.py
+++ b/django/conf/__init__.py
@@ -16,7 +16,6 @@ from pathlib import Path
 import django
 from django.conf import global_settings
 from django.core.exceptions import ImproperlyConfigured, ValidationError
-from django.core.validators import URLValidator
 from django.utils.deprecation import RemovedInDjango40Warning
 from django.utils.functional import LazyObject, empty
 
@@ -132,14 +131,8 @@ class LazySettings(LazyObject):
         Useful when the app is being served at a subpath and manually prefixing
         subpath to STATIC_URL and MEDIA_URL in settings is inconvenient.
         """
-        # Don't apply prefix to valid URLs.
-        try:
-            URLValidator()(value)
-            return value
-        except (ValidationError, AttributeError):
-            pass
-        # Don't apply prefix to absolute paths.
-        if value.startswith('/'):
+        # Don't apply prefix to absolute paths and URLs.
+        if value.startswith(('/', 'http://', 'https://')):
             return value
         from django.urls import get_script_prefix
         return '%s%s' % (get_script_prefix(), value)

comment:7 Changed 7 months ago by Mariusz Felisiak

Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

comment:8 Changed 7 months ago by Hasan Ramezani

Mariusz,

I didn't realize that you assigned the ticket to yourself. I just created a PR based on Florian Apolloner proposed patch.
Feel free to close my PR if you have something else in your mind.

Thanks

comment:9 Changed 7 months ago by Mariusz Felisiak

Has patch: set

comment:10 Changed 7 months ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:11 Changed 7 months ago by GitHub <noreply@…>

Resolution: fixed
Status: assignedclosed

In e13b7140:

Fixed #32304 -- Fixed prefixing STATIC_URL and MEDIA_URL by SCRIPT_NAME for absolute URLs with no domain.

Thanks Adam Hooper for the report.

Regression in c574bec0929cd2527268c96a492d25223a9fd576.

comment:12 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 5fdc81d:

[3.1.x] Fixed #32304 -- Fixed prefixing STATIC_URL and MEDIA_URL by SCRIPT_NAME for absolute URLs with no domain.

Thanks Adam Hooper for the report.

Regression in c574bec0929cd2527268c96a492d25223a9fd576.
Backport of e13b71403bd1568abed237858127677144d43d23 from master

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