Opened 14 years ago

Closed 13 years ago

Last modified 11 years 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 14 years ago.
XFrameOptionsMiddleware + tests
clickjack.diff (12.4 KB ) - added by rniemeyer 14 years ago.
Improved patch: added decorators, tests and some flexibility
clickjacking_patch_with_docs.txt (18.8 KB ) - added by rniemeyer 14 years ago.
Patch, tests and docs
clickjacking_patch_with_docs.diff (18.8 KB ) - added by rniemeyer 14 years ago.
Renamed patch to .diff i h8 eclipse

Download all attachments as: .zip

Change History (16)

by rniemeyer, 14 years ago

Attachment: clickjacking.diff added

XFrameOptionsMiddleware + tests

comment:1 by rniemeyer, 14 years ago

Needs documentation: set
Status: newassigned

comment:2 by Thejaswi Puthraya, 14 years ago

Component: UncategorizedHTTP handling

comment:3 by Daniel F Moisset, 14 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Łukasz Rekucki, 14 years ago

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.

by rniemeyer, 14 years ago

Attachment: clickjack.diff added

Improved patch: added decorators, tests and some flexibility

comment:5 by rniemeyer, 14 years ago

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

comment:6 by Luke Plant, 14 years ago

+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.

by rniemeyer, 14 years ago

Patch, tests and docs

by rniemeyer, 14 years ago

Renamed patch to .diff i h8 eclipse

in reply to:  6 comment:7 by rniemeyer, 14 years ago

Summary: Add middleware for setting X-Frame-Options HTTP header in responsesAdd 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 by Julien Phalip, 13 years ago

Severity: Normal
Type: New feature

comment:9 by Luke Plant, 13 years ago

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 by Luke Plant, 13 years ago

Resolution: fixed
Status: assignedclosed

In [16298]:

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

Many thanks to rniemeyer for the patch!

comment:11 by jfialkoff, 11 years ago

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 by Tim Graham, 11 years ago

I see @xframe_options_exempt on that page.

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