REPOST: URL decoding bugs in apache 0.8.3

[David Robinson <dr...@ast.cam.ac.uk>]

This bug was present in 0.8.2, but doesn't seem to have been fixed for 0.8.3

There's a couple of places where unescape_url() is inappropriately called.

1. PATH_INFO gets URL decoded _again_ before PATH_TRANSLATED is found;
   e.g.
   http://server.com/cgi-bin/test-cgi/hello%2524

   produces
   PATH_INFO = /hello%24
   PATH_TRANSLATED = /opt/httpd/htdocs/hello$

   fix: new routine sub_req_lookup_path() for this case.

2. For internal redirects, the URL gets decoded twice.

   e.g.
   redir contains:
   #!/bin/sh
   echo 'Location: /cgi-bin/test-cgi/byee%2524'
   echo

   then http://server.com/cgi-bin/redir
   produces
   PATH_INFO = /byee$
   PATH_TRANSLATED = /opt/httpd/htdocs/byee$

   fix: remove unescape_url() and friends from internal_redirect().

3. for virtual path redirects, the URL might not be decoded, or parts of
   it might be decoded twice.

   The call to unescape_url() in sub_req_lookup_uri() is in the wrong place;
   the new path should be decoded straight away.

4. (May not be a bug) sub_req_lookup_file wasn't setting the args of the
    new request.

David.

------------------------ begin file enc.patch -----------------------------
*** http_request.h.orig	Wed Jul 19 01:50:25 1995
--- http_request.h	Thu Jul 27 12:21:38 1995
***************
*** 78,83 ****
--- 78,84 ----
   */
  
  request_rec *sub_req_lookup_uri (char *new_file, request_rec *r);
+ extern request_rec *sub_req_lookup_path(char *new_file, request_rec *r);
  request_rec *sub_req_lookup_file (char *new_file, request_rec *r);
  int run_sub_req (request_rec *r);
  void destroy_sub_req (request_rec *r);
*** http_request.c.orig	Tue Jul 25 02:12:45 1995
--- http_request.c	Thu Jul 27 12:22:12 1995
***************
*** 327,334 ****
  {
      request_rec *rnew;
      int res;
!     char *udir;
      
      /* Check for a special case... if there are no '/' characters in new_file
       * at all, then we are looking at a relative lookup in the same directory.
       * That means we don't have to redo any access checks.
--- 327,338 ----
  {
      request_rec *rnew;
      int res;
!     char *udir, *uri;
      
+ /* firstly, decode uri */
+     uri = pstrdup(r->pool, new_file);
+     unescape_url(uri);
+ 
      /* Check for a special case... if there are no '/' characters in new_file
       * at all, then we are looking at a relative lookup in the same directory.
       * That means we don't have to redo any access checks.
***************
*** 351,357 ****
  		      new_file :
  		      make_full_path (rnew->pool, udir, new_file)));
  	
-     unescape_url (rnew->uri);
      getparents (rnew->uri);
  	
      if ((res = translate_name (rnew))
--- 355,360 ----
***************
*** 369,374 ****
--- 372,411 ----
      return rnew;
  }
  
+ /*
+  * This one is for building a new request based on an absolute URL path,
+  * inteneded for the PATH_INFO -> PATH_INFO_TRANSLATED mapping.
+  */
+ request_rec *sub_req_lookup_path (char *new_file, request_rec *r)
+ {
+     request_rec *rnew;
+     int res;
+     
+     rnew = make_sub_request (r);
+     
+     rnew->connection = r->connection; /* For now... */
+     rnew->server = r->server;
+     rnew->request_config = create_request_config (rnew->pool);
+     set_sub_req_protocol (rnew, r);
+ 	
+     rnew->uri = new_file;
+     rnew->args = NULL;
+ 	
+     if ((res = translate_name (rnew))
+ 	|| (res = directory_walk (rnew))
+ 	|| (res = check_access (rnew))
+ 	|| (!auth_type (rnew) ? 0 :
+ 	     ((res = check_user_id (rnew)) && (res = check_auth (rnew))))
+ 	|| (res = find_types (rnew))
+ 	|| (res = run_fixups (rnew))
+ 	)
+     {
+         rnew->status = res;
+     }
+ 
+     return rnew;
+ }
+ 
  request_rec *sub_req_lookup_file (char *new_file, request_rec *r)
  {
      request_rec *rnew;
***************
*** 392,397 ****
--- 429,435 ----
      set_sub_req_protocol (rnew, r);
  	
      rnew->uri = "INTERNALLY GENERATED file-relative req";
+     rnew->args = NULL;
      rnew->filename = ((new_file[0] == '/') ?
  		      new_file :
  		      make_full_path (rnew->pool, fdir, new_file));
***************
*** 604,612 ****
      
      new->prev = r;
      r->next = new;
-     
-     unescape_url(new->uri);
-     getparents (new->uri);
      
      /* We are redirecting.  Treat the internally generated transaction
       * as a GET, since there is not a chance of its getting POST-style
--- 642,647 ----
*** mod_cgi.c.orig	Wed Jul 19 01:38:43 1995
--- mod_cgi.c	Thu Jul 27 12:21:39 1995
***************
*** 190,196 ****
      }
  	
      if (r->path_info && r->path_info[0]) {
! 	request_rec *pa_req = sub_req_lookup_uri (r->path_info, r);
        
          table_set (e, "PATH_INFO", r->path_info);
  
--- 190,196 ----
      }
  	
      if (r->path_info && r->path_info[0]) {
! 	request_rec *pa_req = sub_req_lookup_path(r->path_info, r);
        
          table_set (e, "PATH_INFO", r->path_info);
  
------------------------ end file enc.patch -----------------------------