fix(api-service): add SSRF validation to HTTP step test endpoint fixes NV-7467#10959
Merged
fix(api-service): add SSRF validation to HTTP step test endpoint fixes NV-7467#10959
Conversation
Co-authored-by: Dima Grossman <dima@grossman.io>
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
📝 WalkthroughWalkthrough
ChangesSSRF Validation + Test updates
Sequence DiagramsequenceDiagram
participant Client
participant Usecase
participant Validator
participant Secrets
participant HTTPClient
participant ExternalAPI
Client->>Usecase: request to test endpoint (resolvedUrl, method, body, headers)
Usecase->>Validator: validateUrlSsrf(resolvedUrl)
alt validation fails
Validator-->>Usecase: error
Usecase-->>Client: 400 { error, resolvedRequest, durationMs, headers: {} }
else validation passes
Validator-->>Usecase: ok
Usecase->>Secrets: decrypt secret key
Secrets-->>Usecase: decrypted key
Usecase->>Usecase: build novu-signature header
Usecase->>HTTPClient: send outbound HTTP request (with signature)
HTTPClient->>ExternalAPI: outbound call
ExternalAPI-->>HTTPClient: response
HTTPClient-->>Usecase: response or HttpClientError
Usecase-->>Client: formatted response
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Co-authored-by: Dima Grossman <dima@grossman.io>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/workflows-v2/e2e/test-http-endpoint.e2e.ts`:
- Around line 18-27: The test currently builds the SSRF URL using
process.env.PORT which can be undefined; change the test to derive the port from
the running test server instead of process.env.PORT (e.g. replace the literal
URL construction that uses `process.env.PORT` with one built from the test
server instance like `server.address().port` or use the test framework's helper
that returns the base URL), so the GET to `/v1/health-check` always targets the
actual listening port; update the URL string in the request invocation (the code
that constructs `http://localhost:${process.env.PORT}/v1/health-check`) to use
the server-derived port and keep the existing `response` assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a557e7b7-59a0-42ea-89ce-5661b95ad055
📒 Files selected for processing (2)
apps/api/src/app/workflows-v2/e2e/test-http-endpoint.e2e.tsapps/api/src/app/workflows-v2/usecases/test-http-endpoint/test-http-endpoint.usecase.ts
Comment on lines
18
to
+27
| url: `http://localhost:${process.env.PORT}/v1/health-check`, | ||
| method: 'GET', | ||
| }, | ||
| }); | ||
|
|
||
| expect(response.status).to.equal(201); | ||
| expect(response.body.data.statusCode).to.equal(400); | ||
| expect(response.body.data.body).to.deep.include({ | ||
| error: 'Requests to "localhost" are not allowed.', | ||
| }); |
Contributor
There was a problem hiding this comment.
Make the SSRF regression test independent of process.env.PORT.
If process.env.PORT is unset, this URL can become invalid and fail with a different error than the intended localhost SSRF rejection.
Proposed fix
- url: `http://localhost:${process.env.PORT}/v1/health-check`,
+ url: 'http://localhost/v1/health-check',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| url: `http://localhost:${process.env.PORT}/v1/health-check`, | |
| method: 'GET', | |
| }, | |
| }); | |
| expect(response.status).to.equal(201); | |
| expect(response.body.data.statusCode).to.equal(400); | |
| expect(response.body.data.body).to.deep.include({ | |
| error: 'Requests to "localhost" are not allowed.', | |
| }); | |
| url: 'http://localhost/v1/health-check', | |
| method: 'GET', | |
| }, | |
| }); | |
| expect(response.status).to.equal(201); | |
| expect(response.body.data.statusCode).to.equal(400); | |
| expect(response.body.data.body).to.deep.include({ | |
| error: 'Requests to "localhost" are not allowed.', | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app/workflows-v2/e2e/test-http-endpoint.e2e.ts` around lines 18
- 27, The test currently builds the SSRF URL using process.env.PORT which can be
undefined; change the test to derive the port from the running test server
instead of process.env.PORT (e.g. replace the literal URL construction that uses
`process.env.PORT` with one built from the test server instance like
`server.address().port` or use the test framework's helper that returns the base
URL), so the GET to `/v1/health-check` always targets the actual listening port;
update the URL string in the request invocation (the code that constructs
`http://localhost:${process.env.PORT}/v1/health-check`) to use the
server-derived port and keep the existing `response` assertions unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The workflow HTTP step test endpoint resolved Liquid templates into a URL and called
HttpClientService.request()without the same SSRF checks as production (ExecuteHttpRequestStep).Changes
validateUrlSsrffrom@novu/application-generic.validateUrlSsrf(resolvedUrl).resolvedRequestincludes the resolved body when present (same shape as success paths).https://httpbin.org/post) for happy-path body tests; add a regression test thathttp://localhost:...returnsstatusCode400 with the SSRF error.Testing
localhost, which SSRF correctly blocks; e2e updated accordingly.Linear Issue: NV-7467
What changed
The API's workflow HTTP step test endpoint now runs SSRF validation on the resolved URL (and related resolved body/headers) before decrypting secrets or making outbound requests. This closes a security gap where test requests previously bypassed production SSRF checks; on validation failure the endpoint returns HTTP 400 with the SSRF error and does not perform the outbound call.
Affected areas
api: TestHttpEndpointUsecase imports and calls validateUrlSsrf after Liquid resolution and before secret lookup/signature creation; on failure it returns a 400 response and includes the resolvedRequest payload (url, method, headers, and body when applicable). E2E tests were updated to use a public HTTPS endpoint for happy-path tests and a new regression test asserts localhost is rejected.
Key technical decisions
Testing
E2E updates added: happy-path tests now target https://httpbin.org/post and a regression test verifies localhost URLs return 400 with the SSRF error; CI shard failures caused by prior localhost tests were addressed. No new npm dependencies or enterprise changes were introduced.