Re: svn commit: r14792 - trunk/subversion/mod_dav_svn

[kf...@collab.net]

dlr@tigris.org writes:
> Refactored mod_dav_svn logic for sanitizing sensitive error messages,
> and logging the original error messages to the Apache httpd log file.
> This change is loosely related to Subversion issue #1051.
> 
> 
> * subversion/mod_dav_svn/util.c
>   (dav_svn_convert_err): Adjuste inline comment and tweaked formatting.
> 
>   (dav_svn__sanitize_error): New function which converts a svn_error_t
>    into a dav_error and replaces the existing error message with a new
>    message.
> 
> 
> * subversion/mod_dav_svn/dav_svn.h
>   (dav_svn__sanitize_error): New function decl.
> 
> 
> * subversion/mod_dav_svn/repos.c
>   (cleanup_fs_access): Removed trailing period for consistency.
> 
>   (dav_svn_get_resource): Used dav_svn__sanitize_error() in three spots.
> 
>   (dav_svn_do_walk): Removed trailing period for consistency, and
>    changed brackets to parenthesis for more appropriate punctuation.
> 
> 
> * subversion/mod_dav_svn/version.c
>   (dav_svn__push_locks): Used dav_svn__sanitize_error().
> 
> 
> --- trunk/subversion/mod_dav_svn/dav_svn.h	(original)
> +++ trunk/subversion/mod_dav_svn/dav_svn.h	Fri May 20 15:30:02 2005
> @@ -21,6 +21,7 @@
>  #define DAV_SVN_H
>  
>  #include <httpd.h>
> +#include <http_log.h>
>  #include <apr_tables.h>
>  #include <apr_xml.h>
>  #include <mod_dav.h>
> @@ -620,6 +621,26 @@
>                                 apr_hash_t *locks,
>                                 apr_pool_t *pool);
>  
> +
> +/* Converts a svn_error_t into a dav_error and replaces the existing
> +   error message with a @a new_msg (if provided).  If the message was
> +   replaced, assumes that sanitization was necessary, and logs the
> +   unsanitized error message to httpd's log.  Destroys the passed in
> +   @a serr (similarly to the dav_svn_convert_err function).
> + 
> +   The error message produced by various APIs -- usually direct
> +   invocation of svn_fs, or indirect via svn_repos -- can contain
> +   security-sensitive data which must be sanitized before handing it
> +   to clients over network access (e.g. the actual server file
> +   system's path to the repository).  Though we don't want to leak
> +   that information back to the client (a security risk), we do want
> +   to log the real error on the server side.
> + */
> +dav_error *
> +dav_svn__sanitize_error(svn_error_t *serr,
> +                        const char *new_msg,
> +                        int http_status,
> +                        request_rec *r);

Nice doc string!

Might suggest using very explicit languange, such as "(if not NULL)"
instead of "(if provided)".  You might want to make it clear what
sanitization means before using it, or at least make it clear that a
definition is coming up.  I got a bit confused reading it at first.

I read the rest of the change, no complaints there.  It mixes some
purely formatting changes in with the core change; ideally I guess
that would be two commits, but not a big deal in this case (IMHO).

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r14792 - trunk/subversion/mod_dav_svn

[Daniel Rall <dl...@finemaltcoding.com>]

On Fri, 2005-05-20 at 15:24 -0500, kfogel@collab.net wrote:
...
>Might suggest using very explicit languange, such as "(if not NULL)"
>instead of "(if provided)".  You might want to make it clear what
>sanitization means before using it, or at least make it clear that a
>definition is coming up.  I got a bit confused reading it at first.

Thanks, r14793.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org