Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive webhook ingress system with Redis-backed idempotency, BullMQ-based job queuing, observability hooks, signature verification, and dependency injection. The core API is refactored from a basic Express bootstrap to a modular, testable architecture with extensive configuration options and error handling across the monorepo. Changes
Sequence DiagramsequenceDiagram
actor Client
participant API as API Server
participant Redis as Redis<br/>(Idempotency Store)
participant BullMQ as BullMQ<br/>(Job Queue)
participant Observability as Observability<br/>Hooks
Client->>API: POST /webhook<br/>(with signature & payload)
API->>Observability: onIngressStart()
alt Signature Verification
API->>API: Verify HMAC-SHA256
alt Valid Signature
API->>API: Extract eventKey<br/>from payload
else Invalid/Missing Signature
API->>Observability: onVerificationFailure()
API->>Client: 401 Unauthorized
end
end
alt Payload Validation
API->>API: Parse & validate JSON<br/>Check size limits
alt Invalid Payload
API->>Observability: onMalformedPayload()
API->>Client: 400 Bad Request
end
end
alt Idempotency Check
API->>Redis: setIfNotExists(dedupe_key,<br/>ttl_seconds)
alt Key Already Exists
API->>Observability: onDuplicateHit()
API->>Client: 200 OK (deduplicated)
else Key Inserted
API->>BullMQ: enqueue(IngressJobPayload)
alt Enqueue Success
API->>Observability: onEnqueueSuccess()
API->>Client: 202 Accepted
else Enqueue Failure
API->>Observability: onEnqueueFailure()
API->>Redis: delete(dedupe_key)
API->>Client: 500 Server Error
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 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.
Actionable comments posted: 12
🤖 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/index.js`:
- Around line 397-399: The /ready route currently always returns OK; change the
app.get('/ready', ...) handler to perform the same dependency checks used by the
POST '/webhook' path (e.g., verify Redis client connectivity and BullMQ queue
readiness or call the same helper used by webhook enqueuing), and return 200
only if those checks succeed otherwise return 503 with an error body explaining
which dependency failed; reuse existing functions or clients referenced by the
POST '/webhook' logic so readiness mirrors webhook runtime requirements.
- Around line 269-270: The adminRateLimitStore and webhookRateLimitStore
currently grow unbounded because keys are never deleted; replace them with a
TTL-backed cache or add an expiry wrapper plus a periodic sweeper: either swap
new Map() for a small dependency like node-cache or lru-cache configured with a
TTL, or keep the Map but store objects {count, expiresAt} and add a setInterval
cleanup that deletes expired keys; apply the same change to the other rate-limit
buckets used elsewhere in this file (the other Map-based rate-limit stores) and
ensure any increment/check logic (where these stores are read or updated) uses
the expiry semantics so stale entries get removed or reused.
- Around line 309-316: The isHttpsRequest helper reads the X-Forwarded-Proto
header directly (allowing spoofing when Express trust proxy is not enabled);
replace that logic so isHttpsRequest only relies on req.secure and stop parsing
req.headers['x-forwarded-proto'] directly. If you actually intend to honor
forwarded headers, configure Express trust proxy from TRUST_PROXY (e.g.
app.set('trust proxy', process.env.TRUST_PROXY)) and document that in
.env.example; otherwise remove the header parsing and return req.secure in
isHttpsRequest.
- Around line 195-210: The Redis client returned by createRedisIdempotencyStore
must register an 'error' listener before calling redis.connect(); modify
createRedisIdempotencyStore so that immediately after const redis =
createClient({ url: redisUrl }) you add a noop or logging listener on
redis.on('error', ...) to prevent uncaught error throws, keep ensureConnected
and connectPromise logic unchanged but ensure the listener is attached prior to
invoking redis.connect() inside ensureConnected; reference
createRedisIdempotencyStore, the local redis variable, and ensureConnected when
making this change.
- Around line 227-234: The createBullMqIngressQueue function is passing
connection: { url: redisUrl } which ioredis ignores; instead import/construct an
ioredis client (e.g., new Redis(redisUrl)) and pass that instance as the
connection to Queue (the Queue call that uses INGRESS_QUEUE_NAME and
defaultJobOptions). Update createBullMqIngressQueue to create the Redis
instance, pass connection: redisInstance into the Queue constructor, and ensure
the Redis import (Redis or ioredis) is present and the client lifecycle is
managed as appropriate.
In `@apps/api/src/index.test.ts`:
- Around line 170-179: Await the server's 'listening' event after calling
app.listen(0) and await the 'close' event when tearing down to avoid race
conditions: import once from 'node:events', replace the immediate call to
server.address() with await once(server, 'listening') then call server.address()
and use its port for baseUrl, and when cleaning up call server.close() and await
once(server, 'close') (or await a Promise wrapper around server.close) so the
server is fully started before reading address and fully closed before test
exit; refer to the server variable, app.listen(0), server.address(),
server.close(), and the once helper.
In `@apps/api/src/index.ts`:
- Around line 548-550: The readiness endpoint implemented in the
app.get('/ready') handler must verify external dependencies (Redis and BullMQ)
before returning ok; modify the '/ready' route handler to perform health checks
against the Redis client used for idempotency key reservation and the BullMQ
Queue/QueueScheduler or Worker used for enqueuing jobs (i.e., call their
ping/health/status methods or attempt a lightweight operation such as PING for
Redis and getJobCounts/getRepeatableJobs or queue.client.ping for BullMQ) and
return a non-200 / { ok: false } response when either check fails so the process
is not marked ready if it cannot reserve idempotency keys or enqueue jobs.
Ensure the updated logic is in the same app.get('/ready', ...) handler and
logs/returns clear failure state.
- Around line 603-627: The current flow calls idempotencyStore.setIfNotExists
outside any guard and runs observability.onEnqueueSuccess inside the same try
that rolls back the dedupe key; fix by: 1) wrap idempotencyStore.setIfNotExists
in its own try/catch and call sendWebhookError on store failures so store errors
follow the structured error path; 2) perform ingressQueue.enqueue in a dedicated
try/catch that only deletes the dedupe key when the enqueue itself fails; and 3)
move observability.onEnqueueSuccess out of the enqueue failure path (or invoke
it in a separate try/catch that swallows/logs errors) so exceptions from
observability.onEnqueueSuccess cannot trigger idempotencyStore.delete and turn a
successful enqueue into a retried duplicate.
- Around line 258-263: The dedupe key only uses statuses[*].id (message id) so
different status updates for the same message collide; modify the block that
builds eventParts (the code using valueRecord.statuses, asRecord, and
statusRecord.id) to include the status transition identifier as well (e.g.,
append statusRecord.status or statusRecord.event or statusRecord.type if
present) into the key—fall back to the current id-only behavior only if no
transition field exists—so each distinct status change (sent/delivered/read)
produces a unique dedupe key.
- Around line 326-331: The setIfNotExists function uses ttlSeconds without
ensuring it is a positive integer; update the store boundary (setIfNotExists) to
validate and normalize ttlSeconds (from options.idempotencyTtlSeconds or any
caller) before calling redis.set: ensure it's a finite integer > 0 (e.g.,
parseNumber result must be > 0 and you should Math.floor/truncate decimals) and
throw or return an explicit error when invalid, so Redis SET EX never receives
zero, negative, or non-integer values; also update or centralize validation
logic used by parseNumber to enforce this rule.
- Around line 437-444: The isHttpsRequest helper bypasses Express's trust-proxy
protection by reading the raw 'x-forwarded-proto' header; update isHttpsRequest
to stop inspecting req.headers['x-forwarded-proto'] and rely solely on
req.secure (which respects the app's trust proxy setting) so that spoofed
headers cannot override HTTPS enforcement (keep the function name isHttpsRequest
and remove the manual header parsing logic).
In `@apps/dashboard/middleware.ts`:
- Around line 25-27: The current public-route check uses
pathname.startsWith(route) which allows prefixes like "/health" to match
"/healthz"; update the logic in the middleware where publicRoutes and pathname
are checked (the block that currently returns NextResponse.next()) to require
either an exact match (pathname === route) or a proper subpath boundary by
checking pathname.startsWith(route + "/") for routes that should allow nested
paths; ensure both checks are applied for each entry in publicRoutes so only the
declared path or its explicit subpaths are treated as public.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1b67db2-4f3d-4d15-a10f-32302a2a9d24
⛔ Files ignored due to path filters (7)
apps/api/src/index.d.ts.mapis excluded by!**/*.mapapps/api/src/index.js.mapis excluded by!**/*.mapapps/api/src/index.test.d.ts.mapis excluded by!**/*.mapapps/api/src/index.test.js.mapis excluded by!**/*.mappackage-lock.jsonis excluded by!**/package-lock.jsonpackages/config/src/index.d.ts.mapis excluded by!**/*.mappackages/config/src/index.js.mapis excluded by!**/*.map
📒 Files selected for processing (17)
apps/api/.env.exampleapps/api/package.jsonapps/api/src/index.d.tsapps/api/src/index.jsapps/api/src/index.test.d.tsapps/api/src/index.test.jsapps/api/src/index.test.tsapps/api/src/index.tsapps/dashboard/middleware.tsapps/worker/.env.exampleapps/worker/package.jsonapps/worker/tsconfig.jsoneslint.config.mjspackages/config/src/index.d.tspackages/config/src/index.jspackages/config/src/index.tsscripts/local-dev-down.sh
💤 Files with no reviewable changes (1)
- scripts/local-dev-down.sh
| export const createRedisIdempotencyStore = (redisUrl) => { | ||
| const redis = createClient({ url: redisUrl }); | ||
| let connectPromise = null; | ||
| const ensureConnected = async () => { | ||
| if (redis.isOpen) { | ||
| return; | ||
| } | ||
| if (!connectPromise) { | ||
| connectPromise = redis | ||
| .connect() | ||
| .then(() => undefined) | ||
| .finally(() => { | ||
| connectPromise = null; | ||
| }); | ||
| } | ||
| await connectPromise; |
There was a problem hiding this comment.
Register a Redis error listener before connecting.
createClient() is an EventEmitter, and node-redis explicitly requires at least one error listener. Without it, a transient Redis/network failure on this ingress path can throw and take down the API process instead of surfacing a controlled 5xx. (github.com)
🔧 Minimal hardening
export const createRedisIdempotencyStore = (redisUrl) => {
const redis = createClient({ url: redisUrl });
+ redis.on('error', (error) => {
+ console.error(JSON.stringify({
+ ts: new Date().toISOString(),
+ level: 'error',
+ event: 'redis.client.error',
+ error: error.message,
+ }));
+ });
let connectPromise = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.js` around lines 195 - 210, The Redis client returned by
createRedisIdempotencyStore must register an 'error' listener before calling
redis.connect(); modify createRedisIdempotencyStore so that immediately after
const redis = createClient({ url: redisUrl }) you add a noop or logging listener
on redis.on('error', ...) to prevent uncaught error throws, keep ensureConnected
and connectPromise logic unchanged but ensure the listener is attached prior to
invoking redis.connect() inside ensureConnected; reference
createRedisIdempotencyStore, the local redis variable, and ensureConnected when
making this change.
| export const createBullMqIngressQueue = (redisUrl) => { | ||
| const queue = new Queue(INGRESS_QUEUE_NAME, { | ||
| connection: { url: redisUrl }, | ||
| defaultJobOptions: { | ||
| removeOnComplete: true, | ||
| removeOnFail: false, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual code in the file
cd apps/api/src && cat -n index.js | sed -n '227,234p'Repository: Ryanakml/ChatFuse
Length of output: 378
🌐 Web query:
BullMQ connection configuration ioredis URL options
💡 Result:
BullMQ’s connection option is passed straight to ioredis (so you configure Redis exactly the way ioredis expects). (docs.bullmq.io)
What you can pass to connection
1) An ioredis options object
import { Queue } from "bullmq";
const queue = new Queue("myqueue", {
connection: {
host: "localhost",
port: 6379,
username: "default",
password: "secret",
db: 0,
// tls: { ... } // if needed
},
});BullMQ warns not to use ioredis keyPrefix; use BullMQ’s own prefix option instead. (docs.bullmq.io)
2) An existing ioredis client instance (best way to use a URL)
ioredis supports creating a client from a Redis URL string (URI). (ioredis.readthedocs.io)
import { Queue, Worker } from "bullmq";
import IORedis from "ioredis";
const redis = new IORedis(process.env.REDIS_URL!, {
// extra ioredis options here (e.g. tls, reconnect strategy, etc.)
});
const queue = new Queue("myqueue", { connection: redis });
const worker = new Worker("myqueue", async job => {}, {
connection: redis,
});Notes on “URL options”
- The URL itself can include
user:password@host:port/db(standard Redis URI form). (ioredis.readthedocs.io) - For other settings (TLS, retry strategies, timeouts, etc.), pass them in the second argument to
new IORedis(url, options)as shown above. (ioredis.readthedocs.io)
If you tell me your exact Redis URL (redacting secrets) and what you’re trying to set (TLS, Sentinel, Cluster, VPC, etc.), I can show the precise BullMQ + ioredis config.
Citations:
- 1: https://docs.bullmq.io/guide/connections
- 2: https://docs.bullmq.io/guide/connections
- 3: https://ioredis.readthedocs.io/en/latest/API/
- 4: https://ioredis.readthedocs.io/en/latest/API/
- 5: https://ioredis.readthedocs.io/en/latest/API/
🌐 Web query:
ioredis connection options url parameter format
💡 Result:
In ioredis, the “URL” (connection string) follows the standard Redis URI form:
- TCP (no TLS):
redis://[[username]:[password]@]host[:port][/db] - TCP (TLS):
rediss://[[username]:[password]@]host[:port][/db]
Examples from the official README:
new Redis("redis://:authpassword@127.0.0.1:6380/4"); // password + db
new Redis("redis://username:authpassword@127.0.0.1:6380/4"); // username + password + db
new Redis("rediss://redis.my-service.com"); // TLS via scheme[1]
Important about “URL parameters” (?key=value): ioredis documents the URL form as above (auth in authority, db in the path). For other connection options (e.g., tls settings, connectTimeout, etc.), pass them as the options object (either as the constructor argument, or as the 2nd argument when the 1st is a URL). [2]
Constructor shape (URL + options object):
new Redis("redis://user:pass@host:6379/0", { /* extra options like tls, timeouts */ });[2]
Sources:
[1] ioredis GitHub README (connection string examples, redis:// and rediss://) (github.com)
[2] ioredis API docs (new Redis([port], [host], [options]), URL string + options object) (ioredis.readthedocs.io)
Citations:
Pass BullMQ an ioredis client instance, not a url property in an options object.
BullMQ forwards the connection option directly to ioredis. ioredis accepts Redis URLs as a constructor string (e.g., new Redis("redis://..."), not as a url property in an options object. The current code passes connection: { url: redisUrl }, which ioredis will not recognize—causing it to ignore REDIS_URL and fall back to localhost defaults.
The correct approach is to create an ioredis instance first and pass it to BullMQ:
const redis = new Redis(redisUrl);
const queue = new Queue(INGRESS_QUEUE_NAME, {
connection: redis,
defaultJobOptions: { ... },
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.js` around lines 227 - 234, The createBullMqIngressQueue
function is passing connection: { url: redisUrl } which ioredis ignores; instead
import/construct an ioredis client (e.g., new Redis(redisUrl)) and pass that
instance as the connection to Queue (the Queue call that uses INGRESS_QUEUE_NAME
and defaultJobOptions). Update createBullMqIngressQueue to create the Redis
instance, pass connection: redisInstance into the Queue constructor, and ensure
the Redis import (Redis or ioredis) is present and the client lifecycle is
managed as appropriate.
| const adminRateLimitStore = new Map(); | ||
| const webhookRateLimitStore = new Map(); |
There was a problem hiding this comment.
Prune or externalize these rate-limit buckets.
Both stores only ever accumulate keys. Expired entries are overwritten on reuse but never deleted, so a spray of unique IPs can keep growing memory indefinitely and turn the limiter into a memory-DoS vector. A TTL-backed cache/store or periodic sweep would close that hole.
Also applies to: 341-356, 370-381
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.js` around lines 269 - 270, The adminRateLimitStore and
webhookRateLimitStore currently grow unbounded because keys are never deleted;
replace them with a TTL-backed cache or add an expiry wrapper plus a periodic
sweeper: either swap new Map() for a small dependency like node-cache or
lru-cache configured with a TTL, or keep the Map but store objects {count,
expiresAt} and add a setInterval cleanup that deletes expired keys; apply the
same change to the other rate-limit buckets used elsewhere in this file (the
other Map-based rate-limit stores) and ensure any increment/check logic (where
these stores are read or updated) uses the expiry semantics so stale entries get
removed or reused.
| const isHttpsRequest = (req) => { | ||
| const forwardedProto = req.headers['x-forwarded-proto']; | ||
| if (typeof forwardedProto === 'string') { | ||
| const proto = forwardedProto.split(',')[0] ?? ''; | ||
| return proto.trim() === 'https'; | ||
| } | ||
| return req.secure; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and understand the context around line 309-316
cd apps/api/src && wc -l index.js && echo "---" && sed -n '300,325p' index.jsRepository: Ryanakml/ChatFuse
Length of output: 1014
🏁 Script executed:
# Search for express app initialization and trust proxy configuration
rg "trust proxy" --type js -B 2 -A 2Repository: Ryanakml/ChatFuse
Length of output: 359
🏁 Script executed:
# Check if there's any proxy configuration in the codebase
rg "app\.set\(" --type js -A 1 | head -50Repository: Ryanakml/ChatFuse
Length of output: 216
🏁 Script executed:
# Look for any environment-specific or deployment configuration
find . -name "*.json" -o -name "*.env*" -o -name "*.config.*" | grep -E "(package|env|config)" | head -20Repository: Ryanakml/ChatFuse
Length of output: 613
🏁 Script executed:
# Search for trustProxy variable definition
rg "trustProxy" --type js -B 5 -A 2Repository: Ryanakml/ChatFuse
Length of output: 1156
🏁 Script executed:
# Check the .env.example to understand configuration
cat apps/api/.env.exampleRepository: Ryanakml/ChatFuse
Length of output: 386
🏁 Script executed:
# Search for usage of isHttpsRequest function
rg "isHttpsRequest" --type js -B 2 -A 2Repository: Ryanakml/ChatFuse
Length of output: 613
🏁 Script executed:
# Also check if there are any security-related tests or comments
rg "https|secure|proxy" --type js -i | grep -E "(test|spec|check)" | head -20Repository: Ryanakml/ChatFuse
Length of output: 704
Use req.secure instead of reading X-Forwarded-Proto directly.
The current code bypasses Express's trust proxy security boundary by directly reading the X-Forwarded-Proto header, even when trust proxy is disabled. Since TRUST_PROXY defaults to false and is not configured in .env.example, untrusted clients can spoof the header with X-Forwarded-Proto: https to bypass the HTTPS check. Express only treats forwarded headers as safe when trust proxy is explicitly configured to own them.
Safer implementation
- const isHttpsRequest = (req) => {
- const forwardedProto = req.headers['x-forwarded-proto'];
- if (typeof forwardedProto === 'string') {
- const proto = forwardedProto.split(',')[0] ?? '';
- return proto.trim() === 'https';
- }
- return req.secure;
- };
+ const isHttpsRequest = (req) => req.secure;📝 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.
| const isHttpsRequest = (req) => { | |
| const forwardedProto = req.headers['x-forwarded-proto']; | |
| if (typeof forwardedProto === 'string') { | |
| const proto = forwardedProto.split(',')[0] ?? ''; | |
| return proto.trim() === 'https'; | |
| } | |
| return req.secure; | |
| }; | |
| const isHttpsRequest = (req) => req.secure; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.js` around lines 309 - 316, The isHttpsRequest helper
reads the X-Forwarded-Proto header directly (allowing spoofing when Express
trust proxy is not enabled); replace that logic so isHttpsRequest only relies on
req.secure and stop parsing req.headers['x-forwarded-proto'] directly. If you
actually intend to honor forwarded headers, configure Express trust proxy from
TRUST_PROXY (e.g. app.set('trust proxy', process.env.TRUST_PROXY)) and document
that in .env.example; otherwise remove the header parsing and return req.secure
in isHttpsRequest.
| app.get('/ready', (_req, res) => { | ||
| res.json({ ok: true }); | ||
| }); |
There was a problem hiding this comment.
Make /ready reflect webhook dependencies.
This endpoint stays green even when Redis/BullMQ are unavailable, so orchestration can keep routing traffic to an instance that cannot dedupe or enqueue webhook events. /health can stay shallow, but /ready should exercise the dependencies the POST /webhook path actually needs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.js` around lines 397 - 399, The /ready route currently
always returns OK; change the app.get('/ready', ...) handler to perform the same
dependency checks used by the POST '/webhook' path (e.g., verify Redis client
connectivity and BullMQ queue readiness or call the same helper used by webhook
enqueuing), and return 200 only if those checks succeed otherwise return 503
with an error body explaining which dependency failed; reuse existing functions
or clients referenced by the POST '/webhook' logic so readiness mirrors webhook
runtime requirements.
| setIfNotExists: async (key, ttlSeconds) => { | ||
| await ensureConnected(); | ||
| const result = await redis.set(key, '1', { | ||
| EX: ttlSeconds, | ||
| NX: true, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/api/src/index.ts | sed -n '320,340p'Repository: Ryanakml/ChatFuse
Length of output: 579
🏁 Script executed:
# Search for where setIfNotExists is called to understand how ttlSeconds is provided
rg -A 3 -B 3 "setIfNotExists" apps/api/src/Repository: Ryanakml/ChatFuse
Length of output: 3214
🏁 Script executed:
# Check if there's any validation logic in the file or related files
rg "ttlSeconds|isInteger" apps/api/src/ -A 2 -B 2Repository: Ryanakml/ChatFuse
Length of output: 1396
🏁 Script executed:
# Find where idempotencyTtlSeconds is defined or set
rg "idempotencyTtlSeconds" apps/api/src/ -B 5 -A 2Repository: Ryanakml/ChatFuse
Length of output: 4574
🏁 Script executed:
# Check for any configuration or environment variable handling related to TTL
rg "TTL|ttl" apps/api/src/index.ts -B 2 -A 2Repository: Ryanakml/ChatFuse
Length of output: 964
🏁 Script executed:
# Find parseNumber function definition
rg "function parseNumber|const parseNumber|export.*parseNumber" apps/api/src/ -A 5Repository: Ryanakml/ChatFuse
Length of output: 654
🏁 Script executed:
# Also check imports at top of the file to see where parseNumber comes from
head -50 apps/api/src/index.ts | rg "import|parseNumber"Repository: Ryanakml/ChatFuse
Length of output: 363
Validate TTL is a positive integer before passing to Redis.
ttlSeconds lacks validation. The parseNumber function only checks finiteness, allowing 0, negative values, and decimals. Additionally, direct injection via options.idempotencyTtlSeconds bypasses parseNumber entirely. Redis SET with EX rejects non-positive values at runtime and silently truncates decimals, breaking idempotency semantics. Guard this at the store boundary so both env-driven and injected callers are protected.
Suggested fix
setIfNotExists: async (key, ttlSeconds) => {
+ if (!Number.isInteger(ttlSeconds) || ttlSeconds <= 0) {
+ throw new Error(`Invalid idempotency TTL: ${ttlSeconds}`);
+ }
await ensureConnected();
const result = await redis.set(key, '1', {
EX: ttlSeconds,
NX: true,
});📝 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.
| setIfNotExists: async (key, ttlSeconds) => { | |
| await ensureConnected(); | |
| const result = await redis.set(key, '1', { | |
| EX: ttlSeconds, | |
| NX: true, | |
| }); | |
| setIfNotExists: async (key, ttlSeconds) => { | |
| if (!Number.isInteger(ttlSeconds) || ttlSeconds <= 0) { | |
| throw new Error(`Invalid idempotency TTL: ${ttlSeconds}`); | |
| } | |
| await ensureConnected(); | |
| const result = await redis.set(key, '1', { | |
| EX: ttlSeconds, | |
| NX: true, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.ts` around lines 326 - 331, The setIfNotExists function
uses ttlSeconds without ensuring it is a positive integer; update the store
boundary (setIfNotExists) to validate and normalize ttlSeconds (from
options.idempotencyTtlSeconds or any caller) before calling redis.set: ensure
it's a finite integer > 0 (e.g., parseNumber result must be > 0 and you should
Math.floor/truncate decimals) and throw or return an explicit error when
invalid, so Redis SET EX never receives zero, negative, or non-integer values;
also update or centralize validation logic used by parseNumber to enforce this
rule.
| const isHttpsRequest = (req: express.Request) => { | ||
| const forwardedProto = req.headers['x-forwarded-proto']; | ||
| if (typeof forwardedProto === 'string') { | ||
| const proto = forwardedProto.split(',')[0] ?? ''; | ||
| return proto.trim() === 'https'; | ||
| } | ||
| return req.secure; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and check the full context around the suspicious function
wc -l apps/api/src/index.tsRepository: Ryanakml/ChatFuse
Length of output: 86
🏁 Script executed:
# Check the surrounding code in the file
sed -n '420,460p' apps/api/src/index.tsRepository: Ryanakml/ChatFuse
Length of output: 1213
🏁 Script executed:
# Search for trust proxy configuration in Express setup
rg -i "trust.*proxy" apps/api/src/ -A 2 -B 2Repository: Ryanakml/ChatFuse
Length of output: 1422
🏁 Script executed:
# Look for where isHttpsRequest is used
rg "isHttpsRequest" apps/api/src/ -A 3 -B 2Repository: Ryanakml/ChatFuse
Length of output: 1357
🏁 Script executed:
# Check if there's any proxy configuration in the main entry point
rg "app\.(use|set)" apps/api/src/index.ts | head -20Repository: Ryanakml/ChatFuse
Length of output: 153
🏁 Script executed:
# Search for any proxy-related middleware or header validation
rg "x-forwarded" apps/api/src/ -iRepository: Ryanakml/ChatFuse
Length of output: 230
🏁 Script executed:
# Check if there's any other security validation for the forwarded headers
rg "req\.headers" apps/api/src/index.ts | head -20Repository: Ryanakml/ChatFuse
Length of output: 121
🌐 Web query:
Express.js req.secure behavior trust proxy false x-forwarded-proto
💡 Result:
In Express, req.secure is essentially:
req.secure === (req.protocol === 'https')
And req.protocol behaves like this:
- If
trust proxyevaluates tofalse(default), Express does not trust/consultX-Forwarded-Proto; it determines the protocol from the direct connection to Node (i.e., whether the socket is TLS-encrypted). So if you’re behind a TLS-terminating reverse proxy / load balancer (client→proxy is HTTPS, proxy→Node is HTTP), thenreq.securewill befalseeven if the original request was HTTPS, even ifX-Forwarded-Proto: httpsis present. [1] - If
trust proxyis enabled, Express will useX-Forwarded-Proto(when present) to setreq.protocol, which makesreq.securereflect the original client protocol. [1][2]
To make req.secure respect X-Forwarded-Proto, you must do both:
- Configure your proxy to set/overwrite
X-Forwarded-Proto(and relatedX-Forwarded-*headers). - Enable Express trust proxy appropriately, e.g.:
app.set('trust proxy', true); // or 1, or a specific subnet/IP listExpress warns that enabling trust proxy means you’re trusting headers that can be spoofed unless your last trusted proxy strips/overwrites them. [2]
Sources: Express req.protocol behavior with trust proxy and X-Forwarded-Proto. [1][2]; Express “behind proxies” guidance and security note. [2]
[1] (jiwon-park.github.io)
[2] (expressjs.com)
Citations:
- 1: https://jiwon-park.github.io/en/5x/api.html?utm_source=openai
- 2: https://expressjs.com/en/guide/behind-proxies.html?utm_source=openai
🏁 Script executed:
# Check what happens in the middleware when isHttpsRequest returns false
sed -n '440,460p' apps/api/src/index.tsRepository: Ryanakml/ChatFuse
Length of output: 496
🏁 Script executed:
# Find the middleware that uses isHttpsRequest
sed -n '450,480p' apps/api/src/index.tsRepository: Ryanakml/ChatFuse
Length of output: 818
🏁 Script executed:
# Look for the complete HTTPS enforcement middleware
rg -A 10 "app.use.*isHttpsRequest" apps/api/src/index.tsRepository: Ryanakml/ChatFuse
Length of output: 43
🏁 Script executed:
# Search more broadly for the middleware that uses isHttpsRequest
rg -B 2 -A 8 "isHttpsRequest" apps/api/src/index.tsRepository: Ryanakml/ChatFuse
Length of output: 633
Don't read x-forwarded-proto directly—it bypasses the HTTPS enforcement gate.
A direct HTTP client can send X-Forwarded-Proto: https and pass the HTTPS check because this function reads the raw header regardless of the TRUST_PROXY setting. When TRUST_PROXY is false (the likely default), Express correctly ignores spoofed forwarded headers for req.secure, but this function circumvents that protection by checking the header first.
Use req.secure only, which respects the trust proxy configuration:
Suggested fix
- const isHttpsRequest = (req: express.Request) => {
- const forwardedProto = req.headers['x-forwarded-proto'];
- if (typeof forwardedProto === 'string') {
- const proto = forwardedProto.split(',')[0] ?? '';
- return proto.trim() === 'https';
- }
- return req.secure;
- };
+ const isHttpsRequest = (req: express.Request) => req.secure;📝 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.
| const isHttpsRequest = (req: express.Request) => { | |
| const forwardedProto = req.headers['x-forwarded-proto']; | |
| if (typeof forwardedProto === 'string') { | |
| const proto = forwardedProto.split(',')[0] ?? ''; | |
| return proto.trim() === 'https'; | |
| } | |
| return req.secure; | |
| }; | |
| const isHttpsRequest = (req: express.Request) => req.secure; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.ts` around lines 437 - 444, The isHttpsRequest helper
bypasses Express's trust-proxy protection by reading the raw 'x-forwarded-proto'
header; update isHttpsRequest to stop inspecting
req.headers['x-forwarded-proto'] and rely solely on req.secure (which respects
the app's trust proxy setting) so that spoofed headers cannot override HTTPS
enforcement (keep the function name isHttpsRequest and remove the manual header
parsing logic).
| app.get('/ready', (_req, res) => { | ||
| res.json({ ok: true }); | ||
| }); |
There was a problem hiding this comment.
Make /ready depend on Redis and BullMQ health.
This currently reports ready even if the process cannot reserve idempotency keys or enqueue jobs. That will route webhook traffic to an instance that is not actually able to serve it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.ts` around lines 548 - 550, The readiness endpoint
implemented in the app.get('/ready') handler must verify external dependencies
(Redis and BullMQ) before returning ok; modify the '/ready' route handler to
perform health checks against the Redis client used for idempotency key
reservation and the BullMQ Queue/QueueScheduler or Worker used for enqueuing
jobs (i.e., call their ping/health/status methods or attempt a lightweight
operation such as PING for Redis and getJobCounts/getRepeatableJobs or
queue.client.ping for BullMQ) and return a non-200 / { ok: false } response when
either check fails so the process is not marked ready if it cannot reserve
idempotency keys or enqueue jobs. Ensure the updated logic is in the same
app.get('/ready', ...) handler and logs/returns clear failure state.
| const firstSeen = await idempotencyStore.setIfNotExists(dedupeKey, idempotencyTtlSeconds); | ||
| if (!firstSeen) { | ||
| observability.onDuplicateHit(ingressContext, { | ||
| eventKey, | ||
| }); | ||
| res.status(200).json({ ok: true }); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await ingressQueue.enqueue({ | ||
| eventKey, | ||
| payload: coerceJsonValue(payload), | ||
| receivedAt: new Date().toISOString(), | ||
| }); | ||
| observability.onEnqueueSuccess(ingressContext, { | ||
| eventKey, | ||
| }); | ||
| } catch { | ||
| await idempotencyStore.delete(dedupeKey); | ||
| observability.onEnqueueFailure(ingressContext, { | ||
| eventKey, | ||
| errorCode: 'ENQUEUE_FAILED', | ||
| }); | ||
| sendWebhookError(res, 503, 'ENQUEUE_FAILED', 'Failed to enqueue webhook event'); |
There was a problem hiding this comment.
Keep observability out of the enqueue rollback path.
A throw from onEnqueueSuccess() after queue.add() succeeds will land in this catch, delete the dedupe key, and turn a successfully queued webhook into a retried duplicate. setIfNotExists() is also outside the guarded block, so store failures skip the structured webhook error path entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.ts` around lines 603 - 627, The current flow calls
idempotencyStore.setIfNotExists outside any guard and runs
observability.onEnqueueSuccess inside the same try that rolls back the dedupe
key; fix by: 1) wrap idempotencyStore.setIfNotExists in its own try/catch and
call sendWebhookError on store failures so store errors follow the structured
error path; 2) perform ingressQueue.enqueue in a dedicated try/catch that only
deletes the dedupe key when the enqueue itself fails; and 3) move
observability.onEnqueueSuccess out of the enqueue failure path (or invoke it in
a separate try/catch that swallows/logs errors) so exceptions from
observability.onEnqueueSuccess cannot trigger idempotencyStore.delete and turn a
successful enqueue into a retried duplicate.
| if (publicRoutes.some((route) => pathname.startsWith(route))) { | ||
| return NextResponse.next(); | ||
| } |
There was a problem hiding this comment.
Tighten public-route matching to avoid prefix auth bypass.
pathname.startsWith(route) makes /healthz and /health/... public too, which widens the unauthenticated surface beyond the declared route. Match the exact path, or an explicit subpath boundary, instead.
🔐 Suggested fix
- if (publicRoutes.some((route) => pathname.startsWith(route))) {
+ if (
+ publicRoutes.some(
+ (route) => pathname === route || pathname.startsWith(`${route}/`),
+ )
+ ) {
return NextResponse.next();
}📝 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.
| if (publicRoutes.some((route) => pathname.startsWith(route))) { | |
| return NextResponse.next(); | |
| } | |
| if ( | |
| publicRoutes.some( | |
| (route) => pathname === route || pathname.startsWith(`${route}/`), | |
| ) | |
| ) { | |
| return NextResponse.next(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/middleware.ts` around lines 25 - 27, The current public-route
check uses pathname.startsWith(route) which allows prefixes like "/health" to
match "/healthz"; update the logic in the middleware where publicRoutes and
pathname are checked (the block that currently returns NextResponse.next()) to
require either an exact match (pathname === route) or a proper subpath boundary
by checking pathname.startsWith(route + "/") for routes that should allow nested
paths; ensure both checks are applied for each entry in publicRoutes so only the
declared path or its explicit subpaths are treated as public.
1 participant
Summary by CodeRabbit
New Features
Chores