On Tue, Sep 14, 2010 at 06:08:59PM -0000, cmpilato_at_apache.org 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;
> }
>
>
Received on 2010-11-09 21:02:05 CET