Skip to content

Allow updating custom certificates from the UI#5649

Open
F1nal04 wants to merge 4 commits into
NginxProxyManager:developfrom
F1nal04:update-manual-certs
Open

Allow updating custom certificates from the UI#5649
F1nal04 wants to merge 4 commits into
NginxProxyManager:developfrom
F1nal04:update-manual-certs

Conversation

@F1nal04

@F1nal04 F1nal04 commented Jun 11, 2026

Copy link
Copy Markdown

Why

Replacing an expiring custom certificate currently requires deleting the certificate and re-uploading it as a new one, which means detaching and re-attaching it on every host that uses it. This PR adds an Edit action to the certificates table (custom certificates only) that opens the existing custom certificate modal in edit mode, so replacement files can be uploaded to the same certificate record in place.

Implementation notes:

  • Reuses the existing POST /nginx/certificates/{certID}/upload endpoint, which already supported replacing files on an existing certificate — no new API surface.
  • internalCertificate.upload() now reloads nginx when the edited certificate is attached to any proxy/redirection/404 host or stream, so the replacement certificate is served immediately (previously the old certificate kept being served from memory until an unrelated reload).
  • The custom certificate modal follows the same id: number | "new" pattern as the other host modals, and useCertificate mirrors useProxyHost's handling of "new".
  • Extends the cypress certificate lifecycle test to upload replacement files and assert the certificate's domain names update.

Verified end-to-end in the dev stack: create → edit/replace → expiry/domains update in UI and on disk (/data/custom_ssl/npm-{id}/), and an in-use certificate is served by nginx with the new files immediately after saving.

Related: #4425 tackles the same problem but adds a new PUT endpoint/schema and appears stalled; this PR is a smaller alternative that reuses the existing upload flow. Happy to close in favour of it if that one gets picked up again.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Code refactoring
  • API changes
  • Performance improvement
  • Test addition or update

AI Usage

  • AI was used to write this
  • AI was used to review this

@toviszsolt toviszsolt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I took a look at the code and gathered a few observations and suggestions to polish this up before merging, aiming for maximum security and a smoother UX.

  • Validation & Security
    Domain Matching: It's great that validateCertificate() runs before the upload. However, the API currently doesn't verify if the new certificate's domain and owner match the old ones, creating a potential risk for a domain mismatch.
    Suggestion: It would be safer to validate on the API side as well to ensure the new certificate's domain matches the expectations of the original record.

  • Error Handling & Nginx Reload
    Asynchronous Error Handling: Right now, if the upload succeeds but the Nginx reload fails, the UI shows a success state even though the server isn't actually running the new certificate. Additionally, a network error during editing leaves the component state a bit ambiguous.
    Suggestion: If the Nginx reload fails, the modal should display a clear warning to the user, and we should also clarify the component's fallback state after a network error.

Overall, the core logic looks super solid. If we patch these security and error cases, it's going to be completely bulletproof.

@F1nal04

F1nal04 commented Jul 3, 2026

Copy link
Copy Markdown
Author

Thanks for the review! I went through each point. Here's what I found and changed.

Owner matching: I traced this to be sure: upload() fetches the record via internalCertificate.get(access, …), which filters by owner_user_id for non-admin users, and update() re-checks the certificates:update permission. The owner is never modified during upload, so I don't think there's a gap here. Happy to look again if you had a specific path in mind.

Domain matching: Replacing domain_names with the uploaded cert's CN is pre-existing behavior on develop; the PR just made it reachable from the edit UI. I considered rejecting mismatches on the API side, but NPM doesn't enforce cert/host domain coverage anywhere (any cert can be attached to any host), and a hard rejection would block legitimate cases like domain migrations. Instead the edit modal now warns when the uploaded cert's domain differs from the record's and requires a second save to confirm. Verified in the browser: a mismatch blocks with the warning, a matching domain saves through. Worth noting: NPM only parses the subject CN, never SANs, so SAN-only changes stay invisible. That's a pre-existing data-model limitation I'd leave for a separate PR.

Nginx reload errors: The reload is awaited, not fire-and-forget. A failure rejects through the API and the modal stays open with an error alert, so the UI didn't report success even before this fix. What it showed, though, was just a generic "Internal Error" (the CommandError from the reload isn't public, so the error handler masks the details). I've wrapped that case in a public ConfigurationError so the user now sees what actually happened: "The certificate was saved but nginx could not be reloaded and may still be serving the previous certificate: …". I confirmed the flow by breaking the nginx config in my dev stack and uploading.

The new domain-check helper has unit tests and the existing Cypress certificate spec still passes. Let me know if you'd like a different approach on any of these.

@nginxproxymanagerci

Copy link
Copy Markdown

Docker Image for build 4 is available on DockerHub:

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

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!

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