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

Async clipboard read and race conditions. #180

Open
snianu opened this issue Jun 24, 2022 · 6 comments
Open

Async clipboard read and race conditions. #180

snianu opened this issue Jun 24, 2022 · 6 comments

Comments

@snianu
Copy link
Contributor

snianu commented Jun 24, 2022

In Chromium browsers, the data from the system clipboard is accessed asynchronously from Browser process (that calls the system clipboard APIs). When the browser reads the clipboard data, it will send that to the renderer, which will sanitize (if necessary) the data before sending it to JS. If the contents change between the browser read and the renderer sending to JS (sanitization can take some time), we do NOT notice this and pass this (outdated) clipboard data to JS.
What should be the right behavior here?
Should async clipboard APIs reject the promise if the clipboard data changed between the read call and the data that is returned after the read is executed successfully?
On Windows, we could detect changes to the system clipboard by checking for the sequence numbers, or is it OK to send data that may not be in the clipboard at the time of read in JS?
@annevk @whsieh @BoCupp-Microsoft

@snianu snianu added the Agenda+ label Jun 24, 2022
@css-meeting-bot
Copy link
Member

The Web Editing Working Group just discussed async clipboard read and race conditions.

The full IRC log of that discussion <Travis> Topic: async clipboard read and race conditions
<Travis> github: https://github.com//issues/180
<Travis> Checking to see if Anupam can join to discuss...
<Travis> whsieh: the behavior is basically checking (in Windows) the sequence number
<Travis> .. apple has a similar principle. If the data contents changed between binding the requested content and the read, we will throw.
<Travis> johanneswilm: is this going to result in unexpected errors?
<Travis> whsieh: there are lots of cases where errors can be thrown (in general).
<Travis> .. So, I'm not sure this is a problem.
<Travis> .. E.g., if the user decides to cancel out of a paste operation, that is also an error.
<Travis> .. so perhaps this can be one of them?

@whsieh
Copy link

whsieh commented Jul 14, 2022

Cocoa platforms have a very similar concept to sequence numbers, called changeCount. If this value changes between when the clipboard access is "approved" by the user and when the clipboard data is actually read, WebKit will currently reject the promise.

@snianu
Copy link
Contributor Author

snianu commented Jul 15, 2022

@whsieh Re: changeCount, does it change when the clipboard content changes or does it only change whenever there is an ownership change? On Windows, the sequence numbers changes whenever the clipboard content changes.

@whsieh
Copy link

whsieh commented Aug 11, 2022

@whsieh Re: changeCount, does it change when the clipboard content changes or does is it only change whenever there is an ownership change? On Windows, the sequence numbers changes whenever the clipboard content changes.

I see — yeah, changeCount is only changed when the pasteboard owner changes. That said, there may be other system APIs that would allow us to determine when the pasteboard contents change on macOS and iOS (on macOS, it's possible to read the generation count, whereas on iOS, a global notification is dispatched when the clipboard changes content).

@css-meeting-bot
Copy link
Member

The Web Editing Working Group just discussed async clipboard read & race conditions.

The full IRC log of that discussion <Travis> Topic: async clipboard read & race conditions
<Travis> github: https://github.com//issues/180
<Travis> snianu: we looking to figure out if the clipboard content changes between reading system clipboard and santization step... should we just return the content read from the clipboard, throw an error, or return empty clipboard.
<Travis> .. macOS, iOS have limits as noted by whsieh in the issue.
<Travis> Travis: do you have a preference?
<Travis> snianu: came up while working on image sanitization... encode/decode takes some time.
<Travis> .. when the author makes the read call, the user gave them permission.
<Travis> .. at that moment, the user is expecting the data in the clipboard.
<Travis> .. I think it makes sense to return the data (not throw)
<Travis> .. what if the user switches to another apps, puts something else on the clipboard?
<Travis> .. if the browser already read the data, then the clipboard content could have changed.
<Travis> .. currently (in chromium) plan is to return an empty image.
<Travis> .. we could also throw a data error.
<Travis> Travis: interesting that the plan is to return an empty image.
<Travis> .. what to do with non-image cases?
<Travis> whsieh: I think we should try to match behavior of other apps on the system (for copy/paste)
<Travis> .. this isn't a common scenario.
<Travis> .. might be able to craft an image to take a lot of time to decode and switch...
<Travis> .. in that sense, it probably affects images more than other types (like markup)
<Travis> .. Always possible to hold a clipboard item and re-read it over and over...
<Travis> .. wondering what web author expectation is?
<Travis> .. expect authors to be prepared to handle exceptions.
<Travis> travis: returning old data (original data post processing) sounds good to me.
<Travis> whsieh: Q: if the change occurs before the first raw clipboard read, then what happens?
<Travis> snianu: In Chromium we read all the data at once... I see the issue because with delayed rendering, we don't want to do that...
<Travis> whsieh: In a delayed world, might make sense to throw an error. Other actions might lead to unpredictable behavior (depending on whether you've read the raw data or not)
<Travis> snianu: Sounds good.
<Travis> smaug: There's always a possibility of a race condition.
<Travis> snianu: So, will resolve to throw an error in this case.
<Travis> whsieh: Might be interesting to test this for interop as well.
<Travis> .. having the platform stomp over the clipboard content.
<Travis> .. we should continue thinking about that.

@annevk
Copy link
Member

annevk commented Oct 1, 2022

In what kind of scenarios can this happen? When there is another app writing to the clipboard or the end user is lightning fast?

Thinking through possible scenarios a bit I think I agree with @smaug----'s remark that there's always a possibility for races here and not all of them you will be able to detect.

It seems fine to allow an exception in case something is considered amiss, but I would expect this kind of thing to not be common except on a system that's compromised in some manner.

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

No branches or pull requests

5 participants