notification: Pass the desktop-id to the notification impl #904

3v1n0 · 2022-11-02T15:32:34Z

Notification implementers expects that the app-id is always matching the
application desktop-id, but while this is true for flatpaks is not true
for snaps, so include an extra desktop-id value the notification so that
can be used by the portals to properly implement actions and notify the
shell.

src/xdp-utils.c

robert-ancell

LGTM

We may use it in multiple places, so save it for the whole time.

Different container implementations may have different way to expose desktop-id's vs app-id's, so let's provide a way to explicitly get that.

Notification implementers expects that the app-id is always matching the application desktop-id, but while this is true for flatpaks is not true for snaps, so include an extra desktop-id value the notification so that can be used by the portals to properly implement actions and notify the shell.

This is particularly useful when changing the portal and want to run the tests against it.

Make possible to test applications by allow mocking various applications properties and capabilities. This is simpler for now, but will help various cases in future.

We used to load the desktop files in multiple scenarios (mostly to give feedback) assuming that was the same of the application id, but this is not true for snaps, so let's use the generic functions we have for this.

This allows to have some better checks at code level, but also to use real app-id's values in tests and potentially also true desktop files. This is done by making each test file to load a .xdp-app-info key file that is written by the test suite and parsed by the portal to replicate the expected information. As per this we need to do some minor adjustments to make tests to pass

…ted one

And another one to check that is ignored when matching the app-id.

3v1n0 · 2022-11-03T10:19:07Z

Ok, this has been fixed a bit, and I've added tests. Making this testable involved some changes, but they were required also for #786.

The gtk side of this is instead in flatpak/xdg-desktop-portal-gtk#401

smcv

(Implementation/tests not reviewed in detail)

I think this needs a way for x-d-p to signal to the impl which of the keys in the dict are app-controlled (therefore attacker-controlled), and which of the keys are x-d-p-controlled. Otherwise, an impl like the one in GNOME Shell would have no way to distinguish between a new version of x-d-p (where it can trust the desktop-entry or desktop-id) and an old version (where it cannot).

Perhaps this means we need a new AddNotification2(a{sv}: trusted_fields, s: notification_id, a{sv}: content) where the trusted_fields include an app ID and optionally a .desktop ID, or something like that; and a capability discovery mechanism so that x-d-p can discover whether to call AddNotification2 or AddNotification on the impl? (Or it could even call the new one unconditionally and fall back to the old one on UnknownMethod.)

smcv · 2022-11-03T11:39:45Z

data/org.freedesktop.impl.portal.Notification.xml

+            <listitem>
+              <para>
+              The desktop ID of the application sending the notification.
+              This may me omitted in case the provided @app_id is matching it.


Suggested change

This may me omitted in case the provided @app_id is matching it.

If not specified, the default is the provided @app_id followed by <literal>.desktop</literal>.

smcv · 2022-11-03T11:44:39Z

data/org.freedesktop.impl.portal.Notification.xml

+        these private meta-data keys (all optional):
+        <variablelist>
+          <varlistentry>
+            <term>desktop-id s</term>


This might be better as desktop-entry which would be consistent with the fdo Notifications spec? (It's the desktop ID with the .desktop suffix removed, for instance firefox or org.gnome.Epiphany.)

Ok, fair enough. I'm also wondering if it would be the case to use impl- prefix or something for extra parameters, to make it clear, and maybe just filter them out better if in future we add more stuff here.

smcv · 2022-11-03T11:46:05Z

src/location.c

          title = g_strdup_printf (_("Give %s Access to Your Location?"), name);
-          if (g_desktop_app_info_has_key (info, "X-Geoclue-Reason"))
-            subtitle = g_desktop_app_info_get_string (info, "X-Geoclue-Reason");
+          if (G_DESKTOP_APP_INFO (info) &&


Suggested change

if (G_DESKTOP_APP_INFO (info) &&

if (G_IS_DESKTOP_APP_INFO (info) &&

smcv · 2022-11-03T11:48:13Z

src/xdp-utils.c

  return app_info->id;
 }

+const char *
+xdp_app_info_get_desktop_id (XdpAppInfo *app_info)


If this is named get_desktop_id then it should return a desktop ID as per the spec, including the .desktop suffix. Or if it's going to return a string without the suffix, call it get_desktop_entry.

smcv · 2022-11-03T11:48:39Z

src/xdp-utils.c

-      desktop_id = NULL;
-      break;
-    }
+  if (xdp_app_info_get_desktop_id (app_info))


Assign this to a temporary variable to avoid calling it redundantly

smcv · 2022-11-03T11:52:47Z

src/notification.c

+  desktop_id = xdp_app_info_get_desktop_id (app_info);
+  if (desktop_id && g_strcmp0 (xdp_app_info_get_id (app_info), desktop_id) != 0)
+    g_variant_builder_add (&n, "{sv}", "desktop-id", g_variant_new_string (desktop_id));
+
  for (i = 0; i < g_variant_n_children (notification); i++)


We should either silently remove a caller-supplied desktop-entry (or desktop-id, whichever one you use), or allow it but reject values (fail the request) that are not owned by this particular app.

For Flatpak, there is a mechanism for deciding which .desktop IDs are allowed in an app (they must be either the app ID or prefixed with the app ID, I can't remember the precise details).

For Snap, maybe that's in the metadata?

The desktop-id field is being set on the call from the portal to the backend and the value comes from snapd so it is correct. There's no desktop-id field received from the call from the app to the portal, so this can't be set to an invalid value.

I thought the portal was copying the contents of the a{sv} from the caller as-is.

I now see that check_notification() only allows certain keys, and rejects any call that has others (and in particular it would reject a notification that has a desktop-entry or desktop-id. If this is a guarantee, then I'm still a little uncomfortable about mixing trusted and untrusted fields in the same dict, but we don't strictly need new API.

@smcv yeah, I had more or less the same concern.

So, while using impl- prefix could help avoid such stuff to be "leaked" to the app, it still depends on implementer.

But the alternative was to add another method call AddNotification2 with platform/private data, and it seemed also too much to me for just this.

robert-ancell · 2022-11-04T10:42:07Z

Raised in flatpak/xdg-desktop-portal-gtk#401 by @TingPing:

I feel like there should be some validation that the desktop-id is correct.

An application should not be able to report itself as a different application. So we should check that the desktop-id is prefixed by the app-id.

The purpose of this change is to fix the case where desktop-id doesn't match the app-id, which is for older applications that have not yet migrated to the new system (we are working on fixing this for snaps currently). In the modern case this field is not used and everything can work off the app-id.

Regarding validation - does the portal backend not trust the values sent by the portal? It doesn't seem to currently do any validation of the app-id. I was under the impression that the backends should only be called by the portal, and applications should only contact the portal.

smcv · 2022-11-04T12:07:55Z

Regarding validation - does the portal backend not trust the values sent by the portal? It doesn't seem to currently do any validation of the app-id. I was under the impression that the backends should only be called by the portal, and applications should only contact the portal.

Yes, I think that's correct. Sandboxed apps should not be allowed to contact the backend directly.

When writing my previous comments I thought that the portal API was to pass through arbitrary notification a{sv} fields as-is, which would make the notification a{sv} untrusted (app-controlled, therefore potentially attacker-controlled), even though the method call as a whole is trusted. The documentation says the format of notification is the same as in the portal API, which suggested to me that the portal implementation was expected to pass it through as-is.

I now see that check_notification() rejects all keys that are not specifically documented in o.fd.p.Notification as supported. This design still makes me a little uncomfortable, because it's mixing trusted and untrusted data in the same dict: for instance, after this PR, icon is attacker-controlled (and could be used to impersonate a more-trusted app like Firefox if the backend isn't careful!) but desktop-id is trusted. I think it would be OK if documented clearly.

Perhaps something like this in the app-facing portal API:

AddNotification:
...

        The format of the serialized notification is a vardict, with
        the following supported keys, all of which are optional:

        <variablelist>
...
        </variablelist>

        Unsupported keys are not allowed, and will cause the notification to be rejected with an error.
        Keys with incorrect types are also not allowed.

        In particular, the extra keys that are documented in
        org.freedesktop.impl.portal.Notification.AddNotification()
        are not allowed here.

and in the impl:

        AddNotification:
...

        @notification can contain any of the keys documented in
        org.freedesktop.portal.Notification.AddNotification(), such as `title`.
        The portal will ensure that they have their documented types:
        for example, if `title` is present, it is guaranteed to be a string.
        Their content is supplied by the application and should be treated
        as untrusted.

        In addition, @notification can contain keys provided by the portal itself.
        The backend may assume that these keys are trusted.

        <variablelist>
...

I think check_notification() also deserves a comment, perhaps something like this:

/* This function is a security boundary: we must not allow the app to specify
 * keys not documented in org.freedesktop.portal.Notification, otherwise
 * backends would be unable to know which keys are trusted information from
 * the portal and which keys are attacker-controlled. */

smcv · 2022-11-04T12:13:07Z

The purpose of this change is to fix the case where desktop-id doesn't match the app-id, which is for older applications that have not yet migrated to the new system (we are working on fixing this for snaps currently)

Is the assumption here that each Snap app contains up to one .desktop ID in its metadata, and that metadata is trusted? (For instance, if a compromised or malicious Snap app had metadata that claimed to implement firefox.desktop, would that be considered to be a Snap problem that is not x-d-p's responsibility to solve?)

robert-ancell · 2022-11-04T12:46:29Z

Is the assumption here that each Snap app contains up to one .desktop ID in its metadata, and that metadata is trusted? (For instance, if a compromised or malicious Snap app had metadata that claimed to implement firefox.desktop, would that be considered to be a Snap problem that is not x-d-p's responsibility to solve?)

Correct. The metadata that x-d-p is accessing contains up to one .desktop ID, and that is guaranteed to map to an actually installed .desktop file. The metadata comes from snapd, and is not set by the snap other than as part of the packaging metadata which is validated on install.

GeorgesStavracas · 2022-11-17T13:47:15Z

IIRC me @3v1n0 and @jadahl talked about this during the Ubuntu Summit and me and @jadahl were not convinced this is the correct approach - it'd just delay breakage to other parts of the stack. Instead, the Snap code should instead store and report the correct application id.

3v1n0 · 2022-11-17T22:09:21Z

IIRC me @3v1n0 and @jadahl talked about this during the Ubuntu Summit and me and @jadahl were not convinced this is the correct approach - it'd just delay breakage to other parts of the stack. Instead, the Snap code should instead store and report the correct application id.

So, yeah... Basically, the only way we have to handle this properly as I was suspecting since the my initial analysis some months ago, is to basically use the approach of #769, but updating it so that the whole desktop-id is used all the times (at least when we've one) and permissions are migrated in all the cases.

Since testing of this is already split out I think it's better to close this, while I'll move some fixes to a new PR.

3v1n0 force-pushed the desktop-id-notifications branch from 72cbdd3 to c99b086 Compare November 2, 2022 15:45

robert-ancell reviewed Nov 2, 2022

View reviewed changes

src/xdp-utils.c Outdated Show resolved Hide resolved

robert-ancell approved these changes Nov 2, 2022

View reviewed changes

3v1n0 force-pushed the desktop-id-notifications branch from c99b086 to bf0c70d Compare November 2, 2022 23:07

3v1n0 mentioned this pull request Nov 2, 2022

notification: Use notification desktop-id to select backend and notify flatpak/xdg-desktop-portal-gtk#401

Closed

3v1n0 added 2 commits November 3, 2022 01:06

xdp-utils: Keep track of the desktop-id in snap applications

1396ff0

We may use it in multiple places, so save it for the whole time.

xdp-utils: Add a function to explicitly get the application desktop-id

478e7f5

Different container implementations may have different way to expose desktop-id's vs app-id's, so let's provide a way to explicitly get that.

3v1n0 force-pushed the desktop-id-notifications branch from bf0c70d to fd5d556 Compare November 3, 2022 01:12

3v1n0 added 2 commits November 3, 2022 06:08

tests/meson: Ensure xdg-desktop-portal is built when running tests

34c9021

This is particularly useful when changing the portal and want to run the tests against it.

3v1n0 force-pushed the desktop-id-notifications branch from fd5d556 to 761cf3c Compare November 3, 2022 05:11

3v1n0 added 2 commits November 3, 2022 10:37

xdp-utils: Add AppInfo Kind TEST for testing purposes

e47fcec

Make possible to test applications by allow mocking various applications properties and capabilities. This is simpler for now, but will help various cases in future.

3v1n0 force-pushed the desktop-id-notifications branch from 761cf3c to d41bc26 Compare November 3, 2022 09:37

3v1n0 added 4 commits November 3, 2022 10:46

open-uri: Do not leak the default-app info ID

387c9e5

tests/notifications: Ensure that the application ID is matching expec…

5bfdd03

…ted one

tests/notification: Add test to verify that desktop file is sent to impl

ac93698

And another one to check that is ignored when matching the app-id.

3v1n0 force-pushed the desktop-id-notifications branch from d41bc26 to ac93698 Compare November 3, 2022 09:46

3v1n0 mentioned this pull request Nov 3, 2022

Add Persistence notification flags #786

Closed

smcv requested changes Nov 3, 2022

View reviewed changes

3v1n0 mentioned this pull request Nov 9, 2022

xdp-utils: Add AppInfo Kind TEST for testing purposes #909

Open

3v1n0 closed this Nov 17, 2022

jadahl mentioned this pull request Oct 25, 2024

notification: Add the desktop-file-id key for portal backends #1488

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notification: Pass the desktop-id to the notification impl #904

notification: Pass the desktop-id to the notification impl #904

3v1n0 commented Nov 2, 2022

robert-ancell left a comment

3v1n0 commented Nov 3, 2022

smcv left a comment

smcv Nov 3, 2022

smcv Nov 3, 2022

3v1n0 Nov 9, 2022

smcv Nov 3, 2022

smcv Nov 3, 2022

smcv Nov 3, 2022

smcv Nov 3, 2022

robert-ancell Nov 4, 2022

smcv Nov 4, 2022

3v1n0 Nov 9, 2022

robert-ancell commented Nov 4, 2022 •

edited

Loading

smcv commented Nov 4, 2022

smcv commented Nov 4, 2022

robert-ancell commented Nov 4, 2022

GeorgesStavracas commented Nov 17, 2022

3v1n0 commented Nov 17, 2022

	This may me omitted in case the provided @app_id is matching it.
	If not specified, the default is the provided @app_id followed by <literal>.desktop</literal>.

	if (G_DESKTOP_APP_INFO (info) &&
	if (G_IS_DESKTOP_APP_INFO (info) &&

notification: Pass the desktop-id to the notification impl #904

notification: Pass the desktop-id to the notification impl #904

Conversation

3v1n0 commented Nov 2, 2022

robert-ancell left a comment

Choose a reason for hiding this comment

3v1n0 commented Nov 3, 2022

smcv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert-ancell commented Nov 4, 2022 • edited Loading

smcv commented Nov 4, 2022

smcv commented Nov 4, 2022

robert-ancell commented Nov 4, 2022

GeorgesStavracas commented Nov 17, 2022

3v1n0 commented Nov 17, 2022

robert-ancell commented Nov 4, 2022 •

edited

Loading