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.
Comments
Comment #2
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedHere is the patch.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedFixed an important typo in the issue summary - I wrote "disable clickjacking" when I actually meant "disable clickjacking protection".
Comment #4
jweowu CreditAttribution: jweowu commentedThanks David.
I'm inclined to split the
SECKIT_X_FRAME_DISABLE
case from thedefault
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?Comment #5
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.
Comment #7
jweowu CreditAttribution: jweowu commentedThanks David. Committed.
Comment #9
jweowu CreditAttribution: jweowu commentedIt 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.
Comment #10
kmoll CreditAttribution: kmoll at Appnovation for Pfizer, Inc. commentedI will port this over.
Comment #11
kmoll CreditAttribution: kmoll at Appnovation for Pfizer, Inc. commentedComment #12
kmoll CreditAttribution: kmoll at Appnovation for Pfizer, Inc. commentedHere is a patch for D8.
Comment #13
kmoll CreditAttribution: kmoll at Appnovation for Pfizer, Inc. commentedComment #14
jribeiro CreditAttribution: jribeiro at CI&T for Pfizer, Inc. commented+1 RTBC
Comment #15
badjava CreditAttribution: badjava at Metasun for Pfizer, Inc. commentedI will review this today.
Comment #17
badjava CreditAttribution: badjava at Metasun for Pfizer, Inc. commentedThanks to everyone for helping fix this issue.