Skip to content

Webserver Parsing Issue #5261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
6 tasks done
ascillato opened this issue Oct 19, 2018 · 26 comments
Closed
6 tasks done

Webserver Parsing Issue #5261

ascillato opened this issue Oct 19, 2018 · 26 comments
Assignees

Comments

@ascillato
Copy link
Contributor

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it. No stack dump
  • I have filled out all fields below.

Platform

  • Hardware: [ESP-12]
  • Core Version: [18/oct/2018 and 6/oct/2018]
  • Development Env: [Arduino IDE]
  • Operating System: [Windows]

Settings in IDE

  • Module: [Generic ESP8266 Module]
  • Flash Mode: [dout]
  • Flash Size: [4MB]
  • lwip Variant: [v2 Higher Bandwidth]
  • Reset Method: [nodemcu]
  • Flash Frequency: [40Mhz]
  • CPU Frequency: [80Mhz]
  • Upload Using: [SERIAL]
  • Upload Speed: [115200] (serial upload only)

Problem Description

Detailed problem description goes here.

In the last core (18-oct-2018) several changes were introduced to solve the parsing issue for Alexa and others, (Thanks a lot for that 👍, very appreciated) but a new problem has been introduced.

Tasmota uses Javascript in the Timers Config Webpage to allow the user to select times, days, etc. If using the core from 6-oct-2018, this Config Webpage is working fine, but using the last stage core, it does not work. The menu is being shown by the web browser and you can interact with it, but the parameters are not being saved, so are not being passed to Tasmota.

Debug has been enabled for both cores and also the Tasmota debug for the files attached. You can see that the amount of parameters were taking into account in the old core but in the new core are being ignored and passed as a plain text.

For the test in both core, the same sequence was done in the Tasmota webmenu.

In the last core, when hitting save, the page get reloaded and started over because no parameter is sent to Tasmota.

MCVE Sketch

https://github.com/arendst/Sonoff-Tasmota

Debug Messages

Attached are the full debugs from core stage 6 oct 2018 (works fine) and core stage 18 oct 2018 (does not work).

6oct core - working.txt

last core - dont work.txt

@earlephilhower
Copy link
Collaborator

Just looking at the source in Parsing.cpp, I see that in the old version if the type was www-form-urlencoded the body was appended to the search string (by setting flag isEncoded). That variable is now gone, and the body is never put into the searchString passed into _parse(). So that explains why it's all coming back as "plain" and not args.

Why that change was done will need @d-a-v . Possibly related to Alexa sending illegal HTTP and us trying to accept it...

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 19, 2018

This time http request is displayed in debug mode so we can see it in logs.
Of course Alexa should be tested as well, hoping to not break something else.

@ascillato
Copy link
Contributor Author

ascillato commented Oct 19, 2018

@d-a-v

Your PR works amazing. Now parsing has been solved! Thanks a lot 👍

Now, we need to test Alexa.

@Jason2866, @reloxx13 Please, can you test Alexa again but with @d-a-v's PR? Thanks.

@Jason2866
Copy link
Contributor

@d-a-v Just tried latest commit. Alexa isnt working anymore with this :-(

@earlephilhower
Copy link
Collaborator

@Jason2866 Can you attach a debug log for @d-a-v on a failing connection? There's obviously something very subtle going on here.

@Jason2866
Copy link
Contributor

New client
17:33:34.533 -> request: GET /ay HTTP/1.1
17:33:34.533 -> method: GET url: /ay search: 
17:33:34.533 -> headerName: Host
17:33:34.533 -> headerValue: 192.168.2.149
17:33:34.533 -> headerName: Connection
17:33:34.533 -> headerValue: keep-alive
headerName: User-Agent
17:33:34.568 -> headerValue: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36
17:33:34.568 -> headerName: Accept
17:33:34.568 -> headerValue: */*
17:33:34.568 -> headerName: Referer
17:33:34.568 -> headerValue: http://192.168.2.149/
17:33:34.568 -> headerName: Accept-Encoding
17:33:34.568 -> headerValue: gzip, deflate
17:33:34.568 -> headerName: Accept-Language
17:33:34.568 -> headerValue: de-DE,de;q=0.9,en-US;q=0.8,en;q=0.7
17:33:34.568 -> args: 
17:33:34.568 -> args count: 0
17:33:34.568 -> args: 
args count: 0
17:33:34.603 -> Request: /ay
17:33:34.603 -> Arguments: 
17:33:34.603 -> final list of key/value pairs:
New client
17:33:35.121 -> request: PUT /api/vr0cFpv41CwnP3PpmyLwZaX0BtBARCMSNr6fkZc9/lights/1/state HTTP/1.1
17:33:35.121 -> method: PUT url: /api/vr0cFpv41CwnP3PpmyLwZaX0BtBARCMSNr6fkZc9/lights/1/state search: 
17:33:35.155 -> headerName: Host
17:33:35.155 -> headerValue: 192.168.2.149
17:33:35.155 -> headerName: Accept
17:33:35.155 -> headerValue: */*
17:33:35.155 -> headerName: Content-type
17:33:35.155 -> headerValue: application/x-www-form-urlencoded
17:33:35.155 -> headerName: Content-Length
17:33:35.190 -> headerValue: 12
17:33:35.190 -> args: {"on": true}
17:33:35.190 -> args count: 1
17:33:35.190 -> args: {"on": true}
17:33:35.190 -> args count: 1
17:33:35.190 -> Request: /api/vr0cFpv41CwnP3PpmyLwZaX0BtBARCMSNr6fkZc9/lights/1/state
17:33:35.190 -> Arguments: {"on": true}
17:33:35.190 -> final list of key/value pairs:
17:33:35.190 ->   key:'{"on": true}' value:''
17:33:35.190 ->   key:'plain' value:'{"on": true}'
17:33:35.190 -> request handler not found
New client
17:33:37.506 -> request: GET /ay HTTP/1.1
17:33:37.541 -> method: GET url: /ay search: 
17:33:37.541 -> headerName: Host
17:33:37.541 -> headerValue: 192.168.2.149
17:33:37.541 -> headerName: Connection
17:33:37.541 -> headerValue: keep-alive
17:33:37.541 -> headerName: User-Agent
17:33:37.541 -> headerValue: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36
17:33:37.575 -> headerName: Accept
17:33:37.575 -> headerValue: */*
17:33:37.575 -> headerName: Referer
17:33:37.575 -> headerValue: http://192.168.2.149/
17:33:37.575 -> headerName: Accept-Encoding
17:33:37.575 -> headerValue: gzip, deflate
17:33:37.575 -> headerName: Accept-Language
17:33:37.575 -> headerValue: de-DE,de;q=0.9,en-US;q=0.8,en;q=0.7
17:33:37.575 -> args: 
17:33:37.575 -> args count: 0
17:33:37.575 -> args: 
17:33:37.575 -> args count: 0
17:33:37.575 -> Request: /ay
17:33:37.575 -> Arguments: 
17:33:37.575 -> final list of key/value pairs:
New client

@Jason2866
Copy link
Contributor

New client
17:35:38.508 -> request: GET /ay HTTP/1.1
method: GET url: /ay search: 
17:35:38.543 -> headerName: Host
17:35:38.543 -> headerValue: 192.168.2.149
17:35:38.543 -> headerName: Connection
17:35:38.543 -> headerValue: keep-alive
17:35:38.543 -> headerName: User-Agent
17:35:38.543 -> headerValue: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36
17:35:38.578 -> headerName: Accept
17:35:38.578 -> headerValue: */*
17:35:38.578 -> headerName: Referer
17:35:38.578 -> headerValue: http://192.168.2.149/
17:35:38.578 -> headerName: Accept-Encoding
17:35:38.578 -> headerValue: gzip, deflate
17:35:38.578 -> headerName: Accept-Language
17:35:38.578 -> headerValue: de-DE,de;q=0.9,en-US;q=0.8,en;q=0.7
17:35:38.578 -> args: 
17:35:38.578 -> args count: 0
17:35:38.578 -> args: 
17:35:38.578 -> args count: 0
17:35:38.578 -> Request: /ay
17:35:38.578 -> Arguments: 
17:35:38.578 -> final list of key/value pairs:
New client
17:35:39.195 -> request: PUT /api/vr0cFpv41CwnP3PpmyLwZaX0BtBARCMSNr6fkZc9/lights/1/state HTTP/1.1
17:35:39.195 -> method: PUT url: /api/vr0cFpv41CwnP3PpmyLwZaX0BtBARCMSNr6fkZc9/lights/1/state search: 
17:35:39.228 -> headerName: Host
17:35:39.228 -> headerValue: 192.168.2.149
17:35:39.228 -> headerName: Accept
17:35:39.228 -> headerValue: */*
17:35:39.228 -> headerName: Content-type
17:35:39.228 -> headerValue: application/x-www-form-urlencoded
17:35:39.262 -> headerName: Content-Length
17:35:39.262 -> headerValue: 12
17:35:39.262 -> args: {"on": true}
17:35:39.262 -> args count: 1
17:35:39.262 -> args: {"on": true}
17:35:39.262 -> args count: 1
17:35:39.262 -> Request: /api/vr0cFpv41CwnP3PpmyLwZaX0BtBARCMSNr6fkZc9/lights/1/state
17:35:39.262 -> Arguments: {"on": true}
17:35:39.262 -> final list of key/value pairs:
17:35:39.262 ->   key:'{"on": true}' value:''
17:35:39.262 ->   key:'plain' value:'{"on": true}'
17:35:39.262 -> request handler not found
New client
17:35:41.561 -> request: GET /ay HTTP/1.1
17:35:41.561 -> method: GET url: /ay search: 
17:35:41.561 -> headerName: Host
17:35:41.561 -> headerValue: 192.168.2.149
17:35:41.561 -> headerName: Connection
17:35:41.561 -> headerValue: keep-alive
17:35:41.561 -> headerName: User-Agent
17:35:41.561 -> headerValue: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36
17:35:41.594 -> headerName: Accept
17:35:41.594 -> headerValue: */*
17:35:41.594 -> headerName: Referer
17:35:41.594 -> headerValue: http://192.168.2.149/
17:35:41.594 -> headerName: Accept-Encoding
17:35:41.594 -> headerValue: gzip, deflate
17:35:41.594 -> headerName: Accept-Language
17:35:41.594 -> headerValue: de-DE,de;q=0.9,en-US;q=0.8,en;q=0.7
17:35:41.594 -> args: 
17:35:41.594 -> args count: 0
17:35:41.594 -> args: 
17:35:41.594 -> args count: 0
17:35:41.594 -> Request: /ay
17:35:41.594 -> Arguments: 
17:35:41.628 -> final list of key/value pairs:

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 19, 2018

Can Alexa use the second key ?
'plain' was before on the first

@Jason2866
Copy link
Contributor

@d-a-v
There are no options for Alexa. Just cover device in Alexa app. Nothing more.
Or did i understand you false and you need a different log file?

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 19, 2018

I was mentioning the Alexa code/library using core's WebServer, is it using only the first resulted key/value pair ?
Because your logs show 'plain' which is what was expected by the other Alexa-fixing PR.

@reloxx13
Copy link

reloxx13 commented Oct 19, 2018

its this line i think:
https://github.com/arendst/Sonoff-Tasmota/blob/a5c5ddaee6ada07937db3ae75ef3f4c4023c21f6/sonoff/xplg_wemohue.ino#L666-L668

if (1 == WebServer->args()) {
      response = "[";

      StaticJsonBuffer<400> jsonBuffer;
      JsonObject &hue_json = jsonBuffer.parseObject(WebServer->arg(0));
      if (hue_json.containsKey("on")) {

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 19, 2018

Thanks.
We can try and swap results.
We could give a map.
You could loop through the result array.
dilemma :-)

@reloxx13
Copy link

reloxx13 commented Oct 19, 2018

im an right that WebServer->args() are 2 now and not 1 ?
you are filling the args twice in the args array?
17:33:35.190 -> final list of key/value pairs:
17:33:35.190 -> key:'{"on": true}' value:''
17:33:35.190 -> key:'plain' value:'{"on": true}'

that way the "if args() == 1" wont match ?


WAS (my old workign log):
final list of key/value pairs:
key:'plain' value:'{"on": true}'

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 19, 2018

That's the purpose of this PR serving old behaviour and Alexa workaround too.
I guess your previous own fix did the same thing and probably gave you the same result modulo swapped arguments.

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 19, 2018

@ascillato @reloxx13

What about replacing your (alexa)

JsonObject &hue_json = jsonBuffer.parseObject(WebServer->arg(0));

by

JsonObject &hue_json = jsonBuffer.parseObject(WebServer->arg("plain"));

?

@ascillato
Copy link
Contributor Author

@d-a-v With last changes in your PR, parsing works fine.

Just missing the Alexa Test.

@Jason2866 @reloxx13 Please, Can you test that? Thanks again.

@Jason2866
Copy link
Contributor

@d-a-v @ascillato @reloxx13
Alexa isnt working.

New client
request: GET /ay HTTP/1.1
11:39:45.034 -> method: GET url: /ay search: 
headerName: Host
11:39:45.069 -> headerValue: 192.168.2.149
11:39:45.069 -> headerName: Connection
11:39:45.069 -> headerValue: keep-alive
11:39:45.069 -> headerName: User-Agent
11:39:45.069 -> headerValue: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36
11:39:45.069 -> headerName: Accept
11:39:45.069 -> headerValue: */*
11:39:45.069 -> headerName: Referer
11:39:45.069 -> headerValue: http://192.168.2.149/?
11:39:45.069 -> headerName: Accept-Encoding
11:39:45.104 -> headerValue: gzip, deflate
11:39:45.104 -> headerName: Accept-Language
11:39:45.104 -> headerValue: de-DE,de;q=0.9,en-US;q=0.8,en;q=0.7
11:39:45.104 -> args: 
11:39:45.104 -> args count: 0
11:39:45.104 -> args: 
11:39:45.104 -> args count: 0
11:39:45.104 -> Request: /ay
11:39:45.104 -> Arguments: 
11:39:45.104 -> final list of key/value pairs:
New client
11:39:45.173 -> request: PUT /api/vr0cFpv41CwnP3PpmyLwZaX0BtBARCMSNr6fkZc9/lights/1/state HTTP/1.1
11:39:45.207 -> method: PUT url: /api/vr0cFpv41CwnP3PpmyLwZaX0BtBARCMSNr6fkZc9/lights/1/state search: 
11:39:45.207 -> headerName: Host
11:39:45.207 -> headerValue: 192.168.2.149
11:39:45.207 -> headerName: Accept
11:39:45.207 -> headerValue: */*
11:39:45.207 -> headerName: Content-type
11:39:45.207 -> headerValue: application/x-www-form-urlencoded
11:39:45.241 -> headerName: Content-Length
11:39:45.241 -> headerValue: 12
11:39:45.241 -> args: {"on": true}
11:39:45.241 -> args count: 1
11:39:45.241 -> args: {"on": true}
11:39:45.241 -> args count: 1
11:39:45.241 -> Request: /api/vr0cFpv41CwnP3PpmyLwZaX0BtBARCMSNr6fkZc9/lights/1/state
11:39:45.241 -> Arguments: {"on": true}
11:39:45.241 -> final list of key/value pairs:
11:39:45.241 ->   key:'{"on": true}' value:''
11:39:45.241 ->   key:'plain' value:'{"on": true}'
11:39:45.241 -> request handler not found
New client
11:39:48.003 -> request: GET /ay HTTP/1.1
method: GET url: /ay search: 
11:39:48.038 -> headerName: Host
11:39:48.038 -> headerValue: 192.168.2.149
11:39:48.038 -> headerName: Connection
11:39:48.038 -> headerValue: keep-alive
11:39:48.038 -> headerName: User-Agent
11:39:48.038 -> headerValue: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36
11:39:48.072 -> headerName: Accept
11:39:48.072 -> headerValue: */*
11:39:48.072 -> headerName: Referer
11:39:48.072 -> headerValue: http://192.168.2.149/?
11:39:48.072 -> headerName: Accept-Encoding
11:39:48.072 -> headerValue: gzip, deflate
11:39:48.072 -> headerName: Accept-Language
11:39:48.072 -> headerValue: de-DE,de;q=0.9,en-US;q=0.8,en;q=0.7
11:39:48.072 -> args: 
11:39:48.072 -> args count: 0
11:39:48.072 -> args: 
11:39:48.072 -> args count: 0
11:39:48.072 -> Request: /ay
11:39:48.072 -> Arguments: 
11:39:48.072 -> final list of key/value pairs:

@Monarch73
Copy link

I consider my old PR to be foolproove in regards to this...maybe someone might want to check it again. Its simple yet effective.

#4151

@ascillato
Copy link
Contributor Author

@arendst

What do you think of @d-a-v approach?

What about replacing your (alexa)

JsonObject &hue_json = jsonBuffer.parseObject(WebServer->arg(0));

by

JsonObject &hue_json = jsonBuffer.parseObject(WebServer->arg("plain"));

@ascillato
Copy link
Contributor Author

ascillato commented Oct 23, 2018

Hi,

With the actual version of the core (22-oct-2018) all webserver parsings has stopped to work. Sorry.

Using the PR #5262 with the first changes, the webserver parsing was fixed, but now with the recent commits in it, the parsing stop to work again.

The core at commit d742df8 all webserver parsings were working. And if adding the PR #5222 also works the phillips hue emulation, used by Alexa.

@ascillato
Copy link
Contributor Author

Hi,

I have tested that just reverting the commit 64dd492 all the webserver parsings start to work again.
And if adding the PR #5222, also the plain data parsing will work, enabling again the use of Phillips Hue emulation for Alexa (as was working in core 2.3.0, and then the support was broke from 2.4.0)

I propose to revert the referenced commit and to add the PR #5222, as to make the core work again without braking the compatibility with other projects as some users start to complain (comment).

And then, do the D-A-V's optimizing rework of the webparsing but from a working esp8266 core. I will be glad to help testing.

Hope this helps to solve the actual webserver parsing issue.

@arendst
Copy link

arendst commented Oct 23, 2018

@ascillato regarding replacing arg(0) with arg("plain") it would be a problem for code like:

  for (args = 0; args < WebServer->args(); args++) {
    String json = WebServer->arg(args);
  }

but that could be rewritten too...

So in short, I could workaround it hoping no-one wants to use "plain" as a real argument.

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 23, 2018

@ascillato

I propose to revert the referenced commit and to add the PR #5222, as to make the core work again without braking the compatibility with other projects as some users start to complain (comment).

This is a fair request.
I just reverted the #5262 so everything should be working.
If you confirm everything is working on your side, then I think we can merge because it is a bugfix PR. I will pursue these changes in another PR.

@devyte @earlephilhower what do you think of merging #5262 now ?

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 23, 2018

@arendst @ascillato @reloxx13 @Monarch73 @dragondaud

The alexa issue arose with several different PRoposals to fix it.

Current maintainers (we are currently only three volunteers, that's pretty low) are not the authors of the current webserver code that you are using. The reason why we did not merge those PRs is because we could not deeply understand them: either inefficient (duplicate content) or with unknown side effects. I personally watched them several times. We were not ready to take the responsibility for breaking things and we had more urgent fixes for the core stability.
I am not a heavy user of the webserver, but because we are going to make the 2.5.0 release, I decided to dive into these issues.

Webserver's api gives url arguments and header by name and index. Let's take an example with a url like this: http://server/page?keya=x;keyb=y with json content {somekey: somevalue}.
There is currently a (weird to me) workaround to add content data directly into args with the following result (1):

arg0: key='{somekey: somevalue}' value=''
arg1: key='keya' value='x'
arg2: key='keyb' value='y'

or (2)

arg0: key='keya' value='x'
arg1: key='keyb' value='y'
arg2: key='{somekey: somevalue}' value=''

So you (users of webserver) are using key(0) to get your json data to parse (so we are expecting that content is always in key 0)
Is that what should be a good api ?
This addition is I think a quick fix for some old issue.
Now, the current alexa fix is to add another entry in arguments:

argn: key='plain' value='{somekey: somevalue}'

This is my current #5262 and also one of your PRs.
I am not satisfied by this (including #5262).
#5262 was fixing your issues yesterday, then I pushed a new commit to #5262 that broke everything again. It is now reverted, and I proposed to my co-maintainers above to merge it now, after your confirmation, so you can move forward as requested by @ascillato.

I will pursue #5262 to break things again because, as @reloxx13 told me, it is just inefficient to do that way. content must not be in arguments, and must not be duplicated into two argument entries.

Breaking things is not good, and that's the reason of #5269.
I propose to introduce new defines that will help us updating the API, so @arendst 's example above

  for (args = 0; args < WebServer->args(); args++) {
    String json = WebServer->arg(args);
  }

would become something like

#if CORE_ESP8266_VERSION < 24001
  // true for all previous releases including 2.2.0, 2.3.0, 2.4.x
  for (args = 0; args < WebServer->args(); args++) {
    String json = WebServer->arg(args);
  }
#else 
  // from now on, use new api, something like:
  String json;
  WebServer->getContent(json);
#endif 

The new api would be #5262 successor.
I plan to

  • deprecate server.arg(int) because no one should rely on constants, and consistent server.arg(keyname) is already here - we are supposed to know the keys, aren't we ? - and we still can loop over arguments to get key names String keyname = server->argName(i).
  • remove any content addition to arguments
  • propose a new call to get content that would have to be explicitely called with something like
    server->getContent(myContent); // myContent is now ready to be parsed by some JSON lib

This is not a big change, but I think it will put things a bit more right.

@ascillato
Copy link
Contributor Author

@d-a-v

Testing the actual core with the PR #5262, webserver parsing start to work again.

@ascillato
Copy link
Contributor Author

Now with last version all works fine.

Thanks a lot for your help !!!!! It is very appreciated. You have done a great job !!!!

Thanks :) 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants