Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undeprecate X-Frame-Options #25663

Merged
merged 1 commit into from
Jan 23, 2025
Merged

Conversation

lukewarlow
Copy link
Contributor

Summary

Unmark X-Frame-Options as deprecated

Test results and supporting details

This was added in #24466

When people view the X-Frame-Options page they're presented with:

image

A warning that says this feature may be removed from browsers isn't helpful for a security feature that we realistically cannot remove from browsers.

By all means MDN can and should (and does per mdn/content#35942) point to CSP frame-ancestors as a (more powerful) alternative, this is deprecation warning isn't useful nor accurate.

Related issues

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@github-actions github-actions bot added data:http Compat data for HTTP features. https://developer.mozilla.org/docs/Web/HTTP size:xs [PR only] 0-6 LoC changed labels Jan 15, 2025
@lukewarlow
Copy link
Contributor Author

This was discussed in a Web App Sec WG meeting and it was the opinion of many that this warning should be removed.

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the current wording of the deprecated banner is unfortunate, especially if it prompts users to remove the header without a corresponding CSP policy.

However, I don't think we should unmark this feature as deprecated in BCD just to get rid of the corresponding banner on MDN. It is a legacy feature after all, and BCD's notion of deprecated features includes this.

There is ongoing work by @Rumyra et al. to replace the banners this year, so we might want to wait until then.

If we see urgency, we might be able to adjust the wording to avoid that users draw the wrong conclusions.

/cc @Rumyra

@lukewarlow
Copy link
Contributor Author

I would argue that in this case the functionality isn't deprecated though even by BCDs definition. There's nothing wrong with using the header, it's never going to be removed. When you need more granularity or if you're otherwise using CSP then frame-ancestors is an alternative. But setting X-frame-options to deny or sameorigin is a legitimate approach?

@caugner
Copy link
Contributor

caugner commented Jan 15, 2025

BCD's guidelines actually mention the word "legacy" explicitly, see: https://github.com/mdn/browser-compat-data/blob/main/docs/data-guidelines/index.md#setting-deprecated

Is there a good reason to call this feature "legacy" in the spec, if it doesn't convey any notion of discouragement regarding it's use?

@caugner
Copy link
Contributor

caugner commented Jan 16, 2025

The way I see it we have three options:

  1. We don't change the spec or BCD, but update the wording of the banner on MDN.
  2. We ask the spec WG to remove the word "legacy", which allows us to un-deprecate the feature in BCD.
  3. We update the BCD guidelines and no longer consider "legacy" as deprecation.

@caugner caugner added the meeting agenda Issues or pull requests in need of discussion in a project meeting. label Jan 16, 2025
@tunetheweb
Copy link
Contributor

I agree the "legacy" wording in the spec is a little unclear. I've raised this issue to discuss that further: whatwg/html#10936

However I read the guidelines that the use of that word is "evidence for setting it to true" it is not a necessity, particularly given this comment:

Do not set deprecated to true for features that are merely old or unpopular, no matter how many considered harmful blog posts they may have garnered. For example, although web developers may prefer fetch over XMLHttpRequest, XMLHttpRequest is not deprecated.

Discouraging X-Frame-Options is the wrong thing to do here. For the basic use case X-Frame-Options is sufficient, there is no downside to using it, and the use of frame-ancestors offers no additional benefits (however, for more complex use cases it does). Additionally, CSP usage considerably trails X-Frame-Options (and only half of those use frame-ancestors!).

By discouraging X-Frame-Options (especially without advising the alternative CSP instead - which is complex to set up), MDN is encouraging sites to downgrade their security for no benefit.

@caugner
Copy link
Contributor

caugner commented Jan 23, 2025

@Elchi3 @ddbeck Any concerns with un-deprecating this header? The corresponding HTML spec change removing the "legacy" wording has landed.

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

FYI @wbamberg

@caugner caugner added needs content update This PR needs a corresponding update to mdn/content to update the documentation and removed meeting agenda Issues or pull requests in need of discussion in a project meeting. labels Jan 23, 2025
@caugner caugner changed the title Unmark X-Frame-Options as deprecated Undeprecate X-Frame-Options Jan 23, 2025
@caugner caugner merged commit 10dd18e into mdn:main Jan 23, 2025
8 checks passed
@lukewarlow lukewarlow deleted the undeprecate-xframe-options branch January 23, 2025 17:00
@lukewarlow
Copy link
Contributor Author

Thanks for everyone's help on this one!

@caugner
Copy link
Contributor

caugner commented Jan 23, 2025

@Elchi3 @wbamberg Is the status on the MDN page synced automatically?

Edit: Looks like @OnkarRuikar does this manually?

@wbamberg
Copy link
Contributor

wbamberg commented Jan 23, 2025

It's automated. From https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Page_structures/Feature_status#how_are_feature_statuses_specified_in_content:

you should consider these mechanisms read-only as their inclusion in the content is automated

@tunetheweb
Copy link
Contributor

i think the status is automated by the custom message about frame-ancestors warning at the top isn't. So opened mdn/content#37774 to remove both.

@wbamberg
Copy link
Contributor

i think the status is automated by the custom message about frame-ancestors warning at the top isn't. So opened mdn/content#37774 to remove both.

Oh yes, you're right, sorry I missed that. I've merged the content PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:http Compat data for HTTP features. https://developer.mozilla.org/docs/Web/HTTP needs content update This PR needs a corresponding update to mdn/content to update the documentation size:xs [PR only] 0-6 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants