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

[Christian Grothoff]

Thanks for the test.

I've converted it to proper C, added it to Git, and fixed the bug it
demonstrated in 34aa038d..d7709c55.

Happy hacking!

Christian

On 6/3/20 7:45 PM, Markus Doppelbauer wrote:
> I think this patch was wrong.
> The post-processor fails to parse url-encoded post-data.
> A testcase is attached: The value of yyyy should be yyyy.
> 
> Best wishes
> Markus
> 
> 
> -------- Weitergeleitete Nachricht --------
> *Von*: Christian Grothoff <grothoff@gnunet.org
> <mailto:Christian%20Grothoff%20%3cgrothoff@gnunet.org%3e>>
> *Antwort an*: libmicrohttpd development and user mailinglist
> <libmicrohttpd@gnu.org
> <mailto:libmicrohttpd%20development%20and%20user%20mailinglist%20%3clibmicrohttpd@gnu.org%3e>>
> *An*: libmicrohttpd@gnu.org <mailto:libmicrohttpd@gnu.org>
> *Betreff*: Re: [libmicrohttpd] www-form-urlencoded: dealing with keys
> with no value
> *Datum*: Sat, 28 Dec 2019 00:52:16 +0100
> 
> Hi Ethan,
> 
> 
> I've implemented this now in Git master. Feedback welcome...
> 
> 
> Happy hacking!
> 
> 
> Christian
> 
> 
> On 12/25/19 9:45 AM, Ethan Tuttle wrote:
>> 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 <
>> grothoff@gnunet.org
>>  <mailto:grothoff@gnunet.org>
>> > 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 <
>>>> ethan@ethantuttle.com
>>>>  <mailto:ethan@ethantuttle.com>
>>>> >
>>>> 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)
>>>
>>>
>>
>

signature.asc
Description: OpenPGP digital signature