Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved URL display on MOTD #2817

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Improved URL display on MOTD #2817

wants to merge 1 commit into from

Conversation

Tronic
Copy link
Member

@Tronic Tronic commented Sep 5, 2023

Sanic would use any host setting for URL construction. This PR fixes the common cases of normal localhost and wildcard addresses, for which we instead display localhost. Similarly for UNIX sockets, as that format works e.g. with curl. URL to wildcard doesn't work at all, and connecting localhost by IP rather than domain name can be problematic with TLS if not otherwise.

Additionally, hides the port number if standard ports are used.

@Tronic Tronic requested a review from a team as a code owner September 5, 2023 18:06
@Tronic Tronic marked this pull request as draft September 5, 2023 18:50
@Tronic
Copy link
Member Author

Tronic commented Sep 5, 2023

Setting host to localhost instead of IP probably needs to be done in server startup rather than MOTD only, to make url_for work as expected without extra configuration. This is tricky, leaving in draft mode for now.

@ahopkins
Copy link
Member

ahopkins commented Sep 6, 2023

This is tricky, leaving in draft mode for now.

Maybe also time to handle the whole server name issue. 😉

Comment on lines +676 to +696
# colon(:) is legal for a host only in an ipv6 address
url_host = f"[{host}]" if ":" in host else host
url_port = (
""
if (
(proto == "https" and port == 443)
or (proto == "http" and port == 80)
)
else f":{port}"
)

special = {
"127.0.0.1": "IPv4",
"0.0.0.0": "IPv4 wildcard",
"::1": "IPv6",
"::": "IPv6 wildcard",
}.get(host, "")
if special:
return f"({special}) {proto}://localhost{url_port}"

return serve_location
return f"{proto}://{url_host}{url_port}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole bit feels to me like logic we should probably abstract to a utility function somewhere. Don't we do this elsewhere also?

@Tronic
Copy link
Member Author

Tronic commented Sep 6, 2023

Maybe also time to handle the whole server name issue. 😉

That's a can of worms, for sure. Would be useful to first identify all the problems that we are solving here, and things affected by them.

  • External URL forming (btw, should use app.url_for in MOTD too).
    • Scheme and host and port may be different than locally seen
    • Currently by SERVER_NAME, what are the concerns about this (URL vs. host only, host:port)
    • If apps can be externally mounted also at some path other than /, internal URLs are also affected
    • In global context where the app is not yet running env is available, app.run args are not!
  • Host (request header) fallback
    • Local or remote values here? Start giving 400 Bad Request to those without valid header?
  • Listening address(es) to bind (IPs, unix path)
    • Might be displayed separately on MOTD, should always be displayed inside the box?
  • CLI and app.run interfaces for specifying these things?
  • What is --host used for?
    • Binding or hostname, both?
    • Changing the default to localhost would make sense and avoid some special cases
  • Are we adding a --listen shorthand for doing all at once and/or listening to more than one
    • Could specify multiple to bind, but the server can only have one host:port with app.url_for
    • Listen to IPv4 and IPv6 wildcards if :8000 (nothing before colon) is specified (matches Caddy)
    • Default to 80/443 if plain hostname/ip is given?
    • Auto-TLS if given name and no port, background server on 80? (redirect/ACME, matches Caddy)

What'd I miss?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants