Feat/dashboard backfill - #67
Conversation
…agAmount for soft delete
|
Warning Review limit reached
More reviews will be available in 14 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds soft-delete columns to tags and expressions, implements corresponding storage upsert/delete/list operations, introduces HTTP handlers for tags, expressions, API keys, webhook endpoints/deliveries with Zod validation and auth, updates onboarding auth, and registers all new routes. ChangesAPI Expansion with Soft-Delete Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 8
🧹 Nitpick comments (2)
src/routes/http/api/expressions.ts (2)
92-110: 💤 Low valueAvoid reporting expected client errors (
AuthError,ZodError) to Sentry.
Sentry.captureExceptionruns unconditionally before theAuthError/ZodErrorbranches, so every 401 and 400 client-side validation/auth failure is sent to Sentry as an exception, creating noise that masks genuine 5xx faults. Consider capturing only after ruling out the expected error types.♻️ Capture only unexpected errors
} catch (error) { - Sentry.captureException(error, { - extra: { context: "create expression route handler" }, - }); - if (error instanceof AuthError) { builder.setError(401, { type: error.type, message: error.message }); reply.code(401); return { error: error.message }; } if (error instanceof ZodError) { const issues = error.issues .map((issue) => `${issue.path.join(".")}: ${issue.message}`) .join("; "); builder.setError(400, { type: "ValidationError", message: issues }); reply.code(400); return { error: issues }; } const err = error instanceof Error ? error : new Error(String(error)); + Sentry.captureException(err, { + extra: { context: "create expression route handler" }, + }); builder.setError(500, { type: "InternalError", message: err.message });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/http/api/expressions.ts` around lines 92 - 110, The Sentry.captureException call currently runs for all errors including expected AuthError and ZodError; update the catch block in the create expression route handler so you first check if (error instanceof AuthError) and if (error instanceof ZodError) and handle those (builder.setError / reply.code / return) before calling Sentry.captureException, and only call Sentry.captureException(error, { extra: { context: "create expression route handler" } }) in the final else / default branch for unexpected errors.
135-136: ⚡ Quick winValidate
params.keywith Zod instead of casting.
request.paramsis cast to{ key: string }without validation, so a missing/emptykeyflows straight intodeleteExpression. Validating with a small Zod schema (consistent withhandleCreateExpression) removes the cast and rejects bad input with a 400.As per coding guidelines: "Use Zod schemas for all request validation" and "avoid
anyand do NOT cast which can be inferred".♻️ Validate route params
+const deleteExpressionParamsSchema = z.object({ + key: z.string().min(1).max(128), +});- const params = request.params as { key: string }; - const deleted = await deleteExpression(params.key); + const params = deleteExpressionParamsSchema.parse(request.params); + const deleted = await deleteExpression(params.key);This requires adding a
ZodError→ 400 branch in this handler'scatchblock, mirroringhandleCreateExpression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/http/api/expressions.ts` around lines 135 - 136, Replace the unsafe cast of request.params with a Zod-validated params schema (e.g., create a z.object({ key: z.string().min(1) }) similar to handleCreateExpression) and parse request.params before calling deleteExpression so that a missing/empty key is rejected; remove the cast to { key: string } and pass the validated key to deleteExpression, and update the handler's catch block to detect ZodError and return a 400 response (mirroring the ZodError → 400 branch in handleCreateExpression).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/http/api/apiKeys.ts`:
- Around line 60-77: Wrap the API key creation and webhook provisioning in a
single Drizzle transaction so both writes succeed or fail together: inside
handleCreateApiKey replace the separate await createApiKey(...) and await
upsertWebhookEndpoint(...) calls with a single transaction that creates the key
(createApiKey logic) and, if validated.webhookUrl, generates the keypair
(generateWebhookKeyPair) and upserts the webhook endpoint
(upsertWebhookEndpoint), and only call
invalidateWebhookEndpointCache(keyRecord.id) after the transaction commits;
additionally, in handleRevokeApiKey stop casting request.params and validate the
id with a Zod schema (return 400 on validation failure) before performing any DB
revoke logic.
- Around line 199-208: Replace the unsafe cast of request.params with a
Zod-validated params object (e.g., const paramsSchema = z.object({ id:
z.string().uuid() }) and parse request.params) before calling getPostgresDB()
and executing db.update(apiKeysTable). Catch ZodError and return a 400 response
(matching the other handlers in this file) so malformed UUIDs never reach the
update(...) call that sets revoked/revokedAt using DateTime.utc().toISO(); do
not call db.update when validation fails.
In `@src/routes/http/api/tags.ts`:
- Around line 138-139: Replace the unchecked cast of request.params with runtime
validation: create a Zod schema (e.g., tagParamsSchema = z.object({ key:
z.string() })) and parse request.params using tagParamsSchema.parse or safeParse
in the same style as handleCreateTag; on ZodError return a 400 response (or
convert to the same domain error response flow) and only call
deleteTag(params.key) with the validated key. Ensure you reference
request.params, deleteTag, and the new tagParamsSchema (or use Fastify route
generics) so the route validates input before DB operations.
In `@src/routes/http/api/webhookDeliveries.ts`:
- Around line 37-46: The current filter compares query.apiKeyId to
webhookDeliveriesTable.endpointId; change the logic to first find endpoint ids
from webhookEndpointsTable where apiKeyId equals the incoming query.apiKeyId
(e.g., SELECT id FROM webhookEndpointsTable WHERE apiKeyId = query.apiKeyId) and
then filter webhookDeliveriesTable by endpointId IN those ids (use
webhookDeliveriesTable.endpointId IN (...) or a join between
webhookDeliveriesTable and webhookEndpointsTable filtering on
webhookEndpointsTable.apiKeyId). Also update the request parsing path: wrap
listDeliveriesQuerySchema.parse(request.query) so that if it throws a ZodError
you catch it (distinct from AuthError) and map it to a 400 validation
error/response instead of falling through to the generic 500; reference
listDeliveriesQuerySchema.parse and ZodError in the handler catch logic to
ensure Zod validation failures are handled explicitly.
- Around line 34-67: The handler's call to
listDeliveriesQuerySchema.parse(request.query) can throw ZodError but only
AuthError is handled; update the error handling in handleListDeliveries to
detect ZodError (import from zod if needed), build a concatenated issues message
from error.issues (matching other routes), then call builder.setError(400, {
type: "ValidationError", message: concatenatedMessage }), set reply.code(400)
and return { error: concatenatedMessage }; keep the existing AuthError and
generic 500 branches unchanged.
In `@src/storage/db/postgres/helpers/expressions.ts`:
- Around line 51-68: createExpression currently does a non-atomic
select-then-update/insert against expressionsTable (checking isNull(deletedAt))
which can lead to duplicate active rows under concurrency; add a partial unique
index on expressionsTable.key where deleted_at IS NULL in your migrations and
replace the select-then-insert flow in createExpression with an atomic upsert
(INSERT ... ON CONFLICT(key) DO UPDATE SET expr=EXCLUDED.expr) or wrap the logic
in a transaction with appropriate row-level lock handling and explicitly catch
unique-constraint violations to handle races; reference expressionsTable,
createExpression, deletedAt and handle the conflict error path rather than
relying on .limit(1).
In `@src/storage/db/postgres/helpers/tags.ts`:
- Around line 29-45: Wrap the select/insert/update in a single Drizzle
transaction using db.transaction so the upsert for tagsTable is atomic: inside
the transaction, do a SELECT ... WHERE key = key FOR UPDATE (using tagsTable and
db) and if you find an active row update its amount and clear tagCache; if you
find a soft-deleted row revive it by updating amount and nulling deletedAt;
otherwise INSERT the new row; additionally catch unique-constraint errors on the
INSERT and, in that error handler, perform an UPDATE by id for the conflicting
key to avoid duplicate active rows; always delete tagCache for the key after the
write.
In `@src/storage/db/postgres/schema.ts`:
- Around line 265-273: The expressions table lost the uniqueness guarantee on
active keys—restore a partial unique constraint on the key column for
non-deleted rows: add a unique index on expressions.key where deleted_at IS NULL
(reference: expressionsTable, the key column and deletedAt/timestamp
"deleted_at") and add the same partial unique index in the corresponding
migration; ensure any upsert/insert code paths handle unique-constraint errors
explicitly (catch unique violation and handle according to existing upsert
logic) and perform the schema change inside a migration transaction per project
guidelines.
---
Nitpick comments:
In `@src/routes/http/api/expressions.ts`:
- Around line 92-110: The Sentry.captureException call currently runs for all
errors including expected AuthError and ZodError; update the catch block in the
create expression route handler so you first check if (error instanceof
AuthError) and if (error instanceof ZodError) and handle those (builder.setError
/ reply.code / return) before calling Sentry.captureException, and only call
Sentry.captureException(error, { extra: { context: "create expression route
handler" } }) in the final else / default branch for unexpected errors.
- Around line 135-136: Replace the unsafe cast of request.params with a
Zod-validated params schema (e.g., create a z.object({ key: z.string().min(1) })
similar to handleCreateExpression) and parse request.params before calling
deleteExpression so that a missing/empty key is rejected; remove the cast to {
key: string } and pass the validated key to deleteExpression, and update the
handler's catch block to detect ZodError and return a 400 response (mirroring
the ZodError → 400 branch in handleCreateExpression).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 67c23753-c02a-4f5c-955c-1e2e560112fa
📒 Files selected for processing (11)
protosrc/routes/http/api/apiKeys.tssrc/routes/http/api/expressions.tssrc/routes/http/api/onboarding.tssrc/routes/http/api/registerApiRoutes.tssrc/routes/http/api/tags.tssrc/routes/http/api/webhookDeliveries.tssrc/storage/db/postgres/helpers/expressions.tssrc/storage/db/postgres/helpers/tags.tssrc/storage/db/postgres/schema.tssrc/utils/fetchTagAmount.ts
| const params = request.params as { id: string }; | ||
| const db = getPostgresDB(); | ||
| const now = DateTime.utc().toISO(); | ||
|
|
||
| const result = await db | ||
| .update(apiKeysTable) | ||
| .set({ revoked: true, revokedAt: now }) | ||
| .where( | ||
| and(eq(apiKeysTable.id, params.id), eq(apiKeysTable.revoked, false)) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and print the relevant section around the cited lines
FILE="src/routes/http/api/apiKeys.ts"
if [ ! -f "$FILE" ]; then
echo "File not found: $FILE"
exit 1
fi
echo "== File: $FILE =="
# Show a bit more context around lines 180-240
nl -ba "$FILE" | sed -n '160,240p'Repository: ScrawnDotDev/Scrawn
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/routes/http/api/apiKeys.ts"
nl -ba "$FILE" | sed -n '160,240p'Repository: ScrawnDotDev/Scrawn
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/routes/http/api/apiKeys.ts"
wc -l "$FILE"
nl -ba "$FILE" | sed -n '180,230p'Repository: ScrawnDotDev/Scrawn
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/routes/http/api/apiKeys.ts"
nl -ba "$FILE" | sed -n '1,260p' | tail -n +170 | head -n 120Repository: ScrawnDotDev/Scrawn
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/routes/http/api/apiKeys.ts"
nl -ba "$FILE" | sed -n '190,215p'Repository: ScrawnDotDev/Scrawn
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/routes/http/api/apiKeys.ts"
nl -ba "$FILE" | sed -n '190,215p'Repository: ScrawnDotDev/Scrawn
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/routes/http/api/apiKeys.ts"
echo "== $FILE =="
nl -ba "$FILE" | sed -n '170,240p'
echo
echo "== Search for revoke handler and Zod schemas in this file =="
rg -n "handleRevokeApiKey|revokeApiKey|revokeApiKeyParamsSchema|z\.object|request\.params as" "$FILE" || trueRepository: ScrawnDotDev/Scrawn
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/routes/http/api/apiKeys.ts"
echo "== File: $FILE =="
awk 'NR>=160 && NR<=240 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "== Search patterns in $FILE =="
rg -n "handleRevokeApiKey|revokeApiKey|revokeApiKeyParamsSchema|z\.object|request\.params as" "$FILE" || trueRepository: ScrawnDotDev/Scrawn
Length of output: 2791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/routes/http/api/apiKeys.ts"
echo "== Any Zod schemas in this file =="
rg -n "z\.object|safeParse|ZodError|parse\(" "$FILE" || true
echo
echo "== Look for revoke-related schemas/usages =="
rg -n "revoke|revokedAt|params" "$FILE" || trueRepository: ScrawnDotDev/Scrawn
Length of output: 865
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/routes/http/api/apiKeys.ts"
echo "== Any Zod schemas in this file =="
rg -n "z\.object|safeParse|ZodError|parse\(" "$FILE" || true
echo
echo "== Look for revoke-related schemas/usages =="
rg -n "revoke|revokedAt|params" "$FILE" || trueRepository: ScrawnDotDev/Scrawn
Length of output: 865
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/routes/http/api/apiKeys.ts"
awk 'NR>=1 && NR<=130 {printf "%d\t%s\n", NR, $0}' "$FILE"Repository: ScrawnDotDev/Scrawn
Length of output: 4644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the definition of apiKeysTable (and the type of its id column)
rg -n "export const apiKeysTable|apiKeysTable\s*=" -S src/storage/db/postgres/schema* src/storage/db/postgres/schema || true
rg -n "apiKeysTable\.id" -S src/storage/db/postgres/schema src/storage/db/postgres -g'*.ts' || trueRepository: ScrawnDotDev/Scrawn
Length of output: 1221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/storage/db/postgres/schema.ts"
awk 'NR>=60 && NR<=120 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "== Check apiKeysTable.id column type =="
rg -n "apiKeysTable\s*=\s*pgTable|id:\s*[^,]+|uuid|text\(" "$FILE"Repository: ScrawnDotDev/Scrawn
Length of output: 4811
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "handleRevokeApiKey" src/routes src || trueRepository: ScrawnDotDev/Scrawn
Length of output: 537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/routes/http/api/registerApiRoutes.ts"
awk 'NR>=60 && NR<=140 {printf "%d\t%s\n", NR, $0}' "$FILE"Repository: ScrawnDotDev/Scrawn
Length of output: 2357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/routes/http/api/registerApiRoutes.ts"
rg -n "schema|params|z\.object|Zod|uuid" "$FILE" || trueRepository: ScrawnDotDev/Scrawn
Length of output: 45
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "instanceof ZodError|ZodError" src | head -n 50Repository: ScrawnDotDev/Scrawn
Length of output: 2429
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "request\.params as|parse\(request\.params\)" src/routes/http/api || trueRepository: ScrawnDotDev/Scrawn
Length of output: 330
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for FILE in src/routes/http/api/tags.ts src/routes/http/api/expressions.ts; do
echo "==== $FILE (params casting area) ===="
awk 'NR>=110 && NR<=190 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "== ZodError handling in $FILE =="
rg -n "if\s*\(\s*error\s+instanceof\s+ZodError|ZodError" "$FILE"
echo
doneRepository: ScrawnDotDev/Scrawn
Length of output: 4417
Validate api-keys/:id with Zod (no request.params cast) and return 400 for invalid UUIDs.
src/storage/db/postgres/schema.ts defines apiKeysTable.id as a UUID (uuid("id")), so const params = request.params as { id: string } can let malformed ids hit the update query and end up as a 500. Parse params with Zod and map ZodError to a 400 like the other handlers in this file.
🛠️ Suggested change
+const revokeApiKeyParamsSchema = z.object({
+ id: z.string().uuid("Invalid API key ID"),
+});
+
export async function handleRevokeApiKey(
request: FastifyRequest,
reply: FastifyReply
): Promise<Record<string, unknown> | { error: string }> {
@@
- const params = request.params as { id: string };
+ const params = revokeApiKeyParamsSchema.parse(request.params);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/http/api/apiKeys.ts` around lines 199 - 208, Replace the unsafe
cast of request.params with a Zod-validated params object (e.g., const
paramsSchema = z.object({ id: z.string().uuid() }) and parse request.params)
before calling getPostgresDB() and executing db.update(apiKeysTable). Catch
ZodError and return a 400 response (matching the other handlers in this file) so
malformed UUIDs never reach the update(...) call that sets revoked/revokedAt
using DateTime.utc().toISO(); do not call db.update when validation fails.
| try { | ||
| const existing = await db | ||
| .select({ id: expressionsTable.id }) | ||
| .from(expressionsTable) | ||
| .where( | ||
| and(eq(expressionsTable.key, key), isNull(expressionsTable.deletedAt)) | ||
| ) | ||
| .limit(1); | ||
|
|
||
| if (existing[0]) { | ||
| await db | ||
| .update(expressionsTable) | ||
| .set({ expr }) | ||
| .where(eq(expressionsTable.id, existing[0].id)); | ||
| return; | ||
| } | ||
|
|
||
| await db.insert(expressionsTable).values({ key, expr }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'expressions' --type=ts -g '*schema*'
fd -e sql | xargs rg -nl -i 'expressions' 2>/dev/null
fd -e sql | xargs rg -nP -i 'unique.*key|key.*unique' 2>/dev/nullRepository: ScrawnDotDev/Scrawn
Length of output: 788
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show full expressionsTable definition (including deletedAt/deleted_at and any indexes)
rg -n "export const expressionsTable" -S src/storage/db/postgres/schema.ts
start=$(rg -n "export const expressionsTable" -S src/storage/db/postgres/schema.ts | head -n1 | cut -d: -f1)
# print ~80 lines around it
python3 - <<PY
import pathlib,sys
path=pathlib.Path("src/storage/db/postgres/schema.ts")
lines=path.read_text().splitlines()
# compute start line from earlier captured env? can't; re-run quickly
import subprocess,shlex,os,glob
PY
# simpler: just re-run with sed using line number
n=$(rg -n "export const expressionsTable" -S src/storage/db/postgres/schema.ts | head -n1 | cut -d: -f1)
sed -n "$((n-10)),$((n+120))p" src/storage/db/postgres/schema.ts
# Search for deletedAt/deleted_at and any unique/index definitions related to expressions
rg -n "expressions.*(deletedAt|deleted_at|deletedAt|deleted_at|unique|index|onConflict)" -S src/storage/db/postgres/schema.ts src/storage/db/postgres/migrations 2>/dev/null || true
# Search all SQL files for expressions uniqueness/indexes
fd -e sql | xargs rg -nP -i "expressions" 2>/dev/null | head -n 50
fd -e sql | xargs rg -nP -i "unique.*\\(.*key|unique.*key|create\\s+unique\\s+index|partial\\s+unique\\s+index|where\\s+deleted" 2>/dev/null | head -n 80
# Inspect createExpression implementation for context/locking/transaction usage
fd -e ts -e tsx src/storage/db/postgres/helpers/expressions.ts 2>/dev/null
rg -n "createExpression" -S src/storage/db/postgres/helpers/expressions.ts
sed -n "1,220p" src/storage/db/postgres/helpers/expressions.tsRepository: ScrawnDotDev/Scrawn
Length of output: 2725
Major: make createExpression atomic to prevent duplicate non-deleted expressions.key rows
File: src/storage/db/postgres/helpers/expressions.ts (lines 51-68)
try {
const existing = await db
.select({ id: expressionsTable.id })
.from(expressionsTable)
.where(and(eq(expressionsTable.key, key), isNull(expressionsTable.deletedAt)))
.limit(1);
if (existing[0]) {
await db
.update(expressionsTable)
.set({ expr })
.where(eq(expressionsTable.id, existing[0].id));
return;
}
await db.insert(expressionsTable).values({ key, expr });createExpressionuses a non-atomic select-then-update/insert flow whileexpressionsTablesupports soft deletes viadeletedAt.- No uniqueness/partial-uniqueness constraint on
expressions.key(e.g.,key WHERE deleted_at IS NULL) was found in the Drizzle schema/migrations searches, so concurrent calls can both miss and insert, creating duplicate active rows;.limit(1)makes subsequent reads nondeterministic. - Fix by enforcing atomicity: add a partial unique index on
(key)wheredeleted_at IS NULLand switch to an atomicINSERT ... ON CONFLICT(or transaction + locking), handling unique-constraint violations explicitly per the guidelines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/db/postgres/helpers/expressions.ts` around lines 51 - 68,
createExpression currently does a non-atomic select-then-update/insert against
expressionsTable (checking isNull(deletedAt)) which can lead to duplicate active
rows under concurrency; add a partial unique index on expressionsTable.key where
deleted_at IS NULL in your migrations and replace the select-then-insert flow in
createExpression with an atomic upsert (INSERT ... ON CONFLICT(key) DO UPDATE
SET expr=EXCLUDED.expr) or wrap the logic in a transaction with appropriate
row-level lock handling and explicitly catch unique-constraint violations to
handle races; reference expressionsTable, createExpression, deletedAt and handle
the conflict error path rather than relying on .limit(1).
| const existing = await db | ||
| .select({ id: tagsTable.id }) | ||
| .from(tagsTable) | ||
| .where(and(eq(tagsTable.key, key), isNull(tagsTable.deletedAt))) | ||
| .limit(1); | ||
|
|
||
| if (existing[0]) { | ||
| await db | ||
| .update(tagsTable) | ||
| .set({ amount }) | ||
| .where(eq(tagsTable.id, existing[0].id)); | ||
| tagCache.delete(key); | ||
| return; | ||
| } | ||
|
|
||
| await db.insert(tagsTable).values({ key, amount }); | ||
| tagCache.delete(key); |
There was a problem hiding this comment.
Make the tag upsert atomic.
This is a TOCTOU write path: two requests for the same key can both miss existing[0], then race into conflicting inserts or duplicate active rows. It also won't revive a soft-deleted row if that key still has a uniqueness constraint. Wrap the lookup/write in a single Drizzle transaction and handle the unique-conflict path explicitly instead of doing a separate select and insert.
As per coding guidelines, "Use Drizzle ORM with transactions; validate all inputs before DB operations; handle unique constraint violations explicitly".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/db/postgres/helpers/tags.ts` around lines 29 - 45, Wrap the
select/insert/update in a single Drizzle transaction using db.transaction so the
upsert for tagsTable is atomic: inside the transaction, do a SELECT ... WHERE
key = key FOR UPDATE (using tagsTable and db) and if you find an active row
update its amount and clear tagCache; if you find a soft-deleted row revive it
by updating amount and nulling deletedAt; otherwise INSERT the new row;
additionally catch unique-constraint errors on the INSERT and, in that error
handler, perform an UPDATE by id for the conflicting key to avoid duplicate
active rows; always delete tagCache for the key after the write.
| export const expressionsTable = pgTable("expressions", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| key: text("key").notNull().unique(), | ||
| key: text("key").notNull(), | ||
| expr: text("expr").notNull(), | ||
| deletedAt: timestamp("deleted_at", { | ||
| withTimezone: true, | ||
| mode: "string", | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Preserve uniqueness for active expression keys.
Dropping .unique() from key without adding a partial unique index allows multiple non-deleted rows with the same key. That makes key-based reads ambiguous and removes the DB guarantee the later upsert path needs.
Suggested schema change
-export const expressionsTable = pgTable("expressions", {
- id: uuid("id").primaryKey().defaultRandom(),
- key: text("key").notNull(),
- expr: text("expr").notNull(),
- deletedAt: timestamp("deleted_at", {
- withTimezone: true,
- mode: "string",
- }),
-});
+export const expressionsTable = pgTable(
+ "expressions",
+ {
+ id: uuid("id").primaryKey().defaultRandom(),
+ key: text("key").notNull(),
+ expr: text("expr").notNull(),
+ deletedAt: timestamp("deleted_at", {
+ withTimezone: true,
+ mode: "string",
+ }),
+ },
+ (table) => ({
+ uniqueActiveKey: uniqueIndex("unique_active_expression_key")
+ .on(table.key)
+ .where(sql`${table.deletedAt} IS NULL`),
+ })
+);Mirror the same constraint in the migration. As per coding guidelines, "Use Drizzle ORM with transactions; validate all inputs before DB operations; handle unique constraint violations explicitly".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/db/postgres/schema.ts` around lines 265 - 273, The expressions
table lost the uniqueness guarantee on active keys—restore a partial unique
constraint on the key column for non-deleted rows: add a unique index on
expressions.key where deleted_at IS NULL (reference: expressionsTable, the key
column and deletedAt/timestamp "deleted_at") and add the same partial unique
index in the corresponding migration; ensure any upsert/insert code paths handle
unique-constraint errors explicitly (catch unique violation and handle according
to existing upsert logic) and perform the schema change inside a migration
transaction per project guidelines.
… show errors in UI
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/http/api/webhookEndpoints.ts (1)
343-357:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
ZodErrorhandling inhandleSendTestWebhook.
sendTestSchema.parse(body)on line 290 can throwZodError, but the catch block lacks a handler for it. Validation failures will return 500 instead of 400, inconsistent withhandleCreateWebhookEndpoint.🐛 Proposed fix
if (error instanceof AuthError) { builder.setError(401, { type: error.type, message: error.message }); reply.code(401); return { error: error.message }; } + if (error instanceof ZodError) { + const issues = error.issues + .map((issue) => `${issue.path.join(".")}: ${issue.message}`) + .join("; "); + builder.setError(400, { type: "ValidationError", message: issues }); + reply.code(400); + return { error: issues }; + } + const err = error instanceof Error ? error : new Error(String(error));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/http/api/webhookEndpoints.ts` around lines 343 - 357, The catch block in handleSendTestWebhook currently treats all errors as 500; add handling for ZodError thrown by sendTestSchema.parse(body) so validation failures return 400 like handleCreateWebhookEndpoint: detect error instanceof ZodError, call builder.setError(400, { type: "ValidationError", message: error.message }) (or similar payload consistent with existing validation responses), set reply.code(400) and return the validation error details; preserve existing Sentry.captureException and existing AuthError and generic 500 handling for other cases.
🧹 Nitpick comments (1)
src/routes/http/api/webhookEndpoints.ts (1)
85-97: 💤 Low valueDefensive check is unreachable.
The condition
targetApiKeyId !== auth.apiKeyId && auth.role !== "dashboard"can never be true: whenauth.role !== "dashboard", the ternary on lines 85-88 guaranteestargetApiKeyId === auth.apiKeyId. The check is harmless but dead code.♻️ Suggested simplification
const targetApiKeyId = validated.apiKeyId && auth.role === "dashboard" ? validated.apiKeyId : auth.apiKeyId; - if (targetApiKeyId !== auth.apiKeyId && auth.role !== "dashboard") { - builder.setError(403, { - type: "PermissionDenied", - message: "Only dashboard keys can set webhooks for other keys", - }); - reply.code(403); - return { error: "Only dashboard keys can set webhooks for other keys" }; - } - const targetKey = await getApiKeyRoleById(targetApiKeyId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/http/api/webhookEndpoints.ts` around lines 85 - 97, The conditional guarding webhook assignment is dead code: because targetApiKeyId is computed via the ternary using auth.role, the subsequent if (targetApiKeyId !== auth.apiKeyId && auth.role !== "dashboard") can never be true; remove that unreachable if-block and simplify the assignment to compute targetApiKeyId only based on auth.role and validated.apiKeyId (keep references to targetApiKeyId, auth.role, validated.apiKeyId) so the logic is clear and there is no redundant permission check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/routes/http/api/webhookEndpoints.ts`:
- Around line 343-357: The catch block in handleSendTestWebhook currently treats
all errors as 500; add handling for ZodError thrown by
sendTestSchema.parse(body) so validation failures return 400 like
handleCreateWebhookEndpoint: detect error instanceof ZodError, call
builder.setError(400, { type: "ValidationError", message: error.message }) (or
similar payload consistent with existing validation responses), set
reply.code(400) and return the validation error details; preserve existing
Sentry.captureException and existing AuthError and generic 500 handling for
other cases.
---
Nitpick comments:
In `@src/routes/http/api/webhookEndpoints.ts`:
- Around line 85-97: The conditional guarding webhook assignment is dead code:
because targetApiKeyId is computed via the ternary using auth.role, the
subsequent if (targetApiKeyId !== auth.apiKeyId && auth.role !== "dashboard")
can never be true; remove that unreachable if-block and simplify the assignment
to compute targetApiKeyId only based on auth.role and validated.apiKeyId (keep
references to targetApiKeyId, auth.role, validated.apiKeyId) so the logic is
clear and there is no redundant permission check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b094bec0-c636-4494-b06a-66fd7e3a2f28
📒 Files selected for processing (3)
src/routes/http/api/apiKeys.tssrc/routes/http/api/webhookEndpoints.tssrc/storage/db/postgres/helpers/apiKeys.ts
…KeyId, catch ZodError in listDeliveries
Signed-off-by: Devyash Saini <dysaini2004@gmail.com>
Greptile SummaryThis PR adds dashboard backfill endpoints for API key management (create/list/revoke), tag and expression CRUD with soft-deletion, webhook delivery listing, and expands webhook endpoint operations to allow dashboard keys to manage webhooks on behalf of other keys.
Confidence Score: 3/5Not safe to merge: multiple new endpoints expose write operations on shared data and full delivery history including event payloads to any authenticated key. The delivery listing endpoint returns every webhook delivery in the system — including request bodies — to any valid API key without scope restriction. The four new tag/expression mutation handlers similarly accept any authenticated key, allowing test or production keys to overwrite or delete the global tag amounts used in expression evaluation. These are current auth boundary defects on newly added paths. src/routes/http/api/webhookDeliveries.ts, src/routes/http/api/tags.ts, src/routes/http/api/expressions.ts, and src/routes/http/api/apiKeys.ts all need dashboard role guards before these endpoints are safe to expose. Important Files Changed
Reviews (2): Last reviewed commit: "fix: add inputCacheTokens to expression ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| name: validated.name, | ||
| key: apiKeyHash, | ||
| role: validated.role, | ||
| expiresAt: expiresAt.toISO(), | ||
| }); | ||
|
|
There was a problem hiding this comment.
handleCreateApiKey authenticates the caller but never checks that auth.role === "dashboard". As written, any valid test or production key can call POST /api/v1/api-keys and mint new production keys — a direct privilege escalation. A test key should not be able to create a production key.
|
|
||
| if ((result.count ?? 0) === 0) { | ||
| builder.setError(404, { | ||
| type: "NotFoundError", | ||
| message: "API key not found or already revoked", | ||
| }); | ||
| reply.code(404); | ||
| return { error: "API key not found or already revoked" }; | ||
| } | ||
|
|
||
| builder.setSuccess(200); | ||
| reply.code(200); | ||
| return { message: "API key revoked" }; |
There was a problem hiding this comment.
handleRevokeApiKey authenticates the caller but does not verify the caller has the dashboard role. Any live test or production key can call DELETE /api/v1/api-keys/:id with any UUID and revoke any other key in the system, including the dashboard key itself.
| reply: FastifyReply | ||
| ): Promise<Record<string, unknown> | { error: string }> { | ||
| const builder = createWideEventBuilder( | ||
| generateRequestId(), | ||
| request.method, | ||
| request.url | ||
| ); | ||
|
|
||
| try { | ||
| const auth = await authenticateHttpApiKey(request.headers.authorization); |
There was a problem hiding this comment.
production webhook URL at key-creation time
createApiKeySchema accepts any valid URL for webhookUrl regardless of the requested role. When role: "production" is submitted, the webhookUrl may be plain HTTP. handleCreateWebhookEndpoint enforces HTTPS for production keys, but that check is absent here, so handleCreateApiKey creates the endpoint via upsertWebhookEndpoint with a non-HTTPS URL before any such guard can fire.
| export async function getApiKeyRoleById( | ||
| id: string | ||
| ): Promise<{ role: "dashboard" | "production" | "test" } | null> { | ||
| const db = getPostgresDB(); | ||
|
|
||
| try { | ||
| const [record] = await db | ||
| .select({ role: apiKeysTable.role }) | ||
| .from(apiKeysTable) | ||
| .where(eq(apiKeysTable.id, id)) | ||
| .limit(1); | ||
|
|
||
| return record ?? null; | ||
| } catch (e) { | ||
| throw StorageError.queryFailed( | ||
| "Failed to look up API key role", | ||
| e instanceof Error ? e : new Error(String(e)) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
getApiKeyRoleById does not filter revoked or expired keys
Both handleCreateWebhookEndpoint and handleSendTestWebhook call getApiKeyRoleById(targetApiKeyId) to decide whether the target is valid. The query has no revoked = false or expiry filter, so a revoked (or expired) key will still return a role and pass the role check, allowing webhooks to be attached to or test events sent to keys that have already been revoked.
| webhookEndpointId: webhookEndpointsTable.id, | ||
| }) | ||
| .from(apiKeysTable) | ||
| .leftJoin( | ||
| webhookEndpointsTable, | ||
| and( | ||
| eq(apiKeysTable.id, webhookEndpointsTable.apiKeyId), | ||
| isNull(webhookEndpointsTable.deletedAt) | ||
| ) | ||
| ) | ||
| .where( | ||
| and(ne(apiKeysTable.role, "dashboard"), eq(apiKeysTable.revoked, false)) | ||
| ) | ||
| .orderBy(apiKeysTable.createdAt); | ||
|
|
||
| builder.setSuccess(200); | ||
| reply.code(200); | ||
| return { keys }; | ||
| } catch (error) { | ||
| Sentry.captureException(error, { | ||
| extra: { context: "list API keys handler" }, | ||
| }); | ||
|
|
||
| if (error instanceof AuthError) { | ||
| builder.setError(401, { type: error.type, message: error.message }); | ||
| reply.code(401); | ||
| return { error: error.message }; |
There was a problem hiding this comment.
handleListApiKeys lacks a role guard
Any valid test or production key can call GET /api/v1/api-keys and enumerate every non-revoked non-dashboard key in the system, including their webhook URLs and public keys. This endpoint should require auth.role === "dashboard" to prevent one tenant's key from discovering the full key inventory.
| await authenticateHttpApiKey(request.headers.authorization); | ||
|
|
||
| const query = listDeliveriesQuerySchema.parse(request.query); | ||
| const db = getPostgresDB(); | ||
|
|
||
| let conditions = undefined; | ||
| if (query.apiKeyId) { | ||
| const endpoints = await db | ||
| .select({ id: webhookEndpointsTable.id }) | ||
| .from(webhookEndpointsTable) | ||
| .where(eq(webhookEndpointsTable.apiKeyId, query.apiKeyId)); | ||
| const ids = endpoints.map((e) => e.id); | ||
| if (ids.length > 0) { | ||
| conditions = inArray(webhookDeliveriesTable.endpointId, ids); | ||
| } else { | ||
| conditions = eq(webhookDeliveriesTable.endpointId, ""); | ||
| } | ||
| } |
There was a problem hiding this comment.
handleListDeliveries performs only authentication, not authorization. When apiKeyId is omitted, the query returns every delivery in the system across all API keys. Even when apiKeyId is supplied, there is no check that it belongs to the calling key, so any test or production key can read the full event history — including requestBody — of every other key by iterating UUIDs. Consistent with the rest of the dashboard API, this endpoint should require auth.role === "dashboard" before serving any results.
| const authHeader = request.headers.authorization; | ||
| await authenticateHttpApiKey(authHeader); | ||
|
|
There was a problem hiding this comment.
handleCreateTag and handleDeleteTag (lines 88–90 and 139–141) authenticate the caller but do not verify auth.role === "dashboard". Any live test or production key can create or overwrite tags (altering the billing/rate-limit amounts used in expression evaluation) or delete them. The same pattern appears in handleCreateExpression and handleDeleteExpression in expressions.ts (lines 330–332 and 393–395). Given that handleSendTestWebhook was intentionally tightened to dashboard-only in this same PR, these write operations should follow the same guard.
2 participants
Summary by CodeRabbit