lovasoa / SQLpage

SQL-only webapp builder, empowering data analysts to build websites and applications quickly
https://sql.ophir.dev
MIT License
882 stars 62 forks source link

Default SQLPage Listener Should be Localhost Instead of 0.0.0.0 #387

Closed amrutadotorg closed 3 weeks ago

amrutadotorg commented 3 weeks ago

Hi, here is my config

{
    "database_url": "postgres://postgres:pass@localhost:5432/pp?application_name=my_application",
    "environment": "development"
}

when I run the sqlpage (tested on macOS version) it prints

[2024-06-09T11:00:24.313Z INFO  sqlpage::webserver::http] Will start HTTP server on 0.0.0.0:8080
[2024-06-09T11:00:24.314Z INFO  sqlpage] Server started successfully.
        SQLPage is now listening on http://127.0.0.1:8080

but in fact it listens on *

❯ lsof -i :8080

COMMAND     PID USER   FD   TYPE            DEVICE SIZE/OFF NODE NAME
sqlpage.b 34892    r   12u  IPv4 0xeed33cf6542d0eb      0t0  TCP *:http-alt (LISTEN)

adding to the config "listen_on": "127.0.0.1:8080" shows that the sqlpage listens on localhost only

sqlpage.b 34923 user 12u IPv4 0x5c3bcf833f5ba732 0t0 TCP localhost:http-alt (LISTEN)

I would suggest that SQLPage should always listen on localhost as default.

djyotta commented 3 weeks ago

localhost is an alias for 127.0.0.1 though right?

lsof -i @localhost:8080
# for IPv4 it is same output as
lsof -i @127.0.0.1:8080

For IPv6 there is a subtle difference because localhost maps to both IPv4 and IPv6 local addresses. IPv6 local address is obviously not 127.0.0.1... so that could be misleading if SQLPage is listening on IPv6 socket also... is/does it?

djyotta commented 3 weeks ago

Looks like SQLPage does not listen on IPv6 socket: https://github.com/lovasoa/SQLpage/blob/8a8b7550891756539fe7528c02777aeb6d5bb24d/src/main.rs#L35 ~~So then there is no difference between localhost and 127.0.0.1 in this context. I think.~~

EDIT: actually, I linked to display code not anything to do with actual opened socket

amrutadotorg commented 3 weeks ago

Based on the SQLPage output it should listen only on localhost/127.0.0.1 right? but it doesn't. It listens on *.

djyotta commented 3 weeks ago

Hmm, but in your output of lsof it says "localhost". EDIT: Lol, I missed that this is after expressly configured (facepalm) 0.0.0.0 is "any IPv4" (not localhost).

It looks like the code replaces 0.0.0.0 if configured with 127.0.0.1 in the welcome message (not great), but you have to configure it to listen on 0.0.0.0 in the first place (I wouldn't). We should not disguise listening on 0.0.0.0 as 127.0.0.1 - I agree with that.

Just the example output you give is confusing because it's not demonstrating this issue. No, I just can't read apparently.

@lovasoa is this what it looks like? : https://github.com/lovasoa/SQLpage/blob/8a8b7550891756539fe7528c02777aeb6d5bb24d/src/app_config.rs#L101 Looks like that is "any IPv4" to me, but can't find a definition in the docs for all zero IPv4: https://doc.rust-lang.org/std/net/enum.SocketAddr.html

djyotta commented 3 weeks ago

@amrutadotorg good catch.

amrutadotorg commented 3 weeks ago

Ah yes, thank you I messed up the text. Updated it. It should be less confusing now.

lovasoa commented 3 weeks ago

You are right, the replacing is confusing. But I'm not sure about listening only on localhost by default. It would make deployment harder for non technical folks and break existing installations.

amrutadotorg commented 3 weeks ago

I see. So maybe the SQLPage should explicitly state about the implications of having the app listing on all interfaces. And the option to turn it off. This should be enough.

djyotta commented 3 weeks ago

Now the link says http://0.0.0.0:8080 I think that link won't work (probably browser dependant).

To aid non-technical users:

lovasoa commented 3 weeks ago

Now the link says http://0.0.0.0:8080/ I think that link won't work (probably browser dependant).

Do you have an example of a software that doesn't work with this URL ? It seems to work everywhere for me.

image

lovasoa commented 3 weeks ago

But indeed, maybe putting a link to localhost would be better, to avoid the "insecure browsing" restrictions. Or we can switch to listening only on localhost by default, and give a detailed warning about the breaking change in the CHANGELOG and release notes.

lovasoa commented 3 weeks ago

Do you want to do that ?

djyotta commented 3 weeks ago

Ah, I was going from memory on http://0.0.0.0:8080 not loading in browser. I appear to be mistaken.

I'm undecided about the default. Personally, I would default to 127.0.0.1, but you are right in that it's an extra hurdle for non-tech person.

I guess I could at least add a message with the link to 127.0.0.1 instead of 0.0.0.0 when address is all zeroes as that doesn't break anything.

I could make the default 127.0.0.1 in a separate PR and then it can be pulled at will if the community wants it.