ESP8266WebServer url decoding is performed early on x-www-form-urlencoded buffer

[innodron]

Description

Any text input field containing '&' (encoded %26) is wrongly parsed as further arguments because parsing.cpp is performing urlDecode() on the entire buffer for "application/x-www-form-urlencoded" type header. Instead, url decoding should be executed on the parsed arguments themselves in _parseArguments().
(Indeed 2.4.0-rc.1 _parseArguments() is already performing urlDecode() on the arguments parsed)

ESP8266WebServer/src/Parsing.cpp
185:      if (contentLength > 0) {
186:        if (searchStr != "") searchStr += '&';
187:        if(isEncoded){
188:          //url encoded form
189:          String decoded = urlDecode(plainBuf); <-- Cause of error

hence 
plainBuf: arg0=arg0_value&arg1=arg1_value&arg2=inner_arg0%3Dinner_arg0_value%26inner_arg1%3Dinner_arg1_value%26inner_arg2%3Dinner_arg2_value
becomes
decoded : arg0=arg0_value&arg1=arg1_value&arg2=inner_arg0=inner_arg0_value&inner_arg1=inner_arg1_value&inner_arg2=inner_arg2_value

which then parsed in _parseArguments() as:

arg0       : arg0_value
arg1       : arg1_value
arg2       : inner_arg0=inner_arg0_value
inner_arg1 : inner_arg1_value
inner_arg2 : inner_arg2_value

though it should have been parsed to:

arg0       : arg0_value
arg1       : arg1_value
arg2       : inner_arg0=inner_arg0_value&inner_arg1=inner_arg1_value&inner_arg2=inner_arg2_value

I fixed the issue temporarily as following:

185:      if (contentLength > 0) {
186:        if (searchStr != "") searchStr += '&';
187:        //if(isEncoded){
188:          //url encoded form
189:        //  String decoded = urlDecode(plainBuf); --> plainBuf should not be decoded. Parameters should be decoded instead
190:        //  size_t decodedLen = decoded.length();
191:        //  memcpy(plainBuf, decoded.c_str(), decodedLen);
192:        //  plainBuf[decodedLen] = 0;
193:          searchStr += plainBuf;
194:        //}

though it may need a finer resolution by the original author(s)

Hardware

Hardware: Sparkfun ESP8266 Thing Developer
Core Version: 2.4.0-rc.1

Settings in IDE

Module: Sparkfun ESP8266 Thing Developer
Flash Size: 512
CPU Frequency: 80Mhz
Flash Mode: qio
Flash Frequency: 40Mhz
Upload Using: SERIAL
Reset Method: nodemcu

[devyte]

@innodron does #3313 fix your issue?

[innodron]

I checked #3313 and it seems like the bug I mentioned is addressed here. However, I am not quite sure how to download the version containing #3313 fix from GitHub so I can test it.

[devyte]

@innodron just google github how to test pr locally

[innodron]

• I did a through comparison of 3313 commit of parsing.cpp, and relevant other issues/commits ESP8266WebServer does not handle special characters in JSON body #2198, Url decode fixes #2212, Fix missing separator between get and post arguments #2222, Fix plain post #2241, Fix: urlDecode done before parsing args #2956, incorrect bug fix. ESP8266 url encoded delimitters parsed incorrectly #3415, Fix to parse parameters in the URL of a POST with empty content. #3398 to the latest master.
• Then merged the suggested parsing.cpp changes in 3313 to the latest master, except:

324c316 
<     arg.key = data.substring(pos, equal_sign_index);
---
>     arg.key = urlDecode(data.substring(pos, equal_sign_index));

since I cannot see the reason why key would need decoding

• Test result: fixes the issue I reported.

• Here is the diff for parsing.cpp from master to tested version

187d186
<         if (searchStr != "") searchStr += '&';
190,193c189
<           String decoded = urlDecode(plainBuf);
<           size_t decodedLen = decoded.length();
<           memcpy(plainBuf, decoded.c_str(), decodedLen);
<           plainBuf[decodedLen] = 0;
---
>           if (searchStr != "") searchStr += '&';

@devyte Should I create a PR?

[devyte]

@igrr There seems to be a thorough analysis here. Do we want a new PR for this, or add these changes to an existing PR? If the latter, to which one?

[devyte]

I just merged #3313 , after confirmation from another user.
@innodron I understand that your fix was slightly different. If you think something should be changed, please make a PR, and I'll take a look.
Closing this in the meantime.