Potential authz issue in an already-approved backport (Re: svn commit: r1033111 - in /subversion/branches/1.6.x: ./ subversion/libsvn_repos/ subversion/tests/cmdline/ subversion/tests/cmdline/svnsync_tests_data/)

[Daniel Shahaf <d....@daniel.shahaf.name>]

Can someone please review the 

         /* ### authz considerations? */

line in this patch, and decide whether there are no issues and we should
remove that line, or there indeed are issues[1] and we should fix them
and backport the fix?

Thanks,

Daniel


[1] perhaps a call to authz_read_func() at the copyfrom calculation is missing?

hwright@apache.org wrote on Tue, Nov 09, 2010 at 17:32:14 -0000:
> Author: hwright
> Date: Tue Nov  9 17:32:13 2010
> New Revision: 1033111
> 
> URL: http://svn.apache.org/viewvc?rev=1033111&view=rev
> Log:
> Merge r962377, r962378 from trunk:
> 
>  * r962377, r962378
>    Fix svnsync handling of directory copyfrom.
>    Justification:
>      Could lead to sync'd repositories being different from the master.
>    Concerns:
>      http://article.gmane.org/gmane.comp.version-control.subversion.devel/120590
>    Votes:
>      +1: danielsh, philip, cmpilato
> 
> 
> Modified: subversion/branches/1.6.x/subversion/libsvn_repos/replay.c
> URL: http://svn.apache.org/viewvc/subversion/branches/1.6.x/subversion/libsvn_repos/replay.c?rev=1033111&r1=1033110&r2=1033111&view=diff
> ==============================================================================
> --- subversion/branches/1.6.x/subversion/libsvn_repos/replay.c (original)
> +++ subversion/branches/1.6.x/subversion/libsvn_repos/replay.c Tue Nov  9 17:32:13 2010
> @@ -194,6 +194,8 @@ add_subdir(svn_fs_root_t *source_root,
>        svn_fs_path_change2_t *change;
>        svn_boolean_t readable = TRUE;
>        svn_fs_dirent_t *dent;
> +      const char *copyfrom_path = NULL;
> +      svn_revnum_t copyfrom_rev = SVN_INVALID_REVNUM;
>        const char *new_path;
>        void *val;
>  
> @@ -216,6 +218,13 @@ add_subdir(svn_fs_root_t *source_root,
>            /* If it's a delete, skip this entry. */
>            if (change->change_kind == svn_fs_path_change_delete)
>              continue;
> +          else if (change->change_kind == svn_fs_path_change_replace)
> +            {
> +              /* ### Can this assert fail? */
> +              SVN_ERR_ASSERT(change->copyfrom_known);
> +              copyfrom_path = change->copyfrom_path;
> +              copyfrom_rev = change->copyfrom_rev;
> +            }
>          }
>  
>        if (authz_read_func)
> @@ -227,12 +236,31 @@ add_subdir(svn_fs_root_t *source_root,
>  
>        if (dent->kind == svn_node_dir)
>          {
> +          svn_fs_root_t *new_source_root;
> +          const char *new_source_path;
>            void *new_dir_baton;
>  
> -          SVN_ERR(add_subdir(source_root, target_root, editor, edit_baton,
> +          if (copyfrom_path)
> +            {
> +              svn_fs_t *fs = svn_fs_root_fs(source_root);
> +              SVN_ERR(svn_fs_revision_root(&new_source_root, fs, copyfrom_rev, pool));
> +              new_source_path = copyfrom_path;
> +            }
> +          else
> +            {
> +              new_source_root = source_root;
> +              new_source_path = svn_path_join(source_path, dent->name,
> +                                              subpool);
> +            }
> +
> +          /* ### authz considerations?
> +           *
> +           * I think not; when path_driver_cb_func() calls add_subdir(), it
> +           * passes SOURCE_ROOT and SOURCE_PATH that are unreadable.
> +           */
> +          SVN_ERR(add_subdir(new_source_root, target_root, editor, edit_baton,
>                               new_path, *dir_baton,
> -                             svn_path_join(source_path, dent->name,
> -                                           subpool),
> +                             new_source_path,
>                               authz_read_func, authz_read_baton,
>                               changed_paths, subpool, &new_dir_baton));
>  
> 
> Modified: subversion/branches/1.6.x/subversion/tests/cmdline/svnsync_tests.py
> URL: http://svn.apache.org/viewvc/subversion/branches/1.6.x/subversion/tests/cmdline/svnsync_tests.py?rev=1033111&r1=1033110&r2=1033111&view=diff
> ==============================================================================
> --- subversion/branches/1.6.x/subversion/tests/cmdline/svnsync_tests.py (original)
> +++ subversion/branches/1.6.x/subversion/tests/cmdline/svnsync_tests.py Tue Nov  9 17:32:13 2010
> @@ -761,6 +761,11 @@ def commit_a_copy_of_root(sbox):
>    #Testcase for issue 3438.
>    run_test(sbox, "repo_with_copy_of_root_dir.dump")
>  
> +# issue #3641
> +def descend_into_replace(sbox):
> +  "descending into replaced dir looks in src"
> +  run_test(sbox, "descend_into_replace.dump", subdir='/trunk/H',
> +           exp_dump_file_name = "descend_into_replace.expected.dump")
>  
>  ########################################################################
>  # Run the tests
> @@ -800,6 +805,7 @@ test_list = [ None,
>                copy_bad_line_endings,
>                delete_svn_props,
>                commit_a_copy_of_root,
> +              descend_into_replace,
>               ]
>  
>  if __name__ == '__main__':
> 
>