Skip to content
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

Accept WebServer Query Arguments not containing an equals sign (and ALEXA issue) #5222

Closed
wants to merge 2 commits into from

Conversation

ascillato
Copy link
Contributor

This PR fixes the following issues and also Alexa queries.

The actual codebase behavior is to completely ignore the argument (if not contains an equals sign), yet still include it in the argument count. (The use of "key=value" arguments is a convention, not an actual requirement in URLs.) This PR aims to fix that.

For example:

You will get an args() result.

the foo is lost, no args(), nothing.

Another example:

Amazon Echo (Alexa) sends a put-request with content-type "application/x-www-form-urlencoded" set in the headers but sends out json encoded data. _parseArguments fails to parse the arguments (which is correct) but completly discards the data. This change checks if the parser picked something up and if it didn't, it just adds it as plain data to the arguments.

Please, take this PR into account in order to solve these issues. Thanks.

@reloxx13, @holgerlembke, @jasonharper, @OFreddy, @Monarch73, @arendst, @andrethomas, @Jason2866, @d-a-v

Fixes several issues and also Alexa queries.

Previous behavior was to completely ignore the argument (if not contains an equals sign), yet still include it in the argument count.
(The use of "key=value" arguments is a convention, not an actual requirement in URLs.)

For example, Amazon Echo sends a put-request with content-type "application/x-www-form-urlencoded" set in the headers but sends out json encoded data. _parseArguments fails to parse the arguments (which is correct) but completly discards the data. This change checks if the parser picked something up and if it didn't, it just adds it as plain data to the arguments.
@devyte devyte self-requested a review October 10, 2018 20:59
@devyte devyte self-assigned this Oct 10, 2018
@devyte devyte added this to the 2.5.0 milestone Oct 10, 2018
@d-a-v
Copy link
Collaborator

d-a-v commented Oct 10, 2018

Thanks for bringing this issue again and before 2.5.0, because the ESP deserves chats with humans.
This PR does not address http://xxxx/index.html?foo?bar.
Is this ever likely to happen ?

@ascillato
Copy link
Contributor Author

ascillato commented Oct 10, 2018

Hi

The query http://xxxx/index.html?foo?bar is not used as far as I know.

This PR is mainly to solve the old Alexa issue (look at the opening dates of the referred still open issues)

If you want the library to also address that query, we can make another PR for that after this one.

Now it is urged this fix for Alexa. Hope this will be merged ASAP because for Sonoff-Tasmota we are making this patch manually to give Alexa compatible precompiled bins for our users. The people that does not make this patch and use the core as it is right now, then they rise issues at Sonoff-Tasmota repository saying that Alexa does not work. This also happens for the other Tasmota alike softwares.

With this Pull Request, we can solve this issue for all the softwares that uses this core and Alexa.

Please, take this PR into account in order to solve these issues. Thanks.

@Jason2866
Copy link
Contributor

@d-a-v Tested @ascillato PR solved the Tasmota issue(s)
Would be great if quick merged:--)
Thx again for your great work, without nice projects would be impossible

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 11, 2018

After a discussion with @devyte and @earlephilhower,
it appears that we should be able to handle this (& and ; are usual separators)
http://xxxx/index.html?foo&bar&lol=&ness=&key=val;another=way;to;separate=

This PR being essentially a rewrite of #4151, let us some days to address the above request type.

Quoting @earlephilhower:

https://www.skorks.com/2010/05/what-every-developer-should-know-about-urls/
query – these on the other hand are very common as every web developer would know. This is the preferred way to send some parameters to a resource on the server. These are key=value pairs and are separated from the rest of the URL by a ? (question mark) character and are normally separated from each other by & (ampersand) characters. What you may not know is the fact that it is legal to separate them from each other by the ; (semi-colon) character as well.

@devyte
Copy link
Collaborator

devyte commented Oct 11, 2018

Also, the reason the other two PRs are still open is that they had different solutions to the alexa bug, and it was not yet clear which of the two was cleaner. That is still pending.

@holgerlembke
Copy link
Contributor

Just because I saw my name… :)

In my opinion we need to

  • get the entire url
  • get the part after the ?
  • get the part after the #
  • get the foo=baa-pairs

It should be able to handle things like http://invalid/folder/page.html?foo=baa&answer=42#fragment

Everything else looks to me like vodoo and might be better solved by individual magic.

@ascillato
Copy link
Contributor Author

The 3 opened PR that solves Alexa Bug (#4151, #4236, #4573) are aiming the same thing (Accept WebServer Query Arguments not containing an equals sign)

The cleaner solution IMHO is 4151, that is why I added that to this PR since 4151 is rebased by the actual codebase.

I agree with you all. I think that the library should handle & and ; as separators to be able to parse:

http://xxxx/index.html?foo&bar&lol=&ness=&key=val;another=way;to;separate=

But all that should be done in another PR. This PR aims just to allow the library to accept as input:

http://xxxx/index.html?foo

With just that simple solution, Alexa bug is gone.

Then we can make the library to accept the other parameters too.

Please, take this PR into account in order to solve this issue for all the softwares that uses this core and Alexa.. Thanks.

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 11, 2018

What do you think of #5230 ?

@ascillato
Copy link
Contributor Author

@d-a-v

Like a lot your approach!

Please, use that to solve all these issues 👍

@andrethomas
Copy link

Looks good @d-a-v ;)

@ascillato ascillato mentioned this pull request Oct 11, 2018
@ascillato
Copy link
Contributor Author

The PR #5230 now solves also the parsing issue with Alexa. If 5230 is merged, this PR is no longer needed and can be closed.

@devyte
Copy link
Collaborator

devyte commented Oct 17, 2018

Closing in favor of #5252 .

@devyte devyte closed this Oct 17, 2018
@ascillato ascillato mentioned this pull request Oct 23, 2018
6 tasks
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

Successfully merging this pull request may close these issues.

None yet

7 participants