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

Make ContentFrame allow cross-origin clipboard-write permission #6003

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Jan 17, 2024

This PR ensures the iframe wrapped by ContentFrame allows clipboard-write from any origin.

This is required to make copying exported annotations work when proxying a site with via inside an LMS assignment.

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

The situation with regard to the clipboard-write permission is confusing. w3c/clipboard-apis#164 suggests that it was removed from the spec. However my understanding is that from our collective testing, Chrome still requires it so we'll have to implement it.

LGTM but can you update the comment to describe which applications need the permission, not which pieces of functionality use it, as that is going to get out of date.

// "fullscreen" - Enables full-screen button in player
allow="autoplay; clipboard-write; fullscreen"
allow="autoplay; clipboard-write *; fullscreen"
Copy link
Member

Choose a reason for hiding this comment

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

I wondered what the value was if the origin is not explicitly specified. According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Permissions_Policy for the allow attribute it is 'src' (ie. same origin as the URL in the iframe's src attribute).

In the context of Via's video player, the origin of the video player itself matches 'src', which explains why this worked. However the 'src' origin doesn't match the client's sidebar, which is why we're having to add the wildcard here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I tried to find a piece of documentation explaining that and couldn't find it for some reason.

@@ -17,9 +17,9 @@ export default function ContentFrame({ url, iframeRef }: ContentFrameProps) {
// too).
//
// "autoplay" - Enables Play button to work without first clicking on video
// "clipboard-write" - Used by "Copy transcript" button
// "clipboard-write *" - Used to copy YouTube transcripts and exported annotations, cross-origin
Copy link
Member

Choose a reason for hiding this comment

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

Rather than spelling out the individual features that use the permission, which is going to get outdated, it would be easier to spell out which applications need it: Via's video player, the Hypothesis client sidebar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense

@acelaya acelaya merged commit b8c402d into main Jan 18, 2024
8 checks passed
@acelaya acelaya deleted the cross-origin-clipboard branch January 18, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants