Changed webhook trigger default request behavior#27219
Conversation
ref https://linear.app/ghost/issue/ONC-1597/ - Webhook triggers now use `request-external` by default instead of `@tryghost/request` - Internal IP destinations can still be enabled with `security.allowWebhookInternalIPs` for self-hosted setups To allow webhooks to reach internal IP destinations, add to your Ghost config: ``` { "security": { "allowWebhookInternalIPs": true } } ```
WalkthroughA new security configuration flag 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/core/shared/config/env/config.testing.json (1)
17-19: Consider keeping the shared test baseline on the new default.Setting this to
truein the global testing configs means any webhook spec that forgets to override it will exercise@tryghost/requestinstead of the newrequest-externalpath. If only a small set of suites need localhost delivery, I'd opt them in locally so default test coverage stays aligned with production. The same applies toghost/core/core/shared/config/env/config.testing-mysql.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/shared/config/env/config.testing.json` around lines 17 - 19, The shared test config currently sets security.allowWebhookInternalIPs to true which causes tests to exercise the old internal webhook path; change security.allowWebhookInternalIPs back to the new default (false) in both config.testing.json and config.testing-mysql.json so suites that need localhost delivery must opt-in locally, i.e., update the security.allowWebhookInternalIPs value in the JSON objects referenced in those two files.ghost/core/test/integration/services/webhook-request.test.js (1)
65-96: These specs still don't exercise the new internal-IP contract.Both branches post to a public host, and the
falsebranch also switchesenvtodevelopment, so the tests can pass without ever proving that internal targets are rejected by default and allowed when the flag is enabled. A localhost/127.0.0.1case in each branch would lock down the behavior this config is meant to control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/services/webhook-request.test.js` around lines 65 - 96, Tests currently don't verify internal-IP behavior; both branches use a public WEBHOOK_TARGET and the false branch sets env=development which bypasses IP checks. Update the two test cases in webhook-request.test.js to include an additional sub-case each that targets a localhost address (e.g., http://127.0.0.1:PORT or http://localhost:PORT) and uses nock to intercept that host/path; for the branch where configUtils.set('security:allowWebhookInternalIPs', false) assert that the POST to localhost is rejected/not attempted (scope.isDone() should be false or trigger should throw) and for the branch with allowWebhookInternalIPs true assert the localhost POST is delivered successfully; keep setupWebhookModel(), instantiation of new WebhookTrigger({models, payload, limitService}) and trigger.trigger(WEBHOOK_EVENT, {}) the same so the tests exercise the internal-IP contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/shared/config/env/config.testing.json`:
- Around line 17-19: The shared test config currently sets
security.allowWebhookInternalIPs to true which causes tests to exercise the old
internal webhook path; change security.allowWebhookInternalIPs back to the new
default (false) in both config.testing.json and config.testing-mysql.json so
suites that need localhost delivery must opt-in locally, i.e., update the
security.allowWebhookInternalIPs value in the JSON objects referenced in those
two files.
In `@ghost/core/test/integration/services/webhook-request.test.js`:
- Around line 65-96: Tests currently don't verify internal-IP behavior; both
branches use a public WEBHOOK_TARGET and the false branch sets env=development
which bypasses IP checks. Update the two test cases in webhook-request.test.js
to include an additional sub-case each that targets a localhost address (e.g.,
http://127.0.0.1:PORT or http://localhost:PORT) and uses nock to intercept that
host/path; for the branch where
configUtils.set('security:allowWebhookInternalIPs', false) assert that the POST
to localhost is rejected/not attempted (scope.isDone() should be false or
trigger should throw) and for the branch with allowWebhookInternalIPs true
assert the localhost POST is delivered successfully; keep setupWebhookModel(),
instantiation of new WebhookTrigger({models, payload, limitService}) and
trigger.trigger(WEBHOOK_EVENT, {}) the same so the tests exercise the
internal-IP contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b030875a-108a-478e-b14d-bb00e8549a41
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
ghost/core/core/server/services/webhooks/webhook-trigger.jsghost/core/core/shared/config/defaults.jsonghost/core/core/shared/config/env/config.testing-mysql.jsonghost/core/core/shared/config/env/config.testing.jsonghost/core/test/integration/services/webhook-request.test.jsghost/core/test/unit/server/services/webhooks/trigger.test.js
2 participants
ref https://linear.app/ghost/issue/ONC-1597/
request-externalby default instead of@tryghost/requestsecurity.allowWebhookInternalIPsfor self-hosted setupsTo allow webhooks to reach internal IP destinations, add to your Ghost config: