webextensions: add a portal for managing WebExtensions native messaging servers #705
Conversation
|
This looks like an implementation of the easy parts of #655, which is missing the hard parts, most notably a security model: if I'm reading correctly, this gives every Flatpak/Snap app access to all webextensions, and trusts every Flatpak/Snap app to say what domain it is acting on behalf of. Of course it's useful to have an implementation of the easy parts, but it cannot be merged without someone doing the harder work to design a security model for it, and then implement that security model. I certainly wouldn't want anyone to get the impression that this PR is basically finished and just waiting for reviewers to say yes. At the risk of stating the obvious, it is not OK for an untrusted app |
|
The answer to "is this a static permission" or "is this a user prompting issue" is, IMO, both. A sandboxed browser is the one responsible for checking whether the appropriate sites are or aren't accessing the native messaging hosts they are permitted to. The http/JS/... session can't be moved out of the browser, there's nothing the portal can do to know or verify the origin domain that I can reasonably think of. So the static permission is "I trust this browser to have some internal confinement and be able to identify a site to me correctly" -> "therefore it may speak to the native messaging host portal and identify a site to the user". The user consent would be specific and timely "[Browser] wants [Website] to access [Application] on your system." and provides the user the choice to verify several things:
|
|
I agree that the access control side needs to be completed: that's why I marked the PR as draft to indicate that it wasn't ready to be merged. I published this more so there would be something to get feedback on the API and be able to test if it was fit for purpose. As for the permission dialog, the pieces of information we have for a prompt dialog are:
I think (1) and (4) would be the most important pieces of information to include in the access dialog. The extension ID is not likely to be useful to the user (especially in the Chromium case, or Firefox extensions still using UUIDs as their IDs). If we want to provide a human readable name for the extension, then the browser would need to provide that to the API. That said, I'm not sure whether it makes sense to include information about the extension in the access dialog, since it is effectively self asserted data from an untrusted client: we're simply using it in the portal implementation to help the browser implement its own security policies within the app sandbox. |
|
Also, in follow-up to #705 (comment), the native messaging host is invoked by a browser extension rather than a web page. So in the case of the chrome-gnome-shell native messaging server, it isn't the extensions.gnome.org website talking to it directly: rather it talks to an extension that in turn invokes the native messaging server. So I don't think it makes sense to include website details in the access dialog either. |
I'm not sure this is quite right. The native hosts are installed on the host system, or if they are exported via a sandboxing runtime such as Flatpak or Snap, the metadata provided around naming (or icons, etc) is at an equivalent trust level to eg, the names/icons in a .desktop file, which the portal already relies on heavily (ie via XdpAppInfo) for user decisions around access/launching/etc. (Of course, whether Flatpak should be exporting app icons and descriptions unmodified is a separate question, the security model there seems to be about filtering by app ID, which is not what the user sees, but that's not a regression introduced by this PR so I wouldn't get bogged down in it. If we look at eg exporting native message hosts from a Flatpak, this would need some attention.)
Oh yes. That's a bit less useful indeed. Is there a way we can allow the browser (ie trusted enough to have the static permission) to provide optional additional info here that identifies the extension in a slightly better way than the ID? Presumably our fallback implementation of this is a stub launcher that just calls to the proxy, and the sandboxed browser is unaware that there's any sandbox exit going on (I got chrome-gnome-shell working once on this basis) but if we move to submitting patches we might be able to do better? |
|
If browsers have static permission to allow access to native messaging servers the native messaging servers already only have to deal with trusted parties (i.e. everyone talking to a native messaging server is authenticated as a browser). Similarly if flatpak only allows to export native messaging servers with the name matching the appid, the browsers only deal with authenticated parties. Any kind of dynamic permission dialog in the portal can only show the appid of the browser and the appid/name of the native messaging server which is completely redundant. We can't show any relevant information in the portal because we don't understand the mechanisms of the native messaging server. Instead the browser can (and probably should) ask the user if it's okay for a specific add-on to talk to a specific app/native messaging server. (If we didn't have static permissions for browsers it would be the responsibility of the native messaging server to ask the user if it want's to allow a specific request from a specific browser/app.) |
|
I feel like this is leaving out an important aspect of native messaging hosts, which would be the browser communicating with other Flatpaks (e.g. Chrome talking to 1password). That's technically already possible & I have some PoC code locally in that direction, but if there's going to be a full portal, it probably needs to handle that case too to have a unified implementation. |
jhenstridge
force-pushed
the
native-messaging-portal
branch
from
a21ad86
to
21113e2
Compare
|
I've updated the PR to add the suggested permission checks: both a static check in The text in the access dialog could use some work, as I'm not sure how best to describe the native messaging server in a way that would be meaningful to the user. If it's decided that a static permission is sufficient, then I guess we can remove this check and this wouldn't matter. |
|
It would be useful to add a |
|
I'm playing with this, and I noticed that if I manually kill the spawned process, the portal crashes with the following message: |
|
An update on my work to integrate this in Firefox: I've submitted a WIP patch that's mostly functional, but I still need to figure out why reading/writing from/to the passed file descriptors fails, Firefox complains that the files are closed. |
|
And another update: I figured out the file descriptors issue, so this is now working as expected, in my limited testing. I am now building a snap with this patch to test under proper confinement. |
|
A note for my future self: when testing this against e.g. the firefox snap and the chrome-gnome-shell native connector, the following command is useful to grant the required permission: |
There was a problem hiding this comment.
@jhenstridge can you elaborate a bit more on the TODO around checking whether the requesting app is a browser in xdp_app_info_is_web_browser()?
src/webextensions.c
Outdated
| hosts_path_str = g_getenv ("XDG_DESKTOP_PORTAL_WEB_EXTENSIONS_PATH"); | ||
| if (hosts_path_str == NULL) | ||
| { | ||
| hosts_path_str = "/usr/lib/mozilla/native-messaging-hosts:/etc/opt/chrome/native-messaging-hosts:/etc/chromium/native-messaging-hosts"; |
There was a problem hiding this comment.
Should this also include /etc/opt/edge/native-messaging-hosts for Microsoft Edge?
There was a problem hiding this comment.
I'm honestly not sure about this one. If we have any messaging hosts supporting Edge, would the json files they install for Chrome likely cover the IDs of the Edge extension too?
jhenstridge
force-pushed
the
native-messaging-portal
branch
from
a90b52d
to
8df492e
Compare
jhenstridge
force-pushed
the
native-messaging-portal
branch
from
b970156
to
643831a
Compare
|
Thanks for merging the fix for the access dialog. In my testing, I'm still consistently seeing the following problem: the first time the user is prompted for authorization and accepts, the portal will spawn the native connector and pass the file descriptors to the client (firefox snap in my tests), but the client sees the file descriptors as closed. Closing the client and re-opening it doesn't help. Only when the portal is terminated and restarted does communication through FDs start working normally. |
And with the following patch to grant authorization right away instead of prompting the user, the problem goes away: --- a/src/webextensions.c
+++ b/src/webextensions.c
@@ -392,7 +392,7 @@ handle_start_in_thread (GTask *task,
}
app_id = xdp_app_info_get_id (request->app_info);
- permission = get_permission_sync (app_id, PERMISSION_TABLE, arg_name);
+ permission = PERMISSION_YES;
if (permission == PERMISSION_ASK || permission == PERMISSION_UNSET)
{
guint access_response = 2; |
jhenstridge
force-pushed
the
native-messaging-portal
branch
from
dd10691
to
fd77dae
Compare
This ensures the session won't be freed while the callback is running, as could happen when close_sessions_in_thread_func() runs. While this creates a reference cycle, it will be broken when the session is closed.
Firefox also documents /usr/lib64/mozilla/native-messaging-hosts as part of the search path. Also include locations based on xdg-desktop-portal's install location.
…ession. At this point, the browser has signalled the native messaging host to exit by closing its standard input. If the process is still running when the browser closes the session, then it is not responding.
…ther to provide Chromium-ish or Mozilla-ish behaviour This affects the following behaviour: 1. the search path for manifest files. 2. whether to match the "allowed_origins" or "allowed_extensions" key. 3. the command line arguments passed to the native messaging server. The tests have also been updated to use a shell script as the test native messaging server. Previously we just used /bin/cat, since it would stick around until its standard input was closed. This broke once we started passing arguments to the server. So now use a shell script that calls cat with no arguments.
While Chromium based browsers are more common, we only have an implementation for Firefox for now, which is not setting this option.
jhenstridge
force-pushed
the
native-messaging-portal
branch
from
7a3bb13
to
4f25231
Compare
|
Progress!!! 😮😭 |
| } | ||
|
|
||
| static void | ||
| web_extensions_session_class_init (WebExtensionsSessionClass *klass) |
There was a problem hiding this comment.
(Question to myself during review)
Where is web_extensions_session_class_init used? Similarly for web_extensions_class_init.
Upon some searching around, I found that the code pattern here resembles https://docs.gtk.org/gobject/concepts.html#object-instantiation
and that there is a line elsewhere in this file with: G_DEFINE_TYPE (WebExtensionsSession, web_extensions_session, session_get_type ());
...and that an example of equivalent code without macros is shown at: https://docs.gtk.org/gobject/concepts.html#instantiatable-classed-types-objects
and that there is some documentation around when the finalize functions are invoked, at: https://docs.gtk.org/gobject/concepts.html#reference-count
P.S. Given my unfamiliarity with these conventions, I am assuming that someone else has performed a review of these aspects. I am only focusing on issues that require domain knowledge of the WebExtensions / native messaging / Firefox / Chrome aspects of this.
There was a problem hiding this comment.
Sorry for the delay, and ack, I've added some comments documenting the flow of execution a bit as I understood it. I'll be addressing more of your comments soon as well.
| object_class->finalize = web_extensions_session_finalize; | ||
|
|
||
| session_class = (SessionClass *)klass; | ||
| session_class->close = web_extensions_session_close; |
There was a problem hiding this comment.
So this sets up the handler of org.freedesktop.portal.Session::Close.
Would be nice to have a comment, so that it is easier to grep for the handler that was mentioned in data/org.freedesktop.portal.WebExtensions.xml
| session_class->close = web_extensions_session_close; | |
| // Register handler for org.freedesktop.portal.Session::Close | |
| session_class->close = web_extensions_session_close; |
| web_extensions_session_close (Session *session) | ||
| { | ||
| WebExtensionsSession *web_extensions_session = (WebExtensionsSession *)session; | ||
|
|
There was a problem hiding this comment.
Add comment to clarify.
| // This function can be called repeatedly, e.g. by an explicit | |
| // "org.freedesktop.portal.Session::Close" message followed | |
| // by a call to finalize due to session's refcount reaching zero. |
| g_variant_builder_init (&opt_builder, G_VARIANT_TYPE_VARDICT); | ||
| g_variant_builder_add (&opt_builder, "{sv}", "deny_label", g_variant_new_string (_("Don't allow"))); | ||
| g_variant_builder_add (&opt_builder, "{sv}", "grant_label", g_variant_new_string (_("Allow"))); | ||
| if (!xdp_dbus_impl_access_call_access_dialog_sync (access_impl, |
There was a problem hiding this comment.
I previously posted feedback at #705 (comment) that did not get an answer. I've tried to find an answer, below. Please take a look and verify/challenge my answer.
It may take a while before the permission is granted and/or launch, during which the initiator of the session could already be gone. E.g. in any of the following situations:
- The extension has explicitly closed the port.
- The page within the extension has closed.
- The extension was disabled or unloaded.
- The browser quit.
Does the current implementation guarantee that (1) the session is closed and (2) the native app is killed as expected?
The current function is handle_start_in_thread, which calls xdp_dbus_impl_access_call_access_dialog_sync. This seems to block the thread until the user has approved or denied the request for a browser extension to connect through the portal.
In all situations from my bullet list, org.freedesktop.portal.Session::Close is called, unless the browser was interrupted/terminated before it could gracefully end the session. The implementation in this PR seems to rely on web_extensions_session_finalize to clean up the session if al references got lost. Since SESSION_AUTOLOCK_UNREF (g_object_ref (session)); is used above, I presume that the refcount will never drop to zero as long as we are in this class.
So my conclusion is:
- if the browser crashes or is killed while the prompt is shown, then the implementation here does not get notified. After the prompt is approved, the native messaging host app is launched a few lines below, and then when the refcount drops to zero, the native app is immediately killed.
- if the browser closes through
org.freedesktop.portal.Session::Close, thenweb_extensions_session_closeis called. Does this call await the completion of thishandle_start_in_threadcall? Are they even called on the same thread? (I suspect that the answer to both is no) If the two run sequentially, then the cleanup follows the same flow as in the previous bullet point. If they do run in parallel, then we'd end up with a native messaging host that outlives the caller (browser extension inside web browser). That is bad.
In the current implementation, Start can take an arbitrary amount of time. Even if the calling browser has shut down a long time ago, it is technically possible for the prompt to be somewhere on screen, and trigger the launch of the native messaging host application (followed by an attempt to quit it). This is acceptable from the browser's perspective, but is it too from the portal's perspective? Please add ample comments/documentation somewhere!
There was a problem hiding this comment.
As things currently stand, handle_start_in_thread is holding the session object's mutex when it pops up the access dialog. So a call to the Close method would block when it tried to acquire the mutex. The same would be true if the browser exited and we were trying to clean up.
What does seem to be missing is that we aren't passing the cancellable to call_access_dialog_sync. This would let the browser withdraw the access dialog early by calling Close on the Request object returned by Start, and ensure the cleanup (in close_requests_in_thread_func) doesn't block should the browser exit.
There was a problem hiding this comment.
What does seem to be missing is that we aren't passing the cancellable to
call_access_dialog_sync.
The cancellable is now passed in c739835 .
This would let the browser withdraw the access dialog early by calling
Closeon the Request object returned byStart, and ensure the cleanup (inclose_requests_in_thread_func) doesn't block should the browser exit.
This is currently not implemented on the browser side, I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1903338 for that.
Is the session automatically canceled (and the prompt dismissed) if the session is closed (e.g. due to the browser shutting down)? Could we have a unit test for that?
|
|
||
| if (web_extensions_session->state == WEB_EXTENSIONS_SESSION_STATE_CLOSED) return; | ||
|
|
||
| web_extensions_session->state = WEB_EXTENSIONS_SESSION_STATE_CLOSED; |
There was a problem hiding this comment.
Is it safe to immediately transition web_extensions_session->state to the WEB_EXTENSIONS_SESSION_STATE_CLOSED state?
If this web_extensions_session_close function does not run on the same thread as handle_start_in_thread, then trouble happens when we that function is being executed (and state is WEB_EXTENSIONS_SESSION_STATE_STARTING):
- Data races, e.g. concurrent write to
web_extensions_session->state. - The native messaging host application launches and is never terminated. It can outlive the browser process that requested the session.
To avoid this issue, you need to synchronize access to the members touched here. And/or have checkpoint(s) in handle_start_in_thread that avoids the process launch or quits the launches process if the session was closed.
If possible, please add a unit test where the caller tries to close the session when the program is still being launched and verify that reasonable behavior happens (i.e. program exits when the caller is no longer there, no crashes, try compiling with TSan to check for thread data races).
There was a problem hiding this comment.
There's two places this function is called:
- the
session_close()function insrc/session.c, via thesession_class->closefunction pointer. This function expects the caller to lock the session's mutex (usually via theSESSION_AUTOLOCK_UNREFmacro). - in
web_extensions_session_finalize, at which point the last reference to the session has been released.
There was a problem hiding this comment.
James's clarifications/explanations as to why it's safe to do this were added as comments in the code.
|
|
||
| if (web_extensions_session->child_pid > 0) | ||
| { | ||
| kill (web_extensions_session->child_pid, SIGKILL); |
There was a problem hiding this comment.
Unconditional SIGKILL is not the way to go. For a description the correct termination sequence, see #705 (comment)
If you need inspiration on how to implement a waitpid-with-timeout, here is an example: https://source.chromium.org/chromium/chromium/src/+/main:base/process/process_posix.cc;l=36;drc=f4a00cc248dd2dc8ec8759fb51620d47b5114090
There was a problem hiding this comment.
As mentioned in the previous comment threads, xdg-desktop-portal can only implement part of the shutdown process. We do not hold on to copies of the process's stdin/stdout file descriptors, since that would prevent the browser from signalling the process to terminate by closing its own copies of the file descriptors.
From the browser's perspective, the shutdown process would be:
- close the process's stdin/stdout file descriptors obtained from xdg-desktop-portal.
- Wait for a D-Bus
Closedsignal on theorg.freedesktop.portal.Sessionobject, which will be triggered on SIGCHLD via theg_child_watch_add_fullhandler. - If the signal doesn't come in time, call the
Closemethod on the session object.
So the code here is just doing the final SIGKILL, and leaving the timeout policy up to the browser.
There was a problem hiding this comment.
@bandali0 What was described above is not implemented in the Firefox-side patch. To see the relevant code, use Ctrl+F "closeSession" at https://phabricator.services.mozilla.com/D140803. Are you going to implement it?
On the portal patch here, please add a comment that explains that the responsibility of graceful killing the application has been delegated to Firefox, and that the SIGKILL here is a last shot attempt to clean up if needed.
P.S. This is the third time I'm posting the comment, because for some reason it ended up in the wrong place when I posted before.
There was a problem hiding this comment.
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1903309 as a follow-up on the Firefox side.
If we want graceful termination, then we'd need a new message in the protocol (to send SIGTERM), don't we?
src/web-extensions.c
Outdated
| { | ||
| case WEB_EXTENSIONS_SESSION_MODE_CHROMIUM: | ||
| /* Chrome and Chromium search paths documented here: | ||
| * https://developer.chrome.com/docs/apps/nativeMessaging/#native-messaging-host-location |
There was a problem hiding this comment.
| * https://developer.chrome.com/docs/apps/nativeMessaging/#native-messaging-host-location | |
| * https://developer.chrome.com/docs/extensions/nativeMessaging/#native-messaging-host-location |
Fixed documentation link. "apps" is deprecated in Chrome and should not be referenced.
src/web-extensions.c
Outdated
| g_ptr_array_add (search_path, g_strdup (SYSCONFDIR "opt/chrome/native-messaging-hosts")); | ||
| g_ptr_array_add (search_path, g_strdup (SYSCONFDIR "chromium/native-messaging-hosts")); |
There was a problem hiding this comment.
| g_ptr_array_add (search_path, g_strdup (SYSCONFDIR "opt/chrome/native-messaging-hosts")); | |
| g_ptr_array_add (search_path, g_strdup (SYSCONFDIR "chromium/native-messaging-hosts")); | |
| g_ptr_array_add (search_path, g_strdup (SYSCONFDIR "/opt/chrome/native-messaging-hosts")); | |
| g_ptr_array_add (search_path, g_strdup (SYSCONFDIR "/chromium/native-messaging-hosts")); |
You're missing / after SYSCONFDIR. SYSCONFDIR does not end with a /, according to the current uses in the source: https://github.com/search?q=repo%253Aflatpak%252Fxdg-desktop-portal%20SY
src/web-extensions.c
Outdated
| g_ptr_array_add (search_path, g_strdup ("/usr/lib/mozilla/native-messaging-hosts")); | ||
| g_ptr_array_add (search_path, g_strdup ("/usr/lib64/mozilla/native-messaging-hosts")); | ||
| /* And the same for xdg-desktop-portal's configured prefix */ | ||
| g_ptr_array_add (search_path, g_strdup (LIBDIR "mozilla/native-messaging-hosts")); |
There was a problem hiding this comment.
| g_ptr_array_add (search_path, g_strdup (LIBDIR "mozilla/native-messaging-hosts")); | |
| g_ptr_array_add (search_path, g_strdup (LIBDIR "/mozilla/native-messaging-hosts")); |
Added slash assuming that it was missing by mistake.
Is there any documentation or reference that supports the use of the LIBDIR compile time constant? Please add documentation somewhere...
| session can at any time be closed using | ||
| org.freedesktop.portal.Session::Close, or may at any time be | ||
| closed by the portal implementation, which will be signalled | ||
| via org.freedesktop.portal.Session::Closed. |
There was a problem hiding this comment.
Elsewhere in this review (and earlier, at #705 (comment)) I described the expected behavior when a session is closed.
Let's explicitly document what one can expect to happen when the Close is received, and what cause when Closed is signaled. See my comment elsewhere in this review round to get an idea of what you could document.
jhenstridge
force-pushed
the
native-messaging-portal
branch
from
49cba93
to
c739835
Compare
If a new browser wants to support the interface, would that not require a change in x-d-p anyway because that browser would have its own manifest search paths? E.g. MyCoolNewBrowser™ wouldn't instruct developers to place their native manifests in
Does the browser even want to do that itself? What is the problem with just having the browser call
How can this currently be used anywhere in production? I thought the PR is not merged?? There also is confusion elsewhere about this. Can somebody explain? |
To help folks looking to work on the Web Extensions portal without prior familiarity with the XDG Desktop Portal and related code-bases like one of its main dependencies GLib and its object system.
Explain the session closure behaviour from browsers' perspective and link to the document about Firefox's implementation. Also, add references about arguments passed to the native executable in the Mozilla/Firefox and Chromium modes.
| * https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Native_manifests#manifest_location | ||
| */ | ||
| /* Add per-user directories */ | ||
| g_ptr_array_add (search_path, g_build_filename (g_get_home_dir (), ".mozilla", "native-messaging-hosts", NULL)); |
There was a problem hiding this comment.
We might want to also handle $XDG_CONFIG_HOME/mozilla here?
There was a problem hiding this comment.
@lissyx
Your comment is related to https://bugzilla.mozilla.org/show_bug.cgi?id=1903246 right?
Is there canonical documentation that documents this expected location?
There was a problem hiding this comment.
yes, it will be taken care of https://bugzilla.mozilla.org/show_bug.cgi?id=1905711, but i dont think we can update MDN so soon yet without any expected timeline, and i wanted to make sure there was a note of that somewhere
There was a problem hiding this comment.
Even if XDG_CONFIG_HOME were to be supported, then developers of native messaging components should still continue to write in the "old" location to support environments that do not support XDG_CONFIG_HOME. Therefore I think that it is acceptable to proceed with the current logic as is, and handle XDG_CONFIG_HOME later.
Do you agree with that assessment?
There was a problem hiding this comment.
Obviously, both will have to work, that's what we are already doing on Firefox side, only new profiles would default to $XDG_CONFIG_HOME use. I did mention « also » at first, i was not planning on replacing the path just having to dig into the code and see how to handle both
There was a problem hiding this comment.
Obviously, both will have to work, that's what we are already doing on Firefox side, only new profiles would default to
$XDG_CONFIG_HOMEuse. I did mention « also » at first, i was not planning on replacing the path just having to dig into the code and see how to handle both
I was referring to external application developers, that they can't only support XDG_CONFIG_HOME, and that therefore it may be acceptable for the portal to not recognize XDG_CONFIG_HOME.
The timeline of shipping support for XDG_CONFIG_HOME is unclear to me. If it is "relatively soon", say a few months at most, then I suppose that we can integrate it in the portal right away.
I asked for documentation or a description of the XDG_CONFIG_HOME feature because that would make it more obvious what the desired order of lookup should be.
There was a problem hiding this comment.
Obviously, both will have to work, that's what we are already doing on Firefox side, only new profiles would default to
$XDG_CONFIG_HOMEuse. I did mention « also » at first, i was not planning on replacing the path just having to dig into the code and see how to handle bothI was referring to external application developers, that they can't only support XDG_CONFIG_HOME, and that therefore it may be acceptable for the portal to not recognize XDG_CONFIG_HOME.
yes, that's why we try legacy first
The timeline of shipping support for XDG_CONFIG_HOME is unclear to me. If it is "relatively soon", say a few months at most, then I suppose that we can integrate it in the portal right away.
there's no hardcoded deadline yet, a few months might depend on the definition of "few" :), but it's obviously not going to land within the next weeks, and we probably dont want to wait years. So we definitively dont want to wait too much to integrate in the portal to make sure it has time to reach end users. Might also have to prepare backports patches for distros in case that is needed?
I asked for documentation or a description of the XDG_CONFIG_HOME feature because that would make it more obvious what the desired order of lookup should be.
canonical point of entry is still the (rather long) bug https://bugzilla.mozilla.org/show_bug.cgi?id=259356. But to summarize quickly:
MOZ_LEGACY_HOME=1: we continue with the existing behavior~/.mozilla/exists?: we continue with the existing behavior- otherwhise we
$XDG_CONFIG_HOME
There was a problem hiding this comment.
we probably dont want to wait years
Please don't. Flatpak browsers have been crippled for far too long.
There was a problem hiding this comment.
we probably dont want to wait years
Please don't. Flatpak browsers have been crippled for far too long.
before this gets out of control, there's 0 reason to hold the current PR for $XDG_CONFIG_HOME handling.
There was a problem hiding this comment.
To recap, the XDG_CONFIG_HOME handling effort in Firefox (https://bugzilla.mozilla.org/show_bug.cgi?id=259356) is about making sure that Firefox adheres to the platform conventions as specified in XDG's Basedir spec.
In the context of Firefox only, this is about the ability to read and write to an application-specific location, located at $XDG_CONFIG_HOME/mozilla/ instead of ~/.mozilla/. For compatibility reasons, the lookup order is ~/.mozilla/ first, then $XDG_CONFIG_HOME/mozilla/. Firefox may create files if they do not exist yet, and to force the current default of creating in ~/.mozilla/, MOZ_LEGACY_HOME=1 can be set (if unset, $XDG_CONFIG_HOME/mozilla/ would be created).
In the context of nativeMessaging and Firefox, it is relevant to know that this location is written to by external applications. Firefox treats them as read-only. Or, in the context of this portal, the portal reads from here and notifies Firefox of the discovered external native messaging applications.
The filesystem layout as seen by Firefox (inside the sandbox of the Snap package) can be independent of the filesystem layout outside of the sandbox (as seen by the portal). Therefore the mechanism to look up native messaging paths can be independent too.
Since the native messaging portal consumes the locations in a read-only fashion, there is no compatibility risk with unconditionally including $XDG_CONFIG_HOME/mozilla/native-messaging-hosts in the search_path (at the end). The only potential change in behavior is that the portal would be able to find and offer a native messaging host application, where Firefox would otherwise not be able to (until the patch stack associated with https://bugzilla.mozilla.org/show_bug.cgi?id=1905711 lands).
| To close a session, the browser should: | ||
| 1. close the process's stdin/stdout/stderr file descriptors |
There was a problem hiding this comment.
Firefox doesn't close stdout/stderr, only stdin. It is possible for the native application to continue writing to stderr which could help with debugging if desired. Here is a suggestion that is more accurate.
| 1. close the process's stdin/stdout/stderr file descriptors | |
| 1. close the process's stdin file descriptors, optionally also stdout/stderr. |
| closed by the portal implementation, which will be signalled | ||
| via org.freedesktop.portal.Session::Closed. | ||
| To close a session, the browser should: |
There was a problem hiding this comment.
The steps below assume the presence of a process + pipes. That is only if Start + GetPipes have already returned. If it is before, then we should just close the session, right? Here is a tweak to clarify when the following sequence should be followed:
| To close a session, the browser should: | |
| To close a session that progressed past Start() + GetPipes(), the browser should: |
| * https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Native_manifests#manifest_location | ||
| */ | ||
| /* Add per-user directories */ | ||
| g_ptr_array_add (search_path, g_build_filename (g_get_home_dir (), ".mozilla", "native-messaging-hosts", NULL)); |
There was a problem hiding this comment.
Even if XDG_CONFIG_HOME were to be supported, then developers of native messaging components should still continue to write in the "old" location to support environments that do not support XDG_CONFIG_HOME. Therefore I think that it is acceptable to proceed with the current logic as is, and handle XDG_CONFIG_HOME later.
Do you agree with that assessment?
There was a problem hiding this comment.
Looks good from Mozilla / Firefox's perspective.
There is a request added by @lissyx about XDG_HOME_CONFIG at #705 (comment) ; let's add that as well so that the portal is forward compatible with Firefox's behavior in the "near" future.
One question before signing off, related to security: This portal allows Firefox extensions in Firefox to run a native messaging application outside of the sandbox, through the portal. The location of the binary can be specified in multiple locations (in this patch in get_manifest_search_path at web-extensions.c), which includes locations such as ~/.mozilla/native-messaging-hosts/. What ensures that these locations are inaccessible to Firefox? For example, if ~/.mozilla/ were to be readable AND writable to Firefox within the Snap package, then a vulnerability resulting in a file writing primitive within the sandbox can result in arbitrary code execution outside of the sandbox through the portal. Please document this clearly.
| * https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Native_manifests#manifest_location | ||
| */ | ||
| /* Add per-user directories */ | ||
| g_ptr_array_add (search_path, g_build_filename (g_get_home_dir (), ".mozilla", "native-messaging-hosts", NULL)); |
There was a problem hiding this comment.
To recap, the XDG_CONFIG_HOME handling effort in Firefox (https://bugzilla.mozilla.org/show_bug.cgi?id=259356) is about making sure that Firefox adheres to the platform conventions as specified in XDG's Basedir spec.
In the context of Firefox only, this is about the ability to read and write to an application-specific location, located at $XDG_CONFIG_HOME/mozilla/ instead of ~/.mozilla/. For compatibility reasons, the lookup order is ~/.mozilla/ first, then $XDG_CONFIG_HOME/mozilla/. Firefox may create files if they do not exist yet, and to force the current default of creating in ~/.mozilla/, MOZ_LEGACY_HOME=1 can be set (if unset, $XDG_CONFIG_HOME/mozilla/ would be created).
In the context of nativeMessaging and Firefox, it is relevant to know that this location is written to by external applications. Firefox treats them as read-only. Or, in the context of this portal, the portal reads from here and notifies Firefox of the discovered external native messaging applications.
The filesystem layout as seen by Firefox (inside the sandbox of the Snap package) can be independent of the filesystem layout outside of the sandbox (as seen by the portal). Therefore the mechanism to look up native messaging paths can be independent too.
Since the native messaging portal consumes the locations in a read-only fashion, there is no compatibility risk with unconditionally including $XDG_CONFIG_HOME/mozilla/native-messaging-hosts in the search_path (at the end). The only potential change in behavior is that the portal would be able to find and offer a native messaging host application, where Firefox would otherwise not be able to (until the patch stack associated with https://bugzilla.mozilla.org/show_bug.cgi?id=1905711 lands).
This is an implementation of a slightly modified version of the interface proposed in #655 for starting WebExtensions native messaging servers on behalf of a confined web browser.
The main change was to remove the use of Request objects, as I'm not waiting for a backend impl server to do the work. It might be worth adding that back though to give more flexibility in the future though.
There are some missing features at present:
Fixes #655.