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

[Markus Doppelbauer]

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>

Antwort an: libmicrohttpd development and user mailinglist <libmicrohttpd@gnu.org>

An: 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

> 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

>

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)

testcase.cpp
Description: Text Data