ayushsharma82 / WebSerial

A remote terminal library for wireless microcontrollers to log, monitor or debug your firmware/product
https://webserial.pro
GNU Affero General Public License v3.0
461 stars 107 forks source link

AsyncWebSocket::setAuthentication uses const char* not String #83

Closed aminhajali closed 3 weeks ago

aminhajali commented 3 weeks ago

in the WeSerial.cpp: void WebSerialClass::begin(AsyncWebServer server, const char url) { _server = server; _ws = new AsyncWebSocket("/wserial");

if (_authenticate) { _ws->setAuthentication(_username, _password); //<-- must use _ws->setAuthentication(_username.c_str(), _password.c_str()); }

mathieucarbou commented 3 weeks ago

in the WeSerial.cpp: void WebSerialClass::begin(AsyncWebServer server, const char url) { _server = server; _ws = new AsyncWebSocket("/wserial");

if (_authenticate) { _ws->setAuthentication(_username, _password); //<-- must use _ws->setAuthentication(_username.c_str(), _password.c_str()); }

What is the reason ? Pointers should still be valid within setAuthentication

ayushsharma82 commented 3 weeks ago

@mathieucarbou Anyhow, I still updated it. It's in dev branch

aminhajali commented 3 weeks ago

https://github.com/me-no-dev/ESPAsyncWebServer/blob/f71e3d427b5be9791a8a2c93cf8079792c3a9a26/src/ESPAsyncWebServer.h#L338

AsyncWebHandler& setAuthentication(const char *username, const char *password){  _username = String(username);_password = String(password); return *this; };
ayushsharma82 commented 3 weeks ago

Released in v2.0.2

ayushsharma82 commented 3 weeks ago

@aminhajali , Ah that's why the confusion. We recently switched to @mathieucarbou 's fork of AsyncWebServer. It has a lot of fixes and support Strings for setAuthentication function. Ref: https://github.com/mathieucarbou/ESPAsyncWebServer/blob/main/src/ESPAsyncWebServer.h#L366

Release Ref: https://github.com/ayushsharma82/WebSerial/releases/tag/v2.0.0

mathieucarbou commented 3 weeks ago

I've juste tested in my project, I have the same code base (in pro):


void WebSerialLiteClass::setAuthentication(const String& username, const String& password) {
  _username = username;
  _password = password;
  _authenticate = !_username.isEmpty() && !_password.isEmpty();
  if (_ws != nullptr) {
    _ws->setAuthentication(_username.c_str(), _password.c_str());
  }
}

void WebSerialLiteClass::begin(AsyncWebServer *server, const char* url) {
  _server = server;
  _ws = new AsyncWebSocket("/wserial");

  [...]

  if (_authenticate) {
    _ws->setAuthentication(_username.c_str(), _password.c_str());
  }

I tested:

  WebSerialLite.begin(&webServer, "/console");
  WebSerialLite.setAuthentication(YASOLR_ADMIN_USERNAME, config.get(KEY_ADMIN_PASSWORD).c_str());

and

  WebSerialLite.setAuthentication(YASOLR_ADMIN_USERNAME, config.get(KEY_ADMIN_PASSWORD).c_str());
  WebSerialLite.begin(&webServer, "/console");

Both work fine. There is no pointer issue or dereferencing because the implementation is saving the string in a String.

https://github.com/me-no-dev/ESPAsyncWebServer/blob/f71e3d427b5be9791a8a2c93cf8079792c3a9a26/src/ESPAsyncWebServer.h#L338

AsyncWebHandler& setAuthentication(const char *username, const char *password){  _username = String(username);_password = String(password); return *this; };

@aminhajali You are using the wrong fork.

mathieucarbou commented 3 weeks ago

@ayushsharma82 : but he has a point that to be backward compatible with existing ESPAsyncWebServer project and forks, setAuthentication(char *, char*) should be used. I didn't pay attention to that fats when doing my PR... Sorry.

ayushsharma82 commented 3 weeks ago

I'll mark it as closed. Fixed as of v2.0.2.

@aminhajali Thanks for bringing this to our attention.