s00500 / ESPUI

A simple web user interface library for ESP32 and ESP8266
https://valencia.lbsfilm.at/midterm-presentation/
Other
932 stars 174 forks source link

alternate port is broken #275

Closed d-a-v closed 11 months ago

d-a-v commented 11 months ago

Describe the bug

Running server on an alternate port is broken: websocket connection cannot be established

https://github.com/s00500/ESPUI/blob/8b64b185a4d545dafc0d86123e385fe961f3de78/data/js/controls.js#L229-L237

To Reproduce

nodoubtman commented 11 months ago

it is working on port 81, 82 on my side :)

:)

d-a-v commented 11 months ago

I just retried on esp8266 with the gui example. It does not work when connecting to http://ip:999 (and does work when replacing 999 by 80).

-    ESPUI.begin("ESPUI Control");
+    ESPUI.begin("ESPUI Control", nullptr, nullptr, 999);

Maybe @MartinMueller2003 can explain, when using non-standard ports, why this change works on your side but fails on mine:

https://github.com/s00500/ESPUI/commit/999d465c790e31c25fe54c56579c236a14362299#diff-240404bac9df1f87b6f87b070657b4e4ecf58136668154496fc9d165fafbc0e5L219-L221

MartinMueller2003 commented 11 months ago

I looked at the code. The default port number is 80 so using that value should always work. All values are sent to the AsyncWebService API without modification or checks for validity. There may be some rules being enforced by the Web Services.

d-a-v commented 11 months ago

The issue happens when another port value is used as when doing this in the gui example

-    ESPUI.begin("ESPUI Control");
+    ESPUI.begin("ESPUI Control", nullptr, nullptr, 999);

and connecting to http://ip:999

MartinMueller2003 commented 11 months ago

Understood. I am saying that the value you are giving is getting passed into the libraries used by ESPUI. ESPUI does no checking, filtering or validation. If a value does not work then the problem is deeper in the kernel, beyond where I can dig.

iangray001 commented 11 months ago

Yeah if there is an issue then I don't think it could be related to ESPUI. Port 999 is a privileged port. Does it work with port numbers above 1024?

MartinMueller2003 commented 11 months ago

I would test with the user defined ports between 33000 and 63000.

d-a-v commented 11 months ago

I am sorry, there seems to be a misunderstanding. I did my best to describe again my issue and propose the verified fix in #276.

How can this be explained otherwise ?

d-a-v commented 11 months ago

Yeah if there is an issue then I don't think it could be related to ESPUI. Port 999 is a privileged port. Does it work with port numbers above 1024?

This limitation only happens on TCP/UDP servers on OSes (mac linux windows). In this case, the ESP is the server and we don't have such limitation. Webbrowser are clients and they are allowed to connect to any serviced port.

MartinMueller2003 commented 11 months ago

Missed the link to the diff. Yes there is a bug in the Java Script that fails to use the correct port when one of the std ports is not the one being used. I will fix this tomorrow.

d-a-v commented 11 months ago

Understood. I am saying that the value you are giving is getting passed into the libraries used by ESPUI. ESPUI does no checking, filtering or validation. If a value does not work then the problem is deeper in the kernel, beyond where I can dig.

That is true. If I understand, the kernel you mention is the webserver which is also the websocket server (ESPAsync libs). They both operate on the same port. It is 80 by default but I wish to change it as the ESPUI API allows me in .begin(). So the javascript code uploaded to the webbrowswer must connect back on the same port as where the webserver operates. But you removed last september this possibility in https://github.com/s00500/ESPUI/commit/999d465c790e31c25fe54c56579c236a14362299#diff-240404bac9df1f87b6f87b070657b4e4ecf58136668154496fc9d165fafbc0e5L219-L221 .

d-a-v commented 11 months ago

Missed the link to the diff. Yes there is a bug in the Java Script that fails to use the correct port when one of the std ports is not the one being used. I will fix this tomorrow.

Thanks

s00500 commented 11 months ago

Fix should be merged to master now, thanks @MartinMueller2003