fix(api-service): Validate metadata keys, use correct signature header#10792
Merged
fix(api-service): Validate metadata keys, use correct signature header#10792
Conversation
Add strict validation for metadata signal keys (regex, forbidden keys, max length) and expose isValidMetadataSignalKey; update DTO validator and runtime handler to reject unsafe keys to prevent prototype pollution. Use HttpHeaderKeysEnum.NOVU_SIGNATURE when sending bridge requests so the framework can verify HMACs. Add SSRF validation for bridgeUrl/devBridgeUrl in UpdateAgent via validateUrlSsrf and assertSafeBridgeUrl to prevent pointing the bridge at internal or metadata endpoints.
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: PR title must end with 'fixes TICKET-ID' (e.g., 'fixes NOV-123') or include ticket ID in branch name |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds strict metadata signal key validation and rejects undefined metadata values; adds per-request SSRF validation for bridge URLs (update and runtime); and standardizes the bridge signature header constant. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/api/src/app/agents/usecases/update-agent/update-agent.usecase.ts (1)
130-132: Format the early return per the project rule.
if (!url) return;violates the required blank line before return statements. As per coding guidelines, “Include a blank line before everyreturnstatement.”🎨 Proposed formatting adjustment
- if (!url) return; + if (!url) { + + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/update-agent/update-agent.usecase.ts` around lines 130 - 132, In the assertSafeBridgeUrl method, adjust the early-return formatting to satisfy the project's rule by inserting a blank line immediately before the return statement; locate the private async assertSafeBridgeUrl(url: string | undefined | null, field: string) function and add a blank line before the existing "return" so the method follows "Include a blank line before every return statement" style guideline.
🤖 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/agents/dtos/agent-reply-payload.dto.ts`:
- Around line 61-62: Update the defaultMessage() in AgentReplyPayloadDto (method
defaultMessage) so the metadata key error text precisely reflects the regex:
keys must be 1–128 characters long, consist of letters, digits and the
separators "-", "_" and ":", must not begin or end with a separator, and must
not contain consecutive separators; keep the note that trigger signals require
workflowId. Make the new message concise but include these exact constraints so
rejected keys are clearer to users.
In
`@apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts`:
- Around line 275-287: The runtime path currently validates only signal.key but
allows signal.value to be undefined which gets assigned into merged and later
omitted by JSON.stringify; update the logic in handle-agent-reply.usecase.ts
around the signals loop to mirror the DTO by rejecting undefined metadata
values: when iterating signals (same loop that calls isValidMetadataSignalKey),
check signal.value !== undefined and throw BadRequestException with a clear
message for invalid/undefined values, and only assign merged[signal.key] =
signal.value if the value passes this check.
In `@apps/api/src/app/agents/usecases/update-agent/update-agent.usecase.ts`:
- Around line 39-44: The update-time SSRF checks (assertSafeBridgeUrl for
bridgeUrl and devBridgeUrl) are insufficient because
BridgeExecutorService.fireWithRetries performs the actual fetch without runtime
validation; add an immediate SSRF validation call inside
BridgeExecutorService.fireWithRetries just before any fetch() (reusing
assertSafeBridgeUrl or an exported validator) to re-validate the URL(s) being
fetched (bridgeUrl and devBridgeUrl) and fail fast if invalid, ensuring the
fetch cannot be invoked on DNS-rebound or pre-existing unsafe addresses.
---
Nitpick comments:
In `@apps/api/src/app/agents/usecases/update-agent/update-agent.usecase.ts`:
- Around line 130-132: In the assertSafeBridgeUrl method, adjust the
early-return formatting to satisfy the project's rule by inserting a blank line
immediately before the return statement; locate the private async
assertSafeBridgeUrl(url: string | undefined | null, field: string) function and
add a blank line before the existing "return" so the method follows "Include a
blank line before every return statement" style guideline.
🪄 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: 60de1d1c-80a0-4ea3-8c14-918870590d63
📒 Files selected for processing (4)
apps/api/src/app/agents/dtos/agent-reply-payload.dto.tsapps/api/src/app/agents/services/bridge-executor.service.tsapps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.tsapps/api/src/app/agents/usecases/update-agent/update-agent.usecase.ts
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
1 participant
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.
Add strict validation for metadata signal keys (regex, forbidden keys, max length) and expose isValidMetadataSignalKey; update DTO validator and runtime handler to reject unsafe keys to prevent prototype pollution. Use HttpHeaderKeysEnum.NOVU_SIGNATURE when sending bridge requests so the framework can verify HMACs. Add SSRF validation for bridgeUrl/devBridgeUrl in UpdateAgent via validateUrlSsrf and assertSafeBridgeUrl to prevent pointing the bridge at internal or metadata endpoints.
What changed? Why was the change needed?
What changed
This PR hardens agent bridge handling and metadata processing to fix security gaps: it enforces strict validation on metadata signal keys (pattern, length, and forbidden prototype-related keys) to prevent prototype pollution; it switches bridge request signatures to the framework's HttpHeaderKeysEnum.NOVU_SIGNATURE so HMACs can be verified; and it adds SSRF checks for bridgeUrl/devBridgeUrl to stop agents pointing at internal or metadata endpoints. Validation is applied at DTO level and at runtime when handling agent replies.
Affected areas
api: Added and exported isValidMetadataSignalKey and tightened DTO validation for agent reply payloads; updated handle-agent-reply to reject unsafe or undefined metadata keys before merging; updated bridge-executor to use HttpHeaderKeysEnum.NOVU_SIGNATURE for outbound requests and to run per-request SSRF validation; added SSRF validation and assertSafeBridgeUrl checks in update-agent use case; updated e2e tests to cover SSRF rejection and adjusted fixture URLs.
Key technical decisions
Testing
Added/updated e2e tests for bridge URL management to assert SSRF rejections and adjusted bridge URL fixtures; recommend unit tests for the new isValidMetadataSignalKey behavior (regex, length, forbidden keys) and for runtime rejection of undefined metadata values.
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer