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
Index: subversion/tests/cmdline/lock_tests.py
===================================================================
--- subversion/tests/cmdline/lock_tests.py (revision 1148778)
+++ subversion/tests/cmdline/lock_tests.py (working copy)
@@ -1720,7 +1720,25 @@
1, 'unlock', pi_path)
svntest.actions.run_and_verify_status(wc_dir, expected_status)
+#----------------------------------------------------------------------
+def lock_invalid_token(sbox):
+ "verify pre-lock hook returning invalid token"
+ sbox.build()
+
+ hook_path = os.path.join(sbox.repo_dir, 'hooks', 'pre-lock')
+ svntest.main.create_python_hook_script(hook_path, 'import sys\n'
+ 'sys.stdout.write("multiline token\\nsecond line")\n'
+ 'sys.exit(0)\n')
+
+ fname = 'iota'
+ file_path = os.path.join(sbox.wc_dir, fname)
+
+ svntest.actions.run_and_verify_svn(None, None,
+ "svn: E160037: Lock token contains
illegal characters",
+ 'lock', '-m', '', file_path)
+
+
########################################################################
# Run the tests
@@ -1768,6 +1786,7 @@
cp_isnt_ro,
update_locked_deleted,
block_unlock_if_pre_unlock_hook_fails,
+ lock_invalid_token,
]
if __name__ == '__main__':
