On Mon, Mar 13, 2023 at 5:48 PM Yann Ylavic <ylavic....@gmail.com> wrote:
>
> On Mon, Mar 13, 2023 at 5:42 PM Eric Covener <cove...@gmail.com> wrote:
> >
> > On Mon, Mar 13, 2023 at 12:31 PM Yann Ylavic <ylavic....@gmail.com> wrote:
> > >
> > > On Mon, Mar 13, 2023 at 5:25 PM Eric Covener <cove...@gmail.com> wrote:
> > > >
> > > > On Mon, Mar 13, 2023 at 12:05 PM Yann Ylavic <ylavic....@gmail.com>
> > > > wrote:
> > > > >
> > > > > I could get where you want to with the attached patch (before you
> > > > > disabled the test in r1908350).
> > > > > It makes so that anything BNEG'ed overrides BCTLS, which can be useful
> > > > > to let space through while still encoding controls.
> > > > > I had to fix the local_bctls_nospace RewriteRule to not encode '/'
> > > > > (not expected in the result) and add a missing '/' in the result too.
> > > >
> > > >
> > > > + RewriteRule ^/modules/rewrite/escaping/local_bctls_nospace/(.*)
> > > > /?$1 "[B= /,BNEG,BCTLS]"
> > > > + [
> > > > "/modules/rewrite/escaping/local_bctls_nospace/foo/bar/%20baz/%0d"
> > > > => "foo/bar/ baz/%0d"], # CTLS but allow space
> > > >
> > > > I'm not sure I understand this, / isn't a CTL so why do we need to
> > > > exclude it with B+BNEG to have it passed through in the test?
> > >
> > > With BNEG everything not in B= is encoded, so "[B= ?,BNEG]"
> > > (regardless of BCTLS) will encode '/'.
> >
> > Ah, I see. I thought/hoped it would become relative to CTLS. Maybe
> > best to keep it simple.
>
> That's one way to implement it though, if BCTLS is there it could be a
> negation of that only, no strong opinion on this..
> With the current implementation the BCTLS above is implied already, so
> "[B= /,BNEG]" would be the same.
Maybe instead of BNEG we can implement BNE= (backref noescape
charset), so one could "[B,BNE=/]" to encode everything but '/', or
"[BCTLS,BNE= ?]" for controls but space, etc.. BNE= always wins over
B=, and is meaningless alone.
Something like the attached patch now?
Index: modules/mappers/mod_rewrite.c
===================================================================
--- modules/mappers/mod_rewrite.c (revision 1908352)
+++ modules/mappers/mod_rewrite.c (working copy)
@@ -177,7 +177,7 @@ static const char* really_last_key = "rewrite_real
#define RULEFLAG_QSLAST (1<<19)
#define RULEFLAG_QSNONE (1<<20) /* programattic only */
#define RULEFLAG_ESCAPECTLS (1<<21)
-#define RULEFLAG_ESCAPENEG (1<<22)
+#define RULEFLAG_NOESCAPEBACKREF (1<<22)
/* return code of the rewrite rule
* the result may be escaped - or not
@@ -329,6 +329,7 @@ typedef struct {
int skip; /* number of next rules to skip */
int maxrounds; /* limit on number of loops with N flag */
char *escapes; /* specific backref escapes */
+ char *noescapes; /* specific backref chars not to escape */
} rewriterule_entry;
typedef struct {
@@ -429,7 +430,9 @@ static apr_global_mutex_t *rewrite_mapr_lock_acqui
static const char *rewritemap_mutex_type = "rewrite-map";
/* Optional functions imported from mod_ssl when loaded: */
-static char *escape_backref(apr_pool_t *p, const char *path, const char *escapeme, int flags);
+static char *escape_backref(apr_pool_t *p, const char *path,
+ const char *escapeme, const char *noescapeme,
+ int flags);
/*
* +-------------------------------------------------------+
@@ -688,7 +691,9 @@ static APR_INLINE unsigned char *c2x(unsigned what
* Escapes a backreference in a similar way as php's urlencode does.
* Based on ap_os_escape_path in server/util.c
*/
-static char *escape_backref(apr_pool_t *p, const char *path, const char *escapeme, int flags)
+static char *escape_backref(apr_pool_t *p, const char *path,
+ const char *escapeme, const char *noescapeme,
+ int flags)
{
char *copy = apr_palloc(p, 3 * strlen(path) + 1);
const unsigned char *s = (const unsigned char *)path;
@@ -695,12 +700,12 @@ static APR_INLINE unsigned char *c2x(unsigned what
unsigned char *d = (unsigned char *)copy;
int noplus = (flags & RULEFLAG_ESCAPENOPLUS) != 0;
int ctls = (flags & RULEFLAG_ESCAPECTLS) != 0;
- int neg = (flags & RULEFLAG_ESCAPENEG) != 0;
unsigned char c;
while ((c = *s)) {
- if ((ctls ? !TEST_CHAR(c, T_VCHAR_OBSTEXT) : !escapeme)
- || (escapeme && ((ap_strchr_c(escapeme, c) != NULL) ^ neg))) {
+ if (((ctls ? !TEST_CHAR(c, T_VCHAR_OBSTEXT) : !escapeme)
+ || (escapeme && ap_strchr_c(escapeme, c)))
+ && (!noescapeme || !ap_strchr_c(noescapeme, c))) {
if (apr_isalnum(c) || c == '_') {
*d++ = c;
}
@@ -2487,11 +2492,13 @@ static char *do_expand(char *input, rewrite_ctx *c
if (bri->source && n < AP_MAX_REG_MATCH
&& bri->regmatch[n].rm_eo > bri->regmatch[n].rm_so) {
span = bri->regmatch[n].rm_eo - bri->regmatch[n].rm_so;
- if (entry && (entry->flags & RULEFLAG_ESCAPEBACKREF)) {
+ if (entry && (entry->flags & (RULEFLAG_ESCAPEBACKREF |
+ RULEFLAG_NOESCAPEBACKREF))) {
/* escape the backreference */
char *tmp2, *tmp;
tmp = apr_pstrmemdup(pool, bri->source + bri->regmatch[n].rm_so, span);
- tmp2 = escape_backref(pool, tmp, entry->escapes, entry->flags);
+ tmp2 = escape_backref(pool, tmp, entry->escapes, entry->noescapes,
+ entry->flags);
rewritelog(ctx->r, 5, ctx->perdir, "escaping backreference '%s' to '%s'",
tmp, tmp2);
@@ -3583,8 +3590,9 @@ static const char *cmd_rewriterule_setflag(apr_poo
cfg->escapes = val;
}
}
- else if (!strcasecmp(key, "NEG")) {
- cfg->flags |= RULEFLAG_ESCAPENEG;
+ else if (!strcasecmp(key, "NE") && val && *val) {
+ cfg->flags |= RULEFLAG_NOESCAPEBACKREF;
+ cfg->noescapes = val;
}
else if (!strcasecmp(key, "NP") || !strcasecmp(key, "ackrefernoplus")) {
cfg->flags |= RULEFLAG_ESCAPENOPLUS;
@@ -3854,7 +3862,6 @@ static const char *cmd_rewriterule(cmd_parms *cmd,
err, a1, a2, a3);
}
- /* arg3: optional flags field */
newrule->forced_mimetype = NULL;
newrule->forced_handler = NULL;
newrule->forced_responsecode = HTTP_MOVED_TEMPORARILY;
@@ -3863,6 +3870,9 @@ static const char *cmd_rewriterule(cmd_parms *cmd,
newrule->cookie = NULL;
newrule->skip = 0;
newrule->maxrounds = REWRITE_MAX_ROUNDS;
+ newrule->escapes = newrule->noescapes = NULL;
+
+ /* arg3: optional flags field */
if (a3 != NULL) {
if ((err = cmd_parseflagfield(cmd->pool, newrule, a3,
cmd_rewriterule_setflag)) != NULL) {
Index: framework/t/conf/extra.conf.in
===================================================================
--- framework/t/conf/extra.conf.in (revision 1908352)
+++ framework/t/conf/extra.conf.in (working copy)
@@ -272,8 +272,9 @@
RewriteRule ^/modules/rewrite/escaping/local/(.*) /?$1
RewriteRule ^/modules/rewrite/escaping/local_b/(.*) /?$1 [B]
RewriteRule ^/modules/rewrite/escaping/local_bctls/(.*) /?$1 [BCTLS]
+ RewriteRule ^/modules/rewrite/escaping/local_bctls_nospace/(.*) /?$1 "[BCTLS,BNE= ?]"
RewriteRule ^/modules/rewrite/escaping/local_bctls_andslash/(.*) /?$1 [B=/,BCTLS]
- RewriteRule ^/modules/rewrite/escaping/local_b_noslash/(.*) /?$1 [B=/,BNEG]
+ RewriteRule ^/modules/rewrite/escaping/local_b_noslash/(.*) /?$1 [B,BNE=/]
RewriteRule ^/modules/rewrite/escaping/local_b_justslash/(.*) /?$1 [B=/]
RewriteRule ^/modules/rewrite/escaping/redir/(.*) http://@SERVERNAME@:@PORT@/?$1 [R]
RewriteRule ^/modules/rewrite/escaping/redir_ne/(.*) http://@SERVERNAME@:@PORT@/?$1 [R,NE]
Index: framework/t/modules/rewrite.t
===================================================================
--- framework/t/modules/rewrite.t (revision 1908352)
+++ framework/t/modules/rewrite.t (working copy)
@@ -38,6 +38,7 @@ my @bflags = (
# t/conf/extra.conf.in
[ "/modules/rewrite/escaping/local_b/foo/bar/%20baz%0d" => "foo%2fbar%2f+baz%0d"], # this is why [B] sucks
[ "/modules/rewrite/escaping/local_bctls/foo/bar/%20baz/%0d" => "foo/bar/+baz/%0d"], # spaces and ctls only
+ [ "/modules/rewrite/escaping/local_bctls_nospace/foo/bar/%20baz/%0d" => "foo/bar/ baz/%0d"], # ctls but no space
[ "/modules/rewrite/escaping/local_bctls_andslash/foo/bar/%20baz/%0d" => "foo%2fbar%2f+baz%2f%0d"], # not realistic, but opt in to slashes
[ "/modules/rewrite/escaping/local_b_noslash/foo/bar/%20baz/%0d" => "foo/bar/+baz/%0d"], # negate something from [B]
[ "/modules/rewrite/escaping/local_b_justslash/foo/bar/%20baz/" => "foo%2fbar%2f baz%2f"], # test basic B=/
