🐛 Fix: the HTTP-01 challenge directory created by acme.sh may block subsequent uninstall.#2060
🐛 Fix: the HTTP-01 challenge directory created by acme.sh may block subsequent uninstall.#2060southp wants to merge 1 commit intoTryGhost:mainfrom
acme.sh may block subsequent uninstall.#2060Conversation
WalkthroughThis PR inserts a new step into the Nginx extension SSL setup sequence that ensures the web root directory exists (calls Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extensions/nginx/test/extension-spec.js (1)
403-412: Consider asserting the path argument for better test coverage.The test verifies that
ensureDiris called, but doesn't verify it's called with the correctrootPath. This would make the test more robust against regressions.🔧 Suggested improvement
return tasks[3].task().then(() => { expect(ensureDirStub.calledOnce).to.be.true; + expect(ensureDirStub.calledWithExactly('/var/www/ghost/system/nginx-root')).to.be.true; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/nginx/test/extension-spec.js` around lines 403 - 412, The test currently checks that ensureDir was called but not that it was called with the expected web root path; update the assertion after calling tasks[3].task() to verify ensureDirStub was invoked with the correct rootPath value (use the same rootPath used by proxyNginx/getTasks or derive it from ext), e.g. assert ensureDirStub.calledOnceWith(expectedRootPath). Locate the ensureDir stub assignment (proxy['fs-extra'].ensureDir = ensureDirStub), the proxyNginx(proxy) call, and the tasks[3].task() invocation to add the calledWith assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@extensions/nginx/test/extension-spec.js`:
- Around line 403-412: The test currently checks that ensureDir was called but
not that it was called with the expected web root path; update the assertion
after calling tasks[3].task() to verify ensureDirStub was invoked with the
correct rootPath value (use the same rootPath used by proxyNginx/getTasks or
derive it from ext), e.g. assert ensureDirStub.calledOnceWith(expectedRootPath).
Locate the ensureDir stub assignment (proxy['fs-extra'].ensureDir =
ensureDirStub), the proxyNginx(proxy) call, and the tasks[3].task() invocation
to add the calledWith assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5356aabc-145f-4ddf-8141-6251c649d54b
📒 Files selected for processing (2)
extensions/nginx/index.jsextensions/nginx/test/extension-spec.js
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Free Tier Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
…g up. `acme.sh` is run by `root` and will create the working directory, i.e. webroothere, used for acme challenge if it doesn't exist. When that happens, the webroot directory will be owned by root, making the subsequent `ghost uninstall` fail. This commit precreates that directory before running `acme.sh`, so the subsequent clean-up can be carried out as expected.
southp
force-pushed
the
fix/pre-create-acme-working-directory
branch
from
cb972e4 to
6c55d06
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extensions/nginx/test/extension-spec.js (1)
422-422: Consider resolving setup tasks bytitleinstead of array index.Adding one task required renumbering a large part of this suite. Looking tasks up by
titlewould make future sequencing changes much less brittle.♻️ Possible cleanup
function getTasks(ext, argv = {}) { ext.setupSSL(Object.assign({argv}, ctx)); expect(ext.ui.log.called, 'getTasks: ui.log').to.be.false; expect(ext.ui.listr.calledOnce, 'getTasks: ui.listr').to.be.true; - return ext.ui.listr.args[0][0]; + const tasks = ext.ui.listr.args[0][0]; + return { + all: tasks, + byTitle(title) { + return tasks.find(task => task.title === title); + } + }; }-const tasks = getTasks(ext, {sslemail: 'test@example.com', sslstaging: true}); -return tasks[4].task().then(() => { +const {byTitle} = getTasks(ext, {sslemail: 'test@example.com', sslstaging: true}); +return byTitle('Getting SSL Certificate from Let\'s Encrypt').task().then(() => {Also applies to: 446-448, 460-461, 482-483, 495-496, 528-528, 547-547, 561-561, 576-576
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/nginx/test/extension-spec.js` at line 422, The tests are brittle because they call setup tasks by numeric indices (e.g., tasks[4].task()) — change these to find the task object by its title and invoke its task() method instead (e.g., tasks.find(t => t.title === 'Your Task Title').task()). Update every occurrence where tasks is indexed (including the other instances noted) to use a title-based lookup and add a small assertion or guard that the found task exists before calling .task() to avoid runtime errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@extensions/nginx/test/extension-spec.js`:
- Line 422: The tests are brittle because they call setup tasks by numeric
indices (e.g., tasks[4].task()) — change these to find the task object by its
title and invoke its task() method instead (e.g., tasks.find(t => t.title ===
'Your Task Title').task()). Update every occurrence where tasks is indexed
(including the other instances noted) to use a title-based lookup and add a
small assertion or guard that the found task exists before calling .task() to
avoid runtime errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a435c36d-6708-44ea-96d2-18aa8ced16d7
📒 Files selected for processing (2)
extensions/nginx/index.jsextensions/nginx/test/extension-spec.js
1 participant
Summary
Ghost CLI uses
acme.shto perform HTTP-01 challenge for automating SSL provisioning. The script is run byroot, and will create its own working directory,system/nginx-root, if it doesn't exist. That means the directory will also owned byroot. Thus, when a user performsghost uninstalllater, the process may fail as they'd likely do it using a non-root account.This PR fixes the issue by pre-creating the web root directory, since the directory doesn't really need to be owned by the root.
Test plan
./node_modules/.bin/mocha --recursive extensions/**/testshould pass.ghost installunder a non-root account.ghost uninstallshould succeed.Not sure if there is any better way of testing this. It would be great if there is a way to run the test suite partially through
yarn testand a way to test the server setup part in a local sandboxed environment. These both are out of scope of this PR, though :)Note
Low Risk
Small, localized change to the SSL setup task list plus test updates; no changes to certificate generation logic or auth/security behavior beyond directory creation.
Overview
Prevents
ghost uninstallfailures after SSL provisioning by creatingsystem/nginx-rootup front duringsetupSSL, ensuring it isn’t first created/owned byrootwhenacme.shruns.Updates the Nginx extension unit tests to cover the new “create web root directory” listr task and adjusts task indices for subsequent SSL setup steps (
acme.generate, dhparam, headers, config, restart).Written by Cursor Bugbot for commit 6c55d06. This will update automatically on new commits. Configure here.