make HTTP Update Server more secure #2104
Conversation
me-no-dev
commented
Jun 6, 2016
me-no-dev
commented
- added option for authentication
- added option to change the url for upload
* added option for authentication * added option to change the url for upload
| public: | ||
| ESP8266HTTPUpdateServer(bool serial_debug=false); | ||
| void setup(ESP8266WebServer *server=NULL); | ||
| void setup(ESP8266WebServer *server, const char * path="/update", const char * username=NULL, const char * password=NULL); |
There was a problem hiding this comment.
Let's use a pair of overloaded functions instead of a function with bunch of default arguments here.
I.e.
void setup(ESP8266WebServer *server); // used to keep code backward compatible, calls setup(server, "/update", NULL, NULL)
void setup(ESP8266WebServer *server, const char * path); // optional, add this if you want... not really necessary IMO. calls setup(server, path, NULL, NULL);
void setup(ESP8266WebServer *server, const char * path, const char* username, const char* password); // new overload, all arguments are requiredReason for such a design is that it's easier to refactor/extend/deprecate an interface based on overloaded functions compared to an interface which uses function with many default arguments. This was very obvious with ESP8266HTTPClient and ESP8266HTTPUpdate, which used default arguments heavily. It was really painful to separate functions which needed TLS from the ones which didn't...
Current coverage is 26.31%@@ master #2104 diff @@
==========================================
Files 19 19
Lines 3634 3634
Methods 328 328
Messages 0 0
Branches 585 585
==========================================
Hits 956 956
Misses 2513 2513
Partials 165 165
|
|
better? |
|
|
||
| void loop(void){ | ||
| httpServer.handleClient(); | ||
| delay(1); |
There was a problem hiding this comment.
yeah... was in the other example, so I left it there thinking might be needed. I'll remove it from both
|
done |
|
let me add one more thing. message to the browser is a bit cryptic :) |
interesting, the meta did not refresh if the successResponse is put in "R"
| _server->on(path, HTTP_POST, [&](){ | ||
| if(!_authenticated) | ||
| return _server->requestAuthentication(); | ||
| _server->send(200, "text/html", Update.hasError() ? _failedResponse : _successResponse); | ||
| ESP.restart(); |
There was a problem hiding this comment.
@igrr you think maybe we should not reboot if Update failed?
There was a problem hiding this comment.
I'll merge this as is, let's address this in a separate pull request. I think a good idea would be to add update callbacks similar to ESP8266HTTPUpdate (client). Actually maybe we need to review APIs of HTTP update client and HTTP update server and make them more uniform. Let's discuss further on gitter.
