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

Fix enduser_setup default POST request #2852

Merged
merged 3 commits into from
Aug 1, 2019
Merged

Conversation

HHHartmann
Copy link
Member

@HHHartmann HHHartmann commented Jul 25, 2019

partially fixes #2847.

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.

Rational
the out of the box enduser_setup does not work as the param parsing fails.
It assumes to have a ' ' or '&' to terminate the params.
While this is true for the GET request (where the params are part of the URL which is followed by " HTTP/1.1" it does not hold for POST requests, where the message ends with the last char of the params.

GET /url?wifi_ssid=ssid&wifi_password=pass HTTP/1.1

This fix returns a false positive if the request is malformed and does not have a space to terminate the URL.
But since message parsing is not very strict anyhow that should be OK (for now)

@HHHartmann HHHartmann added this to the Next release milestone Jul 25, 2019
Copy link
Member

@marcelstoer marcelstoer left a comment

Choose a reason for hiding this comment

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

I am still wondering why it didn't fail when I tested the module before we merged it 😕.

I didn't understand half of the C-code I was just scanning but IMO this isn't a proper fix.

This violates the behavior documented in @return of the function but that's just a detail. I feel a fix should go into enduser_setup_http_handle_credentials around lines 817ff.

  • check if it's a GET request, we use if (strncmp(data, "GET ", 4) == 0) elsewhere, and if so then run the existing code
  • else run some alternate code that handles wifi_ssid=xxx&wifi_password=aa<EOL> or wifi_password=aa&wifi_ssid=xxx<EOL> or custom_param=bb&wifi_password=aa&wifi_ssid=xxx<EOL> (i.e. searching for & or <EOL>?)

@HHHartmann
Copy link
Member Author

@marcelstoer Marcel, I agree that the fix ist not perfect. That's the problem with comments, they always are not adapted to code changes. But that's a different discussion.

A Proper fix would IMHO be to extract the params in the respective GET and POST handler and pass only this substring wifi_ssid=ssid&wifi_password=pass to enduser_setup_http_handle_credentials.

I could also leave enduser_setup_srch_str as is and add some "not found" handling to the calling function.
In any case this should go in the next release together with #2827 and #2810 or none of them.

@marcelstoer
Copy link
Member

A Proper fix would IMHO be to extract the params in the respective GET and POST handler and pass only this substring wifi_ssid=ssid&wifi_password=pass to enduser_setup_http_handle_credentials.

True, that sounds like a good plan - as long as we keep in mind that the string may contain other key=value pairs in arbitrary order.

In any case this should go in the next release together with #2827 and #2810 or none of them.

👍 IMHO rolling back is not an option, forward-fixing is.

@HHHartmann
Copy link
Member Author

This should be fixed in a better way now.
Also found and fixed a memory leak closing a file (in rare circumstances)

*
* Search string for first occurence of any char in srch_str.
* Search string for first occurence of deliemiter '&' or ' '.
Copy link
Member

@marcelstoer marcelstoer Jul 29, 2019

Choose a reason for hiding this comment

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

I think there are few naming/linguistic issues. All of them tiny but in combination this isn't easy to comprehend:

  • I don't understand the second sentence in this comment.
  • @return -1 if... - no longer true
  • get_len(const char *str) is extremely generic, can the function and the param be named more precisely?
    • *str is expected to be a "value&key=..." string, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok. To avoid more checkins:

/**
 * Get length of param value
 * 
 * This is being called with a fragment of the parameters passed in the 
 * URL for GET requests or part of the body of a POST request.
 * The string will look like one of these
 * "SecretPassword HTTP/1.1"
 * "SecretPassword&wifi_ssid=..."
 * "SecretPassword"
 * The string is searched for the first occurence of deliemiter '&' or ' '.
 * If found return the length up to that position.
 * If not found return the length of the string.
 *
 */
static int enduser_setup_get_lenth_of_param_value(const char *str)

@marcelstoer marcelstoer merged commit 0659e55 into nodemcu:dev Aug 1, 2019
@HHHartmann
Copy link
Member Author

Are you using an enduser_setup.html file on the SPIFFS Filesystem or the built in version?
The call to store the data has changed.

@KT819GM
Copy link

KT819GM commented Sep 18, 2019

Omg, answered so fast,before I deleted question by seeing that I'm posting into pull request. I'm using builtin version, will try to read how to use it properly, thought so, that I did not saw all the changes. Thank you

@HHHartmann HHHartmann deleted the fix-eus branch May 1, 2020 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants