[Django] #27590: Prevent public access of staticfiles manifest
Django
------------------------------------------------+------------------------
Reporter: David Sanders | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
A standard Django deploy has all staticfiles accessible to all users. This
is understandable, if undesirable. By itself this is not a huge problem
since those on the public Internet don't know the filenames of all of the
files a deployment has, and fuskering the entire possible namespace isn't
feasible and is also detectable.
However, deployments that make use of `ManifestStaticFilesStorage` will
most likely expose a master list of all static files to anyone who wants
to look. It's not a huge security risk because you shouldn't be depending
on security through obscurity, but there's certainly a leg up given when
there's a master list of all files. Due to the way
`ManifestStaticFilesStorage` is setup, the manifest ends up in the
directory of publicly served files. If the files are stored locally this
can be fixed by blacklisting the file from webserver access and only
letting Django itself read the file off the local filesystem. This is the
approach I've taken once I discovered the issue - I have a server
deployment running Apache serving files on the local filesystem, but have
CloudFront in front of that which fetches from Apache if the cache misses.
I've since blacklisted the staticfiles manifest and invalidated any cached
copies in CloudFront.
Here's what I consider the risks of having a publicly exposed staticfiles
manifest:
- Easily find trade secrets in JavaScript files meant to be used only
internally by staff users
- Find hardcoded secrets in internal files - anything in the static tree
gets listed here, even pre-processed files like coffee or less if the
developers use django-compressor
- Find potential attack vectors by finding normally unlisted files that
are exploitable which could be used to form URLs in phishing emails
- Possible novel way to fingerprint Django versions using the easy master
list of files, could be used to quickly identify potentially vulnerable
Django servers
All that said, I don't have a great solution to the problem that Django
itself could implement. Currently Django writes the manifest to the
staticfiles root so it's always going to be readable unless you take extra
steps. The real stickler is deployments that use something like
[https://github.com/jschneier/django-
storages/blob/master/storages/backends/s3boto.py#L189 S3BotoStorage] which
in effect needs Django to be able to access the manifest remotely. My
understanding of that setup (I don't use it) would be that on load Django
is going to read the manifest from S3, so it needs to be accessible over
the web by default. Further steps could be taken to make it only
accessible to Django itself, but that requires user action.
Potential solutions:
- Encrypt the manifest on disk, decrypt on load into memory - loses human
readability for debugging purposes but hides it from prying eyes by
default
- Fast-track ticket #26029 to make staticfiles storage configuration allow
passing options to storage - use options to change manifest path somewhere
non-public or configure a secret header to use with S3 to only give Django
access to the file.
On a related note, this discovery has made me extra paranoid about the
exposure of internal files meant for staff only and now I'm looking at a
way to formalize restricted access to the files. With the exposure of the
staticfiles manifest it's clear much of the business logic we use (in
JavaScript under admin) is by default visible to the Web if you know the
URL.
--
Ticket URL: <https://code.djangoproject.com/ticket/27590>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: David Sanders | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Comment:
Florian says,
I think we should just bite the dust and add a setting which the static
manifest gets written to (directly, without involving storage). Moving
this file out of `STATIC_ROOT` is the only sensible solution I see.
`S3BotoStorage` should redirect the manifest to a different (secure)
bucket if needed. Maybe we can make it easier to detect that this file is
"sensitive".
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:1>
Django
Reporter: David Sanders | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Kevin Christopher Henry):
At #28764 I reached a similar conclusion for very different reasons.
I don't see much point in making the location configurable, though. It
seems to me that the file should just be stored to some standard location
within the codebase. The obvious analogy here would be to
`makemigrations`, which does the same thing. As I argue in the other
ticket, this is a configuration file that affects Django's behavior, is
tied to a specific commit, and has nothing to do with the actual serving
of static files. Storing it in the codebase would solve David's issues
above, would solve the correctness and performance issues I mentioned, and
would do so for all users without any new settings. Are there advantages
to storing the file in an external location that I've overlooked?
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:2>
Django
Reporter: David Sanders | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Kevin Christopher Henry):
* cc: Kevin Christopher Henry (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:3>
Django
Reporter: David Sanders | Owner: jwygoda
Type: Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jwygoda):
* owner: nobody => jwygoda
* status: new => assigned
Comment:
I've created a pr adding STATICFILES_MANIFEST setting allowing storing
manifest file with code.
https://github.com/django/django/pull/12187
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:4>
Django
Reporter: David Sanders | Owner: Jarosław
Type: | Wygoda
Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carsten Fuchs):
* cc: Carsten Fuchs (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:5>
Django
Reporter: David Sanders | Owner: Jarosław
Type: | Wygoda
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carsten Fuchs):
Maybe the `staticfiles.json` should also be "pretty printed" as a part of
this change?
After this change, many users will likely keep this file with the project
code and thus under version control. Having a well defined indentation and
key order would keep the diffs small and comprehensible.
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:6>
Django
Reporter: David Sanders | Owner: Jarosław
Type: | Wygoda
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carsten Fuchs):
Replying to [comment:4 Jarosław Wygoda]:
> I've created a pr adding STATICFILES_MANIFEST setting allowing storing
manifest file with code.
> https://github.com/django/django/pull/12187
Will you set the "has patch" flag for this ticket?
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:7>
Django
Reporter: David Sanders | Owner: Jarosław
Type: | Wygoda
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: David Sanders | Owner: Jarosław
Type: | Wygoda
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
I think we need to look at where we're headed before we simply add a
setting here. If we consider the upgrade path, what we don't want to do is
add a new setting and then need to deprecate it as soon as we improve the
configuration options, with something along the lines of #26029.
#26029 is needed because you currently need to provide a storage subclass
in order to provide configuration parameters:
{{{
# In settings.py, where `CustomStorage` adjusts configuration in
`__init__()`.
STATICFILES_STORAGE = 'my_app.CustomStorage'
}}}
The complaint in #26029 is that providing the subclass is cumbersome, and
we should be able to specify a dictionary like `DATABASES` and so on.
But we should solve that problem over there.
The issue here is that `ManifestStaticFilesStorage` has no way of
specifying where to store the manifest.
There's a
[https://code.djangoproject.com/attachment/ticket/27541/manifest_storage.patch
patch file] for #27541 (which I'm going to close as a duplicate of this
ticket) which suggests adding an optional `manifest_storage` kwarg to
`ManifestFilesMixin` — here you'd pass a configured storage to override
where to store the manifest file:
{{{
# Something like...
class CustomManifestStorage(ManifestStaticFilesStorage):
def __init__(self, *args, **kwargs):
manifest_storage = FileSystemStorage(
location='path/for/manifest'
)
super().__init__(*args, manifest_storage=manifest_storage,
**kwargs)
}}}
Using a storage covers the ''separate s3 bucket'' use-case (and similar)
but also avoids the cumbersome
[https://github.com/django/django/pull/12187/files#diff-
f5c7100e3528e9f6edb98cd5a3d33133bdde6286a89fa472f89980fb07364a8eR366
ManifestHandler wrapper from the initial PR here].
I think with that, and tests, and documentation of the new parameter, with
an example of how to configure it, we have a fix here. (That the
configuration is a little cumbersome would hopefully spur renewed interest
in pushing forward with #26029.)
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: David Sanders | Owner: Jarosław
Type: | Wygoda
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Kevin Christopher Henry):
I argued in #28764 and comment:2 that the location of `staticfiles.json`
shouldn't be configurable at all, that it should just be stored in the
code base along with other configuration files (including automatically
generated ones, like migration files). What are the use cases for storing
this file anywhere else?
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: David Sanders | Owner: Jarosław
Type: | Wygoda
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Jarosław Wygoda):
Making the location of `staticfiles.json` configurable allows us to
maintain a backward compatibility by setting the default manifest path to
`STATIC_ROOT`. If we don't want to maintain a backward compatibility then
I can make manifest location not configurable and get rid of
`ManifestHandler`. Manifest will be created in the `BASE_DIR` instead.
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: David Sanders | Owner: Jarosław
Type: | Wygoda
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Kevin Christopher Henry):
If we take a step back, the problem here isn't that there's no way to
configure the manifest location, it's that the standard location for it is
bad. That's the reason people want to change it. So rather than making it
''possible'' for people to avoid the problem by introducing a new
customization mechanism, we should just fix the standard value so that no
one has the problem in the first place.
If we accept that the default manifest location needs to change at some
point then there will inevitably be a backwards incompatibility.
One way to address that would be to just document in the release notes
that people using `ManifestStaticFilesStorage` need to run `collectstatic`
after upgrading. If they don't do that they will immediately see a helpful
error message at startup since the manifest won't be found, and they can
fix the problem then. Still, perhaps that's overly burdensome, I'm not
sure.
Another approach would be to have a deprecation cycle where `STATIC_ROOT`
continues to be checked if the manifest isn't found at the designated
location in the code base, showing a deprecation warning in that case.
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: David Sanders | Owner: Jarosław
Type: | Wygoda
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by David Sanders):
Kevin, I think you're neglecting how `collectstatic` is used, and has been
historically used. Changing the default to storing the manifest in the
codebase would be a breaking change for some users. The
`ManifestFilesMixin` hashes files based on content, and the content is
going to depend on people's configurations, and the content will likely
differ depending on the settings used between development and production.
Minifying JS for production is a common use-case, but not for development.
If the manifest by default lives in the codebase, it would both need to be
re-generated after any changes (change in development process), and would
end up with the output for the development environment. Django would
probably need to give a large caveat to ignore the file for source control
otherwise users will shoot themselves in the foot quite often.
The issue I originally brought up with `S3BotoStorage` is that
`ManifestFilesMixin` is a mixin, so adding that to a storage class like
`S3BotoStorage` means it will read and write the manifest from that
storage - in that case from S3. Changing the behavior so that the manifest
doesn't use that storage class could be unintuitive for users. It will
have particular ramifications to how and when they call `collectstatic`
for their deployments - when using a class like `S3BotoStorage` that sends
the files to remote storage, they may not be currently invoking
`collectstatic` at a point where it can write out the manifest into the
codebase and have that change be persistent and make it into the
deployment of the code to production as well. So there'd be a lot of
assumptions about how people are currently using it.
I haven't looked at this issue in 4 years, so that's going from memory.
But I don't believe there's a simple "change the default location"
solution given how many ways `collectstatic` might be used - you're
probably going to break it for somebody. If there were a clear win here I
think I probably would have just made a PR instead of this issue. :-) I
spent a decent amount of time thinking through the problem before punting
it to this issue. I fear if you make it a hard-coded change to the
default, even with a deprecation cycle you'll end up with people coming
out of the woodwork on why they *have* to have it working the way it
originally did, and if there's no configuration ability then you've got a
problem there.
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: David Sanders | Owner: Jarosław
Type: | Wygoda
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
The breaking change is important. We can't just bring that in.
Adding a setting **might** be OK — folks could set it to STATIC_ROOT to
get the old value — BUT #26029 is about restructuring all this anyway: to
add a setting only to need to deprecate it is too much churn. (Even
assuming we'd take the hit on breaking people's deployments as they failed
to make the changes in time.)
Adding the manifest_storage kwarg (taking a storage) is small and simple.
That's why I think we should advance with it. Later, when #26029 comes
along, we can review the exact API we want there — we can either configure
`manifest_storage` directly in the settings, or allow a path there that
will used internally — but that's a separate conversation (one we can
defer).
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: David Sanders | Owner: Jarosław
Type: New feature | Status: assigned
Component: contrib.staticfiles | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* type: Cleanup/optimization => New feature
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: David Sanders | Owner: Jarosław
| Wygoda
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_better_patch: 1 => 0
* version: 1.10 => dev
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: David Sanders | Owner: Jarosław
| Wygoda
Type: New feature | Status: assigned
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: David Sanders | Owner: Jarosław
| Wygoda
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d3c4696596822281c0a5d4b9e3ee27732a4ce092" d3c4696]:
{{{
#!CommitTicketReference repository=""
revision="d3c4696596822281c0a5d4b9e3ee27732a4ce092"
Fixed #27590 -- Allowed customizing a manifest file storage in
ManifestFilesMixin.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27590#comment:18>