In the first draft of the X-Frame-Options spec, the Allow-From header was supposed to be formatted like this (note that colon before the domain):

X-Frame-Options: Allow-From: https://example.com

However, in the final spec there should be no colon:

X-Frame-Options: Allow-From https://example.com

SecKit currently includes the colon, which causes the header to be ignored by Internet Explorer.

Patch to follow.

CommentFileSizeAuthor
#18 2811351-18-X-Frame-Options.patch4.47 KBgg4
#17 interdiff-2811351-13-17.txt621 bytesgg4
#17 2811351-17-X-Frame-Options.patch8.9 KBgg4
#13 2811351-13-X-Frame-Options.patch8.47 KBgg4
#9 seckit-2811351-4.patch1.9 KBDane Powell
#5 seckit-2811351-2.patch720 bytespifagor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dane Powell created an issue. See original summary.

Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell at Acquia commented
Status: Active » Needs review
FileSize
seckit-2811351-2.patch1.05 KB

This removes the colon to fix compatibility with IE and bring it up to spec.

Status: Needs review » Needs work

The last submitted patch, 2: seckit-2811351-2.patch, failed testing.

Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell at Acquia commented
Status: Needs work » Needs review
FileSize
seckit-2811351-4.patch1.9 KB

Fixing test case.

pifagor’s picture

pifagor
Location 🇺🇦 Rivne
CreditAttribution: pifagor commented
Status: Needs review » Active
FileSize
seckit-2811351-2.patch720 bytes

Make sure your patch please

pifagor’s picture

pifagor
Location 🇺🇦 Rivne
CreditAttribution: pifagor commented
Status: Active » Needs review
pifagor’s picture

pifagor
Location 🇺🇦 Rivne
CreditAttribution: pifagor commented

No sooner :(
Diane Powell, add your patch #4 again, as in the simultaneous preservation disappeared. I'm sorry.

Status: Needs review » Needs work

The last submitted patch, 5: seckit-2811351-2.patch, failed testing.

Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell at Acquia commented
Status: Needs work » Needs review
FileSize
seckit-2811351-4.patch1.9 KB
jweowu’s picture

jweowu CreditAttribution: jweowu at Catalyst IT commented
Parent issue: » #2779169: Allow-From headers are incorrect
Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell at Acquia commented
pdenooijer’s picture

pdenooijer CreditAttribution: pdenooijer at Ibuildings commented
Status: Needs review » Reviewed & tested by the community

Patch 4 works fine :)!

gg4’s picture

gg4 CreditAttribution: gg4 commented
Status: Reviewed & tested by the community » Needs review
Related issues: +#2914000: Coding Standard fixes
FileSize
2811351-13-X-Frame-Options.patch8.47 KB

Following the spec a bit closer, see: https://www.owasp.org/index.php/Clickjacking_Defense_Cheat_Sheet and https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Frame-Options. HTTP verbs should be capitalized.

Patch depends on #2914000: Coding Standard fixes.

Status: Needs review » Needs work

The last submitted patch, 13: 2811351-13-X-Frame-Options.patch, failed testing. View results

gg4’s picture

gg4 CreditAttribution: gg4 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: 2811351-13-X-Frame-Options.patch, failed testing. View results

gg4’s picture

gg4 CreditAttribution: gg4 commented
Status: Needs work » Needs review
FileSize
2811351-17-X-Frame-Options.patch8.9 KB
interdiff-2811351-13-17.txt621 bytes
gg4’s picture

gg4 CreditAttribution: gg4 commented
FileSize
2811351-18-X-Frame-Options.patch4.47 KB

Re-roll of #17

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid commented
Status: Needs review » Fixed

I think we fixed all of this in the 8.x-1.x branch in #3000696: X-FRAME-OPTIONS header syntax should be all caps

AFAICS it's not an issue in the D7 branch.

Therefore I'm closing this - please reopen if I've missed something.

gg4’s picture

gg4 CreditAttribution: gg4 commented
Status: Fixed » Needs review

#18 is just code cleanup at this point, but still makes sense to commit for consistency of naming conventions.

  • mcdruid committed 9acf265 on 8.x-1.x authored by Dane Powell 
    Issue #2811351 by bonus, Dane Powell, pifagor: X-Frame-Options Allow-...
mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid commented
Status: Needs review » Fixed

Fair enough; I'm not that crazy about the capitalised labels e.g.:

-          label: 'X-Frame-Allow-From'
+          label: 'X-Frame-ALLOW-FROM'

...but that's pretty much the standard's fault, not yours :)

Thanks @bonus (and @Dane Powell for the original patch).

pifagor’s picture

pifagor
Location 🇺🇦 Rivne
CreditAttribution: pifagor at Drupal Ukraine Community, GOLEMS GABB for GOLEMS GABB, Drupal Ukraine Community commented

Thanks

Status: Fixed » Closed (fixed)

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