Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

FIX: Removes inline scripts for CSP compatibility #171

Merged
merged 1 commit into from
Oct 18, 2017

Conversation

rspeed
Copy link
Contributor

@rspeed rspeed commented Oct 17, 2017

In order to enforce an effective CSP, sites must disallow the use of inline scripts, as they are a common exploit vector for attacks. Unfortunately, django-admin-sortable2 uses them in three places to pass configuration data to the front-end. This causes it to break when such a CSP is enforced.

This PR fixes the issue by splitting the inline scripts into two parts. The configuration data is converted into JSON structures which are left in the templates and the logic is moved to external scripts. The commit message has a more detailed list of the changes.

* Converts configs in `stacked.html` and `tabular.html` to JSON.
* Adds `inline-stacked.js` and `inline-tabular.js` which load the respective configs and initialize sortable formsets.
* Adds the scripts to `SortableInlineAdminMixin.media`.
* Converts config in `change_list.html` to JSON.
* Updates `list-sortable.js` to load the config.
@jrief
Copy link
Owner

jrief commented Oct 17, 2017

Why should it be safer to disallow inlined JavaScript, rather than importing it? Can you point me onto a security site, where this attack vector is explained.

@rspeed
Copy link
Contributor Author

rspeed commented Oct 18, 2017

In short: avoiding inline scripts is a best practice because they can be an attack vector for cross-site scripting.

Here's Google's overview.

@jrief
Copy link
Owner

jrief commented Oct 18, 2017

OK, got it.
Please add a line or two to the changelog, explaining what you're doing.

Since I do configurations in other projects using the same pattern, I'll probably have to adopt them too. Hmmm.

@rspeed
Copy link
Contributor Author

rspeed commented Oct 19, 2017

Done. PR #172. The version of the next release is assumed to be 0.6.17.

And yeah, it's a good idea in general. I've been submitting PRs to a bunch of projects recently for similar reasons.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants