Code

Opened 4 years ago

Closed 3 years ago

Last modified 12 months ago

#14261 closed New feature (fixed)

Add clickjacking protection (X-Frame-Options header)

Reported by: rniemeyer Owned by: rniemeyer
Component: HTTP handling Version: 1.2
Severity: Normal Keywords: clickjacking x_frame_options
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Overview

For security reasons, many sites implement some form of clickjacking protection. Now that most of the modern browsers (IE8+, Firefox 3.6.9+, Chrome 4.1+, Safari 4+, Opera 10.5+) support the X-Frame-Options header, it seems to make sense for Django to support it as well.

Details

Included is a patch for a middleware (based off Paul Osman's work) that will set the X-Frame-Options header for all responses. By default, sets it to 'DENY', but allows for a settings.py value if 'SAMEORIGIN' is desired instead.

I stuck this in a new clickjacking middleware module, but it could obviously go somewhere else if that's not the best location.

Why?

While this is a rather trivial piece of code, it still feels like a worthwhile addition to Django for PR and "batteries included" reasons. If that's not generally agreed upon, I can open up a discussion on django dev. If this is deemed a good idea, then I can add docs to go along with the code/tests.

Attachments (4)

clickjacking.diff (3.9 KB) - added by rniemeyer 4 years ago.
XFrameOptionsMiddleware + tests
clickjack.diff (12.4 KB) - added by rniemeyer 3 years ago.
Improved patch: added decorators, tests and some flexibility
clickjacking_patch_with_docs.txt (18.8 KB) - added by rniemeyer 3 years ago.
Patch, tests and docs
clickjacking_patch_with_docs.diff (18.8 KB) - added by rniemeyer 3 years ago.
Renamed patch to .diff i h8 eclipse

Download all attachments as: .zip

Change History (16)

Changed 4 years ago by rniemeyer

XFrameOptionsMiddleware + tests

comment:1 Changed 4 years ago by rniemeyer

  • Needs documentation set
  • Needs tests unset
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 4 years ago by thejaswi_puthraya

  • Component changed from Uncategorized to HTTP handling

comment:3 Changed 4 years ago by dmoisset

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 4 years ago by lrekucki

  • Patch needs improvement set

It would be good to have a decorator to disable this - something that works like csrf_excempt, but it should allow you to choose between 'deny', 'sameorigin' or disabling it all together. I have at least one form that's embedded in client's page via an IFrame, so if I understand correctly adding this would prevent it from working correctly.

Changed 3 years ago by rniemeyer

Improved patch: added decorators, tests and some flexibility

comment:5 Changed 3 years ago by rniemeyer

The documentation is nearly complete. Will include in updated patch sometime during Pycon.

comment:6 follow-up: Changed 3 years ago by lukeplant

+1, I was going to suggest it myself. The patch looks pretty good. After Django 1.3 is out, we should have some discussion on django-devs about:

  • what the default value should be (I think SAMEORIGIN would make it better for general use, with very little decrease in security).
  • whether we can avoid a new setting
  • whether the middleware should be on by default or in the project template.

Changed 3 years ago by rniemeyer

Patch, tests and docs

Changed 3 years ago by rniemeyer

Renamed patch to .diff i h8 eclipse

comment:7 in reply to: ↑ 6 Changed 3 years ago by rniemeyer

  • Summary changed from Add middleware for setting X-Frame-Options HTTP header in responses to Add clickjacking protection (X-Frame-Options header)

Updated patch with docs (thanks to Sir @elliotthoffman) and changed default from DENY to SAMEORIGIN.

Opened a discussion: http://groups.google.com/group/django-developers/browse_thread/thread/12a590c6fadc0a61

Does the version of this ticket need to be changed? I don't recall setting it, so not sure how that works...

comment:8 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to New feature

comment:9 Changed 3 years ago by lukeplant

  • Easy pickings unset
  • Patch needs improvement unset

Thanks for the great patch, I will commit it shortly.

I did have to change a fair number of things - it was easier for me to do so because I'm familiar with all our standards etc. Since I'd like to encourage you to continue contributing to Django, but want to make it easier to add your contributions, I've included below the list of things I changed, mainly to highlight things you probably wouldn't have been aware of:

  • Docs:
    • wrapped to 80 characters, as per our guidelines for writing docs
    • added some blanklines after headers. This makes it easier to re-wrap in editors, and is consistent with the rest of our docs.
    • corrected some indentation, so that lists appeared correctly formatted.
    • moved the main docs file out of the contrib directory - that's for contrib apps. (The CSRF docs are currently in the wrong place, because that used to be a contrib app, but now is core, and I don't think we've got a good way of doing re-direction yet).
    • added a 'versionadded' Sphinx directive
    • fixed some Python syntax errors (noticed from the lack of code highlighting in the built docs.)
    • used standard capitalization of X-Frame-Options throughout.
    • other misc spelling corrections to docs.
    • added section to ref/middleware.txt
    • added section to ref/settings.txt
  • Code:
    • added setting to django/conf/global_settings.py
    • removed trailing whitespace in various files. (You can often configure your editor to show this, and most colorized diff tools, like colordiff, will highlight this - they can often be used in conjuction with 'svn diff' etc.).
    • used standard capitalization of X-Frame-Options in response header.
    • changed tearDown in tests so that it restores the original value of the setting (which will now always be there, since we added it to gloab_settings.py), rather than remove any setting.
    • removed a Python 2.4 fallback that's no longer necessary.

And also:

  • as discussed, I added the middleware to the project template, commented out.
  • added this to the release notes, as a worthy feature addition.
  • added you to AUTHORS. I put your Google profile as the link - please say if you'd prefer your e-mail address or a different website.

Finally, as a matter of process, you should update the 'Patch needs improvement/Needs docs/Needs tests' flags when you think you've addressed any issues - otherwise the committers will generally wait for the original author to fix the patch up unless they are particularly motivated about the feature/bug.

comment:10 Changed 3 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from assigned to closed

In [16298]:

Fixed #14261 - Added clickjacking protection (X-Frame-Options header)

Many thanks to rniemeyer for the patch!

comment:11 Changed 12 months ago by jfialkoff

  • UI/UX unset

Hopefully I'm not missing it, but it seems like this page (https://docs.djangoproject.com/en/dev/ref/clickjacking/) should mention the exempt decorator but does not.

comment:12 Changed 12 months ago by timo

I see @xframe_options_exempt on that page.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.