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

fix: emit allow attribute on iframe for the clipboard (fixes #3474) #3475

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

vmiklos
Copy link
Contributor

@vmiklos vmiklos commented Feb 16, 2024

As described at
https://sites.google.com/a/chromium.org/dev/Home/chromium-security/deprecating-permissions-in-cross-origin-iframes newer Chrome requires explicit markup for code in an iframe to execute JS that requires permissions, like clipboard.

If this markup is missing, then the user won't be even asked. Use the wildcard syntax, because the COOL JS code in the iframe is not the initial src attribute value of the iframe, it gets changed later.

With this, a permission popup on paste shows up in Chrome even if the paste is perssed on the notebookbar, even if nextcloud is served from one domain and COOL is served from an other domain.

This fixes the document edit case; possibly it should be also added at all other places where the allowfullscreen attribute is used, which is not done in this commit.

…d#3474)

As described at
<https://sites.google.com/a/chromium.org/dev/Home/chromium-security/deprecating-permissions-in-cross-origin-iframes>
newer Chrome requires explicit markup for code in an iframe to execute
JS that requires permissions, like clipboard.

If this markup is missing, then the user won't be even asked.  Use the
wildcard syntax, because the COOL JS code in the iframe is not the
initial src attribute value of the iframe, it gets changed later.

With this, a permission popup on paste shows up in Chrome even if the
paste is perssed on the notebookbar, even if nextcloud is served from
one domain and COOL is served from an other domain.

This fixes the document edit case; possibly it should be also added at
all other places where the allowfullscreen attribute is used, which is
not done in this commit.

Signed-off-by: Miklos Vajna <vmiklos@collabora.com>
@vmiklos
Copy link
Contributor Author

vmiklos commented Feb 19, 2024

To clarify, this patch fixes the main, editing use-case. There are other places where allowfullscreen is used, and possibly this new allow attribute should be added there as well. One place is the read-only view of past revisions. I think we currently don't support copy for read-only views, so it's not useful to have this attribute there (although it doesn't hurt, either).

@vmiklos
Copy link
Contributor Author

vmiklos commented Feb 19, 2024

@juliushaertl could you please review this? Thanks.

@juliushaertl juliushaertl added bug Something isn't working 3. to review Ready to be reviewed labels Feb 19, 2024
@juliushaertl
Copy link
Member

To clarify, this patch fixes the main, editing use-case. There are other places where allowfullscreen is used, and possibly this new allow attribute should be added there as well. One place is the read-only view of past revisions. I think we currently don't support copy for read-only views, so it's not useful to have this attribute there (although it doesn't hurt, either).

Yes, would be good to just apply that consistently to any iframe:

files.js
98:		const $iframe = $('<iframe data-cy="documentframe" id="richdocumentsframe" nonce="' + btoa(OC.requestToken) + '" scrolling="no" allowfullscreen src="' + documentUrl + '" />')

document.js
208:			// iframe that contains the Collabora Online Viewer
209:			const frame = '<iframe data-cy="coolframe" id="loleafletframe" name="loleafletframe_viewer" allowfullscreen nonce="' + btoa(getRequestToken()) + '" style="width:100%;height:100%;position:absolute;"/>'
268:			// iframe that contains the Collabora Online
269:			const frame = '<iframe data-cy="coolframe" id="loleafletframe" name="loleafletframe" nonce="' + btoa(getRequestToken()) + '" scrolling="no" allowfullscreen style="width:100%;height:100%;position:absolute;" />'

@vmiklos
Copy link
Contributor Author

vmiklos commented Feb 19, 2024

Yes, would be good to just apply that consistently to any iframe:

Will do that in a next PR.

@eszkadev eszkadev merged commit 292ba95 into nextcloud:main Feb 19, 2024
44 checks passed
@juliushaertl
Copy link
Member

/backport to stable28

@juliushaertl
Copy link
Member

/backport to stable27

@juliushaertl
Copy link
Member

/backport to stable26

Copy link

backportbot bot commented Feb 20, 2024

The backport to stable26 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable26
git pull origin stable26

# Create the new backport branch
git checkout -b backport/3475/stable26

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 2e9441c5

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/3475/stable26

Error: No changes found in backport branch


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Ready to be reviewed bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iframe lacks allow attribute to use navigator.clipboard in Chrome
3 participants