usersession/userd: query xdg-mime to check for fallback handlers of a given scheme #11200

bboozzoo · 2022-01-04T14:29:38Z

Most of the handling is of xdg-open requests from the snaps is done by
xdg-desktop-portal now, the handler in userd remains a fallback for scenarios
where the desktop setup is incomplete. The code of io.snapcraft.OpenURL()
handler would only allow a handful of schemes to be passed to xdg-open on the
host side. However, updating the list of schemes manually has proven be to
unmaintainable, and got filled with various vendor specific entries.

In #7731 (comment) an
approach was proposed to use xdg-query to find out whether there is handler for
given scheme on the host side, and if so allow the URL to be passed to xdg-open,
which is implemented in this patch.

… given scheme Most of the handling is of xdg-open requests from the snaps id done by xdg-desktop-protal now, the handler in userd remains a fallback for scenarios where the desktop setup is incomplete. The code of io.snapcraft.OpenURL() handler would only allow a handful of schemes to be passed to xdg-open on the host side. However, updating the list of schemes manually has proven be to unmaintainable, and got filled with various vendor specific entries. In snapcore#7731 (comment) an approach was proposed to use xdg-query to find out whether there is handler for given scheme on the host side, and if so allow the URL to be passed to xdg-open, which is implemented in this patch. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

anonymouse64

a couple little nitpicks but overall looks great to me, I do think we need a security review on this however

anonymouse64 · 2022-01-06T00:49:06Z

usersession/userd/launcher.go

 // OpenURL implements the 'OpenURL' method of the 'io.snapcraft.Launcher'
 // DBus interface. Before the provided url is passed to xdg-open the scheme is
 // validated against a list of allowed schemes. All other schemes are denied.
 func (s *Launcher) OpenURL(addr string, sender dbus.Sender) *dbus.Error {
+	logger.Noticef("open url: %q", addr)


should this be a Noticef? Or a Debugf?

anonymouse64 · 2022-01-06T00:50:39Z

usersession/userd/launcher.go

+		if err != nil {
+			logger.Noticef("cannot obtain scheme handler for %q: %v", u.Scheme, err)
+		}
+		isAllowed = isAllowed || has


since we know isAllowed is false in this branch, we could just do

Suggested change

isAllowed = isAllowed || has

isAllowed = has

anonymouse64 · 2022-01-06T00:52:35Z

usersession/userd/launcher_test.go

+	c.Assert(err, NotNil)
+	c.Assert(err, ErrorMatches, `Supplied URL scheme "fallback-scheme" is not allowed`)


Suggested change

c.Assert(err, NotNil)

c.Assert(err, ErrorMatches, `Supplied URL scheme "fallback-scheme" is not allowed`)

c.Assert(err, ErrorMatches, `Supplied URL scheme "fallback-scheme" is not allowed`)

anonymouse64 · 2022-01-06T02:32:51Z

usersession/userd/launcher.go

@@ -153,10 +157,26 @@ func checkOnClassic() *dbus.Error {
 	return nil
 }

+// see https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#file-naming
+var validDesktopFileName = regexp.MustCompile(`^[A-Za-z-_][A-Za-z0-9-_]*(\.[A-Za-z-_][A-Za-z0-9-_]*)*\.desktop$`)


I spot checked a few cases and this looks correct to me

anonymouse64 · 2022-01-06T02:33:35Z

tests/main/xdg-open/task.yaml

@@ -26,6 +26,27 @@ prepare: |
            /                                                 \
            org.freedesktop.DBus.Peer.Ping

+    # setup the handler


thanks for the spread test

MiguelPires

LGTM

…-check-local-providers

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

codecov-commenter · 2022-01-10T10:36:27Z

Codecov Report

Merging #11200 (597fef7) into master (3782a94) will increase coverage by 0.00%.
The diff coverage is 76.92%.

@@           Coverage Diff           @@
##           master   #11200   +/-   ##
=======================================
  Coverage   78.26%   78.26%           
=======================================
  Files         930      930           
  Lines      106315   106340   +25     
=======================================
+ Hits        83210    83231   +21     
- Misses      17936    17939    +3     
- Partials     5169     5170    +1

Flag	Coverage Δ
unittests	`78.26% <76.92%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
usersession/userd/launcher.go	`69.49% <76.92%> (+1.74%)`	⬆️
cmd/snap/cmd_aliases.go	`90.14% <0.00%> (-1.41%)`	⬇️
osutil/synctree.go	`79.24% <0.00%> (+2.83%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3782a94...597fef7. Read the comment docs.

mardy

LGTM with a nitpick :-)

mardy · 2022-01-12T06:55:18Z

usersession/userd/launcher.go

+
+func schemeHasHandler(scheme string) (bool, error) {
+	cmd := exec.Command("xdg-mime", "query", "default", "x-scheme-handler/"+scheme)
+	out, err := cmd.CombinedOutput()


Nitpick: it would be more robust to use just Output() here, in case the xdg-mime program emits some warning on the stderr (or even just some debugging message). For example if the combined output was this:

Warning: myapp.desktop: invalid desktop file [<- stderr] gimp.desktop [<- stdout]

the current implementation of this function would return false, whereas ideally it should return true.

I've added a TODO here to consider using Output. TBH despite my attempts I was not able to make xdg-mime log anything to stderr even whe ~/.config/mimeapps.list was clearly malformed or missing.

alexmurray

URL parsing in Go looks pretty safe - so it doesn't appear a URL could try and inject shell content or similar via the URI scheme - plus exec.Command() doesn't execute a shell either so even if a user could somehow inject extra content, it won't get executed when we go and run xdg-mime. Finally, if a user has installed an application then I don't see a reason not to honor that which this implementation allows.

So from a security (and usability) point of view, this looks great.

Thanks.

…me support Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…-check-local-providers

jhenstridge · 2022-02-02T10:06:47Z

Looking at xdg-desktop-portal, it will skip prompting the user before launching the URI if:

the URI uses a "safe" scheme, which is one of http, https, ftp, mailto, webal or calendar, and there is a default app for that scheme.
there is only one handler for the scheme.
the user has been prompted for permission and chosen the same option more than a set number of times (defaulting to 3).

The proposed behaviour here is not identical (i.e. it's going to differ if there are multiple providers available for a scheme), but seems reasonable. While snaps can modify mime default handlers via userd, it does result in a zenity dialog prompt. If there was a security problem, I'd tend to think that end would be the problem.

bboozzoo · 2022-02-02T10:50:47Z

@mvo5 this is ready to land, can you merge the branch please?

PR snapcore#11200 added a way to query for a handler for any given URL scheme. Extend this and ask the user whether a URL should be opened by a given handler. This is similar to what the desktop portal does, but instead of presenting the app chooser dialog, we only present the user with a yes/no question and the default handler. Note, that the implementation is only for the fallback io.snapcraft.Launcher handler, which would be called only if the desktop portal is not available. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo added 2 commits January 4, 2022 14:45

tests/main/xdg-open: verify fallback handler lookup

7826b1f

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo added the Needs security review Can only be merged once security gave a :+1: label Jan 4, 2022

bboozzoo requested a review from jhenstridge January 4, 2022 14:29

bboozzoo mentioned this pull request Jan 4, 2022

usersession/userd: support Cloudflare WARP URL scheme #11181

Closed

anonymouse64 approved these changes Jan 6, 2022

View reviewed changes

MiguelPires approved these changes Jan 7, 2022

View reviewed changes

bboozzoo added 2 commits January 10, 2022 11:08

Merge remote-tracking branch 'upstream/master' into bboozzoo/xdg-open…

4936233

…-check-local-providers

usersession/userd: simplify code, tweak tests

a424d66

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

mardy approved these changes Jan 12, 2022

View reviewed changes

alexmurray approved these changes Feb 1, 2022

View reviewed changes

alexmurray removed the Needs security review Can only be merged once security gave a :+1: label Feb 1, 2022

bboozzoo added 2 commits February 2, 2022 07:41

usersession/userd: leave a TODO about possibility of improving xdg-mi…

04dfd9b

…me support Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge remote-tracking branch 'upstream/master' into bboozzoo/xdg-open…

597fef7

…-check-local-providers

jhenstridge approved these changes Feb 2, 2022

View reviewed changes

mvo5 merged commit 3885231 into snapcore:master Feb 2, 2022

bboozzoo mentioned this pull request Mar 8, 2022

usersession/userd: ask the user whether to open the URL by given app #11477

Closed

alexmurray mentioned this pull request Oct 19, 2023

Add "jetbrains" to the whitelist of URL schemes handled by xdg-open #13070

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

usersession/userd: query xdg-mime to check for fallback handlers of a given scheme #11200

usersession/userd: query xdg-mime to check for fallback handlers of a given scheme #11200

bboozzoo commented Jan 4, 2022 •

edited by anonymouse64

anonymouse64 left a comment

anonymouse64 Jan 6, 2022

anonymouse64 Jan 6, 2022

anonymouse64 Jan 6, 2022

anonymouse64 Jan 6, 2022

anonymouse64 Jan 6, 2022

MiguelPires left a comment

codecov-commenter commented Jan 10, 2022 •

edited

mardy left a comment

mardy Jan 12, 2022

bboozzoo Feb 2, 2022

alexmurray left a comment

jhenstridge commented Feb 2, 2022

bboozzoo commented Feb 2, 2022

		c.Assert(err, NotNil)
		c.Assert(err, ErrorMatches, `Supplied URL scheme "fallback-scheme" is not allowed`)

usersession/userd: query xdg-mime to check for fallback handlers of a given scheme #11200

usersession/userd: query xdg-mime to check for fallback handlers of a given scheme #11200

Conversation

bboozzoo commented Jan 4, 2022 • edited by anonymouse64

anonymouse64 left a comment

Choose a reason for hiding this comment

anonymouse64 Jan 6, 2022

Choose a reason for hiding this comment

anonymouse64 Jan 6, 2022

Choose a reason for hiding this comment

anonymouse64 Jan 6, 2022

Choose a reason for hiding this comment

anonymouse64 Jan 6, 2022

Choose a reason for hiding this comment

anonymouse64 Jan 6, 2022

Choose a reason for hiding this comment

MiguelPires left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 10, 2022 • edited

Codecov Report

mardy left a comment

Choose a reason for hiding this comment

mardy Jan 12, 2022

Choose a reason for hiding this comment

bboozzoo Feb 2, 2022

Choose a reason for hiding this comment

alexmurray left a comment

Choose a reason for hiding this comment

jhenstridge commented Feb 2, 2022

bboozzoo commented Feb 2, 2022

bboozzoo commented Jan 4, 2022 •

edited by anonymouse64

codecov-commenter commented Jan 10, 2022 •

edited