Skip to content
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

Handle Thunderbird as a special case for mail with attachment. #447

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nteodosio
Copy link

@nteodosio nteodosio commented Oct 13, 2023

Thunderbird no longer accepts attachments in the mailto URI, it just ignores them. This breaks Nautilus right-click > Email, for example. More context: https://gitlab.gnome.org/GNOME/nautilus/-/issues/2431.

Creates a special case for Thunberbird then, using the -compose option.

Note: This is essentially a smaller diff than it looks, but the indentation level change makes it seem larger. For reference,
the patch without indentation level changes.

@nteodosio
Copy link
Author

Ah so g_string_free_and_steal is only for Glib >2.76, while Ubuntu 22.04 is on 2.72. I'll rewrite that bit then.

@jadahl
Copy link
Contributor

jadahl commented Oct 13, 2023

Special casing apps this way is not reasonable IMO. If Thunderbird stopped supporting mailto as command line argument, then you need to create a compatibility wrapper that handles this, and make that the entry point for the mailto: hander call. This wrapper thing would either be shipped by thunderbird, or by the package (flatpak or snap), thus need no special casing in all possible portals.

Thunderbird no longer accepts the mailto URI. This breaks Nautilus
right-click > Email, for example.

Creates a special case for Thunberbird then, using the -compose option.
@seb128
Copy link

seb128 commented Oct 13, 2023

Special casing apps this way is not reasonable IMO. If Thunderbird stopped supporting mailto as command line argument, then you need to create a compatibility wrapper that handles this, and make that the entry point for the mailto: hander call. This wrapper thing would either be shipped by thunderbird, or by the package (flatpak or snap), thus need no special casing in all possible portals.

The point of not handling 'attachment=' in mailto: is to avoid making it too easy to trick users into attaching a file without noticing. Imagine a website with a 'contact us for free trial' link which points to a 'mailto:...attachement=~/.ssh/.id_rsa'....

https://www.nds.ruhr-uni-bochum.de/media/nds/veroeffentlichungen/2020/08/15/mailto-paper.pdf has some reference to the issue

Adding a wrapper that would make attachment in mailto work again would just add back the vulnerability.

The problem existed in evolution (CVE-2011-3201) and was fixed there by blocking attachments from /etc and hidden directories but it's somewhat fragile (you can have private content outside of those paths) and not a solution thunderbird (and some other mailers) were comfortable using.

Thunderbird is not likely to change and a popular email client for linux users...

@jadahl
Copy link
Contributor

jadahl commented Oct 13, 2023

I thought this was only about missing mailto: not about attachment. Automagic attachments must not be supported indeed.

@seb128
Copy link

seb128 commented Oct 13, 2023

Sorry the PR description is probably missing some context, the problem is discussed in more details in https://gitlab.gnome.org/GNOME/nautilus/-/issues/2431

but basically the issue is that the way the portal add attachements today is by constructing a 'mailto:....attachement=' url and calling the mail client on it, which is incompatible with thunderbird, which makes the nautilus sendto feature just not work with thunderbird. One alternative would be to hide the 'sendto' entry from nautilus when the default mailer is thunderbird (or another client not handling the mailto scheme for attachements)

@nteodosio
Copy link
Author

I apologize, the description was not indeed correctly contextualized.

I edited the main comment with the link from seb128 for better visibility.

Copy link

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've a bit the same concerns regarding the hardcoding app-special cases into the portal.

Wondering if we can handle this somewhere else from the launcher instead. Or even provide a new way to launch an app when using some local file that as to be passed via the document portal?

Btw, syntax style has to be fixed, see https://developer.gnome.org/documentation/guidelines/programming/coding-style.html

Comment on lines +50 to +55
/* The commandline can be complicated, e.g.
* env BAMF_DESKTOP_FILE_HINT=/var/lib/snapd/.../thunderbird_thunderbird.desktop /snap/bin/thunderbird %u
* so just match for thunderbird.
*/
old_cmdline = g_app_info_get_commandline(info);
if(strstr(old_cmdline, "thunderbird")) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't use g_app_info_get_executable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that in the example command line commented above, that would be env.

sep = "&";

for (i = 0; cc[i]; i++)
is_tb_snap = strstr(old_cmdline, "/snap/bin/thunderbird");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check this from the application type

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what is that? Does it have something to do with "content type" as in https://docs.gtk.org/gio/iface.AppInfo.html?

@ebassi
Copy link
Collaborator

ebassi commented Oct 20, 2023

Drive-by comment: the coding style is all over the place, and does not match the existing one. Please, use a style consistent with the rest of the file you're modifying.

@jadahl
Copy link
Contributor

jadahl commented Oct 20, 2023

I've a bit the same concerns regarding the hardcoding app-special cases into the portal.

Indeed, we should not do that.

One alternative would be to hide the 'sendto' entry from nautilus when the default mailer is thunderbird (or another client not handling the mailto scheme for attachements)

Maybe sendto should be removed from nautilus if attachments via mailto: is considered dangerous?

@jadahl
Copy link
Contributor

jadahl commented Oct 20, 2023

Adding a wrapper that would make attachment in mailto work again would just add back the vulnerability.

How would it be different adding a wrapper doing what this PR is doing, but inside the Thunderbird snap/flatpak?

@nteodosio
Copy link
Author

Please, use a style consistent with the rest of the file you're modifying.

Pardon that, I took note of 3v1n0's comment on that but given the low probability of the changes being merged I didn't act on it yet.

How would it be different adding a wrapper doing what this PR is doing, but inside the Thunderbird snap/flatpak?

That should work, but I'm afraid that would have to be done for all forms of packaging, correct?

@seb128
Copy link

seb128 commented Oct 24, 2023

How would it be different adding a wrapper doing what this PR is doing, but inside the Thunderbird snap/flatpak?

Hum, what would the wrapper do exactly? If we add a script that 'translate' a mailto: url including an attachment to one not including one and appending the option to the cmdline instead then we would restore the problem that a click on a url from a website would allow to attach a filename no? Or we would need from the wrapper to be able to tell what is the caller and only allow that if it's nautilus...

@jadahl
Copy link
Contributor

jadahl commented Oct 24, 2023

Hum, what would the wrapper do exactly?

Isn't translating mailto into something Thunderbird understands exactly what is attempted to be introduced here?

The callee won't know who the caller is, no, so it's not possible to "allow list" nautilus in Thunderbird, so I guess if that was the intention, it wont work. Allow listing doesn't seem like a sane solution either way. So it seems like nautilus send-to feature relies on a feature that has potentially significant security implications, and perhaps best to not do that anymore at all.

@seb128
Copy link

seb128 commented Oct 24, 2023

Isn't translating mailto into something Thunderbird understands exactly what is attempted to be introduced here?

yes, but the point is that we want to do it only for safe callers. Thinking about it more though the current patch wouldn't achieve that either since it's not checking the caller, and such is not really different from the wrapper (it works in our case only because firefox doesn't use the portal atm).

It seems either nautilus should drop the option or go back to have custom code rather than relying on the portal...

@jadahl
Copy link
Contributor

jadahl commented Oct 24, 2023

If a sandboxed nautilus can use custom Thunderbird specific code to make it compose an E-mail with a predetermined attachment, what'd stop any other app from doing the same? Or is your assumption that nautilus is outside the sandbox?

@seb128
Copy link

seb128 commented Oct 24, 2023

If a sandboxed nautilus can use custom Thunderbird specific code to make it compose an E-mail with a predetermined attachment, what'd stop any other app from doing the same? Or is your assumption that nautilus is outside the sandbox?

I was speaking about the case outside the sandbox (I don't know if any distribution provides nautilus sandboxed today, that has other challenges for a filemanager that users would probably expect to tbe able to access any part of their filesystem). But yes, once sandboxed it has no way to invoke the email client

@jadahl
Copy link
Contributor

jadahl commented Oct 24, 2023

Sounds like the best long term solution is to drop support for "Send file" via E-mail from file managers then.

@nteodosio
Copy link
Author

At least from the user perspective that seems like a nice and reasonable feature to have. The typical thing even Windows XP already had, it looks like a small but solid step back to drop that.

Especially if we consider e.g. Evolution, that warns about the attachment instead of preventing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants