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

Re: Input validation observations

From: Noorul Islam K M <noorul_at_collab.net>
Date: Fri, 03 Dec 2010 15:04:26 +0530

Julian Foad <julian.foad_at_wandisco.com> writes:

> On Thu, 2010-12-02 at 13:58 +0530, Noorul Islam K M wrote:
>
>> Julian Foad <julianfoad_at_btopenworld.com> writes:
>>
>> > On Tue, 2010-11-30 at 18:42 +0530, Noorul Islam K M wrote:
>> >
>> >> Julian Foad <julian.foad_at_wandisco.com> writes:
>> >>
>> >> > I tried some potentially invalid inputs to "svn" a week or two ago and
>> >> > made notes on what I found. Just posting here in case anyone wants to
>> >> > do something about one or more of them.
>> >> >
>> >> > Noorul, I'm including you in the "To" addresses because you said you
>> >> > were looking for more small tasks to do, so feel free to pick one of
>> >> > these if you want to.
>> >>
>> >> Thank you! I will start working on these one by one.
>> >
>> > Great!
>> >
>> >
>> >> > Where I end with a question mark, it means I'm not sure if we want this
>> >> > change, it's just a suggestion.
>> >> >
>> >> > * "svn checkout ^/ ^/y" -> "A asf/cxf, A asf/cxf/utils, ...". (Don't
>> >> > try this without being ready on the Ctrl-C or Ctrl-\!) It seems to
>> >> > ignore "^/y" and create ./(basename(^/)); should fail: "'^/y' is not a
>> >> > WC path".
>> >> >
>> >> > * "svn checkout ^/subversion/trunk/build ^/y" -> "Checked out revision
>> >> > 1040465. URL 'https://svn.apache.org/repos/asf/y' doesn't exist".
>> >> > Bleach - that's just crazy. Should fail: "'^/y' is not a WC path".
>> >> >
>> >> > * "svn copy a ^/b c" doesn't detect the mixed source types in cl, only
>> >> > in lib; should reject them at CLI level?
>> >> >
>> >> > * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
>> >> > in revision REV"?
>> >> >
>> >> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
>> >> > mix URL and local targets"?
>> >> >
>> >> > * "svn mkdir a ^/" -> "Can't create directory
>> >> > 'https:/svn.apache.org/repos/asf'"; should not print the URL as if it's
>> >> > a local path.
>> >> >
>> >> > * "svn mv ^/ ^/" -> "Cannot move path
>> >> > 'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
>> >> > as if it's a local path.
>> >> >
>> >> > * "svn update ^/a" -> "Skipped
>> >> > 'https://svn.apache.org/repos/asf/a' ..."; should fail: "'^/a' is not a
>> >> > WC path"?
>> >> >
>> >>
>> >> svn help update says that the argument should be a PATH. I think it is
>> >> right to force the user to enter local PATH.
>> >
>> > Good. Thanks for finding that. I agree. That change will make some of
>> > my recent patch (r1040232) redundant: it will no longer need to remove
>> > URL targets from the array of targets, since it will just return an
>> > error if it finds any, like you already did in some of the other
>> > subcomands.
>> >
>>
>> I further looked into the code, since update accepts multiple targets,
>> the program skips URL targets assuming that there might one or more
>> local paths as part of targets list. The skipping part is intentional
>> and I am not sure whether we should change this behavior.
>
> I think we should change this behaviour and make "svn update" throw an
> error if any target is a URL.
>

Attached is the patch for same.

Log
[[[

Make 'svn update' verify that URLs are not passed as targets.

* subversion/svn/update-cmd.c,
  subversion/libsvn_client/update.c:
  (svn_cl__update, svn_client_update4): Raise an error if a URL was
  passed. Remove code that notifies 'Skipped' message for URL targets.

* subversion/tests/cmdline/input_validation_tests.py
  (invalid_update_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 1041293)
+++ subversion/tests/cmdline/input_validation_tests.py (working copy)
@@ -234,6 +234,12 @@
   run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'relocate',
                            "^/", "^/", "^/")
 
+def invalid_update_targets(sbox):
+ "non-working copy paths for 'update'"
+ sbox.build(read_only=True)
+ run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'update',
+ "^/")
+
 ########################################################################
 # Run the tests
 
@@ -261,6 +267,7 @@
               invalid_patch_targets,
               invalid_switch_targets,
               invalid_relocate_targets,
+ invalid_update_targets,
              ]
 
 if __name__ == '__main__':
Index: subversion/svn/update-cmd.c
===================================================================
--- subversion/svn/update-cmd.c (revision 1041293)
+++ subversion/svn/update-cmd.c (working copy)
@@ -110,6 +110,7 @@
   svn_boolean_t depth_is_sticky;
   struct svn_cl__check_externals_failed_notify_baton nwb;
   apr_array_header_t *result_revs;
+ int i;
 
   SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
                                                       opt_state->targets,
@@ -120,30 +121,16 @@
 
   SVN_ERR(svn_cl__eat_peg_revisions(&targets, targets, scratch_pool));
 
- /* If any targets are URLs, notify that we're skipping them and remove
- them from TARGETS. Although svn_client_update4() would skip them
- anyway, we don't want to pass invalid targets to it, and especially
- not to print_update_summary(). */
- {
- apr_array_header_t *new_targets
- = apr_array_make(scratch_pool, targets->nelts, sizeof(const char *));
- int i;
+ /* If any targets are URLs, display error message and exit. */
+ 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_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ _("'%s' is not a local path"), target);
+ }
 
- for (i = 0; i < targets->nelts; i++)
- {
- const char *target = APR_ARRAY_IDX(targets, i, const char *);
-
- if (svn_path_is_url(target))
- ctx->notify_func2(ctx->notify_baton2,
- svn_wc_create_notify_url(
- target, svn_wc_notify_skip, scratch_pool),
- scratch_pool);
- else
- APR_ARRAY_PUSH(new_targets, const char *) = target;
- }
- targets = new_targets;
- }
-
   /* If using changelists, convert targets into a set of paths that
      match the specified changelist(s). */
   if (opt_state->changelists)
Index: subversion/libsvn_client/update.c
===================================================================
--- subversion/libsvn_client/update.c (revision 1041293)
+++ subversion/libsvn_client/update.c (working copy)
@@ -397,44 +397,45 @@
 
   for (i = 0; i < paths->nelts; ++i)
     {
+ path = APR_ARRAY_IDX(paths, i, const char *);
+
+ if (svn_path_is_url(path))
+ return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("'%s' is not a local path"), path);
+ }
+
+ for (i = 0; i < paths->nelts; ++i)
+ {
       svn_boolean_t sleep;
       svn_boolean_t skipped = FALSE;
       svn_error_t *err = SVN_NO_ERROR;
       svn_revnum_t result_rev;
+ const char *local_abspath;
       path = APR_ARRAY_IDX(paths, i, const char *);
 
       svn_pool_clear(subpool);
 
       if (ctx->cancel_func && (err = ctx->cancel_func(ctx->cancel_baton)))
         break;
-
- if (svn_path_is_url(path))
+
+ SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, subpool));
+ err = svn_client__update_internal(&result_rev, local_abspath,
+ revision, depth, depth_is_sticky,
+ ignore_externals,
+ allow_unver_obstructions,
+ &sleep, FALSE, make_parents,
+ ctx, subpool);
+
+ if (err && err->apr_err != SVN_ERR_WC_NOT_WORKING_COPY)
         {
- skipped = TRUE;
+ return svn_error_return(err);
         }
- else
+
+ if (err)
         {
- const char *local_abspath;
-
- SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, subpool));
- err = svn_client__update_internal(&result_rev, local_abspath,
- revision, depth, depth_is_sticky,
- ignore_externals,
- allow_unver_obstructions,
- &sleep, FALSE, make_parents,
- ctx, subpool);
-
- if (err && err->apr_err != SVN_ERR_WC_NOT_WORKING_COPY)
- {
- return svn_error_return(err);
- }
-
- if (err)
- {
- /* SVN_ERR_WC_NOT_WORKING_COPY: it's not versioned */
- svn_error_clear(err);
- skipped = TRUE;
- }
+ /* SVN_ERR_WC_NOT_WORKING_COPY: it's not versioned */
+ svn_error_clear(err);
+ skipped = TRUE;
         }
 
       if (skipped)
@@ -443,22 +444,9 @@
           if (ctx->notify_func2)
             {
               svn_wc_notify_t *notify;
-
- if (svn_path_is_url(path))
- {
- /* For some historic reason this user error is supported,
- and must provide correct notifications. */
- notify = svn_wc_create_notify_url(path,
- svn_wc_notify_skip,
- subpool);
- }
- else
- {
- notify = svn_wc_create_notify(path,
- svn_wc_notify_skip,
- subpool);
- }
-
+ notify = svn_wc_create_notify(path,
+ svn_wc_notify_skip,
+ subpool);
               (*ctx->notify_func2)(ctx->notify_baton2, notify, subpool);
             }
         }
Received on 2010-12-03 10:37:07 CET

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.