Skip to content

Conversation

DivInstance
Copy link

This change scopes OSC 52 clipboard writes to when the terminal has focus.

Changes:

  1. src/cascadia/TerminalCore/Terminal.hpp: add _isFocused flag.
  2. src/cascadia/TerminalCore/Terminal.cpp: update FocusChanged to track focus state.
  3. src/cascadia/TerminalCore/TerminalApi.cpp: require _isFocused alongside _clipboardOperationsAllowed in CopyToClipboard.

Rationale:

  1. Addresses request in OSC 52 should only function while the terminal has focus #19051 (“OSC 52 should only function while the terminal has focus”).
  2. Prevents background sessions from silently writing to the system clipboard.
  3. Notes:

Behavior remains unchanged when the window is focused.

  1. This only gates VT-driven clipboard writes (OSC 52), not user-initiated copy.

Testing:

  1. Verified clipboard write via OSC 52 succeeds when focused, does nothing when unfocused.
    Lint passed locally.

Link:
Closes #19051.

@DivInstance
Copy link
Author

DivInstance commented Sep 17, 2025 via email

Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines 787 to 788
_isFocused = focused;
return _getTerminalInput().HandleFocus(focused);
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a if (_isFocused != focused) here, if we're at it? This would prevent accidental, redundant HandleFocus calls.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I’ve added the optimization and pushed it to the PR.

@j4james
Copy link
Collaborator

j4james commented Sep 18, 2025

If we think this is an important security feature, should it not also be applied to conhost's OSC 52 implementation? The equivalent method is here:

void ConhostInternalGetSet::CopyToClipboard(const wil::zwstring_view content)

If you want to leave that for a followup, it may be best to keep the issue open so we don't forget about it.

ETA: You could possibly even cover both at the same time by having the check in AdaptDispatch::SetClipboard, and saving the focus state in the TerminalInput class. Although I'm not sure the TerminalInput::HandleFocus is guaranteed to be called in conhost, so it may require a bit of refactoring to make that work.

@DHowett
Copy link
Member

DHowett commented Sep 18, 2025

@DivInstance Your pull request was approved and we were having a discussion about how to handle the same behavior elsewhere. Why'd you close it? :)

@DivInstance DivInstance reopened this Sep 19, 2025
@DivInstance
Copy link
Author

DivInstance commented Sep 19, 2025

Thanks a lot for the feedback! I actually closed the PR by mistake while I was still figuring out the GitHub workflow sorry about that.

On your suggestions:

  1. @lhecker Optimization for redundant calls – Great catch! I’ll add the if (_isFocused != focused) check so we can avoid those extra HandleFocus calls.

  2. @j4james Conhost implementation – I completely agree this should apply to conhost as well. Would you prefer I:

  • Add the same focus check to ConhostInternalGetSet::CopyToClipboard in src/host/outputStream.cpp, or

  • Go with the refactored approach and put the check in AdaptDispatch::SetClipboard?

  1. @DHowett, I’ll make sure OSC 52 should only function while the terminal has focus #19051 is open until both parts are addressed.

Would you like me to take care of both changes in this PR, or should I split conhost into a follow-up?
Also, I really appreciate your guidance here this is my first contribution, so the mentorship means a lot!

@DivInstance
Copy link
Author

DivInstance commented Sep 19, 2025

@j4james I’ve updated ConhostInternalGetSet::CopyToClipboard in (commit 513606d)
to only allow OSC 52 clipboard writes when the console has focus, using the existing CONSOLE_HAS_FOCUS flag.

Appreciate it if you could review and share feedback on this whenever convenient.

Copy link
Collaborator

@j4james j4james left a comment

Choose a reason for hiding this comment

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

Other than the formatting, this looks good to me. Thanks for adding the conhost implementation.

Comment on lines +786 to +793
{
if (_isFocused != focused)
{
_isFocused = focused;
return _getTerminalInput().HandleFocus(focused);
}
return {};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like there are some whitespace formatting issues here that the Code Health checker will probably complain about. There's a runformat.cmd script in the terminal/tools directory that should fix stuff like this for you automatically.

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.

OSC 52 should only function while the terminal has focus
4 participants