Demi Marie Obenour <demiobenour@gmail.com> writes:

> I looked at the mount-flatpak codebase and noticed some potential
> improvements for the future:
>
> - The code uses std::fs::write to write to files in the target
>   directory.  This is safe because the VM isn't running, but it might
>   be better to use libpathrs.

I think it's fine.

> - The code doesn't validate the user-provided app ID, resulting in
>   worse error messages than otherwise possible.  App IDs are always
>   D-Bus well-known names, so there is no risk of the rules changing
>   in future versions of Flatpak.

Even though Flatpak 2 won't use D-Bus?

> - Passing something that isn't a flatpak repository as a parameter
>   will result in a confusing "no such file or directory" error.

Could check that if it's cheap, since this is user-facing.  I would not
spend much time on it though, since ultimately this should not be the
interface people use.

> - Various checks for corrupt repositories could be added, such as:
>   - Wrong number of slashes in "current" symlink.
>   - Wrong number of slashes in runtime path.
>   - Bad "active" symlink.

I'm not sure I see the value?  Does normal Flatpak do these checks?
I don't think we need to check everything that could possibly go wrong
if it wouldn't be a security concern.

> - The code that resolves the "active" symlink is duplicated.

I think there's already a patch sitting at the bottom of my inbox for
you from this.

> - The target mount point is not made "noexec".

Sounds like a good improvement.

> - mount_setattr() is called on a directory FD.  I suspect this
>   changes parameters of the source mount.  It might be better to use
>   open_tree_attr() with OPEN_TREE_CLONE instead.

Well, does it change the source mount or doesn't it?  If there's a bug,
fix it; if there isn't, don't.  (I do think you are probably right though.)