On 04/18/2008 03:04 PM, Jim Jagielski wrote:
On Apr 18, 2008, at 8:55 AM, Plüm, Rüdiger, VF-Group wrote:
IMHO it cannot include the query args except for the nocanon
case as in all other cases r->uri was used which does not contain
query args.
This is different from the situation in mod_proxy_http where we
can act as real (forward) proxy. mod_proxy_ajp always acts as a gateway.
Oh... I wasn't aware you were referring *only* to the AJP impl
(I saw that the patch was against that, but assumed you meant
its m_p_h analog as well)...
In fact I meant both. But even in the mod_proxy_http case is does not
matter because we have the switch (r->proxyreq) there which lets us
distinguish between forward and reverse proxy requests. I assume that
the comparison between r->uri and r->unparsed_uri is legacy code that
was used before we had r->proxyreq.
I'm all for good looking code ;)
So what about the following patch (it effectively reverts yours and adds in
mine)?
Index: modules/proxy/mod_proxy_ajp.c
===================================================================
--- modules/proxy/mod_proxy_ajp.c (Revision 649787)
+++ modules/proxy/mod_proxy_ajp.c (Arbeitskopie)
@@ -31,7 +31,6 @@
{
char *host, *path, *search, sport[7];
const char *err;
- const char *pnocanon;
apr_port_t port = AJP13_DEF_PORT;
/* ap_port_of_scheme() */
@@ -59,27 +58,17 @@
/*
* now parse path/search args, according to rfc1738
- *
- * N.B. if this isn't a true proxy request, then the URL _path_
- * has already been decoded. True proxy requests have
- * r->uri == r->unparsed_uri, and no others have that property.
*/
- pnocanon = apr_table_get(r->notes, "proxy-nocanon");
- if ((r->uri == r->unparsed_uri) || pnocanon) {
- search = strchr(url, '?');
- if (search != NULL)
- *(search++) = '\0';
- }
- else
- search = r->args;
+ search = NULL;
/* process path */
- if (pnocanon) {
+ if (apr_table_get(r->notes, "proxy-nocanon")) {
path = url; /* this is the raw path */
}
else {
path = ap_proxy_canonenc(r->pool, url, strlen(url), enc_path, 0,
r->proxyreq);
+ search = r->args;
}
if (path == NULL)
return HTTP_BAD_REQUEST;
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (Revision 649787)
+++ modules/proxy/mod_proxy_http.c (Arbeitskopie)
@@ -36,7 +36,6 @@
char *host, *path, *search, sport[7];
const char *err;
const char *scheme;
- const char *pnocanon;
apr_port_t port, def_port;
/* ap_port_of_scheme() */
@@ -69,19 +68,7 @@
}
/* now parse path/search args, according to rfc1738 */
- /* N.B. if this isn't a true proxy request, then the URL _path_
- * has already been decoded. True proxy requests have r->uri
- * == r->unparsed_uri, and no others have that property.
- */
- pnocanon = apr_table_get(r->notes, "proxy-nocanon");
- if ((r->uri == r->unparsed_uri) ||
- ((r->proxyreq == PROXYREQ_REVERSE) && pnocanon)) {
- search = strchr(url, '?');
- if (search != NULL)
- *(search++) = '\0';
- }
- else
- search = r->args;
+ search = NULL;
/* process path */
/* In a reverse proxy, our URL has been processed, so canonicalise
@@ -91,12 +78,13 @@
switch (r->proxyreq) {
default: /* wtf are we doing here? */
case PROXYREQ_REVERSE:
- if (pnocanon) {
+ if (apr_table_get(r->notes, "proxy-nocanon")) {
path = url; /* this is the raw path */
}
else {
path = ap_proxy_canonenc(r->pool, url, strlen(url),
enc_path, 0, r->proxyreq);
+ search = r->args;
}
break;
case PROXYREQ_PROXY:
If you have no further objections I would commit it.
Regards
Rüdiger
Index: modules/proxy/mod_proxy_ajp.c
===================================================================
--- modules/proxy/mod_proxy_ajp.c (Revision 649787)
+++ modules/proxy/mod_proxy_ajp.c (Arbeitskopie)
@@ -31,7 +31,6 @@
{
char *host, *path, *search, sport[7];
const char *err;
- const char *pnocanon;
apr_port_t port = AJP13_DEF_PORT;
/* ap_port_of_scheme() */
@@ -59,27 +58,17 @@
/*
* now parse path/search args, according to rfc1738
- *
- * N.B. if this isn't a true proxy request, then the URL _path_
- * has already been decoded. True proxy requests have
- * r->uri == r->unparsed_uri, and no others have that property.
*/
- pnocanon = apr_table_get(r->notes, "proxy-nocanon");
- if ((r->uri == r->unparsed_uri) || pnocanon) {
- search = strchr(url, '?');
- if (search != NULL)
- *(search++) = '\0';
- }
- else
- search = r->args;
+ search = NULL;
/* process path */
- if (pnocanon) {
+ if (apr_table_get(r->notes, "proxy-nocanon")) {
path = url; /* this is the raw path */
}
else {
path = ap_proxy_canonenc(r->pool, url, strlen(url), enc_path, 0,
r->proxyreq);
+ search = r->args;
}
if (path == NULL)
return HTTP_BAD_REQUEST;
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (Revision 649787)
+++ modules/proxy/mod_proxy_http.c (Arbeitskopie)
@@ -36,7 +36,6 @@
char *host, *path, *search, sport[7];
const char *err;
const char *scheme;
- const char *pnocanon;
apr_port_t port, def_port;
/* ap_port_of_scheme() */
@@ -69,19 +68,7 @@
}
/* now parse path/search args, according to rfc1738 */
- /* N.B. if this isn't a true proxy request, then the URL _path_
- * has already been decoded. True proxy requests have r->uri
- * == r->unparsed_uri, and no others have that property.
- */
- pnocanon = apr_table_get(r->notes, "proxy-nocanon");
- if ((r->uri == r->unparsed_uri) ||
- ((r->proxyreq == PROXYREQ_REVERSE) && pnocanon)) {
- search = strchr(url, '?');
- if (search != NULL)
- *(search++) = '\0';
- }
- else
- search = r->args;
+ search = NULL;
/* process path */
/* In a reverse proxy, our URL has been processed, so canonicalise
@@ -91,12 +78,13 @@
switch (r->proxyreq) {
default: /* wtf are we doing here? */
case PROXYREQ_REVERSE:
- if (pnocanon) {
+ if (apr_table_get(r->notes, "proxy-nocanon")) {
path = url; /* this is the raw path */
}
else {
path = ap_proxy_canonenc(r->pool, url, strlen(url),
enc_path, 0, r->proxyreq);
+ search = r->args;
}
break;
case PROXYREQ_PROXY:
