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

Re: Lock token encoding (was Re: svn commit: r1147882 - /subversion/trunk/subversion/libsvn_repos/hooks.c)

From: Ivan Zhakov <ivan_at_visualsvn.com>
Date: Wed, 20 Jul 2011 18:17:21 +0400

Hi Daniel,

Thanks for looking into this issue. See my comments below.

>> >> FWIW, I think UTF-8 is a fine choice, especially if we've been internally
>> >> handling this string as UTF-8 already.  But let's document the fact
>> >> somewhere, please (preferably in the pre-lock-hook template comments).
>> >>
>> >
>> > Looking again, svn_fs.h:2059 documents that lock tokens are URI's.
>> > (and, consequently, are printable ASCII)
>> >
>> > We also need them to be XML-safe for their use in ra_dav LOCK responses.
>> >
>> > So, I'm thinking that:
>> >
>> > * We should validate the supplied lock tokens, not for being well-formed
>> >  UTF-8 as in Ivan's change, but for being ASCII-only.
>> >
>> > * We should move the validation to the FS layer.
>> >
>> > Thoughts?
>> >
>> I'm fine with this approach, but I think we should leave explicit
>> validation of lock tokens in pre-lock hook handling for better
>> diagnostic.
>
> I don't think that's necessary, given that we don't (and likely won't)
> do anything between calling the pre-lock hook and calling svn_fs_lock().
>
> Anyway, untested patch:
>
[..]
> ===================================================================
> --- subversion/libsvn_fs/fs-loader.c    (revision 1147872)
> +++ subversion/libsvn_fs/fs-loader.c    (working copy)
> @@ -28,6 +28,7 @@
>  #include <apr_md5.h>
>  #include <apr_thread_mutex.h>
>  #include <apr_uuid.h>
> +#include <apr_uri.h>
>
>  #include "svn_types.h"
>  #include "svn_dso.h"
> @@ -1303,6 +1304,31 @@ svn_fs_lock(svn_lock_t **lock, svn_fs_t *fs, const
>         return svn_error_create
>           (SVN_ERR_XML_UNESCAPABLE_DATA, NULL,
>            _("Lock comment contains illegal characters"));
> +    }
> +
> +  /* Enforce that the token be an XML-safe URI. */
> +  if (token)
> +    {
> +      apr_uri_t uri;
> +      apr_status_t status;
> +
> +      status = apr_uri_parse(pool, token, &uri);
> +      if (status)
> +        return svn_error_createf(SVN_ERR_FS_BAD_LOCK_TOKEN,
> +                                 svn_error_wrap_apr(status, NULL),
> +                                 _("Can't parse token '%s' as a URI"),
> +                                 token);
> +
> +      if (strcmp(uri.scheme, "opaquelocktoken"))
> +        return svn_error_createf(SVN_ERR_FS_BAD_LOCK_TOKEN, NULL,
> +                                 _("Lock token URI '%s' has bad scheme; "
> +                                   "expected '%s'"),
> +                                 token, "opaquelocktoken");
> +
In my test apr_uri_parse() returning uri.scheme == NULL. See attached
patch with test.

-- 
Ivan Zhakov

Received on 2011-07-20 16:18:17 CEST

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