ampproject / ampproject/amphtml

FR: `sandbox` iframes in `amp-facebook` components

Open
#34,181 2 comments 0 reactions 0 assignees View on GitHub
Component: amp-facebook Component: amp-facebook-comments Stale Type: Feature Request WG: components
Dominant language
JavaScript
Stars
14.9k
Forks
4.1k
PR merge metrics
No merged PRs in 30d

Description

We currently exclude `amp-facebook`, `amp-facebook-comments`, `amp-facebook-like`, and `amp-facebook-page` from applying `sandbox` attributes to their generated `iframe` element:
https://github.com/ampproject/amphtml/blob/36e818946a3231685e407297b8180a48f869f626/src/3p-frame.js#L158-L161

This is a tracking issue to introduce `sandbox` attributes for these components, which _may_* no longer be critical.

*Our manual tests show that embedded Facebook posts of all the above types don't appear to be affected aside from one side affect: an alarming error to the console, likely due to the 3p script altering `document.domain`, which is a `sandbox`-non-compliant operation. For all other intents and purposes the component appears to continue to work despite throwing this error.
![image](https://user-images.githubusercontent.com/10456171/116926681-a44b4e00-ac28-11eb-92eb-563ce44f1672.png)

It is possible that we were not able to detect potential breakages due to confirmation bias from manually testing our examples exclusively, and that [Domain Verification](https://developers.facebook.com/docs/sharing/domain-verification/) may have something to do with the `document.domain` access that [historically caused `sandbox` FB iframes to break](https://github.com/ampproject/amphtml/blob/36e818946a3231685e407297b8180a48f869f626/extensions/amp-facebook/0.1/test/test-amp-facebook.js#L72-L74).

@alanorozco suggests to verify if we can safely introduce `sandbox` by accessing facebook analytics from embedding comments configured to a certain domain or facebook page.

[Related discussion](https://github.com/ampproject/amphtml/pull/33852#discussion_r617587842)

Contributor guide

Open the contributing guide

Research direction

Start in src/3p-frame.js at the sandbox exclusion and review extensions/amp-facebook/0.1/test/test-amp-facebook.js around the historical document.domain failure. Check the related discussion in PR #33852, then verify sandboxed amp-facebook, comments, like, and page embeds, including comments configured for a domain or Facebook page. Done means the components and relevant analytics continue working without unacceptable errors or regressions.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
frontend, security
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.