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

Reject illegal paths in OSC 9;9 #14093

Merged
merged 5 commits into from
Sep 28, 2022
Merged

Reject illegal paths in OSC 9;9 #14093

merged 5 commits into from
Sep 28, 2022

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Sep 27, 2022

closes #12378

@DHowett
Copy link
Member Author

DHowett commented Sep 27, 2022

There is an open question as to whether we should do this here, reject the path outright, or do it in every place that uses the path.

  • Accepting the path with sanitization will result in invalid paths being passed through outright and later failures (when those paths fail to exist.)
  • Rejecting the path is probably the safest option. Should I do that instead?
  • We could make this part of the OSC 9;9 state machine, but that's a lot more work and doing so may result in behavior inconsistent from that of ConEmu
  • Doing it everywhere that uses a path (including session save/restore) would probably result in us forgetting someplace.

src/inc/til/string.h Outdated Show resolved Hide resolved
src/inc/til/string.h Outdated Show resolved Hide resolved
src/terminal/adapter/adaptDispatch.cpp Outdated Show resolved Hide resolved
@DHowett DHowett changed the title Strip out non-path characters in OSC 9;9 Reject illegal paths in OSC 9;9 Sep 27, 2022
@DHowett
Copy link
Member Author

DHowett commented Sep 27, 2022

@msftbot merge this in 1 minute

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 27, 2022
@ghost
Copy link

ghost commented Sep 27, 2022

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Tue, 27 Sep 2022 22:56:45 GMT, which is in 1 minute

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@DHowett
Copy link
Member Author

DHowett commented Sep 28, 2022

Tests passed in an earlier rev (and the only changes are formatting.)

@DHowett DHowett merged commit fc0ef37 into main Sep 28, 2022
@DHowett DHowett deleted the dev/duhowett/osc9001 branch September 28, 2022 01:46
DHowett added a commit that referenced this pull request Sep 28, 2022
Paths that contain illegal path components will be dropped
in the dispatcher.

(cherry picked from commit fc0ef37)
Service-Card-Id: 85900829
Service-Version: 1.15
DHowett added a commit that referenced this pull request Sep 28, 2022
Paths that contain illegal path components will be dropped
in the dispatcher.

(cherry picked from commit fc0ef37)
Service-Card-Id: 85900830
Service-Version: 1.16
@ghost
Copy link

ghost commented Oct 18, 2022

🎉Windows Terminal v1.15.2874 has been released which incorporates this pull request.:tada:

Handy links:

DHowett added a commit that referenced this pull request Dec 1, 2022
Paths that contain illegal path components will be dropped
in the dispatcher.

Service-Version: 1.15
(cherry picked from commit fc0ef37)

Service-Version: 1.12

(cherry picked from commit a187987)
@ghost
Copy link

ghost commented Dec 14, 2022

🎉Windows Terminal Preview v1.16.3463.0 and v1.16.3464.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty path to OSC 9;9; crashes Terminal window
3 participants