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

Fix spawning new tabs/windows in directory with urlunsafe paths #799

Merged
merged 1 commit into from
May 18, 2021

Conversation

Arvedui
Copy link
Contributor

@Arvedui Arvedui commented May 15, 2021

The CWD is internally stored as URL. The path component is therefore stored percent_encoded. It must be decoded before use.

Copy link
Owner

@wez wez left a comment

Choose a reason for hiding this comment

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

Thanks for digging in!
We'll need to remove the unwrap, but this otherwise sounds OK.
Please also update the other (unfortunate) duplicate block of code!

let path = url.path().to_string();
let path = percent_decode_str(url.path())
.decode_utf8()
.unwrap()
Copy link
Owner

Choose a reason for hiding this comment

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

We can't tolerate an unwrap here; it will kill everything that the user is doing if a program has set a bogus cwd.

A failure to decode to utf8 should result in the path being treated as None.

There's a similar section of code over there that should also have the same treatment:
https://github.com/wez/wezterm/blob/main/wezterm-mux-server-impl/src/sessionhandler.rs#L615

The CWD is internally stored as URL. The path component is therefore
stored percent_encoded. It must be decoded before use.
@Arvedui
Copy link
Contributor Author

Arvedui commented May 16, 2021

Thanks for your feedback. Changed the patch accordingly. I did not test the changes in sessionhandler.rs though, because it is not clear to me how to do that.

@wez wez merged commit dbc278e into wez:main May 18, 2021
wez added a commit that referenced this pull request May 18, 2021
@wez
Copy link
Owner

wez commented May 18, 2021

Thanks!

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.

2 participants