You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ji...@apache.org on 2008/04/17 21:20:18 UTC
svn commit: r649239 - in /httpd/httpd/trunk/modules/proxy: mod_proxy_ajp.c
mod_proxy_http.c
Author: jim
Date: Thu Apr 17 12:20:16 2008
New Revision: 649239
URL: http://svn.apache.org/viewvc?rev=649239&view=rev
Log:
handle ? in cases where nocanon is in effect
Modified:
httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c
httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c?rev=649239&r1=649238&r2=649239&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c Thu Apr 17 12:20:16 2008
@@ -31,6 +31,7 @@
{
char *host, *path, *search, sport[7];
const char *err;
+ const char *pnocanon;
apr_port_t port = AJP13_DEF_PORT;
/* ap_port_of_scheme() */
@@ -63,7 +64,8 @@
* has already been decoded. True proxy requests have
* r->uri == r->unparsed_uri, and no others have that property.
*/
- if (r->uri == r->unparsed_uri) {
+ pnocanon = apr_table_get(r->notes, "proxy-nocanon");
+ if ((r->uri == r->unparsed_uri) || pnocanon) {
search = strchr(url, '?');
if (search != NULL)
*(search++) = '\0';
@@ -72,7 +74,7 @@
search = r->args;
/* process path */
- if (apr_table_get(r->notes, "proxy-nocanon")) {
+ if (pnocanon) {
path = url; /* this is the raw path */
}
else {
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=649239&r1=649238&r2=649239&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Thu Apr 17 12:20:16 2008
@@ -36,6 +36,7 @@
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() */
@@ -72,7 +73,9 @@
* has already been decoded. True proxy requests have r->uri
* == r->unparsed_uri, and no others have that property.
*/
- if (r->uri == r->unparsed_uri) {
+ 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';
@@ -88,7 +91,7 @@
switch (r->proxyreq) {
default: /* wtf are we doing here? */
case PROXYREQ_REVERSE:
- if (apr_table_get(r->notes, "proxy-nocanon")) {
+ if (pnocanon) {
path = url; /* this is the raw path */
}
else {
Re: svn commit: r649239 - in /httpd/httpd/trunk/modules/proxy:
mod_proxy_ajp.c mod_proxy_http.c
Posted by Dirk-Willem van Gulik <di...@webweaving.org>.
On Thu, 17 Apr 2008 jim@apache.org wrote:
> Log:
> handle ? in cases where nocanon is in effect
Lovely - this fixes one of my ajp problems as well (which I thus wrongly
was blaming on the cache).
Is this now hooked in everywhere ? Or do we still need to do a scan
throughout everything ?
Dw
Re: svn commit: r649239 - in /httpd/httpd/trunk/modules/proxy: mod_proxy_ajp.c
mod_proxy_http.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/17/2008 10:10 PM, Ruediger Pluem wrote:
>
> I am frankly open here: The old code already looked ugly in respect of
> this and
> the new code IMHO even looks uglier. What about the following patch
Of course I forgot a :-) here.
Regards
Rüdiger
Re: svn commit: r649239 - in /httpd/httpd/trunk/modules/proxy: mod_proxy_ajp.c mod_proxy_http.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Apr 19, 2008, at 2:49 PM, Ruediger Pluem wrote:
>
>
> On 04/19/2008 06:22 PM, Jim Jagielski wrote:
>> On Apr 19, 2008, at 6:11 AM, Ruediger Pluem wrote:
>>>
>>> If you have no further objections I would commit it.
>>>
>> Hmmmm.... from what I can see, the conditional for '(r->uri == r-
>> >unparsed_uri)'
>> doesn't seem to be required anymore in our local code, so it
>> seems safe enough to remove. When that's done, the code reduces
>> to your patch...
>> I am somewhat concerned about possible regressions with that
>> reduction however (hence my version which retained it - unless I
>> am confident that regression won't occur, I tend not to remove
>> legacy codepaths :) )... But I see no real reason not to commit
>
> So do I. But I am confident that there is no regression :-).
Well, I'm like 99% sure. It's just that some modules directly
create a proxy subreq and may expect this logic flow... if they
do, then it is arguably their problem :)
>
> I just committed (r649840).
+1
Re: svn commit: r649239 - in /httpd/httpd/trunk/modules/proxy: mod_proxy_ajp.c
mod_proxy_http.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/19/2008 06:22 PM, Jim Jagielski wrote:
>
> On Apr 19, 2008, at 6:11 AM, Ruediger Pluem wrote:
>>
>> If you have no further objections I would commit it.
>>
>
> Hmmmm.... from what I can see, the conditional for '(r->uri ==
> r->unparsed_uri)'
> doesn't seem to be required anymore in our local code, so it
> seems safe enough to remove. When that's done, the code reduces
> to your patch...
>
> I am somewhat concerned about possible regressions with that
> reduction however (hence my version which retained it - unless I
> am confident that regression won't occur, I tend not to remove
> legacy codepaths :) )... But I see no real reason not to commit
So do I. But I am confident that there is no regression :-).
I just committed (r649840). Lets see if I was wrong.
Regards
Rüdiger
Re: svn commit: r649239 - in /httpd/httpd/trunk/modules/proxy: mod_proxy_ajp.c mod_proxy_http.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Apr 19, 2008, at 6:11 AM, Ruediger Pluem wrote:
>
> If you have no further objections I would commit it.
>
Hmmmm.... from what I can see, the conditional for '(r->uri == r-
>unparsed_uri)'
doesn't seem to be required anymore in our local code, so it
seems safe enough to remove. When that's done, the code reduces
to your patch...
I am somewhat concerned about possible regressions with that
reduction however (hence my version which retained it - unless I
am confident that regression won't occur, I tend not to remove
legacy codepaths :) )... But I see no real reason not to commit
Re: svn commit: r649239 - in /httpd/httpd/trunk/modules/proxy: mod_proxy_ajp.c
mod_proxy_http.c
Posted by Ruediger Pluem <rp...@apache.org>.
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
Re: svn commit: r649239 - in /httpd/httpd/trunk/modules/proxy: mod_proxy_ajp.c mod_proxy_http.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
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)...
I'm all for good looking code ;)
Re: svn commit: r649239 - in /httpd/httpd/trunk/modules/proxy: mod_proxy_ajp.c mod_proxy_http.c
Posted by Plüm,
Rüdiger,
VF-Group <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Jim Jagielski [mailto:jim@jaguNET.com]
> Gesendet: Freitag, 18. April 2008 14:51
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r649239 - in
> /httpd/httpd/trunk/modules/proxy: mod_proxy_ajp.c mod_proxy_http.c
>
>
> On Apr 17, 2008, at 4:10 PM, Ruediger Pluem wrote:
> > Index: modules/proxy/mod_proxy_ajp.c
> > ===================================================================
> > --- modules/proxy/mod_proxy_ajp.c (Revision 649232)
> > +++ modules/proxy/mod_proxy_ajp.c (Arbeitskopie)
> > @@ -58,18 +58,8 @@
> >
> > /*
> > * 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.
> > */
> > - if (r->uri == r->unparsed_uri) {
> > - search = strchr(url, '?');
> > - if (search != NULL)
> > - *(search++) = '\0';
> > - }
> > - else
> > - search = r->args;
> > + search = NULL;
> >
> > /* process path */
> > if (apr_table_get(r->notes, "proxy-nocanon")) {
> > @@ -78,6 +68,7 @@
> > 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;
>
> Ummm.... don't we still need to address the case where url
> might include the query args?
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.
Regards
Rüdiger
>
Re: svn commit: r649239 - in /httpd/httpd/trunk/modules/proxy: mod_proxy_ajp.c mod_proxy_http.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Apr 17, 2008, at 4:10 PM, Ruediger Pluem wrote:
> Index: modules/proxy/mod_proxy_ajp.c
> ===================================================================
> --- modules/proxy/mod_proxy_ajp.c (Revision 649232)
> +++ modules/proxy/mod_proxy_ajp.c (Arbeitskopie)
> @@ -58,18 +58,8 @@
>
> /*
> * 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.
> */
> - if (r->uri == r->unparsed_uri) {
> - search = strchr(url, '?');
> - if (search != NULL)
> - *(search++) = '\0';
> - }
> - else
> - search = r->args;
> + search = NULL;
>
> /* process path */
> if (apr_table_get(r->notes, "proxy-nocanon")) {
> @@ -78,6 +68,7 @@
> 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;
Ummm.... don't we still need to address the case where url
might include the query args?
Re: svn commit: r649239 - in /httpd/httpd/trunk/modules/proxy: mod_proxy_ajp.c
mod_proxy_http.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/17/2008 09:20 PM, jim@apache.org wrote:
> Author: jim
> Date: Thu Apr 17 12:20:16 2008
> New Revision: 649239
>
> URL: http://svn.apache.org/viewvc?rev=649239&view=rev
> Log:
> handle ? in cases where nocanon is in effect
>
> Modified:
> httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c?rev=649239&r1=649238&r2=649239&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c Thu Apr 17 12:20:16 2008
> @@ -31,6 +31,7 @@
> {
> char *host, *path, *search, sport[7];
> const char *err;
> + const char *pnocanon;
> apr_port_t port = AJP13_DEF_PORT;
>
> /* ap_port_of_scheme() */
> @@ -63,7 +64,8 @@
> * has already been decoded. True proxy requests have
> * r->uri == r->unparsed_uri, and no others have that property.
> */
> - if (r->uri == r->unparsed_uri) {
> + pnocanon = apr_table_get(r->notes, "proxy-nocanon");
> + if ((r->uri == r->unparsed_uri) || pnocanon) {
> search = strchr(url, '?');
> if (search != NULL)
> *(search++) = '\0';
> @@ -72,7 +74,7 @@
> search = r->args;
>
> /* process path */
> - if (apr_table_get(r->notes, "proxy-nocanon")) {
> + if (pnocanon) {
> path = url; /* this is the raw path */
> }
> else {
I am frankly open here: The old code already looked ugly in respect of this and
the new code IMHO even looks uglier. What about the following patch which is IMHO much
cleaner:
Index: modules/proxy/mod_proxy_ajp.c
===================================================================
--- modules/proxy/mod_proxy_ajp.c (Revision 649232)
+++ modules/proxy/mod_proxy_ajp.c (Arbeitskopie)
@@ -58,18 +58,8 @@
/*
* 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.
*/
- if (r->uri == r->unparsed_uri) {
- search = strchr(url, '?');
- if (search != NULL)
- *(search++) = '\0';
- }
- else
- search = r->args;
+ search = NULL;
/* process path */
if (apr_table_get(r->notes, "proxy-nocanon")) {
@@ -78,6 +68,7 @@
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;