Le 08/04/2021 à 20:59, Tim Duesterhus a écrit :
Willy,
Christopher,
this is in prepatation for the next normalizer which normalizes character case
of the percent encoding.
The resources I found are not clear on whether a percent that is not followed
by two hex digits is valid or not. Most browsers and servers appear to support
it, so I opted to leave the user a choice whether to reject it or not.
It is probably a good choice
To support this I need to differ between "normalizing failed for internal
reasons" and "normalizing failed, because the user sent garbage".
Overall I am not happy with this patch, because the control flow is ugly. I
also wasn't sure in which cases I need to increase server counters (e.g.
failed_rewrites) or not. I'd be happy if you could give some advice!
The function may be simplified. I'll propose you a new version, based on my
other comments.
[...]
+enum uri_normalizer_err {
+ URI_NORMALIZER_ERR_NONE = 0,
+ URI_NORMALIZER_ERR_TRASH,
+ URI_NORMALIZER_ERR_ALLOC,
+ URI_NORMALIZER_ERR_INVALID_INPUT,
+ URI_NORMALIZER_ERR_BUG = 0xdead,
+};
URI_NORMALIZER_ERR_BUG value is useless. And I guess it could be useful to have
a rewrite error too.
[...]
fail_rewrite:
_HA_ATOMIC_ADD(&sess->fe->fe_counters.failed_rewrites, 1);
if (s->flags & SF_BE_ASSIGNED)
@@ -300,6 +301,24 @@ static enum act_return http_action_normalize_uri(struct
act_rule *rule, struct p
s->flags |= SF_ERR_PRXCOND;
}
goto leave;
+
+ err:
+ switch (err) {
+ case URI_NORMALIZER_ERR_NONE:
You must break here, otherwise, an error is returned on success.
+ case URI_NORMALIZER_ERR_BUG:
+ case URI_NORMALIZER_ERR_TRASH:
+ /* These must not happen. */
+ BUG_ON("Logic error during URI normalization.");
+ ret = ACT_RET_ERR;
+ goto leave;
+ case URI_NORMALIZER_ERR_ALLOC:
+ goto fail_alloc;
+ case URI_NORMALIZER_ERR_INVALID_INPUT:
+ ret = ACT_RET_INV;
+ goto leave;
+ }
+
+ my_unreachable();
}
You may rewrite http_action_normalize_uri() this way (on top of your series,
normalizers must be adapted). It is not untested nor compiled.
static enum act_return http_action_normalize_uri(struct act_rule *rule, struct
proxy *px,
struct session *sess, struct
stream *s, int flags)
{
enum act_return ret = ACT_RET_CONT;
struct htx *htx = htxbuf(&s->req.buf);
const struct ist uri = htx_sl_req_uri(http_get_stline(htx));
const struct ist path = http_get_path(uri);
struct ist dst;
struct buffer *replace = alloc_trash_chunk();
enum uri_normalizer_err err;
if (!replace)
goto fail_alloc;
if (!isttest(path))
goto leave;
dst = ist2(replace->area, replace->size);
switch ((enum act_normalize_uri) rule->action) {
case ACT_NORMALIZE_URI_MERGE_SLASHES:
err = uri_normalizer_path_merge_slashes(iststop(path, '?'),
&dst);
if (err != URI_NORMALIZER_ERR_NONE)
break;
if (!http_replace_req_path(htx, dst, 0))
goto fail_rewrite;
break;
case ACT_NORMALIZE_URI_DOTDOT:
case ACT_NORMALIZE_URI_DOTDOT_FULL:
err = uri_normalizer_path_dotdot(iststop(path, '?'), &dst,
rule->action == ACT_NORMALIZE_URI_DOTDOT_FULL);
if (err != URI_NORMALIZER_ERR_NONE)
break;
if (!http_replace_req_path(htx, dst, 0))
goto fail_rewrite;
break;
case ACT_NORMALIZE_URI_SORT_QUERY:
err = uri_normalizer_query_sort(istfind(path, '?'), &dst, '&');
if (err != URI_NORMALIZER_ERR_NONE)
break;
if (!http_replace_req_query(htx, dst))
goto fail_rewrite;
break;
case ACT_NORMALIZE_URI_PERCENT_UPPER:
case ACT_NORMALIZE_URI_PERCENT_UPPER_STRICT:
err = uri_normalizer_percent_upper(path, &dst, rule->action ==
ACT_NORMALIZE_URI_PERCENT_UPPER_STRICT);
if (err != URI_NORMALIZER_ERR_NONE)
break;
if (!http_replace_req_path(htx, dst, 1))
goto fail_rewrite;
break;
}
switch (err) {
case URI_NORMALIZER_ERR_NONE:
break;
case URI_NORMALIZER_ERR_BUG:
case URI_NORMALIZER_ERR_TRASH:
/* These must not happen. */
BUG_ON("Logic error during URI normalization.");
ret = ACT_RET_ERR;
break;
case URI_NORMALIZER_ERR_ALLOC:
goto fail_alloc;
case URI_NORMALIZER_ERR_INVALID_INPUT:
ret = ACT_RET_INV;
goto leave;
}
leave:
free_trash_chunk(replace);
return ret;
fail_alloc:
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_RESOURCE;
ret = ACT_RET_ERR;
goto leave;
fail_rewrite:
_HA_ATOMIC_ADD(&sess->fe->fe_counters.failed_rewrites, 1);
if (s->flags & SF_BE_ASSIGNED)
_HA_ATOMIC_ADD(&s->be->be_counters.failed_rewrites, 1);
if (sess->listener && sess->listener->counters)
_HA_ATOMIC_ADD(&sess->listener->counters->failed_rewrites, 1);
if (objt_server(s->target))
_HA_ATOMIC_ADD(&__objt_server(s->target)->counters.failed_rewrites, 1);
if (!(s->txn->req.flags & HTTP_MSGF_SOFT_RW)) {
ret = ACT_RET_ERR;
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_PRXCOND;
}
goto leave;
}
--
Christopher Faulet
