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

feat: route deprecated sync clipboard read through permission checks #45377

Merged

Conversation

deepak1556
Copy link
Member

@deepak1556 deepak1556 commented Jan 29, 2025

Description of Change

Follow-up to #45277 that makes the feature opt-in behind permission checks to provide a transition period for users.

All the feature related properties and methods are marked deprecated on purpose to hint that this is will removed at some point.

  1. enableDeprecatedPaste webpreference will enable support for the permission checks of document.executeCommand("paste")
  2. Any invocation of the command will trigger session.permissionCheckHandler with type deprecated-sync-clipboard-read
  3. Permission check will not be triggered if there was a recent user interaction since the last permission request was responded. What counts as user interaction relies on https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.h;l=2335-2340. This is based on what is done today for the async clipboard checks https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/chrome_content_browser_client.cc;l=7652-7672

Release Notes

Notes: Add permission support for document.executeCommand("paste")

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 29, 2025
@deepak1556 deepak1556 force-pushed the robo/add_permission_checks_for_deprecated_clipboard branch 2 times, most recently from 0549301 to 007ca8d Compare January 30, 2025 11:39
@deepak1556 deepak1556 changed the title feat: route deprecated clipboard commands through permission checks feat: route deprecated sync clipboard read through permission checks Jan 30, 2025
@deepak1556 deepak1556 force-pushed the robo/add_permission_checks_for_deprecated_clipboard branch from 007ca8d to 6b2459a Compare January 30, 2025 15:36
@deepak1556 deepak1556 added target/33-x-y PR should also be added to the "33-x-y" branch. target/34-x-y PR should also be added to the "34-x-y" branch. target/35-x-y PR should also be added to the "35-x-y" branch. semver/minor backwards-compatible functionality labels Jan 30, 2025
@deepak1556 deepak1556 marked this pull request as ready for review January 30, 2025 15:37
@deepak1556 deepak1556 requested a review from a team as a code owner January 30, 2025 15:37
Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

Can we put an explicit version timeline for deprecation before landing this?

docs/api/structures/web-preferences.md Outdated Show resolved Hide resolved
docs/api/session.md Outdated Show resolved Hide resolved
@deepak1556
Copy link
Member Author

Can we put an explicit version timeline for deprecation before landing this?

Yes that is something I would like to discuss in this PR. Do we match Chrome and keep the API supported till it gets removed from upstream ? Or is there are Electron specific timeline we want to pursue ?

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

API LGTM

Consider this approved from my API / security perspective, I'll leave in depth code review to other folks.

RE removal timeline, I'd like to get rid of this before Chrome does but I'm open to that timeline being dictated by your migration timeline @deepak1556

@pushkin-
Copy link

pushkin- commented Feb 1, 2025

Would you consider a similar opt out for document.execCommand("copy") since this also appears to be "broken" after #45277

@deepak1556
Copy link
Member Author

@pushkin- can you share example of what is broken with copy ? Unlike the paste command which was disabled completely the copy/cut commands rely on the transient activation state similar to browsers as long as a command is executed with the expected time span of user activation which is set to 5 seconds they should work.

@pushkin-
Copy link

pushkin- commented Feb 3, 2025

@deepak1556 I believe that AllowWriteToClipboard was returning false if I remember correctly. Before, I could execute document.execCommand("copy")in one window while focused in another. Now, I have to be focused in the same window. This requires patching up a number of different places in our code to execute that command on the focused window.

Ultimately, it's an issue with our code, but it's something that used to work, and now broke with the 33.3.2 patch. See discussion here #45144

@deepak1556 deepak1556 force-pushed the robo/add_permission_checks_for_deprecated_clipboard branch from c6a1c97 to 68ac3e7 Compare February 3, 2025 18:36
@deepak1556
Copy link
Member Author

I believe that AllowWriteToClipboard was returning false if I remember correctly

That is correct, however the change from 7ecb8ad does not completely block the cut/copy feature as long as you are focused on the window that needs to execute the action which shouldn't be a breaking change for most apps out there.

I will keep this PR focused on the paste command that actually needs an opt-in to allows apps have a reasonable timeline to migrate.

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

API LGTM

@deepak1556 deepak1556 merged commit bec6ddd into main Feb 5, 2025
58 of 59 checks passed
@deepak1556 deepak1556 deleted the robo/add_permission_checks_for_deprecated_clipboard branch February 5, 2025 06:13
Copy link

release-clerk bot commented Feb 5, 2025

Release Notes Persisted

Add permission support for document.executeCommand("paste")

@trop
Copy link
Contributor

trop bot commented Feb 5, 2025

I was unable to backport this PR to "34-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/34-x-y PR should also be added to the "34-x-y" branch. label Feb 5, 2025
@trop
Copy link
Contributor

trop bot commented Feb 5, 2025

I was unable to backport this PR to "33-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/34-x-y needs-manual-bp/33-x-y and removed target/33-x-y PR should also be added to the "33-x-y" branch. labels Feb 5, 2025
@trop
Copy link
Contributor

trop bot commented Feb 5, 2025

I have automatically backported this PR to "35-x-y", please check out #45471

@trop
Copy link
Contributor

trop bot commented Feb 5, 2025

@deepak1556 has manually backported this PR to "34-x-y", please check out #45472

@trop
Copy link
Contributor

trop bot commented Feb 5, 2025

@deepak1556 has manually backported this PR to "33-x-y", please check out #45473

@trop trop bot added in-flight/33-x-y merged/33-x-y PR was merged to the "33-x-y" branch. and removed needs-manual-bp/33-x-y in-flight/33-x-y labels Feb 5, 2025
jkleinsc pushed a commit that referenced this pull request Feb 5, 2025
…45472)

feat: route deprecated sync clipboard read through permission checks (#45377)
@trop trop bot added merged/34-x-y PR was merged to the "34-x-y" branch. merged/35-x-y PR was merged to the "35-x-y" branch. and removed in-flight/34-x-y in-flight/35-x-y labels Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review/requested 🗳 merged/33-x-y PR was merged to the "33-x-y" branch. merged/34-x-y PR was merged to the "34-x-y" branch. merged/35-x-y PR was merged to the "35-x-y" branch. new-pr 🌱 PR opened in the last 24 hours semver/minor backwards-compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants