Add support to ANSI OSC52

[aymanbagabas]

Add support to ANSI OSC52 sequence to manipulate selection and clipboard
data. The sequence specs supports multiple clipboard selections but we
only support the common ones, system and primary clipboard selections.

This adds a new event listener to the common terminal module
onClipboard to allow external implementations to hook into it.

The default ClipboardProvider in xterm-addon-clipboard uses the browser
Clipboard API to read/write from and to the clipboard. The default
ClipboardProvider uses the browser Clipboard API. This means it only
supports read/write to and from the system clipboard. This default
ClipboardProvider is limited to 1MB of data to write (zero means no limit).

TODO:

• Add OSC52 handler to /common
• Implement a clipboard addon that uses Clipboard API
• Add allowClipboardAccess option. Disabled by default
• Add tests
• Find a workaround for the browser focus issue

Reference: https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-Operating-System-Commands
Fixes: #3260

[jerch]

@aymanbagabas Thx for looking into this. A few notes from my side:

The service cannot go into /common, as this is also used for headless lib builds without a browser frontend. Thus the service with navigator notions should be under /browser, while /common may hold a stub, where integrators can hook in their own clipboard backend.

Furthermore clipboard access needs some thinking about the security implications. Imho it should be disallowed by default (xterm even puts it under windowOps security setting), and only granted explicitly by a user interaction (maybe also customizable, so integrators can disable the user feedback, if they have other measures in place). This is especially important for websites with xterm.js, as the browser clipboard might hold totally different content, that should not be accessible by some random terminal app.

Edit: Also the focus issue mentioned in #3260 needs some workaround for the browser part.

[aymanbagabas]

@aymanbagabas Thx for looking into this. A few notes from my side:

The service cannot go into /common, as this is also used for headless lib builds without a browser frontend. Thus the service with navigator notions should be under /browser, while /common may hold a stub, where integrators can hook in their own clipboard backend.

Very reasonable, do you think exposing attachClipboardService(IClipboardService) in /common/CoreTerminal.ts is reasonable?

Furthermore clipboard access needs some thinking about the security implications. Imho it should be disallowed by default (xterm even puts it under windowOps security setting), and only granted explicitly by a user interaction (maybe also customizable, so integrators can disable the user feedback, if they have other measures in place). This is especially important for websites with xterm.js, as the browser clipboard might hold totally different content, that should not be accessible by some random terminal app.

I think adding an option to allow clipboard access makes sense. Perhaps add allowClipboardAccess as a terminal option? In /browser for more security, something like confirm('Allow xterm.js to read from the clipboard?') could be added in the service implementation when a clipboard read is performed. Browsers prompt the user using the Permissions API

Edit: Also the focus issue mentioned in #3260 needs some workaround for the browser part.

Will look into that once the API is in place.

[ghuntley]

Howdy from https://coder.com ala https://github.com/coder. Dropping on by to say thank-you for picking up this item. OSC52 support has been requested by our community and I'm opening up an open-source bounty payable when this item ships. Our way of saying thank-you. ❤️

[Tyriar]

I probably won't be able to review this until early December, sorry about the delay.

[jerch]

Just commenting for now, as I think that this still needs several iterations...

[jerch]

@aymanbagabas To make that event idea more clear:

Imho all it needs in common, is a sync inputhandler function for OSC52 with two abstract interface "endpoints" - retrieve and place clipboard data. For maximum flexibility the endpoints can be feed from an clipboard event, to illustrate that:

const enum ClipboardEventType {
  QUERY = 0,
  SET = 1
}
interface IClipboardEvent {
  type: ClipboardEventType;
  payload: string;
}

// in OSC52 function
// for request: emit {type: ClipboardEventType.QUERY, payload: ''}
// for set: emit {type: ClipboardEventType.SET, payload: 'unicode string data'}

Now all that needs to be done, is to interface that in way, so embedders can hook into it with their clipboard logic. Ideally we create a public API endpoint for this, e.g. onClipboardRequest((ev: IClipboardEvent) => void).

In a second step we can discuss, whether a default implementation should be provided from core repo under /browser (I am not a big fan of this idea for security reasons*), it is prolly a better idea to outsource the clipboard handler logic into an addon. This way embedders have explicitly to hook their logic (hopefully with security in mind), or explicitly to load the addon, where we can place further security warnings and such.

---

[*] Whats the issue with a default impl in core repo? Well it introduces a possible attack vector for clipboard access, which might be a surprising change not spotted by embedders, that might clash with their security policy. By enforcing explicit loading ppl will have to do that on their own behalf, not by a surprising code change in xterm.js thats always active by default.

[aymanbagabas]

@jerch I've made the default implementation into a new addon xterm-addon-clipboard. You still need to enable the allowClipboardAccess option before the addon can work. Using the addon is as simple as loading it into xtermjs.

[jerch]

@aymanbagabas Sorry just saw your last comment - does that error still happen?

[aymanbagabas]

@aymanbagabas Sorry just saw your last comment - does that error still happen?

yes

[aymanbagabas]

This is ready for squash and merge 🙂

[jerch]

Really nice, guess it is almost there. 👍

Sorry for playing the nitpicker here, but I have a few restructuring ideas, esp. around the custom provider API (I really like that concept):

• To me it seems reasonable to move the base64 stuff out of the provider, this way the provider would only have to care for its main task - read or write data from clipboard system xy (e.g. move base64 handling one level up close to the OSC handler).
• Provider API should also allow to return values in sync (e.g. return type would be string | Promise<string> for read), a certain provider impl can then pick whatever is suitable for its clipboard backend.
• Imho there are superfluous try-catch blocks - we typically dont catch sync errors in parser handlers, just let them bubble (terminal is still partially self-healing due to its chunk/callback-based nature).
• Other than sync errors, async errors (.catch path on a promise) should be handled explicitly, otherwise the stack trace gets messed up and hard to debug later on.
• The OSC handler should not filter out anything yet, but instead forward pc to the provider - this way a provider can also implement those other buffer types mentioned in the xterm docs. The default browser provider can just skip/ignore those requests and stick to handling 'c' as clipboard selection type.
• default provider: I'd rename it to BrowserClipboardProvider to make clear, that it implements what a browser offers. The base64 argument should be removed, I'd also apply the limit setting to the reading side and give it a sane default (maybe 1 MB).

Edit:
Maybe reshape the read/write action of the default provider to async functions, as they kinda always operate with async clipboard interfaces.

[aymanbagabas]

Pushed the changes

• default provider: I'd rename it to BrowserClipboardProvider to make clear, that it implements what a browser offers. The base64 argument should be removed, I'd also apply the limit setting to the reading side and give it a sane default (maybe 1 MB).

I've removed the limit because the xterm.js parser already limits the size of an OSC sequence data there's no need to have one in the default browser provider, custom providers can define their implementation with a limit.

[Tyriar]

This is looking good to me, I was careful to review the data we send over the pty and it looks like it's safe. It also doesn't touch anything in ./src so it's low risk to merge.

@jerch any other comments before we merge?

[jerch]

@Tyriar Nope, I think the PR is now good to be merged. I dont have a good understanding of the browser security measures applied for clipboard access. I think there might be a chance, that an earlier given permission will be auto-revoked by the browser, which might cause frictions for long running apps like vim, that try to do something with OSC 52. But as I am unsure about the impact of that, I think we should not try to fix it upfront, but if and when we get followup issues.

@aymanbagabas Thx a lot for getting this done, and your patience with us ❤️