On Tue, Sep 14, 2010 at 06:08:59PM -0000, [email protected] wrote: > Author: cmpilato > Date: Tue Sep 14 18:08:59 2010 > New Revision: 997026 > > URL: http://svn.apache.org/viewvc?rev=997026&view=rev > Log: > For issue #3709 ("Inconsistency between "svn list" and "svn > checkout"), teach the mod_dav_svn tree walker logic to authorize > access to the paths it touches.
Hi Mike, I'm reviewing this for 1.6.14. I have two questions (see below). > * subversion/mod_dav_svn/repos.c > (do_walk): Replace a decade-old TODO about checking authorization on > a directory's children with, you know, code that checks > authorization on a directory's children. > > Modified: > subversion/trunk/subversion/mod_dav_svn/repos.c > > Modified: subversion/trunk/subversion/mod_dav_svn/repos.c > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/repos.c?rev=997026&r1=997025&r2=997026&view=diff > ============================================================================== > --- subversion/trunk/subversion/mod_dav_svn/repos.c (original) > +++ subversion/trunk/subversion/mod_dav_svn/repos.c Tue Sep 14 18:08:59 2010 > @@ -3957,6 +3957,7 @@ do_walk(walker_ctx_t *ctx, int depth) > apr_size_t uri_len; > apr_size_t repos_len; > apr_hash_t *children; > + apr_pool_t *iterpool; > > /* Clear the temporary pool. */ > svn_pool_clear(ctx->info.pool); > @@ -4032,6 +4033,7 @@ do_walk(walker_ctx_t *ctx, int depth) > params->pool); > > /* iterate over the children in this collection */ > + iterpool = svn_pool_create(params->pool); > for (hi = apr_hash_first(params->pool, children); hi; hi = > apr_hash_next(hi)) > { > const void *key; > @@ -4039,6 +4041,8 @@ do_walk(walker_ctx_t *ctx, int depth) > void *val; > svn_fs_dirent_t *dirent; > > + svn_pool_clear(iterpool); > + > /* fetch one of the children */ > apr_hash_this(hi, &key, &klen, &val); > dirent = val; > @@ -4046,7 +4050,16 @@ do_walk(walker_ctx_t *ctx, int depth) > /* authorize access to this resource, if applicable */ > if (params->walk_type & DAV_WALKTYPE_AUTH) > { > - /* ### how/what to do? */ > + const char *repos_relpath = > + apr_pstrcat(iterpool, > + apr_pstrmemdup(iterpool, > + ctx->repos_path->data, > + ctx->repos_path->len), > + key, NULL); Here, key is const void*. Cast it const char* for clarity? Are we guaranteed that repos_path->data always ends with a slash, or that key always starts with one? Why not use svn_{dirent,uri,relpath}_join() or similar? > + if (! dav_svn__allow_read(ctx->info.r, ctx->info.repos, > + repos_relpath, ctx->info.root.rev, > + iterpool)) > + continue; > } > > /* append this child to our buffers */ > @@ -4087,6 +4100,9 @@ do_walk(walker_ctx_t *ctx, int depth) > ctx->uri->len = uri_len; > ctx->repos_path->len = repos_len; > } > + > + svn_pool_destroy(iterpool); > + > return NULL; > } > >
