Urldecode issue

[IgorYbema]

I am trying to figure out why you send lenght of buffer + 1 (pos+1) towards urldecode. I discovered an issue with this and try to understand it.

The issue is when you send a urlencoded string (like test=123%20456) it will result into overflowing of the buffer as the for loop in urlencode will run until pos+1. In a non-encoded string it will stop due to the other check (j < dst_len - 1).

https://github.com/CurlyMoo/webserver/blame/494bffd1350a1de6141b3442634f3de65512f369/webserver.cpp#L172

[CurlyMoo]

Could be a good catch. Now you've mentioned it, i see i don't have a unittest that tests for HTTP GET arguments. I think that would be the first place to start. Could you add a test for that which should prove that you're right?

[CurlyMoo]

I sadly don't have time to dive into it.

[IgorYbema]

I'll try to add it

[IgorYbema]

--- a/main.cpp
+++ b/main.cpp
@@ -134,7 +134,7 @@ struct unittest_t {
     },
     1,
     {
-      { 0, "28813f8d332001c9", "adfasdfffffffffffaadfasdfffffffffffaadfasdfffffffffffaadfasdfffffffffffaadfasdfffffffffffaadfasdffad123" }
+      { 0, "28813f8d332001c9", "ffadfasdfffffffff%20faadfasdfffffffffffaadfasdfffffffffffaadfasdfffffffffffaadfasdfffffffffffaadfasdffad123" }
     }
   },
   {

This small addition to the 2nd unittest will cause it to fail

[IgorYbema]

Tried to make a fix but every, for me logicial change, breaks other tests so clearly I am not able to do so. The webserver.cpp code is a bit hard to read and understand :-)

[CurlyMoo]

Long live unittesting 👍

[IgorYbema]

I am almost sure it is test#14 which is wrong in itself and not my code fixes. But I can't figure out why test#14 is bad. All other tests run fine.

The Content-Length of test#14 seems off but when I change it to the correct size it fails later in the test (even without the fix in the urldecode part).

[CurlyMoo]

Is that the rules one with 39717 bytes?

The value part which contains the GPL license text is 39711 bytes, rules is 5 bytes and then we have the = which is 1 byte. That makes the expected 39717 bytes.

[IgorYbema]

I see now. Then there is something else wrong causing the modified test test#2 (see patch above) going wrong. I can't find it :(

[CurlyMoo]

Fixed the bug and added the unittest. Thanks!

[IgorYbema]

Great find. Took me ages to understand the code let alone to fix the issue :)

[CurlyMoo]

After commenting the conditions in the webserver_parse_post function, it became clear that only this part is used in the previously failing unittest: https://github.com/CurlyMoo/webserver/blob/main/webserver.cpp#L330-L414

Then i just starting comparing the content of the variables line by line in that block to see where they started to differ.

[IgorYbema]

But urldecode is used in other blocks also. Shouldn't those be changed also?

[IgorYbema]

And are you merging this fix into heishamon? Or do you want me to do that?

[CurlyMoo]

But urldecode is used in other blocks also. Shouldn't those be changed also?

Good question. I was looking at that as well yesterday. As you can see, the other places already take pos3 > -1 into account. At least, that's what i think until you or someones else comes up with a unittest that proves me wrong.

And are you merging this fix into heishamon? Or do you want me to do that?

Can you?

[IgorYbema]

This single commit could be added directly but better to add it to the websocket PR I believe

[CurlyMoo]

I would opt for the latter.

[IgorYbema]

Feel free to do so