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

Escape ampersand char for Windows #20

Merged
merged 1 commit into from
Jan 23, 2021

Conversation

archiif
Copy link
Contributor

@archiif archiif commented Jan 21, 2021

Ampersand characters in subtitles causes issues because it is a reserved character in cmd, so we need to escape them.
https://stackoverflow.com/a/35578027/10625876

Ampersand characters in subtitles causes issues because it is a reserved character in cmd, so we need to escape them.
https://stackoverflow.com/a/35578027/10625876
@archiif
Copy link
Contributor Author

archiif commented Jan 22, 2021

Actually, I just realized that this is the tip of the iceberg of what is a pretty serious injection vulnerability.
Maybe a more robust solution is needed?
mpv-player/mpv#4695 (comment)
If you use the external program copyq like this, without spawning cmd:

mp.commandv("run", "copyq", "copy", text)

It no longer trips up on any special characters. But it adds a dependency, so I'm open to better solutions.

@tatsumoto-ren
Copy link
Member

Using powershell instead of cmd on Windows to copy text to clipboard would be a possible workaround.

@archiif
Copy link
Contributor Author

archiif commented Jan 22, 2021

I personally find powershell to be undesirably slow, but that might be due to my old PC.
You also could use the stdin_data argument in mpv's subprocess command so that you can pipe the subtitle string to clip safely, but for some reason that argument doesn't work, at least on Windows.
I opened an issue about it here:
mpv-player/mpv#8503

@tatsumoto-ren
Copy link
Member

Yes, I always tell people to switch to GNU/Linux because Windows is a nightmare. If you're new to GNU/Linux, I recommend Manjaro, it's easy to install and comes with one of the fastest package managers out there - Pacman.

I'm going to merge the pull request because any solution is better than no solution. If a better way to handle this bug is found, we can just revert the commit later.

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