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

webserver: do not count "plain" in argument list #6768

Merged
merged 6 commits into from
Jul 13, 2020

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Nov 13, 2019

"plain" is implicit and is not a real url parameter.
This PR will hide it from the argument counter, but arg("plain") is still available.

fixes #6435

edit:

fixes #7106

#5252 added key "plain" to the list of arguments. Unfortunately as a side effect, this key is also always counted in the argument list. This PR removes this effect.

@devyte
Copy link
Collaborator

devyte commented Nov 14, 2019

I think this could break things. If we're going to break things,we likely should go all the way and rethink this feature.
Some thoughts:

  • The "plain" case is currently meant for holding all args in raw form without parsing
  • We can remove the current "plain" case from the counter (like in this PR)
  • We can make sure the current "plain" arg doesn't show up in any iteration (hold the raw arg string elsewhere, not together with the parsed args)
  • Implement a getter that returns the raw arg string

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the approach to handling the "plain" case should be revisited and improved. See my comment.

@devyte devyte added this to the 3.0.0 milestone Nov 14, 2019
@d-a-v
Copy link
Collaborator Author

d-a-v commented Nov 14, 2019

The "plain" case

.. is the Content = the Payload of the http message.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Apr 8, 2020

Things are currently broken and this PR hopefully fixes the situation:
"plain" is artificially added but must not be counted in the url arguments.

  • point 1: arg("plain") holds payload (POST requests only)
  • point 2: this is this PR
  • point 3: When args are iterated, code must check for iterated name anyway. "plain" is always the last arg. So after this PR, iterating on args counter will never reach the "plain" arg (which is what is expected no?)
  • point 4 is not needed (webserver parses the url, the raw url is not used)

This PR is useful when testing the number of args in POST requests, which seems to be done by issuers.

I think the approach to handling the "plain" case should be revisited and improved.

I totally agree with this. In the meantime, things need fixing.

@devyte
Copy link
Collaborator

devyte commented Apr 27, 2020

@d-a-v
What does args() (number of arguments) return when there were no arguments provided

  • prior to this PR
  • with this PR
    ?

What does arg(0) (argument at index 0) return when the first arg provided has no value

  • prior to this PR
  • with this PR
    ?

What does arg(0) (argument at index 0) return when no args were provided

  • prior to this PR
  • with this PR
    ?

@d-a-v
Copy link
Collaborator Author

d-a-v commented Apr 27, 2020

I was previously thinking that I added arg("plain") in the webserver.
It turns out I didn't, it was there before (#2241). I discovered about this artificial argument when I fixed some bug some time ago (I had to read the code several time, maybe too many times so I finally believed I made this unusual hack).
This arg plain is added on post requests.
When it is added it is always last. So with or without it, there is no change on argument order.
The only consequence I know about it is the reported number of arguments, that this PR fixes.
I originally thought this PR was planned for 2.7.0 but when I realized the issue was there from long time ago I let milestone set to 3.0.0 because fixing it might be a breaking change (which I don't really believe but one never knows).

It is anyway always not safe to rely on numbers and it is preferred to check presence of a variable name, then get its value, so nothing is dependent on arg counter.

The only reason I can see about reading positional arguments is when one iterate over them to bravely parse a request, by looping from 0 to count-1, checking the name, and if it is recognized, getting its value to make something with it. In that case plain should be ignored (or reported?).

@devyte @vdeconinck

This has to be double checked

What does args() (number of arguments) return when there were no arguments provided

before : 0 (not POST) or 1 (POST)
after: 0 (always)

What does arg(0) (argument at index 0) return when the first arg provided has no value

emptyString in all cases

What does arg(0) (argument at index 0) return when no args were provided

before: when it is a POST it can return the request content (== arg("plain")) which is irrelevant in that case (emptyString otherwise)
after: emptyString (even when count == 0 which would make an illegal call)

@vdeconinck
Copy link
Contributor

Hi, @d-a-v , thanks for the explanation.
Just to make things clear, when you say "it is preferred to check presence of a variable name, then get its value, so nothing is dependent on arg counter.", I guess you mean something along the lines of

if (server.hasArg("dir")) {
  String dir = server.arg("dir");
  ...
}

But what is your take on the following construct from a risk/performance/cleanliness point of view:

String dir = server.arg("dir");
if (!dir.isEmpty()) {  
  ...
}

@d-a-v
Copy link
Collaborator Author

d-a-v commented Jul 13, 2020

@vdeconinck

String dir = server.arg("dir");
if (!dir.isEmpty()) {  
  ...
}

Sorry for the late response. Yes, that's better because there's only one search.

@d-a-v d-a-v merged commit e58bb60 into esp8266:master Jul 13, 2020
@d-a-v d-a-v deleted the noplaincount branch July 13, 2020 10:40
@Sparatutto
Copy link

Hi all,
I write here because it seems to be the last thread about "plain".
I had to fix a minor issue on my code and then recompile the project (last 3.0.2 release).
But I still see "plain" counted by server.args() as per #6435.
Meanwhile I got that it has been removed from the count along the released version.

I tried to check for a step back on this but not found.
Can you confirm please which strategy you will keep here?
Do you confirm it will count or not?

arg

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.

ESP8266WebServer args() result to high? server.send send unexpected argument migrating from 2.4.x to 2.5.2
4 participants