Memory usage and JSON/architectural issues

[JonSilver]

Hi @nrwiersma,

Firstly thank you for taking the time to write and share this library. Unfortunately I've stopped using it for the moment as it introduced instability problems into my application, due to memory usage. So I was wondering about some of the architectural choices made with this library. Please correct me if I'm wrong with anything I say here.

I've modified your library to use server->streamFile() instead of server->send(), so the HTML file is streamed directly from flash instead of being read into a string variable before being sent. This has both improved web server performance and reduced impact on heap space, in turn reducing crashes when the HTML file is too big.

I'm also concerned about the security problems of the REST API. Since parameters may include passwords for MQTT brokers, having an open endpoint which freely shows clear text parameter values is a bit of a risk.

The library passes parameters via the REST API using JSON. This creates a large memory overhead and makes it difficult to use standard HTML forms for the PUT, as there is no JSON support built into HTML. Could we not just post server args using multipart/form-data and access each arg without the expensive transformation to and from JSON? Sure it makes it easy on the client where we have the convenience of JavaScript and Ajax, but it's expensive on the ESP8266 side.

Thanks,
Jon

[nrwiersma]

Hi,

Thanks for getting in touch. I will answer as best I can :)

I didnt know about streamFile. I will gladly take a pull request that makes the library more efficient.

So the clear text design was not made with passwords in mind. My thoughts were that the endpoints would be on a controlled network (like your home network) and mainly to hold settings used configure devices, more then usernames and passwords. I would have kept passwords in the compiled code, as they don't really change the same way settings do. I don't think there is a simple way around this problem, but would be interested to hear your thoughts.

JSON is an API standard. When making frontend software I almost always end up moving JSON around. That isnt to say that you are wrong. Standard form encoding is indeed cheaper and quicker. As a middle ground perhaps form encoded could be supported on the POST and JSON on the PUT. What are your thoughts on this?

Thanks for your thoughts,
Nick

[JonSilver]

@nrwiersma Thanks Nick, I've made the amendment to stream direct from file, and created the pull request. Sorry it took a while for me to get around to it. :)

I think the dual approach for POST/PUT might be best - then we get the best of all worlds and avoid all the shenanigans one has to go through to generate JSON from a web form. My C++ is pitiful or I'd probably implement this myself.

As for the clear-text password issue, one could implement a flag on custom field indicating that some sort of internal two-way hash should be used to store the field contents, so at least clear text passwords can't just be obtained by issuing a GET query. Then internally the unhashed clear text password becomes available for use in MQTT connects etc.

What do you think?

Kind regards,
Jon

[nrwiersma]

Hi,

Thanks for the pull request. It is greatly appreciated. It will be released in the next version.

As soon as I get some time, I will write the POST code, just a little swamped at the moment.

With regards to encrypting passwords, this is a little more tricky. My init assumption was that this is in an environment of trust, which is obviously not always the case. The issue is that the GET works on the config struct, so if you can see the clear text password, so can the GET. Perhaps the trick is to be able to set fields with PUT/POST but not GET them. Might this work? I am also concerned about the power and RAM needed for encryption (it is meant to be expensive). Would you like to start a new issue for this topic, and we can discuss this there?

Thanks,
Nick

[JonSilver]

Sure... see you there!

Jon

[nrwiersma]

Hi,

Was looking into the POST request, and I remember why I chose JSON. The server args always returns a string, so typing is an issue. Will need to look into converting types.

[JonSilver]

Yes, types are always a pain when passing data around.

On the memory issue, I've seen a lot of people having memory issues when repeatedly serving web pages from an ESP8266, and more than one has reported that including a "Connection: close" header with each page served fixes this by requesting that the client closes the connection after the data is received. What do you think, @nrwiersma? Is it worth including anyway?

[nrwiersma]

Yes, good idea, this should be added in the next release.

[JonSilver]

Also related to memory usage, could we move as many program string literals as possible into flash memory using PROGMEM? I've tried this locally and it works well to reduce overall RAM usage.

https://gist.github.com/sticilface/e54016485fcccd10950e93ddcd4461a3

Thanks,
Jon

[nrwiersma]

Sounds good. You want to add this as a pull request?

[JonSilver]

Once I've fully tested it all I shall put in a PR 🙂

[JonSilver]

PR submitted. All running very nicely.

[nrwiersma]

Pull request merged, thanks for that.

Have run into a snag with the POST endpoint. The string stream I was planning to use is too big. Need to come up with another plan here. Will move onto the other issues so long.

[JonSilver]

Ok if you want to discuss it and kick some ideas around, let me know.

[nrwiersma]

Thanks, appreciate it. My plan is to look to ArduinoJson for ideas on how to solve it. If you have some ideas, will gladly chat about them.

[nrwiersma]

I just had a thought that might make the POST issue unnecessary. CM provides a function setAPICallback that lets you register your own API endpoints on the web server. My thought is that you can handle the POST yourself, and as you know your types, you wont run into the same issues that CM is. Do you find this acceptable as a solution? Perhaps I need to write more docs to this end.

[nrwiersma]

Sweet, that being the case, I will close this issue.

BTW. I checked the connection close issue, and this is sent by the library, see here,
So there is no need for me to handle this.

[JonSilver]

Hi Nick, sorry I'm neck-deep in one of our own projects but shall try everything as soon as I can.
In the meantime, yes more documentation and/or example code would probably be helpful for most potential users of ConfigManager.

[nrwiersma]

Hi,

Thanks for the feedback. Any particular areas you think the docs need improvement, other then a better API callback docs/example?