-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Treat play_file() and co as internal routines without protection #4059
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
Conversation
Our code and add-ons need a way to play audio from arbitrary locations. I propose we treat the _tag API as dangerous, and the _file API as trusted. Related: #4058
qt/aqt/sound.py
Outdated
| """Play the provided path. | ||
| SECURITY: Filename may be an arbitrary path. For content coming from a collection, | ||
| use play_tag() instead.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, this implies play_tags() does some validation and enforces a media path, while it's actually exactly the same under the hood.
|
This looks like a better approach for add-ons. If we want to leave validation to add-ons dealing with user input, a note about os.path.basename() in the docstrings would be nice. |
|
If my change wasn't what you intended, please feel free to push a follow-up :-) |
* Treat play_file() and co as internal routines without protection Our code and add-ons need a way to play audio from arbitrary locations. I propose we treat the _tag API as suitable for user input, and the _file API for internal use. * Mention basename in the *_file() paths (cherry picked from commit 50b7588)
|
This was possibly the cause of Vocab-Apps/anki-hyper-tts#282 as well ? |
|
I see some av_player.play_file/insert_file calls in the add-on's code, so that's probably it if HyperTTS is playing audio files not in the media directory. |
|
@abdnh can you elaborate on this ? |
|
It was a bug in .6. It should be fixed in .7 already, so please test with that / ask the user to. |
Our code and add-ons need a way to play audio from arbitrary locations. I propose we treat the _tag API as suitable for user input, and the _file API for internal use.
Closes #4058