On Thu, Jun 25, 2020 at 1:27 PM Ruediger Pluem <rpl...@apache.org> wrote:
>
> On 6/25/20 12:13 PM, Yann Ylavic wrote:
> > Index: modules/proxy/mod_proxy.c
> > ===================================================================
> > --- modules/proxy/mod_proxy.c (revision 1879145)
> > +++ modules/proxy/mod_proxy.c (working copy)
>
> > @@ -987,10 +991,10 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
> > "URI path '%s' matches proxy handler '%s'", r->uri,
> > found);
> >
> > - return OK;
> > + return servlet ? DONE : OK;
>
> Why setting it to DONE in the servlet case? Wouldn't that cause
> ap_process_request_internal to be left early?
No, ap_process_request_internal() would just skip r->uri decoding (we
are in pre_trans hook here, since mapping=servlet only happens there).
Anyway, it's an orthogonal change sorry, maybe we still want uri
decoding for directory/location walk in the servlet case, but since
this patch actually modifies r->uri (while other proxy mappings do
not), I thought it could be the final transformation (that's DONE
returned from pre_trans).
The only case where it matters is for encoded control/reserved chars
(unreserved are always decoded during initial normalization). So for
the authz/authn case you mentioned it means that the admin would have
to use/match the encoded form of the path (in <Location>) when the
concerned path contains reserved chars.
Something like <Location "/admin%3Bfoo"> if the final app on Tomcat is
really called "admin;foo" (which I don't recommend!), but it kind of
make sense because with servlet normalization <Location /admin;foo>
would never match..
That's all theoretical and unlikely case, but that's where we are ;)
By the way, I think a more correct patch would be this one (attached),
whether it returns DONE or OK (I kept DONE for now, until we get
further on this lovely encoding discussion :)
The change is that r->uri is rewritten in-place so that
r->parsed_uri.path gets updated too.
Regards;
Yann.
Index: modules/proxy/mod_proxy.c
===================================================================
--- modules/proxy/mod_proxy.c (revision 1879145)
+++ modules/proxy/mod_proxy.c (working copy)
@@ -17,6 +17,7 @@
#include "mod_proxy.h"
#include "mod_core.h"
#include "apr_optional.h"
+#include "apr_strings.h"
#include "scoreboard.h"
#include "mod_status.h"
#include "proxy_util.h"
@@ -574,10 +575,11 @@ static int alias_match(const char *uri, const char
* Inspired by mod_jk's jk_servlet_normalize().
*/
static int alias_match_servlet(apr_pool_t *p,
- const char *uri,
+ const char **urip,
const char *alias)
{
char *map;
+ const char *uri = *urip;
apr_array_header_t *stack;
int map_pos, uri_pos, alias_pos, first_pos;
int alias_depth = 0, depth;
@@ -759,6 +761,8 @@ static int alias_match_servlet(apr_pool_t *p,
if (alias[alias_pos - 1] != '/' && uri[uri_pos - 1] == '/') {
uri_pos--;
}
+
+ *urip = map;
return uri_pos;
}
@@ -872,6 +876,7 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
int mismatch = 0;
unsigned int nocanon = ent->flags & PROXYPASS_NOCANON;
const char *use_uri = nocanon ? r->unparsed_uri : r->uri;
+ const char *servlet_uri = NULL;
if (dconf && (dconf->interpolate_env == 1) && (ent->flags & PROXYPASS_INTERPOLATE)) {
fake = proxy_interpolate(r, ent->fake);
@@ -933,8 +938,9 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
}
else {
if ((ent->flags & PROXYPASS_MAP_SERVLET) == PROXYPASS_MAP_SERVLET) {
+ servlet_uri = r->uri;
+ len = alias_match_servlet(r->pool, &servlet_uri, fake);
nocanon = 0; /* ignored since servlet's normalization applies */
- len = alias_match_servlet(r->pool, r->uri, fake);
}
else {
len = alias_match(r->uri, fake);
@@ -969,7 +975,7 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
*/
int rc = proxy_run_check_trans(r, found + 6);
if (rc != OK && rc != DECLINED) {
- return DONE;
+ return HTTP_CONTINUE;
}
r->filename = found;
@@ -983,14 +989,27 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
apr_table_setn(r->notes, "proxy-noquery", "1");
}
+ if (servlet_uri) {
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO()
+ "Servlet path '%s' (%s) matches proxy handler '%s'",
+ r->uri, servlet_uri, found);
+
+ /* We change r->uri in-place so that r->parsed_uri.path gets
+ * updated too. Normalized servlet_uri is necessarily shorter
+ * than the original r->uri, so strcpy() is fine.
+ */
+ AP_DEBUG_ASSERT(strlen(r->uri) >= strlen(servlet_uri));
+ strcpy(r->uri, servlet_uri);
+ return DONE;
+ }
+
ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO(03464)
"URI path '%s' matches proxy handler '%s'", r->uri,
found);
-
return OK;
}
- return DONE;
+ return HTTP_CONTINUE;
}
static int proxy_trans(request_rec *r, int pre_trans)
@@ -1042,7 +1061,7 @@ static int proxy_trans(request_rec *r, int pre_tra
enc = (dconf->alias->flags & PROXYPASS_MAP_ENCODED) != 0;
if (!(pre_trans ^ enc)) {
int rv = ap_proxy_trans_match(r, dconf->alias, dconf);
- if (DONE != rv) {
+ if (rv != HTTP_CONTINUE) {
return rv;
}
}
@@ -1054,7 +1073,7 @@ static int proxy_trans(request_rec *r, int pre_tra
enc = (ent->flags & PROXYPASS_MAP_ENCODED) != 0;
if (!(pre_trans ^ enc)) {
int rv = ap_proxy_trans_match(r, ent, dconf);
- if (DONE != rv) {
+ if (rv != HTTP_CONTINUE) {
return rv;
}
}
