On Mon, 2009-12-14, I (Julian Foad) wrote:
> Jon Foster wrote:
> > It seems that mod_dav_svn doesn't escape special XML characters like
> > "<" and ">" in the error messages from hook scripts. This causes
> > corrupt XML to be sent across the wire. Here's a Wireshark capture
> > of the response to the PROPPATCH:
> [...]
> It looks like the problem has been there for years. I think this patch
> should fix it. Do you feel like writing a regression test?
I wrote one myself. Attached. I confirmed the bug, but I am having
trouble testing my test. It may be to do with the test suite not quite
properly supporting a build in a separate directory.
> [[[
> In mod_dav_svn, make error output from the post-commit hook XML-safe, to fix
> the "invalid XML" error that occurred if a post-commit error message
> contained "&" or "<" characters.
>
> * subversion/mod_dav_svn/merge.c
> (dav_svn__merge_response): XML-quote the error string.
> --This line, and those below, will be ignored--
>
> Index: subversion/mod_dav_svn/merge.c
> ===================================================================
> --- subversion/mod_dav_svn/merge.c (revision 889737)
> +++ subversion/mod_dav_svn/merge.c (working copy)
> @@ -252,7 +252,9 @@ dav_svn__merge_response(ap_filter_t *out
> post_commit_err_elem = apr_psprintf(pool,
> "<S:post-commit-err>%s"
> "</S:post-commit-err>",
> - post_commit_err);
> + apr_xml_quote_string(pool,
> +
> post_commit_err,
> + 0));
> }
> else
> {
> ]]]
- Julian
Add a regression test for post-revprop error message causing invalid XML if
it contains certain XML characters such as '&' and '<'.
* subversion/tests/cmdline/prop_tests.py
(post_revprop_hook_message_non_xml): New test.
(test_list): Add the new test.
* subversion/tests/cmdline/svntest/actions.py
(disable_revprop_changes): Add an optional argument for specifying extra
text to be included in the hook's error message.
--This line, and those below, will be ignored--
Index: subversion/tests/cmdline/prop_tests.py
===================================================================
--- subversion/tests/cmdline/prop_tests.py (revision 891747)
+++ subversion/tests/cmdline/prop_tests.py (working copy)
@@ -1703,6 +1703,33 @@ def delete_nonexistent_property(sbox):
'propdel', 'yellow',
os.path.join(wc_dir, 'A', 'D', 'G'))
+#----------------------------------------------------------------------
+# Test the marshalling of a post-commit error message that contains XML
+# significant characters such as '&' and '<'. This used to fail over RA-dav,
+# resulting in the error 'XML data was not well-formed' on the client.
+#
+def post_revprop_hook_message_non_xml(sbox):
+ "post-revprop-change hook message with XML chars"
+
+ sbox.build()
+
+ wc_dir = sbox.wc_dir
+ repo_dir = sbox.repo_dir
+
+ text = 'Text with <angle brackets> & ampersand'
+
+ svntest.actions.enable_revprop_changes(repo_dir)
+ svntest.actions.create_failing_hook(repo_dir, 'post-revprop-change', text)
+
+ expected_error = svntest.verify.ExpectedOutput([
+ "svn: post-revprop-change hook failed (exit code 1) with output:\n",
+ "post-revprop-change hook failed: " + text + "\n",
+ ], match_all = False)
+
+ svntest.actions.run_and_verify_svn(None, [], expected_error,
+ 'ps', '--revprop', '-r0', 'p', 'v',
+ wc_dir)
+
########################################################################
# Run the tests
@@ -1743,6 +1770,7 @@ test_list = [ None,
same_replacement_props,
added_moved_file,
delete_nonexistent_property,
+ post_revprop_hook_message_non_xml,
]
if __name__ == '__main__':
Index: subversion/tests/cmdline/svntest/actions.py
===================================================================
--- subversion/tests/cmdline/svntest/actions.py (revision 891747)
+++ subversion/tests/cmdline/svntest/actions.py (working copy)
@@ -1590,16 +1590,17 @@ def enable_revprop_changes(repo_dir):
hook_path = main.get_pre_revprop_change_hook_path(repo_dir)
main.create_python_hook_script(hook_path, 'import sys; sys.exit(0)')
-def disable_revprop_changes(repo_dir):
+def disable_revprop_changes(repo_dir, extra_text=''):
"""Disable revprop changes in the repository at REPO_DIR by creating a
pre-revprop-change hook script that prints "pre-revprop-change" followed
- by its arguments, and returns an error."""
+ by its arguments and the optional extra TEXT, and returns an error."""
hook_path = main.get_pre_revprop_change_hook_path(repo_dir)
main.create_python_hook_script(hook_path,
- 'import sys\n'
- 'sys.stderr.write("pre-revprop-change %s" % " ".join(sys.argv[1:6]))\n'
- 'sys.exit(1)\n')
+ 'import sys\n'
+ 'sys.stderr.write("pre-revprop-change %s\\n" % " ".join(sys.argv[1:6]))\n'
+ 'sys.stderr.write(' + repr(extra_text) + ')\n'
+ 'sys.exit(1)\n')
def create_failing_post_commit_hook(repo_dir):
"""Create a post-commit hook script in the repository at REPO_DIR that always
