[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r31905 - in branches/1.5.x: . subversion/bindings/swig subversion/libsvn_subr

From: Karl Fogel <kfogel_at_red-bean.com>
Date: Sun, 29 Jun 2008 21:18:56 -0400

Daniel Shahaf <d.s_at_daniel.shahaf.co.il> writes:
> hwright_at_tigris.org wrote on Fri, 27 Jun 2008 at 10:10 -0700:
>> Author: hwright
>> Date: Fri Jun 27 10:10:54 2008
>> New Revision: 31905
>>
>> Log:
>> Merge r31884 from trunk:
>>
>> * r31884
>> Fix issue #2242: auth cache picking up password from wrong username entry.
>> Votes:
>> +1: arfrever, danielsh, stylesen
>
> Technically, we still need a third +1 on this (since stylesen isn't
> a full committer). Any volunteers?

I've added my +1, because I think the code is more correct after r31884
than before, but I still have a concern about this change.

subversion/libsvn_subr/simple_providers.c:simple_password_get(), which
is the function affected by r31884, is an implementation of the
'svn_auth__password_get_t()' interface:

   /* A function that stores in *PASSWORD (potentially after decrypting it)
      the user's password. It might be obtained directly from CREDS, or
      from an external store, using REALMSTRING and USERNAME as keys.
      If NON_INTERACTIVE is set, the user must not be involved in the
      retrieval process. POOL is used for any necessary allocation. */
   typedef svn_boolean_t (*svn_auth__password_get_t)
     (const char **password,
      apr_hash_t *creds,
      const char *realmstring,
      const char *username,
      svn_boolean_t non_interactive,
      apr_pool_t *pool);

That doc string is not clear on whether REALMSTRING or USERNAME may be
null. But r31884 assumes that USERNAME will not be null -- it passes
the username directly to strcmp().

Is there ever a situation where a null USERNAME and/or REALMSTRING would
be reasonable? If so, we should fix up the typedef's doc string, and
then tweak simple_password_get() (and perhaps other implementations of
that interface) accordingly.

Below is r31884, for reference.

-Karl

------------------------------------------------------------------------
r31884 | stylesen | 2008-06-26 05:00:37 -0400 (Thu, 26 Jun 2008) | 7 lines
Changed paths:
   M /trunk/subversion/libsvn_subr/simple_providers.c

Fix issue #2242: auth cache picking up password from wrong username entry.

* subversion/libsvn_subr/simple_providers.c
  (simple_password_get): Validate the username for which we get the password.

Approved by: danielsh

Index: trunk/subversion/libsvn_subr/simple_providers.c
===================================================================
--- trunk/subversion/libsvn_subr/simple_providers.c (revision 31883)
+++ trunk/subversion/libsvn_subr/simple_providers.c (revision 31884)
@@ -65,12 +65,17 @@
                     apr_pool_t *pool)
 {
   svn_string_t *str;
- str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_PASSWORD_KEY,
+ str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
                      APR_HASH_KEY_STRING);
- if (str && str->data)
+ if (str && strcmp(str->data, username) == 0)
     {
- *password = str->data;
- return TRUE;
+ str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_PASSWORD_KEY,
+ APR_HASH_KEY_STRING);
+ if (str && str->data)
+ {
+ *password = str->data;
+ return TRUE;
+ }
     }
   return FALSE;
 }

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-30 03:20:14 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.