On Thu, Oct 14, 2021 at 07:48:08PM +0200, Tim Duesterhus wrote:
> Remi,
>
> please find a suggested cleanup for your JWT patch series. I think that
> using the ist functions results in easier to understand code, because you
> don't need to manually calculate lengths and offsets.
>
> Apply with `git am --scissors` to automatically cut the commit message.
>
> -- >8 --
> Using the ist helper functions is arguably cleaner than using C's regular
> string functions on an ist.
It's indeed more readable but I must confess I'm now confused by both
versions regarding what they're trying to do:
> ctx.blk = NULL;
> if (http_find_header(htx, hdr_name, &ctx, 0)) {
> - char *space = NULL;
> - space = memchr(ctx.value.ptr, ' ', ctx.value.len);
> - if (space && strncasecmp("Bearer", ctx.value.ptr,
> ctx.value.len) == 0) {
> - chunk_initlen(&bearer_val, space+1, 0, ctx.value.len -
> (space - ctx.value.ptr) - 1);
I don't see how this can ever match:
- we search for a space in the <len> first characters starting at <ptr>
- if we find one such space, we check if these characters are exactly
equal to the string "Bearer" (modulo the case), and if so we take the
value.
=> by definition this cannot match since there is no space in "Bearer"
It made me doubt about strncasecmp() to the point that I had to write some
code to verify I wasn't mistaken about how it worked.
Rémi, am I missing something or is it just that this code snippet indeed
has a bug that was not spotted by the regtests (which I'm fine with,
they're regression tests, not unit tests seeking 100% coverage) ?
> + struct ist type = istsplit(&ctx.value, ' ');
> +
> + if (isteqi(type, ist("Bearer")) && istend(type) !=
> istend(ctx.value)) {
> + chunk_initlen(&bearer_val, istptr(ctx.value), 0,
> istlen(ctx.value));
Here I would find it more understandable to test like this:
if (isteqi(type, ist("Bearer")) && istlen(ctx.value)) {
Also, I had a check to RFC2617 and there's *at least* one space after
the auth scheme, so we need to skip all of them. For me that would
give this:
struct ist type = istsplit(&ctx.value, ' ');
ctx = istskip(ctx, ' ');
if (isteqi(type, ist("Bearer")) && istlen(ctx.value))
chunk_initlen(&bearer_val, istptr(ctx.value), 0,
istlen(ctx.value));
But I'd like someone to double-check all this, and possibly submit a
fix if anything needs to be fixed (at least the multiple spaces case
seems to require one).
Thanks,
Willy
