Allow updating custom certificates from the UI#5649
Conversation
|
Docker Image for build 2 is available on DockerHub: Note Ensure you backup your NPM instance before testing this image! Especially if there are database changes. Warning Changes and additions to DNS Providers require verification by at least 2 members of the community! |
toviszsolt
left a comment
There was a problem hiding this comment.
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.
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:
POST /nginx/certificates/{certID}/uploadendpoint, 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).id: number | "new"pattern as the other host modals, anduseCertificatemirrorsuseProxyHost's handling of"new".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
AI Usage