-
Notifications
You must be signed in to change notification settings - Fork 577
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
Use whitelisting for video players #3472
Conversation
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.
LGTM, some thoughts.
- We never
whitelist ${DESKTOP}
or similar in related programs (e.g. document-viewer, image-viewer, ...). - Depending on how strict we want it we should remove
whitelist ${DOCUMENTS}
. (see 1.). - I use
disbale-xdg.local
toblacklist
/read-only
also other folders in my home (e.g. git, …) since it is included in blacklisting profiles which don't need wider access. - mpv has a screenshot function, which stores by default under something like ~/screenshot-%date%.png
Looks fine. As I have run with whitelisting profiles for mplayer and mpv for a long time I can confirm both work as expected. On the matter of completely removing disable-xdg.inc I also never whitelist ${DESKTOP} and ${DOCUMENTS} in media player profiles. |
Thanks @rusty-snake and @glitsj16 ! To address your thoughts... 1/2 my completely unproven gut feeling is that folks would be more likely to have videos on Desktop than Documents. Perhaps remove Documents but keep Desktop visible in the sandbox??
|
I can see your point. Personally I can live with removing ${DOCUMENTS} and keeping ${DESKTOP}. About point 3. I have come across situations like these before a few times, meaning that whatever one opts for, users are going to have to decide on loosening/tightening individual profiles via *.local files. In such cases I think it makes more sense to make it harder on users to loosen sandboxes, instead of 'punishing' users that favor more tightened profiles. So in this case I would opt for keeping include disable-xdg.inc, even if that means having a few more lines in the profiles. Hope this makes sense, I might have one of those 'fuzzy logic' brain cells :) |
I also vote for removing ${DOCUMENTS} and keeping ${DESKTOP} - but what about making the latter read-only? |
IMHO we should add
to more profiles in general. |
My concern is with editing file metadata here - for example I believe VLC lets you easily change artist/album names? Maybe not too much an issue with videos but it's not unreasonable to expect people to play i.e. mp3s with video players and read-only access would be an issue here. Also unrelated side note - I messed up by pushing to |
I understand, and I think you're right. Nevertheless I feel a bit uneasy. Write permission for the supported extensions would be probably preferable but it's a bit complicated to achieve. This is an example where something like what I suggested in #3424 would be helpful. |
anyone fine? |
Oops, sorry! I'll try to get this updated ASAP! |
See reviews at #3472 Block $DOCUMENTS Make $DESKTOP read-only
Updated! @rusty-snake as you mention mpv has a screenshot function that saves to $HOME I wonder if we should go back to blacklisting for this one? |
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.
Why adding read-only ${DESKTOP}
while removing its whitelist? (read-only come at the end of the file)
@rusty-snake thanks, gotten a little rusty here, should have kept the whitelist as well... |
We can do it like here (deb6c12#diff-74e11ae375b6951607bb9f53d646c4e4).
|
If it does not already exist, mpv is unlikely to need or to create it, so just whitelist it if it exists. This amends commit 5d74179 ("Use whitelisting for video players (netblue30#3472)", 2020-08-15).
Switches video players over to a whitelist basis.
I've tested celluloid, vlc, and xplayer and they seem to be fine - would really appreciate additional testing! 😃