flathub-infra / flatpak-builder-lint

A linter for flatpak-builder manifests
MIT License
48 stars 115 forks source link

finish-args: Cover more redundant home and host permission variants. #362

Closed bbhtt closed 4 days ago

bbhtt commented 3 months ago

Following are disallowed:

bbhtt commented 3 months ago

This will surely break a bunch of applications, we need to figure out what breaks.

I see every chromium based browser has host and home/.local/share/applications:create

https://github.com/flathub/com.microsoft.Edge/blob/1a2e4585addc0c0490b915422beac7455aa350a4/com.microsoft.Edge.yaml#L39-L41

Hey @TingPing, mind confirming the analysis above? Thanks!

TingPing commented 3 months ago

host or host-{os, etc} ends with :ro and again home/foo or ~/some/path ends in :ro. Read only host variant implies read-only home variants.

when home:ro is present and again home/foo:ro or ~/some/path:ro is present. Read-only home implies read-only subpath of home.

This doesn't appear to be correct. You can have writable subpaths.

bbhtt commented 3 months ago

I think those two are fine? host:ro should imply ~/path:ro?

What you mean is host:ro and ~/somepath:rw which is not blocked?

bbhtt commented 3 months ago

What you mean is host:ro and ~/somepath:rw which is not blocked?

This variant is correct and covered by https://github.com/flathub-infra/flatpak-builder-lint/pull/362/files#diff-67e3430053d7a019a58874f821f60eac91d2970b94288593c06fb6715f2f57b2 and https://github.com/flathub-infra/flatpak-builder-lint/pull/362/files#diff-ccb6365857d305a97e21049ea26e7b4002f71facb4d1d4b2835a298abcaa23cb files to not raise the error.

bbhtt commented 3 months ago

Ok I see where I made the error, I borked the explanation in the post by adding the "implies" part. I'll change it.

What I meant is that it matches host:ro, host-etc:ro, host-os:ro with anything starting with ~ or home and ending with :ro.

Same for home:ro and anything starting with home and ~ and ending with :ro.

TingPing commented 3 months ago

To be clear, how I interpreted it.

If the permissions are home:ro ~/foo, then foo is writable. So ~/foo:ro is correct (sometimes at least).

bbhtt commented 3 months ago

If the permissions are home:ro ~/foo, then foo is writable. So ~/foo:ro is correct (sometimes at least).

Do you have an example for this? What homedir relative directories can be writeable when using home:ro or host:ro or specifically needs to be requested even with host or home (read-write)?

Seems some apps may need ~/.var/app/appid/ for example game launchers might want ~/.var/app/com.valvesoftware.Steam:ro or even writeable. Is there anything else?

Erick555 commented 3 months ago

home is writeable but home/foo or ~/some/path is again writeable. Writeable home implies writeable subpath of home. Some apps may require :create if they expect the folder to be present already. These will have to go through exceptions.

Some apps may intentionally have redundant permissions to support disabling home/host which is optional for them. I think it's better to focus on things that are misleading or may cause issues rather than things that just look too verbose.

bbhtt commented 3 months ago

The redundancy is more often a mistake than intentionally added.

If it is optional then it doesn't belong permanently in static permissions and if people are overriding it anyway they are in their own territory and can add the specific permissions needed.

If they really, really need it for whatever reason they can get an exception.

We already had checks for redundancy, this only expands it.

Erick555 commented 3 months ago

The redundancy is more often a mistake than intentionally added.

Whether it's mistake or not it hurts no one.

If it is optional then it doesn't belong permanently in static permissions

Flatpak doesn't have concept of optional permissions like snap does and maintainers try to please everyone.

if people are overriding it anyway they are in their own territory and can add the specific permissions needed.

Even for tech users it's not obvious which permissions are really needed when they're overshadowed by home/host

We already had checks for redundancy, this only expands it.

Please reconsider if this is right direction then. There was really a lot of breakage in flathub in past months and breaking the builds again for such non-issue is hardly what we need right now.

bbhtt commented 3 months ago

Whether it's mistake or not it hurts no one.

That argument doesn't work. You can say the same way about everything the linter currently blocks or any permission that has no effect on a particular user. A mistake is a mistake.

Anyway as I initially said in the second post here, I won't deploy this without adapting existing apps to the change. No existing apps will be broken.

Even for tech users it's not obvious which permissions are really needed when they're overshadowed by home/host

Then they shouldn't be overriding permissions or maintainers shouldn't recommend overriding without giving the specific permissions needed to keep the app working.

bbhtt commented 4 days ago

Not interested in working on this anymore.