You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Collins-Sussman <su...@red-bean.com> on 2008/06/10 15:35:22 UTC
bug in new plaintext auth-prompt warning
I love that our new svn_cmdline_auth_plaintext_prompt() screams murder
about storing the password in plaintext! However, I ran into a bug
this morning with it. svnmucc was passing a NULL prompt baton into it
(which seems legitimate to me), and the function was just blindly
dereferencing the baton to find the user's config_path. I made a
small patch below which conditionalizes the printing of the
config_path text. It fixes the svnmucc segfault. Any objections to
my committing this?
(I'm asking before I commit, since I've not been paying detailed
attention to this new feature.)
Index: subversion/libsvn_subr/prompt.c
===================================================================
--- subversion/libsvn_subr/prompt.c (revision 31680)
+++ subversion/libsvn_subr/prompt.c (working copy)
@@ -390,11 +390,12 @@
svn_boolean_t answered = FALSE;
const char *prompt_string = _("Store password unencrypted (yes/no)? ");
svn_cmdline_prompt_baton2_t *pb = baton;
- const char *config_path;
-
- SVN_ERR(svn_config_get_user_config_path(&config_path, pb->config_dir,
- SVN_CONFIG_CATEGORY_SERVERS, pool));
+ const char *config_path = NULL;
+ if (pb)
+ SVN_ERR(svn_config_get_user_config_path(&config_path, pb->config_dir,
+
SVN_CONFIG_CATEGORY_SERVERS, pool));
+
SVN_ERR(svn_cmdline_fprintf(stderr, pool,
_("-----------------------------------------------------------------------\n"
"ATTENTION! Your password for authentication realm:\n"
@@ -404,13 +405,19 @@
"can only be stored to disk unencrypted! You are advised to configure\n"
"your system so that Subversion can store passwords encrypted, if\n"
"possible. See the documentation for details.\n"
- "\n"
- "You can avoid future appearances of this warning by setting the value\n"
- "of the 'store-plaintext-passwords' option to either 'yes' or 'no' in\n"
- "'%s'.\n"
- "-----------------------------------------------------------------------\n"
- ), realmstring, config_path));
+ ), realmstring));
+ if (config_path)
+ SVN_ERR(svn_cmdline_fprintf(stderr, pool,
+ _("\n"
+ "You can avoid future appearances of this warning by setting
the value\n"
+ "of the 'store-plaintext-passwords' option to either 'yes' or 'no' in\n"
+ "'%s'.\n"
+ ), realmstring));
+
+ SVN_ERR(svn_cmdline_fprintf(stderr, pool,
+ "-----------------------------------------------------------------------\n"));
+
do
{
svn_error_t *err = prompt(&answer, prompt_string, FALSE, pb, pool);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: bug in new plaintext auth-prompt warning
Posted by Ben Collins-Sussman <su...@red-bean.com>.
Yes yes, of course. Nothing to see here, please move along. (Thank
goodness for code review.)
On Tue, Jun 10, 2008 at 10:37 AM, C. Michael Pilato <cm...@collab.net> wrote:
> Ben Collins-Sussman wrote:
>
> [...]
>
>> @@ -404,13 +405,19 @@
>> "can only be stored to disk unencrypted! You are advised to
>> configure\n"
>> "your system so that Subversion can store passwords encrypted, if\n"
>> "possible. See the documentation for details.\n"
>> - "\n"
>> - "You can avoid future appearances of this warning by setting the
>> value\n"
>> - "of the 'store-plaintext-passwords' option to either 'yes' or 'no'
>> in\n"
>> - "'%s'.\n"
>> -
>> "-----------------------------------------------------------------------\n"
>> - ), realmstring, config_path));
>> + ), realmstring));
>>
>> + if (config_path)
>> + SVN_ERR(svn_cmdline_fprintf(stderr, pool,
>> + _("\n"
>> + "You can avoid future appearances of this warning by setting
>> the value\n"
>> + "of the 'store-plaintext-passwords' option to either 'yes' or 'no'
>> in\n"
>> + "'%s'.\n"
>> + ), realmstring));
>
> ^^^^ shouldn't this be 'config_path' instead of realm_string?
>
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On Demand
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: bug in new plaintext auth-prompt warning
Posted by "C. Michael Pilato" <cm...@collab.net>.
Ben Collins-Sussman wrote:
[...]
> @@ -404,13 +405,19 @@
> "can only be stored to disk unencrypted! You are advised to configure\n"
> "your system so that Subversion can store passwords encrypted, if\n"
> "possible. See the documentation for details.\n"
> - "\n"
> - "You can avoid future appearances of this warning by setting the value\n"
> - "of the 'store-plaintext-passwords' option to either 'yes' or 'no' in\n"
> - "'%s'.\n"
> - "-----------------------------------------------------------------------\n"
> - ), realmstring, config_path));
> + ), realmstring));
>
> + if (config_path)
> + SVN_ERR(svn_cmdline_fprintf(stderr, pool,
> + _("\n"
> + "You can avoid future appearances of this warning by setting
> the value\n"
> + "of the 'store-plaintext-passwords' option to either 'yes' or 'no' in\n"
> + "'%s'.\n"
> + ), realmstring));
^^^^ shouldn't this be 'config_path' instead of realm_string?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: bug in new plaintext auth-prompt warning
Posted by Ben Collins-Sussman <su...@red-bean.com>.
On Tue, Jun 10, 2008 at 11:24 AM, Stefan Sperling <st...@elego.de> wrote:
>
> Yes, this is a bug. Squash it.
Done.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: bug in new plaintext auth-prompt warning
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Jun 10, 2008 at 10:35:22AM -0500, Ben Collins-Sussman wrote:
> I love that our new svn_cmdline_auth_plaintext_prompt() screams murder
> about storing the password in plaintext!
Heh :)
It also haunts people at night if they store their passwords in
plaintext anyway. That's a hidden feature.
> However, I ran into a bug
> this morning with it. svnmucc was passing a NULL prompt baton into it
> (which seems legitimate to me), and the function was just blindly
> dereferencing the baton to find the user's config_path. I made a
> small patch below which conditionalizes the printing of the
> config_path text. It fixes the svnmucc segfault. Any objections to
> my committing this?
>
> (I'm asking before I commit, since I've not been paying detailed
> attention to this new feature.)
Yes, this is a bug. Squash it.
Thanks,
Stefan