Re: svn commit: r583466 - in /httpd/httpd/trunk: include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c

2007-10-10 Thread Roy T. Fielding 

The whole idea behind this routine is just wrong.
That set of characters is insufficient (RFC 3986) and, in
any case, a proxy is not responsible for checking valid
characters in a URI.  Both the original and this new function
should be deleted.

Roy

On Oct 10, 2007, at 6:16 AM, [EMAIL PROTECTED] wrote:


Author: jim
Date: Wed Oct 10 06:16:56 2007
New Revision: 583466

URL: http://svn.apache.org/viewvc?rev=583466view=rev
Log:
Abstract out verification of valid encoding via
ap_proxy_isvalidenc(). Now we can use it in other
proxy protocols.

Modified:
httpd/httpd/trunk/include/ap_mmn.h
httpd/httpd/trunk/modules/proxy/mod_proxy.h
httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
httpd/httpd/trunk/modules/proxy/proxy_util.c

Modified: httpd/httpd/trunk/include/ap_mmn.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ 
ap_mmn.h?rev=583466r1=583465r2=583466view=diff
== 


--- httpd/httpd/trunk/include/ap_mmn.h (original)
+++ httpd/httpd/trunk/include/ap_mmn.h Wed Oct 10 06:16:56 2007
@@ -133,6 +133,7 @@
  * 20070823.0 (2.3.0-dev)  Removed ap_all_available_mutexes_string,
  * ap_available_mutexes_string for macros
  * 20070823.1 (2.3.0-dev)  add ap_send_interim_response()
+ * 20070823.2 (2.3.0-dev)  add ap_proxy_isvalidenc()
  *
  */


Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/ 
mod_proxy.h?rev=583466r1=583465r2=583466view=diff
== 


--- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Wed Oct 10 06:16:56  
2007

@@ -460,6 +460,7 @@
 PROXY_DECLARE(request_rec *)ap_proxy_make_fake_req(conn_rec *c,  
request_rec *r);

 PROXY_DECLARE(int) ap_proxy_hex2c(const char *x);
 PROXY_DECLARE(void) ap_proxy_c2hex(int ch, char *x);
+PROXY_DECLARE(apr_status_t)ap_proxy_isvalidenc(const char *url,  
const char *allowed);
 PROXY_DECLARE(char *)ap_proxy_canonenc(apr_pool_t *p, const char  
*x, int len, enum enctype t,

int forcedec, int proxyreq);
 PROXY_DECLARE(char *)ap_proxy_canon_netloc(apr_pool_t *p, char  
**const urlp, char **userp,


Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/ 
mod_proxy_http.c?rev=583466r1=583465r2=583466view=diff
== 


--- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Wed Oct 10  
06:16:56 2007

@@ -37,9 +37,6 @@
 const char *err;
 const char *scheme;
 apr_port_t port, def_port;
-const char *p;
-const char *allowed = ~$-_.+!*'(),;:@=/; /* allowed 
+reserved from
-
ap_proxy_canonenc */


 /* ap_port_of_scheme() */
 if (strncasecmp(url, http:, 5) == 0) {
@@ -94,15 +91,8 @@
 path = ap_proxy_canonenc(r-pool, url, strlen(url),  
enc_path, 0, r-proxyreq);

 break;
 case PROXYREQ_PROXY:
-for (p = url; *p; ++p) {
-if (!apr_isalnum(*p)  !strchr(allowed, *p)) {
-if (*p == '%'  apr_isxdigit(p[1])  apr_isxdigit 
(p[2])) {

-p += 2; /* an encoded char */
-}
-else {
-return HTTP_BAD_REQUEST; /* reject bad char in  
URL */

-}
-}
+if (ap_proxy_isvalidenc(url, NULL) != APR_SUCCESS) {
+return HTTP_BAD_REQUEST;
 }
 path = url;
 break;

Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/ 
proxy_util.c?rev=583466r1=583465r2=583466view=diff
== 


--- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
+++ httpd/httpd/trunk/modules/proxy/proxy_util.c Wed Oct 10  
06:16:56 2007

@@ -135,6 +135,33 @@
 }

 /*
+ * Confirm that a URL-encoded string only contains
+ * valid encoding, valid chars are passed in allowed.
+ * If allowed is NULL, we use useful default.
+ */
+PROXY_DECLARE(apr_status_t)ap_proxy_isvalidenc(const char *url,
+   const char *allowed)
+
+{
+if (!allowed) {
+allowed = ~$-_.+!*'(),;:@=/; /* allowed+reserved from
+   ap_proxy_canonenc */
+}
+
+for ( ; *url; ++url) {
+if (!apr_isalnum(*url)  !ap_strchr_c(allowed, *url)) {
+if (*url == '%'  apr_isxdigit(url[1])  apr_isxdigit 
(url[2])) {

+url += 2; /* an encoded char */
+}
+else {
+return APR_EGENERAL; /* reject bad char in URL */
+}
+}
+}
+

Re: svn commit: r583466 - in /httpd/httpd/trunk: include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c

2007-10-10 Thread Jim Jagielski 
Good point... forward proxies should just pass through
as is, good or bad.

Will revert the patch and fix Nick's impl tomorrow when I
get back in the office.

On Wed, Oct 10, 2007 at 12:39:56PM -0700, Roy T. Fielding wrote:
 The whole idea behind this routine is just wrong.
 That set of characters is insufficient (RFC 3986) and, in
 any case, a proxy is not responsible for checking valid
 characters in a URI.  Both the original and this new function
 should be deleted.
 
 Roy
 
 On Oct 10, 2007, at 6:16 AM, [EMAIL PROTECTED] wrote:
 
 Author: jim
 Date: Wed Oct 10 06:16:56 2007
 New Revision: 583466
 
 URL: http://svn.apache.org/viewvc?rev=583466view=rev
 Log:
 Abstract out verification of valid encoding via
 ap_proxy_isvalidenc(). Now we can use it in other
 proxy protocols.
 
 Modified:
 httpd/httpd/trunk/include/ap_mmn.h
 httpd/httpd/trunk/modules/proxy/mod_proxy.h
 httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
 httpd/httpd/trunk/modules/proxy/proxy_util.c
 
 Modified: httpd/httpd/trunk/include/ap_mmn.h
 URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ 
 ap_mmn.h?rev=583466r1=583465r2=583466view=diff
 == 
 
 --- httpd/httpd/trunk/include/ap_mmn.h (original)
 +++ httpd/httpd/trunk/include/ap_mmn.h Wed Oct 10 06:16:56 2007
 @@ -133,6 +133,7 @@
   * 20070823.0 (2.3.0-dev)  Removed ap_all_available_mutexes_string,
   * ap_available_mutexes_string for macros
   * 20070823.1 (2.3.0-dev)  add ap_send_interim_response()
 + * 20070823.2 (2.3.0-dev)  add ap_proxy_isvalidenc()
   *
   */
 
 
 Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h
 URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/ 
 mod_proxy.h?rev=583466r1=583465r2=583466view=diff
 == 
 
 --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
 +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Wed Oct 10 06:16:56  
 2007
 @@ -460,6 +460,7 @@
  PROXY_DECLARE(request_rec *)ap_proxy_make_fake_req(conn_rec *c,  
 request_rec *r);
  PROXY_DECLARE(int) ap_proxy_hex2c(const char *x);
  PROXY_DECLARE(void) ap_proxy_c2hex(int ch, char *x);
 +PROXY_DECLARE(apr_status_t)ap_proxy_isvalidenc(const char *url,  
 const char *allowed);
  PROXY_DECLARE(char *)ap_proxy_canonenc(apr_pool_t *p, const char  
 *x, int len, enum enctype t,
 int forcedec, int proxyreq);
  PROXY_DECLARE(char *)ap_proxy_canon_netloc(apr_pool_t *p, char  
 **const urlp, char **userp,
 
 Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
 URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/ 
 mod_proxy_http.c?rev=583466r1=583465r2=583466view=diff
 == 
 
 --- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original)
 +++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Wed Oct 10  
 06:16:56 2007
 @@ -37,9 +37,6 @@
  const char *err;
  const char *scheme;
  apr_port_t port, def_port;
 -const char *p;
 -const char *allowed = ~$-_.+!*'(),;:@=/; /* allowed 
 +reserved from
 -
 ap_proxy_canonenc */
 
  /* ap_port_of_scheme() */
  if (strncasecmp(url, http:, 5) == 0) {
 @@ -94,15 +91,8 @@
  path = ap_proxy_canonenc(r-pool, url, strlen(url),  
 enc_path, 0, r-proxyreq);
  break;
  case PROXYREQ_PROXY:
 -for (p = url; *p; ++p) {
 -if (!apr_isalnum(*p)  !strchr(allowed, *p)) {
 -if (*p == '%'  apr_isxdigit(p[1])  apr_isxdigit 
 (p[2])) {
 -p += 2; /* an encoded char */
 -}
 -else {
 -return HTTP_BAD_REQUEST; /* reject bad char in  
 URL */
 -}
 -}
 +if (ap_proxy_isvalidenc(url, NULL) != APR_SUCCESS) {
 +return HTTP_BAD_REQUEST;
  }
  path = url;
  break;
 
 Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
 URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/ 
 proxy_util.c?rev=583466r1=583465r2=583466view=diff
 == 
 
 --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
 +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Wed Oct 10  
 06:16:56 2007
 @@ -135,6 +135,33 @@
  }
 
  /*
 + * Confirm that a URL-encoded string only contains
 + * valid encoding, valid chars are passed in allowed.
 + * If allowed is NULL, we use useful default.
 + */
 +PROXY_DECLARE(apr_status_t)ap_proxy_isvalidenc(const char *url,
 +   const char *allowed)
 +
 +{
 +if (!allowed) {
 +allowed = ~$-_.+!*'(),;:@=/; /* allowed+reserved from
 +   ap_proxy_canonenc */
 +}
 +
 +for ( ; *url;