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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein created an issue. See original summary.

David_Rothstein’s picture

Status: Active » Needs review
FileSize
1.01 KB

Here is the patch.

David_Rothstein’s picture

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

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

Status: Needs work » Needs review
FileSize
1 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

Status: Needs review » Fixed

Thanks David. Committed.

Status: Fixed » Closed (fixed)

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

jweowu’s picture

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

I will port this over.

kmoll’s picture

Assigned: Unassigned » kmoll
kmoll’s picture

Here is a patch for D8.

kmoll’s picture

Status: Active » Needs review
jribeiro’s picture

Status: Needs review » Reviewed & tested by the community

+1 RTBC

badjava’s picture

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

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.