Julian Foad <julian.foad_at_wandisco.com> writes:
> On Wed, 2010-11-03 at 14:46 +0100, Stefan Sperling wrote:
>
>> On Wed, Nov 03, 2010 at 03:32:25PM +0530, Noorul Islam K M wrote:
>> >
>> > Stefan Sperling <stsp_at_elego.de> writes:
>> > >
>> > > Noorul, if you're looking for more tasks, you could take a look at
>> > > issue #3620 which is about a similar problem:
>> > > http://subversion.tigris.org/issues/show_bug.cgi?id=3620
>> > > I'll happily review and commit patches for issue #3620.
>> > >
>> > > Thanks,
>> > > Stefan
>> >
>> > Here is the patch for revert command. I will look into other commands as
>> > and when time permits.
>> >
>> > Log
>> >
>> > [[[
>> > Make 'svn revert' verify that none of its targets are URLs.
>> >
>> > * subversion/libsvn_client/revert.c,
>> > subversion/svn/revert-cmd.c
>> > (svn_client_revert2, svn_cl__revert): Raise an error if any targets
>> > look like URLs.
>> >
>> > * subversion/tests/cmdline/input_validation_tests.py
>> > (invalid_revert_targets, test_list): New test.
>> >
>> > Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
>> > ]]]
>> >
>> > Thanks and Regards
>> > Noorul
>> >
>>
>> > Index: subversion/tests/cmdline/input_validation_tests.py
>> > ===================================================================
>> > --- subversion/tests/cmdline/input_validation_tests.py (revision 1030340)
>> > +++ subversion/tests/cmdline/input_validation_tests.py (working copy)
>> > @@ -173,6 +173,12 @@
>> > run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'upgrade',
>> > target, target)
>> >
>> > +def invalid_revert_targets(sbox):
>> > + "non-working copy paths for 'revert'"
>> > + sbox.build(read_only=True)
>> > + for target in _invalid_wc_path_targets:
>> > + run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'revert',
>> > + target)
>> >
>> > ########################################################################
>> > # Run the tests
>> > @@ -192,6 +198,7 @@
>> > invalid_log_targets,
>> > invalid_merge_args,
>> > invalid_wcpath_upgrade,
>> > + invalid_revert_targets,
>> > ]
>> >
>> > if __name__ == '__main__':
>> > Index: subversion/svn/revert-cmd.c
>> > ===================================================================
>> > --- subversion/svn/revert-cmd.c (revision 1030340)
>> > +++ subversion/svn/revert-cmd.c (working copy)
>> > @@ -27,6 +27,7 @@
>> >
>> > /*** Includes. ***/
>> >
>> > +#include "svn_path.h"
>> > #include "svn_client.h"
>> > #include "svn_error_codes.h"
>> > #include "svn_error.h"
>> > @@ -47,6 +48,7 @@
>> > svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
>> > apr_array_header_t *targets = NULL;
>> > svn_error_t *err;
>> > + int i;
>> >
>> > SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
>> > opt_state->targets,
>> > @@ -63,6 +65,19 @@
>> >
>> > SVN_ERR(svn_cl__eat_peg_revisions(&targets, targets, scratch_pool));
>> >
>> > + /* Don't even attempt to modify the working copy if any of the
>> > + * targets look like URLs. URLs are invalid input. */
>> > + for (i = 0; i < targets->nelts; i++)
>> > + {
>> > + const char *target = APR_ARRAY_IDX(targets, i, const char *);
>> > +
>> > + if (svn_path_is_url(target))
>> > + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
>> > + NULL,
>> > + _("'%s' is not a local path"),
>> > + target));
>> > + }
>> > +
>> > err = svn_client_revert2(targets, opt_state->depth,
>> > opt_state->changelists, ctx, scratch_pool);
>> >
>> > Index: subversion/libsvn_client/revert.c
>> > ===================================================================
>> > --- subversion/libsvn_client/revert.c (revision 1030340)
>> > +++ subversion/libsvn_client/revert.c (working copy)
>> > @@ -27,6 +27,7 @@
>> >
>> > /*** Includes. ***/
>> >
>> > +#include "svn_path.h"
>> > #include "svn_wc.h"
>> > #include "svn_client.h"
>> > #include "svn_dirent_uri.h"
>> > @@ -37,6 +38,7 @@
>> > #include "client.h"
>> > #include "private/svn_wc_private.h"
>> >
>> > +#include "svn_private_config.h"
>> >
>> >
>> > /*** Code. ***/
>> > @@ -121,6 +123,19 @@
>> > svn_boolean_t use_commit_times;
>> > struct revert_with_write_lock_baton baton;
>> >
>> > + /* Don't even attempt to modify the working copy if any of the
>> > + * targets look like URLs. URLs are invalid input. */
>> > + for (i = 0; i < paths->nelts; i++)
>> > + {
>> > + const char *path = APR_ARRAY_IDX(paths, i, const char *);
>> > +
>> > + if (svn_path_is_url(path))
>> > + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
>>
>> The SVN_ERR_CL_* codes are for use by the command line client, I think.
>> So I've used SVN_ERR_ILLEGAL_TARGET for errors thrown from libsvn_client.
>> But I can fix the error code locally, you don't need to send another patch.
>
> -0 on duplicating this check inside libsvn_client.
>
Did you mean -1?
From svn log -c 965551
Input validation is done both right beneath the client API layer and
within the CLI client. This makes sure that our CLI client behaves well,
i.e. it won't ask the client library to perform operations it knows might
fail due to invalid input. The checks within the client library help third-
party clients which don't perform proper input validation even though they
should.
Thanks and Regards
Noorul
Received on 2010-11-03 16:24:38 CET