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