Re: [libmicrohttpd] www-form-urlencoded: dealing with keys with no value

[Ethan Tuttle]

Thanks for the response, Chrstian.  I'll take a look at the parsing
code, but I don't have high confidence I can fix it to your liking
(and in a secure / performant way).

Otherwise, I'll stick with my uriparser patch until you get around to it.

Happy hacking and happy holidays!

Ethan

On Tue, Dec 24, 2019 at 6:30 AM Christian Grothoff <address@hidden> wrote:
>
> Hi Ethan,
>
> I agree that this is a bug, and yes, we should fix it. Thanks for the
> test case, that is very helpful.
>
> I don't know when I can work on it, but worst-case expect a patch by
> the end of January, if you're lucky and my other bugs go fast (or
> someone else sends a fix), might of course happen earlier.
>
> Happy hacking!
>
> Christian
>
> On Tue, 2019-12-24 at 04:11 -0800, Ethan Tuttle wrote:
> > Given post body "a&b=1", how should MHD interpret the data?
> >
> > I'm at the end of a long investigation and it's come down to that
> > question
> > for post_process_urlencoded() in MHD.
> >
> > MHD currently calls back with ("a&b", "1").  The app I'm working
> > breaks
> > when it doesn't receive a callback for "b".  The http client in this
> > case
> > (that did the urlencoding) is google-http-java-client 1.23.0.
> >
> > The client behavior may be questionable, but MHD's behavior doesn't
> > seem
> > right either.  Isn't there some principle, "clients should strive for
> > conformance, servers should strive for forgiveness".
> >
> > I've attached a patch to MHD to add a failing test, but without a
> > fix.  Is
> > MHD open to changing this behavior, given its maturity?
> >
> > Should I post a bug to the bug tracker?
> >
> > As for relevant standards, the W3C[1] is not detailed enough to cover
> > it.
> > The WhatWG[2] is more specific and allows for empty values and even
> > empty
> > keys.  I'd like to callout uriparser[3], another C library I've
> > patched in
> > as a work-around.  Uriparser documents their handling of these cases
> > well:
> >
> > * NULL in the value member means there was no '=' in the item text as
> > with
> > "?abc&def".
> > * An empty string in the value member means there was '=' in the item
> > as
> > with "?abc=&def".
> >
> > [1] https://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.1
> > [2] https://url.spec.whatwg.org/#urlencoded-parsing
> > [3] https://uriparser.github.io/doc/api/latest/#querystrings
> >
> > commit aa0534af56d135e1b261d127af09c22015c1ff87
> > Author: Ethan Tuttle <address@hidden>
> > Date:   Tue Dec 24 03:50:59 2019 -0800
> >
> >     urlencoding post-processor: add failing tests for keys without
> > values
> >
> > diff --git a/src/microhttpd/test_postprocessor.c
> > b/src/microhttpd/test_postprocessor.c
> > index 75b5ba33..6d5be040 100644
> > --- a/src/microhttpd/test_postprocessor.c
> > +++ b/src/microhttpd/test_postprocessor.c
> > @@ -42,8 +42,18 @@
> >   * five NULL-entries.
> >   */
> >  const char *want[] = {
> > +#define URL_NOVALUE1_DATA "abc&x=5"
> > +#define URL_NOVALUE1_START 0
> > +        "abc", NULL, NULL, NULL, NULL,
> > +        "x", NULL, NULL, NULL, "5",
> > +#define URL_NOVALUE1_END (URL_NOVALUE1_START + 10)
> > +#define URL_NOVALUE2_DATA "abc=&x=5"
> > +#define URL_NOVALUE2_START URL_NOVALUE1_END
> > +        "abc", NULL, NULL, NULL, "",
> > +        "x", NULL, NULL, NULL, "5",
> > +#define URL_NOVALUE2_END (URL_NOVALUE2_START + 10)
> >  #define URL_DATA "abc=def&x=5"
> > -#define URL_START 0
> > +#define URL_START URL_NOVALUE2_END
> >    "abc", NULL, NULL, NULL, "def",
> >    "x", NULL, NULL, NULL, "5",
> >  #define URL_END (URL_START + 10)
> > @@ -125,12 +135,14 @@ value_checker (void *cls,
> >
> >
> >  static int
> > -test_urlencoding (void)
> > +test_urlencoding_case (unsigned int want_start,
> > +                       unsigned int want_end,
> > +                       const char *url_data)
> >  {
> >    struct MHD_Connection connection;
> >    struct MHD_HTTP_Header header;
> >    struct MHD_PostProcessor *pp;
> > -  unsigned int want_off = URL_START;
> > +  unsigned int want_off = want_start;
> >    size_t i;
> >    size_t delta;
> >    size_t size;
> > @@ -147,19 +159,27 @@ test_urlencoding (void)
> >    pp = MHD_create_post_processor (&connection,
> >                                    1024, &value_checker, &want_off);
> >    i = 0;
> > -  size = strlen (URL_DATA);
> > +  size = strlen (url_data);
> >    while (i < size)
> >    {
> >      delta = 1 + MHD_random_ () % (size - i);
> > -    MHD_post_process (pp, &URL_DATA[i], delta);
> > +    MHD_post_process (pp, &url_data[i], delta);
> >      i += delta;
> >    }
> >    MHD_destroy_post_processor (pp);
> > -  if (want_off != URL_END)
> > +  if (want_off != want_end)
> >      return 1;
> >    return 0;
> >  }
> >
> > +static int
> > +test_urlencoding (void) {
> > +  unsigned int errorCount = 0;
> > +  errorCount += test_urlencoding_case(URL_START, URL_END, URL_DATA);
> > +  errorCount += test_urlencoding_case(URL_NOVALUE1_START,
> > URL_NOVALUE1_END, URL_NOVALUE1_DATA);
> > +  errorCount += test_urlencoding_case(URL_NOVALUE2_START,
> > URL_NOVALUE2_END, URL_NOVALUE2_DATA);
> > +  return errorCount;
> > +}
> >
> >  static int
> >  test_multipart_garbage (void)
>
>