feat(api-service): resolve auth from bridge resolver #10663
feat(api-service): resolve auth from bridge resolver #10663
Conversation
Co-authored-by: Dima Grossman <dima@grossman.io>
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
|
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds environmentId validation and per-environment secret-key resolution (cache lookup with fallback to decryption) in NovuBridgeClient.handleRequest, returns 400/404/500 on failure paths, initializes the Novu client with the resolved secretKey (strictAuthentication: true), and constructs NovuRequestHandler with that client. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant NovuBridgeClient
participant InMemoryLRUCacheService
participant GetDecryptedSecretKey
participant NovuClient
participant NovuRequestHandler
Caller->>NovuBridgeClient: handleRequest(req with environmentId)
NovuBridgeClient->>NovuBridgeClient: validate environmentId (400 if missing)
NovuBridgeClient->>InMemoryLRUCacheService: get(VALIDATOR, "bridge-secret-key:${environmentId}", ...)
alt cache hit (valid string)
InMemoryLRUCacheService-->>NovuBridgeClient: secretKey
else cache miss / invalid
NovuBridgeClient->>GetDecryptedSecretKey: execute(GetDecryptedSecretKeyCommand{environmentId})
alt decrypted successfully
GetDecryptedSecretKey-->>InMemoryLRUCacheService: secretKey (cache)
InMemoryLRUCacheService-->>NovuBridgeClient: secretKey
else NotFoundException
GetDecryptedSecretKey-->>NovuBridgeClient: NotFound
else other error
GetDecryptedSecretKey-->>NovuBridgeClient: error
end
end
alt secret resolution error (NotFound)
NovuBridgeClient-->>Caller: 404
else other resolution error or invalid secret
NovuBridgeClient-->>Caller: 500
else resolved
NovuBridgeClient->>NovuClient: initialize(secretKey, strictAuthentication: true)
NovuClient-->>NovuBridgeClient: client instance
NovuBridgeClient->>NovuRequestHandler: construct with client and workflow
NovuRequestHandler-->>NovuBridgeClient: handler ready
NovuBridgeClient-->>Caller: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
scopsy
changed the title
Co-authored-by: Dima Grossman <dima@grossman.io>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/environments-v1/novu-bridge-client.ts`:
- Around line 57-71: Validate that req.params.environmentId is present and
non-empty before using it (environmentId), then call inMemoryLRUCacheService.get
as before but remove the unsafe "as string" cast and check the returned value
from getDecryptedSecretKey.execute (invoked via GetDecryptedSecretKeyCommand)
for null/undefined; if the secret is missing or retrieval throws, log the error
and return/throw an appropriate HTTP error (400/404 for missing environmentId or
secret, 500 for retrieval failures) instead of constructing the Client with a
falsy secret. Ensure errors include contextual details and reference
InMemoryLRUCacheStore.VALIDATOR and the cache key
`bridge-secret-key:${environmentId}` in messages to aid debugging.
🪄 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: 7cac9e7a-08a4-48d7-b6ab-fee064014715
📒 Files selected for processing (1)
apps/api/src/app/environments-v1/novu-bridge-client.ts
|
@cursoragent fix review comments |
|
Here’s what I changed to address the CodeRabbit review. Updates in
|
- Reject missing/blank environmentId with 400 before workflow or cache work - Resolve secret without unsafe cast; treat empty result as 500 with context - Map NotFoundException from GetDecryptedSecretKey to 404; log other errors with InMemoryLRUCacheStore.VALIDATOR and bridge-secret-key cache key for debugging Co-authored-by: Dima Grossman <dima@grossman.io>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/api/src/app/environments-v1/novu-bridge-client.ts (3)
69-87: Note: Cached secrets remain valid for up to 1 hour after rotation.Per the
VALIDATORstore configuration, the cache TTL is 1 hour. If an environment's secret key is rotated, the old (now invalid) secret may continue to be served from cache, causing HMAC validation to fail for up to 60 minutes.Consider whether this is acceptable for your secret rotation strategy, or if you need to add cache invalidation on secret rotation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/environments-v1/novu-bridge-client.ts` around lines 69 - 87, Cached secrets are currently fetched via inMemoryLRUCacheService.get using InMemoryLRUCacheStore.VALIDATOR (cacheKey = `bridge-secret-key:${environmentId}`) which has a 1-hour TTL, so rotated secrets may remain valid in cache for up to 60 minutes; to fix, either (a) call the cache invalidation method for that store/key when secret rotation occurs (invalidate/remove `bridge-secret-key:${environmentId}` from InMemoryLRUCacheStore.VALIDATOR), or (b) reduce the TTL for this cache entry or use a different cacheVariant so rotation takes effect immediately; update the rotation flow that calls getDecryptedSecretKey.execute / GetDecryptedSecretKeyCommand to perform the chosen invalidation or TTL change.
1-1: UsePinoLoggerfrom@novu/application-genericinstead ofLoggerfrom@nestjs/common.The static analysis indicates the project standardizes on
PinoLoggerfor consistent structured logging across the API. This provides better observability with JSON output and correlation IDs.♻️ Proposed fix
-import { Inject, Logger, NotFoundException } from '@nestjs/common'; +import { Inject, NotFoundException } from '@nestjs/common'; import { GetDecryptedSecretKey, GetDecryptedSecretKeyCommand, InMemoryLRUCacheService, InMemoryLRUCacheStore, + PinoLogger, } from '@novu/application-generic';Then update the logger initialization:
export class NovuBridgeClient { - private readonly logger = new Logger(NovuBridgeClient.name); + constructor( + `@Inject`(NovuHandler) private novuHandler: NovuHandler, + private constructFrameworkWorkflow: ConstructFrameworkWorkflow, + private getDecryptedSecretKey: GetDecryptedSecretKey, + private inMemoryLRUCacheService: InMemoryLRUCacheService, + private logger: PinoLogger + ) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/environments-v1/novu-bridge-client.ts` at line 1, Replace the use of Nest's Logger with the project's PinoLogger: change the import to pull PinoLogger from '@novu/application-generic' instead of Logger from '@nestjs/common', update any constructor or property injection that currently types or injects Logger to use PinoLogger, and ensure any Logger.log/debug/error calls on the NovuBridge client (e.g., in the class that references Logger) continue to work with PinoLogger methods and structured JSON output.
89-99: Consider reducing internal details in error responses.The error response includes internal implementation details like store names and cache keys. While useful for debugging, this could provide unnecessary information to potential attackers in production.
♻️ Suggested: Use generic client-facing message, keep details in logs
if (typeof resolved !== 'string' || !resolved.trim()) { this.logger.error( `Bridge secret key missing or invalid after cache lookup (store=${storeName}, cacheKey=${cacheKey}, environmentId=${environmentId})` ); res.status(500).json({ error: 'Failed to resolve environment secret key', - details: `Empty or invalid secret from ${storeName} for cache key ${cacheKey}.`, + details: 'Internal configuration error. Please contact support if this persists.', }); return; }Apply similar changes to other error responses at lines 107-110 and 119-122.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/environments-v1/novu-bridge-client.ts` around lines 89 - 99, The response currently leaks internal details (storeName, cacheKey, environmentId) to the client; update the handler so res.status(500).json returns a generic client-facing message like { error: 'Failed to resolve environment secret key' } while moving the detailed context into this.logger.error (include storeName, cacheKey, environmentId and any error/variable values like resolved) for diagnostics; apply the same change pattern to the other error responses referenced (the blocks around the conditions at lines 107-110 and 119-122) so logs keep internals but HTTP responses remain generic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/app/environments-v1/novu-bridge-client.ts`:
- Around line 69-87: Cached secrets are currently fetched via
inMemoryLRUCacheService.get using InMemoryLRUCacheStore.VALIDATOR (cacheKey =
`bridge-secret-key:${environmentId}`) which has a 1-hour TTL, so rotated secrets
may remain valid in cache for up to 60 minutes; to fix, either (a) call the
cache invalidation method for that store/key when secret rotation occurs
(invalidate/remove `bridge-secret-key:${environmentId}` from
InMemoryLRUCacheStore.VALIDATOR), or (b) reduce the TTL for this cache entry or
use a different cacheVariant so rotation takes effect immediately; update the
rotation flow that calls getDecryptedSecretKey.execute /
GetDecryptedSecretKeyCommand to perform the chosen invalidation or TTL change.
- Line 1: Replace the use of Nest's Logger with the project's PinoLogger: change
the import to pull PinoLogger from '@novu/application-generic' instead of Logger
from '@nestjs/common', update any constructor or property injection that
currently types or injects Logger to use PinoLogger, and ensure any
Logger.log/debug/error calls on the NovuBridge client (e.g., in the class that
references Logger) continue to work with PinoLogger methods and structured JSON
output.
- Around line 89-99: The response currently leaks internal details (storeName,
cacheKey, environmentId) to the client; update the handler so
res.status(500).json returns a generic client-facing message like { error:
'Failed to resolve environment secret key' } while moving the detailed context
into this.logger.error (include storeName, cacheKey, environmentId and any
error/variable values like resolved) for diagnostics; apply the same change
pattern to the other error responses referenced (the blocks around the
conditions at lines 107-110 and 119-122) so logs keep internals but HTTP
responses remain generic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce1dcbc5-e3b3-45bf-b483-b22ae3f05056
📒 Files selected for processing (1)
apps/api/src/app/environments-v1/novu-bridge-client.ts
Node disallows --no-experimental-strip-types in NODE_OPTIONS, which caused pnpm test / npx mocha to exit before running specs. Remove it from api test scripts and .mocharc.json; ts-node still transpiles via TS_NODE_TRANSPILE_ONLY. Co-authored-by: Dima Grossman <dima@grossman.io>
GetPreferences can use InMemoryLRUCacheStore.WORKFLOW_PREFERENCES for WORKFLOW_RESOURCE + USER_WORKFLOW rows. UpsertPreferences wrote to Mongo without clearing that cache, so subsequent reads could return stale data and fail tests/CI when preference-fetch optimization is enabled. Co-authored-by: Dima Grossman <dima@grossman.io>
…ler types) - Replace Nest Logger with injected PinoLogger per API lint rules - Import SharedModule in NovuBridgeModule so LoggerModule/PinoLogger resolves - Cast framework createHandler to Express req/res without explicit any Co-authored-by: Dima Grossman <dima@grossman.io>
- Remove unused SENSITIVE_KEYS from payload-sanitizer (Biome error in nx lint) - Replace require() with dynamic import in get-portal-link e2e for ESM compatibility Co-authored-by: Dima Grossman <dima@grossman.io>
GitHub Actions sets CI=true and NODE_ENV=test; loading newrelic without NEW_RELIC_APP_NAME throws at import time and fails @novu/api-service tests. Use a noop agent in that environment for metrics and cron modules, matching clickhouse-batch.service.ts. Co-authored-by: Dima Grossman <dima@grossman.io>
in-memory-provider/redis-provider used a static newrelic import, which still loaded the agent without NEW_RELIC_APP_NAME and failed CI. Use loadNewRelicOrNoopInCiTest and extend the noop type for optional instrumentDatastore. Co-authored-by: Dima Grossman <dima@grossman.io>
require("newrelic") throws during module init when NEW_RELIC_APP_NAME is
unset, so the decorator try/catch never caught it. Use
loadNewRelicOrNoopInCiTest and add startSegment to the noop agent.
Co-authored-by: Dima Grossman <dima@grossman.io>
Node 22 can execute .ts via experimental type-stripping before ts-node, which fails on decorator syntax. Spawn mocha with --no-experimental-strip-types only when Node major >= 22 so local Node 20 stays valid. Apply the same flag to the novu-v2 e2e shard runner. Co-authored-by: Dima Grossman <dima@grossman.io>
|
@cursoragent revery any change that is not relevat to the original issue, there should be no changes in test suite or setup, etc... |
|
Here is what I did for @scopsy’s request to drop everything that is not part of the original bridge-auth work. Reverted / removed (no longer in the PR vs
|
Restore api test scripts, e2e, payload-sanitizer, preferences cache, and application-generic New Relic shims to match next. Removes run-mocha-unit.cjs and load-newrelic-ci-test-safe.ts per review scope. Co-authored-by: Dima Grossman <dima@grossman.io>
Co-authored-by: Dima Grossman <dima@grossman.io>
2 participants
What changed? Why was the change needed?
The Novu bridge client (
NovuBridgeClient) was updated to enforce framework-level signature validation for incoming requests. This is achieved by resolving the target environment's decrypted API secret key and passing it to the framework client withstrictAuthentication: true.This change was needed to ensure that all requests to the Novu bridge endpoint are properly authenticated and signed using the environment's API key, addressing a previously identified unauthenticated execution path.
What changed
Novu Bridge now enforces environment-scoped signature validation: the NovuBridgeClient resolves the target environment’s decrypted API secret key at request time (with an in-memory LRU cache) and passes it to the framework Client with strictAuthentication: true. This forces incoming bridge requests to be signed with the environment’s API key, closing a previously unauthenticated execution path.
Affected areas
api — apps/api: NovuBridgeClient (environments-v1) now validates environmentId early, resolves and caches per-environment secret keys via GetDecryptedSecretKey and InMemoryLRUCacheService, and constructs the framework Client with the resolved secret and strictAuthentication enabled.
api (configs/tests) — apps/api: removed the NODE_OPTIONS test flag from Mocha config and adjusted test npm scripts (test script/config cleanup only).
libs/application-generic — added InMemoryLRUCacheService injection to UpsertPreferences and added cache invalidation for workflow preferences when relevant.
Key technical decisions
Testing
No new tests were added in this change; existing API/bridge e2e tests and manual verification are expected to validate that worker and dashboard preview flows continue to generate valid environment-specific bridge signatures.
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer
This change leverages the existing framework authentication mechanism. Both the worker and dashboard preview flows already generate bridge request signatures using the correct environment-specific secret keys, ensuring compatibility with this update.
Slack Thread