Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
i/builtin: support desktop-file-ids in desktop files rule generation #14444
i/builtin: support desktop-file-ids in desktop files rule generation #14444
Changes from 1 commit
a315bf8
4c93ce8
b364f5c
b44c8a2
9ef0181
9578c24
e5a4729
b1eede9
93d2e69
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
wonder if this should perhaps be done in snap.Validate()
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.
Ideally, Yes. But we don't know if there are snaps already in the store with desktop files with undesired names. adding the check in snap.Validate() could break existing installs (but maybe for such snaps, we should?).
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.
don't we do any validation on the ids? I'm sure there are some checks that we can do just based on dbus rules, no?
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.
We do validation in the desktop interface's
BeforePreparePlug
snapd/interfaces/builtin/desktop.go
Lines 623 to 633 in b605298
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.
I added the check here to avoid depending on other parts working as expected and having the checks validate input standalone without any assumptions.
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.
I would prefer this is done in
snap.Validate()
since any existing snaps should be updated. However, if we are concerned about breaking things too much, we should ensure that review-tools implements similar validation logic fordesktop-file-ids
so we can avoid snaps being uploaded with anything that might try and abuse this (also recall this needs a snap declaration as well which adds additional protection too).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.
No snaps on the store currently should have desktop-file-ids yet, I think we could add a check for them in snap.Validate() and fail hard there during install/pack. I was confused at first and thought this was for existing desktop file names as well.
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.
I got confused a bit, I don't think we can this check to
snap.Validate()
. As Samuele mentioned the bad plug will not be there to validate since the snap plugs and slots are sanitized before getting validated so when we reachsnap.Validate
a baddesktop-file-ids
attribute won't be there.snapd/snap/info_snap_yaml.go
Lines 241 to 242 in 848d437
I don't see a clean way of force validating
desktop-file-ids
because they are sanitized (and disappear if bad) just before validation. Only thing left of them is an error message underinfo.BadInterfaces
that is shown when runningsnap pack --check-skeleton
.Maybe I am confused about what is required here, but I think adding validation on the container level
snap.validateContainer()
for desktop file names on pack/build as @bboozzoo suggested #14444 (comment) is a great idea + checks from review tools to prevent new revision with weird desktop file names.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.
So it looks like an error here is unexpected and we should return early rather than simply ignore it.
snap pack --check-skeleton
raises an error with bad plugs/slots, but actual pack does not, which I think was intentional.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.
I updated that part to return and error instead of doing the fallback as this indicates something dangerous that the early validation and sanitization was bypassed by the snap.
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.
also why not fail and let AppArmorConnectedPlug return an error?
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.
Good point, Since desktop-file-ids is a new concept, I believe we can just fail. but for existing prefixed desktop files we could break some snaps but it would be very rare. A snap with weird desktop file names + unity7/desktop-legacy plugs.
CC: @pedronis @jhenstridge
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 do we switch to the fallback in this case?
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.
This is to be consistent with the original implementation, which didn't care about existing desktop files.
snapd/interfaces/builtin/utils.go
Lines 116 to 136 in f3d0682
I know a snap depending on the undocumented behavior that unity7 and desktop-legacy gives it access to list desktop files should not be supported, but who knows what snap will break if that changes?
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.
snap.Validate() is also used when packing a snap, maybe it'd be useful to issue a warning if the there's anything wrong with the desktop files.
Hm I don't recall why we don't do any sanity checks of desktop files in validation code, and rather let them fail during install.
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.
Yes, I agree. A warning there wouldn't break existing snaps.
No idea, I recently landed changes there to validate their file types, but there weren't any other validations. I think without epochs we cannot do better on the container validation side.
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.
A followup material, but we could look into tweaking snap.Validate() interface to pass additional parameter describing the scenario in which it is invoked, and in case of pack/build-time validation fail hard on some new checks we introduce.
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.
This sounds like a great idea.
And we should also extend review-tools to implement similar checks against desktop file names etc.
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.
Filed a ticket to track this idea https://warthogs.atlassian.net/browse/SNAPDENG-32333
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.
I just have one comment, I think the desktop file names check should be on the container level, so I think
snap.validateContainer()
would be more appropriate to add the additional scenario parameter.