GNOME Bugzilla – Bug 588432
Seahorse embeds filename "-&25"
Last modified: 2011-05-09 08:24:19 UTC
Please describe the problem: If you decrypt files encrypted with seahorse and try to restore the embeded file name (default behavior of PGP 7.0.3, gpg knows the option --use-embeded-filename), you always get "-&25" als filename of the decrypted file. When encrypting with gpg on the command-line with the same options, the problem does not occur. Steps to reproduce: 1. encrypt file with seahorse 2. decrypt it with PGP 7's default settings or gpg --use-embeded-filename Actual results: decrypted file is always named "-&25" Expected results: foo.bar.pgp should be decrypted to foo.bar (or another embeded filename if foo.bar was renamed after encryption) Does this happen every time? yes Other information: setting an empty filename by adding set-filename "" to the gpg.conf is a workaround, but an ugly one.
It doesn't look like a seahorse bug since "gpg --use-embedded-filename foo.bar.pgp" causes that result. IMO it should be fixed in gpg . (In my case it's "-&24")
Created attachment 173981 [details] [review] 0001-Fixed-bug-when-embedding-filename-with-pgp-encryptio.patch Here it is a patch submitted by Patrick Toomey downstream with some minor modifications made by me. Tested and works for me.
Is this really the best way to detect that we're encrypting? + /* Embed filename during encryption */ + if (strcmp (mode->title, _("Encrypting")) == 0) Perhaps it would be better to use a specific flag in SeahorseToolMode for that?
I agree with Stef. Depending on a translated string for comparisons is a bad idea. While GNOME translations are in general very high in quality, there's always a chance of inconsistencies between translators.
Yes, I had the same feeling.
What about moving the static gboolean mode_encrypt, mode_decrypt... declarations from seahorse-tool.c to seahorse-tool.h ?
Created attachment 174385 [details] [review] Fixed bug when embedding filename with pgp encryption v2 Another proposal
Review of attachment 174385 [details] [review]: I like where this is headed, but think the booleans should go into the SeahorseToolMode structure. It appears to be accessible where they are needed and is cleaner than having the globals.
(In reply to comment #8) > Review of attachment 174385 [details] [review]: > > I like where this is headed, but think the booleans should go into the > SeahorseToolMode structure. It appears to be accessible where they are needed > and is cleaner than having the globals. Yes, that's prettier. The problem I have found is that the booleans are set according to the arguments seahorse-tool is called with. And at that moment the SeahorseToolMode structure is not defined yet. Any other possibilities? static const GOptionEntry options[] = { { "import", 'i', 0, G_OPTION_ARG_NONE, &mode_import, N_("Import keys from the file"), NULL }, { "encrypt", 'e', 0, G_OPTION_ARG_NONE, &mode_encrypt, N_("Encrypt file"), NULL }, { "sign", 's', 0, G_OPTION_ARG_NONE, &mode_sign, N_("Sign file with default key"), NULL }, { "encrypt-sign", 'n', 0, G_OPTION_ARG_NONE, &mode_encrypt_sign, N_("Encrypt and sign file with default key"), NULL }, { "decrypt", 'd', 0, G_OPTION_ARG_NONE, &mode_decrypt, N_("Decrypt encrypted file"), NULL }, { "verify", 'v', 0, G_OPTION_ARG_NONE, &mode_verify, N_("Verify signature file"), NULL }, { "uri-list", 'T', 0, G_OPTION_ARG_NONE, &read_uris, N_("Read list of URIs on standard in"), NULL }, { G_OPTION_REMAINING, 0, 0, G_OPTION_ARG_STRING_ARRAY, &arg_uris, NULL, N_("file...") }, { NULL } };
Hey, I hadn't checked in on this bug in a few weeks and was glad to see it moving forward. Good call on transitioning from the strcmp to the boolean. I knew it was ugly when I did it, but didn't know of the mode boolean variables (it is the first time I've taken a look at the seahorse codebase). In terms of moving the boolean variables to the SeahorseToolMode structure, I think that probably makes a lot of sense, but the currently proposed patch (leveraging the global booleans) probably makes sense given the current codebase. Looking forward to the default install sending PGP users meaningful filenames :-)
Pablo in the interest of getting this closed, I'm fine with having the global booleans. But shouldn't they be extern rather than static?
Any word on this being accepted? I had originally submitted the patch to https://bugs.launchpad.net/ubuntu/+source/seahorse/+bug/398790, and have recently received a message from Sebastien Bacher asking about this moving toward completion.
It is accepted. It just needs that one change made s/static/extern/. Can you make it, test it? And then I'll merge it.
I'll try to take a look and get it done sometime this week.
Created attachment 187435 [details] [review] Fixed bug when embedding filename with pgp encryption v3
Comment on attachment 187435 [details] [review] Fixed bug when embedding filename with pgp encryption v3 This patch includes the original fix and also moves all the required booleans to extern variables as proposed. I did a quick test to validate that the embedded filename is being used by PGP.
Thanks! Merged your patch into master, with a minor fix (removing constness of filename variable). commit 4c3ef9f664f12a96b98c1d6f12898a6cc4c072f3 Author: Patrick Toomey <ptoomey3@mac.com> Date: Mon May 9 10:23:18 2011 +0200 Embed filename when encrypting gpg files. https://bugzilla.gnome.org/show_bug.cgi?id=588432