In #2514136: Add default clickjacking defense to core we are expecting to add a default clickjacking defense to Drupal 7 core. (Not right away, but probably in the next couple months or so.)

The patch there is designed to work with Security Kit as much as possible (and not override this module's settings) but there's one scenario where it's not able to do that: the scenario where a site administrator has configured Security Kit to disable clickjacking protection.

This patch makes it work with that also. It won't have any effect until that other issue goes into core, but I think it would be harmless to commit before then also.

CommentFileSizeAuthor
#12 seckit-integrate-with-core-setting-d8-2661644-12.patch1.53 KBkmoll
#5 seckit-integrate-with-core-setting-2661644-5.patch1 KBDavid_Rothstein
#2 seckit-integrate-with-core-setting-2661644-2.patch1.01 KBDavid_Rothstein

Comments

David_Rothstein created an issue. See original summary.

David_Rothstein’s picture

David_Rothstein commented
Status: Active » Needs review
StatusFileSize
new seckit-integrate-with-core-setting-2661644-2.patch1.01 KB

Here is the patch.

David_Rothstein’s picture

David_Rothstein commented
Issue summary: View changes

Fixed an important typo in the issue summary - I wrote "disable clickjacking" when I actually meant "disable clickjacking protection".

jweowu’s picture

jweowu commented
Status: Needs review » Needs work

Thanks David.

I'm inclined to split the SECKIT_X_FRAME_DISABLE case from the default case, here.

If the user has explicitly selected SECKIT_X_FRAME_DISABLE we want your code to take effect.

The default case would suggest an erroneous configuration, and I don't believe we should disable the core protection in that instance?

We could retain a default: // do nothing clause, but it seems cleaner to me to just remove it entirely?

David_Rothstein’s picture

David_Rothstein commented
Status: Needs work » Needs review
StatusFileSize
new seckit-integrate-with-core-setting-2661644-5.patch1 KB

I guess that makes sense. I can't see any reason why it should be called with the default, but just in case it ever is your idea sounds reasonable.

Here it is with the default case removed.

By the way, I just committed #2514136: Add default clickjacking defense to core so it will probably be included in a core release in about a month from now.

jweowu’s picture

jweowu commented
Status: Needs review » Fixed

Thanks David. Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jweowu’s picture

jweowu commented
Title: Integrate with upcoming Drupal core clickjacking defense » Integrate with Drupal core clickjacking defense
Version: 7.x-1.x-dev » 8.x-1.x-dev
Priority: Major » Normal
Status: Closed (fixed) » Active

It looks like this core feature, whilst new to Drupal 7, is already in Drupal 8.

https://www.drupal.org/node/2514136#comment-10078038

https://www.drupal.org/node/2514152

We should port this change to the 8.x version.

kmoll’s picture

kmoll commented

I will port this over.

kmoll’s picture

kmoll commented
Assigned: Unassigned » kmoll
kmoll’s picture

kmoll commented
StatusFileSize
new seckit-integrate-with-core-setting-d8-2661644-12.patch1.53 KB

Here is a patch for D8.

kmoll’s picture

kmoll commented
Status: Active » Needs review
jribeiro’s picture

jribeiro commented
Status: Needs review » Reviewed & tested by the community

+1 RTBC

badjava’s picture

badjava commented
Assigned: kmoll » badjava

I will review this today.

  • badjava committed 47f8641 on 8.x-1.x authored by kmoll 
    Issue #2661644 by David_Rothstein, kmoll, jweowu, badjava, jribeiro:...
badjava’s picture

badjava commented
Assigned: badjava » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks to everyone for helping fix this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.