Skip to content

fix(portal): allow single-label container hostnames in private network validation - #188

Closed
elf-mouse wants to merge 1 commit into
yc-software:mainfrom
elf-mouse:fix/portal-single-label-hostname
Closed

fix(portal): allow single-label container hostnames in private network validation#188
elf-mouse wants to merge 1 commit into
yc-software:mainfrom
elf-mouse:fix/portal-single-label-hostname

Conversation

@elf-mouse

@elf-mouse elf-mouse commented Aug 4, 2026

Copy link
Copy Markdown

When running under Docker Compose, internal upstream services (such as the authentication broker http://auth:8080) use bare single-label hostnames without dots (e.g. auth).

The previous isPrivateNetworkUrl check rejected any non-IP host missing a domain extension or .local suffix, causing portal boot checks to fail with a misconfiguration error.

This commit updates isPrivateNetworkUrl to accept single-label hostnames (!host.includes(.)) as valid private network hosts.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…k validation

When running under Docker Compose, internal upstream services (such as the authentication broker `http://auth:8080`) use bare single-label hostnames without dots (e.g. `auth`).

The previous `isPrivateNetworkUrl` check rejected any non-IP host missing a domain extension or `.local` suffix, causing portal boot checks to fail with a misconfiguration error.

This commit updates `isPrivateNetworkUrl` to accept single-label hostnames (`!host.includes(.)`) as valid private network hosts.

@rajpratham1 rajpratham1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a small, targeted fix that addresses a realistic deployment scenario without broadening the trust model in a concerning way.

What the PR does well
Fixes a real compatibility issue
Previously, only localhost, certain private domains (.internal, .flycast, .local), and private IP ranges were considered trusted.
This change additionally allows single-label hostnames (e.g. http://auth:8080), which are commonly used for service discovery inside Docker Compose, Kubernetes, and other private container networks.
Minimal change

The implementation is a single condition:

|| !host.includes(".")
This keeps the logic simple and doesn't affect existing trusted hostname checks.
Regression test included

The test adds:

http://auth:8080

ensuring this deployment scenario remains supported in the future.

Security considerations

I don't see this as introducing a significant security risk because:

The validation still requires an http or https URL.
Single-label hostnames are generally not publicly resolvable via normal DNS and are primarily used within private networks.
Existing checks for localhost, .internal, .flycast, and RFC1918 IPs remain unchanged.
Minor observation (non-blocking)

Using !host.includes(".") also accepts any single-label hostname (e.g. printer, db, redis, foo). That is likely intentional for containerized environments, but if the project ever needs stricter validation, a comment explaining that this is specifically to support Docker/Kubernetes service names would improve readability.

@time-attack

Copy link
Copy Markdown
Collaborator

Thanks for this, @elf-mouse — and sorry for the slow response.

We're closing this PR. Per CONTRIBUTING.md, since coding agents write most of the underlying code now, we'd rather receive feature ideas as a short human-written note in adrs/ — informal is great, just the idea in your own words — and bugs as a plain issue. If we're aligned we're happy to burn our own tokens on the implementation, and we'll credit you as co-author.

We don't want your idea to get lost, so we've captured it in #836 to triage on our side. Please feel free to open an adrs/ note if you feel strongly about the approach. Appreciate you engaging with the repo!

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.

3 participants