Skip to content

feat: fix upstream host resolution edge case and support PEM paste in custom certificates#5348

Open
Eric-Terminal wants to merge 3 commits into
NginxProxyManager:developfrom
Eric-Terminal:develop
Open

feat: fix upstream host resolution edge case and support PEM paste in custom certificates#5348
Eric-Terminal wants to merge 3 commits into
NginxProxyManager:developfrom
Eric-Terminal:develop

Conversation

@Eric-Terminal
Copy link
Copy Markdown

This PR addresses two issues:

1) Optional upstream pre-resolution for forward_host (issue #5344)

Problem:
Nginx variable-based upstream resolution may fail for hostnames that are only resolvable through container/system resolver paths (for example extra_hosts mappings like host.docker.internal).

What changed:

  • Added optional env flag: NPM_PRE_RESOLVE_UPSTREAM_HOSTS (default: disabled)
  • During Nginx config generation, NPM can pre-resolve eligible upstream hostnames via system resolver (dns.lookup)
  • Applied to both proxy host forward_host and custom location forward_host
  • If resolution fails, it safely falls back to the original hostname

Behavior:

  • No behavior change unless the new flag is explicitly enabled

2) Allow direct PEM paste in Custom Certificate modal (issue #5347)

Problem:
Custom certificate flow previously required local file selection, which is inconvenient in remote/mobile/thin-client workflows.

What changed:

  • Added textarea inputs for:
    • Certificate PEM
    • Certificate Key PEM
    • Intermediate Certificate PEM (optional)
  • Kept existing file upload inputs unchanged
  • Submit logic now accepts either:
    • uploaded files, or
    • pasted PEM text (converted to File objects client-side)
  • Existing backend validate and upload multipart APIs are reused without contract changes

Behavior:

  • Backward compatible with existing file upload workflow

Docs

  • Added advanced config documentation for NPM_PRE_RESOLVE_UPSTREAM_HOSTS

…mpatibility

Files changed: backend/internal/nginx.js.

Purpose: provide an optional, NPM-side compatibility path for variable-based proxy_pass upstream resolution without changing existing Nginx templates or default behavior.

Implementation: add environment flag NPM_PRE_RESOLVE_UPSTREAM_HOSTS (disabled by default); pre-resolve eligible upstream hostnames via dns.lookup() while generating proxy host and custom location configs; fall back to original hostname when resolution fails.

Result: when enabled, setups using docker-compose extra_hosts (for example host.docker.internal) can avoid resolver host-not-found failures; when disabled, behavior remains backward compatible.
Add an Advanced Configuration section describing when to enable NPM_PRE_RESOLVE_UPSTREAM_HOSTS, example compose env usage, fallback behavior on resolution failure, and operational notes about generation-time resolution.
Add optional textarea inputs for certificate, certificate key, and intermediate certificate in CustomCertificateModal.

At submit time, pasted PEM values are converted to File objects and uploaded via existing multipart API, so backend routes remain unchanged.

Keep file upload path intact and require certificate + certificate key from either file input or pasted text before validation/upload.
@nginxproxymanagerci
Copy link
Copy Markdown

Docker Image for build 1 is available on DockerHub:

nginxproxymanager/nginx-proxy-manager-dev:pr-5348

Note

Ensure you backup your NPM instance before testing this image! Especially if there are database changes.
This is a different docker image namespace than the official image.

Warning

Changes and additions to DNS Providers require verification by at least 2 members of the community!

@jc21 jc21 added the requires-verification Waiting for one or more people to confirm the fix label Feb 24, 2026
@jc21
Copy link
Copy Markdown
Member

jc21 commented May 14, 2026

Code Review

Backend: preResolveUpstreamHost (backend/internal/nginx.js)

Positives:

  • Correctly opt-in via env flag (default disabled) — zero behavior change for existing users.
  • Safe fallback on DNS failure.
  • canPreResolveUpstreamHost guard against variables ($), paths (/), ports (:), and literal IPs is well-considered.

Issues:

1. Single-address resolution can break load-balanced upstreams (Medium)
dns.lookup with all: false returns only one IP. If the upstream hostname resolves to multiple addresses (e.g. a service behind round-robin DNS), pre-resolution pins config to a single IP, silently degrading load balancing. Worth documenting this limitation.

2. Config staleness in dynamic environments (Medium)
The staleness caveat is documented, but deserves a stronger warning. In Docker environments with recreated containers, the pre-resolved IP in the static Nginx config will silently route to a dead container until the proxy host config is manually regenerated — which is likely the most common use-case for this feature.

3. generateConfig mixes async patterns inconsistently (Low)
renderLocations uses await for preResolveUpstreamHost, but generateConfig continues with .then() chains. The forwardHostPromise also mutates the passed-in host object as a side effect inside a .then(). A unified async/await approach would be cleaner and less error-prone.

4. Port-suffixed hosts silently excluded (Low)
canPreResolveUpstreamHost excludes hosts containing : (to skip host:8080 style upstreams). This is correct but undocumented — worth a note since users enabling this feature might expect host.docker.internal:8080 to be pre-resolved.

5. No unit tests added (Low)
preResolveUpstreamHostEnabled, canPreResolveUpstreamHost, and preResolveUpstreamHost are pure/near-pure functions that are straightforward to unit test. Given this touches config generation, tests would add meaningful confidence.


Frontend: CustomCertificateModal.tsx

Positives:

  • buildPemFile helper is clean and reusable.
  • Reuses existing backend API contract without changes — good for backward compatibility.
  • Manual validation in onSubmit correctly replaces the now-removed required HTML attribute.

Issues:

1. No "OR" affordance between file input and textarea (High — UX)
File upload and textarea are rendered one after the other with no visual separator, label, or "OR" divider. A user who provides both will silently have the file take precedence (per buildPemFile's if (file) return file logic). This silent override should be communicated clearly.

2. Textareas have no <label> elements (Medium — Accessibility)
The three new <textarea> fields (certificateText, certificateKeyText, intermediateCertificateText) have no associated <label>. The file inputs above them have labels; the textareas do not. This breaks screen reader support and keyboard navigation for these fields.

3. Noisy indentation changes in the diff (Low)
A large portion of the diff is indentation-only changes on pre-existing <Field> blocks. This makes the diff harder to review and risks obscuring real changes. Ideally pre-existing code wouldn't be reformatted in the same PR.

4. No client-side PEM format validation (Low — UX)
The placeholder -----BEGIN CERTIFICATE----- is a helpful hint, but a lightweight check (e.g. trimmed content starts with -----BEGIN) before submit would catch obvious paste errors before hitting the backend.


Documentation (docs/src/advanced-config/index.md)

Clean and accurate. Suggest adding:

  • A note that hosts with explicit ports (e.g. host.docker.internal:8080) are not eligible for pre-resolution.
  • A note that multi-address upstreams will be pinned to a single IP.

Summary

Area Concern Severity
Backend Single-IP resolution breaks load-balanced upstreams Medium
Backend Config staleness warning needs strengthening in docs Medium
Backend Async pattern inconsistency in generateConfig Low
Backend No unit tests for new utility functions Low
Frontend No "OR" separator; silent file-over-text override High
Frontend Textareas missing <label> — accessibility issue Medium
Frontend Noisy indentation-only diff changes Low

The core approach is sound — opt-in flag with safe fallback for the backend, reusing the existing API contract for the frontend. The two main items to address before merge: UX clarity around file vs. paste priority in the modal, and a docs/behaviour note about single-IP resolution affecting load-balanced upstreams.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requires-verification Waiting for one or more people to confirm the fix

Projects

None yet

2 participants