fix(core): honor webhook statusCode and add signature create-if-absent - #620
fix(core): honor webhook statusCode and add signature create-if-absent#620Dhirenderchoudhary wants to merge 7 commits into
Conversation
|
@Dhirenderchoudhary is attempting to deploy a commit to the corsair Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
app
|
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:
📝 WalkthroughWalkthroughThe PR adds atomic webhook-signature creation, updates Asana and Notion verification, and preserves webhook failure status, error data, and headers through tunnel and web API responses. ChangesWebhook handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WebhookHandler
participant processWebhook
participant Tunnel
participant WebRoute
WebhookHandler->>processWebhook: return failure or throw error
processWebhook->>Tunnel: deliver acknowledged failure response
processWebhook->>WebRoute: return response body and headers
Tunnel->>WebhookHandler: acknowledge with handler status
WebRoute->>WebhookHandler: serialize response with HTTP status
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR propagates webhook handler statuses through direct and tunneled delivery while making first-time webhook-signature and account-config updates concurrency-safe.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Sequence DiagramsequenceDiagram
participant Provider
participant Route as Webhook Route/Tunnel
participant Handler as Plugin Handler
participant Keys as Account Key Manager
participant DB as Account Config DB
Provider->>Route: Verification webhook
Route->>Handler: processWebhook(...)
Handler->>Keys: set_webhook_signature_if_absent(secret)
Keys->>DB: Read encrypted config
Keys->>DB: "CAS update WHERE config = prior config"
alt CAS conflict
DB-->>Keys: No rows updated
Keys->>DB: Re-read and retry
else Signature stored or already matches
DB-->>Keys: Success
end
Handler-->>Route: success/statusCode/error/headers
Route-->>Provider: JSON response with handler status
Reviews (4): Last reviewed commit: "fix(core): refuse DEK rotation when conf..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Plugin PR scorecard —
|
| Check | Status | Notes |
|---|---|---|
| R1 — Scope: one plugin per PR | ❌ | This PR touches: asana, notion |
| R2 — Tests with assertions | ✅ | |
| R3 — Description complete | ✅ | |
| R3 — Linked issue / claim | ✅ | |
| R4 — Demo video / recording | ✅ |
Rules: PLUGIN_PR_RULES.md · re-runs on every push
|
Hey @Dhirenderchoudhary, thanks for the contribution! 🏴☠️ Before a maintainer reviews, please fix the items below — the review re-runs automatically on your next push. Must fix
Knowledge Base Used: PR requirements (rules)
If anything remains after your next push, a bot commit will clean it up; a maintainer always does the final review and merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
packages/notion/webhooks.test.ts (1)
82-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the 500 persistence-failure branch.
The mock reproduces the real
set_webhook_signature_if_absentsemantics well, and the 401 case is covered. The remaining uncovered branch is lines 30-36 ofpackages/notion/webhooks/verification.ts: persistence rejects and no secret is stored, so the handler must returnstatusCode: 500withFailed to persist verification token. Makeset_webhook_signature_if_absentreject whileget_webhook_signatureresolves tonull, then assert that result.🤖 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 `@packages/notion/webhooks.test.ts` around lines 82 - 98, Add a test alongside the existing verification.handler cases that configures set_webhook_signature_if_absent to reject and get_webhook_signature to resolve null, then assert the handler returns success false, statusCode 500, and error “Failed to persist verification token.”packages/asana/webhooks/challenge.ts (1)
80-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect conflict handling. Extract the duplicated rejection block.
The catch path is right: it distinguishes a real conflict (401) from a storage failure (500), and it never echoes a secret it did not persist. The
{ created: false }resolution also falls through to the echo, which matches Asana retry behavior.Lines 88-93 duplicate the rejection at lines 64-68, including the log message. Extract a small
rejectExistingSecret()helper so the two paths cannot drift.🤖 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 `@packages/asana/webhooks/challenge.ts` around lines 80 - 105, Extract the duplicated existing-secret rejection logic into a local rejectExistingSecret() helper, including the warning and the 401 response. Replace both the earlier conflict branch and the catch-path conflict branch with calls to this helper, while preserving the separate 500 storage-failure handling and existing Asana retry behavior.packages/corsair/tests/webhook-signature-if-absent.test.ts (1)
104-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood concurrency coverage. Add the rejected-input case.
The four tests cover create, idempotent reuse, conflict, and the concurrent first write. Two branches of
setWebhookSignatureIfAbsentremain untested: the empty/whitespace value rejection (Webhook signature cannot be empty) and the exhausted-retry path (Failed to set webhook signature atomically). The first is a one-line test.Note that this suite exercises SQLite only, so it does not prove the optimistic-lock
WHERE config = ?clause works on PostgreSQL. See the related comment inpackages/corsair/core/auth/key-manager.ts.🤖 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 `@packages/corsair/tests/webhook-signature-if-absent.test.ts` around lines 104 - 137, Add a focused test for set_webhook_signature_if_absent that passes an empty or whitespace-only value and asserts rejection with the message “Webhook signature cannot be empty,” using the existing test database setup and cleanup pattern. Do not change the concurrency test or add PostgreSQL coverage here.packages/asana/webhooks/challenge.test.ts (1)
123-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the conflict branch inside the catch block.
packages/asana/webhooks/challenge.tslines 83-93 add a new branch: persistence rejects, the handler re-reads the stored secret, and it returns 401 when the stored value differs. This suite does not cover that branch. The existing failure test starts fromcreateContext(null), soget_webhook_signaturekeeps returningnulland only the 500 path runs.Add a case where
set_webhook_signature_if_absentrejects andget_webhook_signatureresolves to a different secret on the second call. AssertstatusCode: 401and that noX-Hook-Secretheader is echoed. That case represents the concurrent-registration race the PR targets.🤖 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 `@packages/asana/webhooks/challenge.test.ts` around lines 123 - 127, Add a test alongside the existing persistence-failure case for the conflict branch in the webhook challenge handler: configure set_webhook_signature_if_absent to reject and get_webhook_signature to return a different stored secret on its re-read, then assert a 401 statusCode and no X-Hook-Secret response header. Use the existing createContext and handler test setup.
🤖 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 `@packages/corsair/core/auth/key-manager.ts`:
- Around line 511-517: Update setWebhookSignatureIfAbsent so it does not
normalize a non-empty signature before storage: either reject values whose
trimmed form differs from the original, or use trim only to detect emptiness and
persist the original value unchanged. Preserve the existing empty-value
rejection while ensuring stored signatures match the values returned and
compared by the Asana and Notion handlers.
- Around line 553-572: The update in the webhook signature flow must serialize
the object returned by encryptConfig before passing it as config in the
corsair_accounts update, while preserving the optimistic-lock comparison against
row.config and PostgreSQL compatibility. Use the existing dialect-safe JSON
serialization approach if available, or apply serialization only for SQLite.
In `@packages/corsair/tunnel/index.ts`:
- Around line 281-293: Update the failed-handler response construction in
processWebhook to include result.response.data in webhookResponse.body while
preserving the existing success, error, status, and headers fields, so tunnel
callers receive the same structured failure details as the www route.
In `@packages/corsair/webhooks/index.ts`:
- Line 306: Update the catch block producing statusCode 500 to log the original
exception internally but return a generic error value in the response,
preventing Error.message from reaching webhook callers. Preserve explicit
handler errors with success: false unchanged.
In `@packages/notion/webhooks/verification.ts`:
- Around line 19-37: Update the catch block around
ctx.keys.set_webhook_signature_if_absent to retain and log the persistence error
before returning the 500 response. Replace the plain existing !== token
comparison with the established timing-safe secretsMatch approach used by the
Asana webhook handler, preserving the existing 401 conflict and 500
persistence-failure outcomes.
---
Nitpick comments:
In `@packages/asana/webhooks/challenge.test.ts`:
- Around line 123-127: Add a test alongside the existing persistence-failure
case for the conflict branch in the webhook challenge handler: configure
set_webhook_signature_if_absent to reject and get_webhook_signature to return a
different stored secret on its re-read, then assert a 401 statusCode and no
X-Hook-Secret response header. Use the existing createContext and handler test
setup.
In `@packages/asana/webhooks/challenge.ts`:
- Around line 80-105: Extract the duplicated existing-secret rejection logic
into a local rejectExistingSecret() helper, including the warning and the 401
response. Replace both the earlier conflict branch and the catch-path conflict
branch with calls to this helper, while preserving the separate 500
storage-failure handling and existing Asana retry behavior.
In `@packages/corsair/tests/webhook-signature-if-absent.test.ts`:
- Around line 104-137: Add a focused test for set_webhook_signature_if_absent
that passes an empty or whitespace-only value and asserts rejection with the
message “Webhook signature cannot be empty,” using the existing test database
setup and cleanup pattern. Do not change the concurrency test or add PostgreSQL
coverage here.
In `@packages/notion/webhooks.test.ts`:
- Around line 82-98: Add a test alongside the existing verification.handler
cases that configures set_webhook_signature_if_absent to reject and
get_webhook_signature to resolve null, then assert the handler returns success
false, statusCode 500, and error “Failed to persist verification token.”
🪄 Autofix
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: 70a2ce31-d153-463a-98d9-37fff77c22c6
📒 Files selected for processing (11)
packages/asana/webhooks/challenge.test.tspackages/asana/webhooks/challenge.tspackages/corsair/core/auth/key-manager.tspackages/corsair/core/auth/types.tspackages/corsair/tests/process-webhook-status.test.tspackages/corsair/tests/webhook-signature-if-absent.test.tspackages/corsair/tunnel/index.tspackages/corsair/webhooks/index.tspackages/notion/webhooks.test.tspackages/notion/webhooks/verification.tswww/src/app/api/webhooks/route.ts
| const encryptedConfig = encryptConfig( | ||
| { ...currentConfig, webhook_signature: normalized }, | ||
| dek, | ||
| ); | ||
|
|
||
| // Optimistic lock on the opaque encrypted blob so a concurrent | ||
| // writer that landed first causes a clean retry instead of a | ||
| // silent overwrite. | ||
| const result = await database.db | ||
| .updateTable('corsair_accounts') | ||
| .set({ | ||
| config: encryptedConfig, | ||
| updated_at: new Date(), | ||
| }) | ||
| .where('id', '=', row.id) | ||
| .where('config', '=', row.config as any) | ||
| .executeTakeFirst(); | ||
|
|
||
| const updated = | ||
| result.numUpdatedRows !== undefined && result.numUpdatedRows > 0n; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the schema definition and migrations for corsair_accounts.config
fd -t f -e ts -e sql | xargs rg -n -C 4 'corsair_accounts' -g '!**/node_modules/**' | rg -n -C 4 -i 'config|jsonb|json|text|addColumn|createTable'
# Show the Kysely database type declaration for the accounts table
ast-grep run --pattern 'interface $NAME { $$$ }' --lang typescript $(fd -t f 'database' -e ts | head -20) 2>/dev/null | rg -n -C 6 'config'Repository: corsairdev/corsair
Length of output: 27227
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- schema definitions ---'
cat -n www/src/db/corsair-schema.ts | sed -n '1,45p'
printf '%s\n' '--- initial migration account table ---'
sed -n '148,175p' www/drizzle/0000_solid_martin_li.sql
printf '%s\n' '--- database types and config helpers ---'
rg -n -C 8 'interface .*Database|corsair_accounts|config:|encryptConfig|decryptConfig|JSONColumnType|Dialect|postgres|sqlite|mysql' packages/corsair www/src packages -g '*.{ts,tsx}' | head -240
printf '%s\n' '--- key manager implementation ---'
cat -n packages/corsair/core/auth/key-manager.ts | sed -n '500,590p'
printf '%s\n' '--- all production config column declarations ---'
rg -n -C 3 'config.*(jsonb|json|text)|jsonb.*config|text.*config|corsair_accounts' www packages demo -g '*.{ts,tsx,sql}' | head -260Repository: corsairdev/corsair
Length of output: 39001
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- database adapter and dialect support ---'
rg -n -C 6 'Kysely|PostgresDialect|SqliteDialect|MysqlDialect|dialect|supported|SQLite|PostgreSQL|MySQL' packages/corsair README.md docs package.json packages -g '*.{ts,tsx,md,json}' | head -300
printf '%s\n' '--- database interface and row types ---'
rg -n -C 12 'export (interface|type).*Database|interface Database|type Database|corsair_accounts:' packages/corsair -g '*.ts' | head -240
printf '%s\n' '--- config serialization helpers ---'
rg -n -C 12 'function parseConfig|const parseConfig|function encryptConfig|const encryptConfig|export .*encryptConfig|JSON.stringify' packages/corsair/core packages/corsair -g '*.ts' | head -260
printf '%s\n' '--- relevant tests ---'
rg -n -C 12 'setWebhookSignatureIfAbsent|webhook_signature|atomically|optimistic|concurrent|race' packages/corsair -g '*.{ts,tsx}' | head -300Repository: corsairdev/corsair
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- database adapter implementation ---'
cat -n packages/corsair/db/kysely/database.ts | sed -n '1,180p'
printf '%s\n' '--- config helper definitions and imports ---'
rg -l 'parseConfig|encryptConfig|decryptConfig' packages/corsair -g '*.ts' | sort -u
rg -n -C 10 'parseConfig|encryptConfig|decryptConfig' packages/corsair/core/auth/key-manager.ts packages/corsair/core -g '*.ts' | head -220
printf '%s\n' '--- account schema type definitions ---'
cat -n packages/corsair/db/index.ts | sed -n '1,55p'
printf '%s\n' '--- account config write/read paths ---'
rg -n -C 8 'config:|row\.config|account\.config|updateAccount' packages/corsair/core/auth/key-manager.ts packages/corsair/core -g '*.ts' | head -260Repository: corsairdev/corsair
Length of output: 42517
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- encryption implementation ---'
cat -n packages/corsair/core/auth/encryption.ts | sed -n '1,240p'
printf '%s\n' '--- database adapter construction ---'
cat -n packages/corsair/db/kysely/database.ts | sed -n '178,280p'
printf '%s\n' '--- SQLite account setup and seed values ---'
cat -n packages/corsair/tests/setup-db.ts | sed -n '1,90p'
cat -n packages/corsair/tests/plugins-test-utils.ts | sed -n '1,90p'
printf '%s\n' '--- webhook concurrency tests ---'
cat -n packages/corsair/tests/key-manager-concurrent-writes.test.ts | sed -n '1,260p'
cat -n packages/corsair/tests/webhook-signature-if-absent.test.ts | sed -n '1,260p'Repository: corsairdev/corsair
Length of output: 20347
🌐 Web query:
Kysely SQLite JSON object parameter serialization better-sqlite3 config TEXT update object
💡 Result:
In Kysely with the SQLite dialect (better-sqlite3), handling JSON objects in TEXT columns involves two distinct steps: serialization (insert/update) and deserialization (selecting). Serialization (Insert/Update) To insert or update JSON objects, you must convert the object into a JSON string before passing it to the query [1][2]. You have three main approaches: 1. Manual Stringification: Use JSON.stringify(yourObject) when providing the value in your query [1][3]. const metadata = { tags: ['tech', 'ai'] }; await db.insertInto('table').values({ data: JSON.stringify(metadata) }).execute; 2. Type-Safe Helpers (jval): Kysely provides eb.jval and sql.jval for type-safe JSON serialization [4]. These helpers automatically serialize the provided value, ensuring type safety without requiring manual JSON.stringify calls [4]. await db.insertInto('table').values((eb) => ({ data: eb.jval({ tags: ['tech', 'ai'] }) })).execute; 3. Third-Party Plugins: You can use plugins like kysely-plugin-serialize to automatically handle JSON serialization (and deserialization) for all non-primitive values [5]. Deserialization (Selecting) SQLite typically returns JSON stored in TEXT columns as simple strings [6][7]. To automatically parse these into JavaScript objects upon selection, use the ParseJSONResultsPlugin [6][8]. 1. Configure the Plugin: Add the plugin when initializing your Kysely instance [6][8]. import { ParseJSONResultsPlugin, Kysely, SqliteDialect } from 'kysely'; import Database from 'better-sqlite3'; const db = new Kysely({ dialect: new SqliteDialect({ database: new Database('db.sqlite') }), plugins: [new ParseJSONResultsPlugin] }); This plugin will attempt to parse valid JSON strings in your query results back into JavaScript objects [6]. Alternatively, you can manually use JSON.parse() on the retrieved string values [3]. Summary of Types For defining table interfaces, use ColumnType to define the difference between the database storage (string) and your application-side type (object) [1][2]. interface Table { metadata: ColumnType<{ tags: string[] }, string, string>; }
Citations:
- 1: https://kysely-org-kysely.mintlify.app/api/types/insertable
- 2: https://kysely-org.github.io/kysely-apidoc/types/JSONColumnType.html
- 3: how to auto serialize/deserialize json type data in sqlite? kysely-org/kysely#318
- 4: feat: type-safe JSON insertions/updates,
eb.jval&sql.jval. kysely-org/kysely#1130 - 5: https://www.npmjs.com/package/kysely-plugin-serialize
- 6: https://github.com/kysely-org/kysely/blob/master/src/plugin/parse-json-results/parse-json-results-plugin.ts
- 7: https://github.com/kysely-org/kysely/blob/e58d31b3/src/helpers/sqlite.ts
- 8: https://kysely-org.github.io/kysely-apidoc/functions/helpers_sqlite.jsonBuildObject.html
🌐 Web query:
PostgreSQL documentation json type equality operator jsonb equality operator
💡 Result:
In PostgreSQL, the availability of equality operators differs significantly between the json and jsonb data types [1]. For the jsonb data type, the standard comparison operators (such as =, <>, <, >, <=, >=) are supported [1]. These operators allow you to compare complete JSON documents for equality and order, and they are also compatible with B-tree and hash indexes, which are useful when you need to efficiently check the equality of entire JSONB documents [2][3]. In contrast, the json data type does not support these standard comparison operators [1]. You cannot use the equality operator (=) directly with the json type [1]. Within the specific context of JSON path expressions (used in functions like jsonb_path_query), different operators are used, such as == for equality comparison, which works on JSON scalar values within those expressions [4].
Citations:
- 1: https://www.postgresql.org/docs/19/functions-json.html
- 2: https://www.postgresql.org/docs/current/datatype-json.html
- 3: https://www.postgresql.org/docs/19/datatype-json.html
- 4: https://www.postgresql.org/docs/16/functions-json.html
Serialize config for SQLite before this update.
corsair_accounts.config is jsonb in PostgreSQL and TEXT in SQLite, so = works for both. However, encryptConfig() returns an object, and Kysely does not serialize objects for SQLite TEXT parameters. The .set({ config: encryptedConfig }) call therefore fails before the lock check. Serialize the value for SQLite or add a dialect-safe JSON serialization layer.
🤖 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 `@packages/corsair/core/auth/key-manager.ts` around lines 553 - 572, The update
in the webhook signature flow must serialize the object returned by
encryptConfig before passing it as config in the corsair_accounts update, while
preserving the optimistic-lock comparison against row.config and PostgreSQL
compatibility. Use the existing dialect-safe JSON serialization approach if
available, or apply serialization only for SQLite.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/corsair/core/auth/key-manager.ts (1)
513-524: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not overwrite config after decryption fails.
If
decryptConfigfails, this code uses{}as the current config. The following CAS write can then replace all stored values. A malformed ciphertext can erase OAuth credentials when a webhook signature update runs.Log the error and rethrow it. Add a regression test that confirms no write occurs after a decryption failure.
Proposed fix
} catch (err) { console.error( `[corsair] Failed to decrypt config for account (tenant: "${tenantId}", integration: "${integrationName}"), starting fresh:`, err, ); - currentConfig = {}; + throw err; }🤖 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 `@packages/corsair/core/auth/key-manager.ts` around lines 513 - 524, Update the decryptConfig error path in the key-manager configuration flow to log the failure and rethrow the original error instead of assigning currentConfig to {}. Ensure the subsequent CAS write is not reached after decryption fails, and add a regression test confirming no write occurs in this scenario.
🤖 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 `@packages/corsair/core/auth/key-manager.ts`:
- Around line 529-546: Update issue_new_dek to avoid the direct
ctx.updateAccount write: perform DEK rotation through an atomic compare-and-swap
that matches both the previously read config and DEK, then updates the config
and DEK together. Reuse the CAS pattern established by casWriteAccountConfig,
and ensure stale snapshots cannot overwrite a concurrently added webhook
signature.
---
Outside diff comments:
In `@packages/corsair/core/auth/key-manager.ts`:
- Around line 513-524: Update the decryptConfig error path in the key-manager
configuration flow to log the failure and rethrow the original error instead of
assigning currentConfig to {}. Ensure the subsequent CAS write is not reached
after decryption fails, and add a regression test confirming no write occurs in
this scenario.
🪄 Autofix
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: ac5e7be5-f503-4469-a1e2-e3795976c85f
📒 Files selected for processing (9)
packages/asana/webhooks/challenge.test.tspackages/asana/webhooks/challenge.tspackages/corsair/core/auth/key-manager.tspackages/corsair/tests/process-webhook-status.test.tspackages/corsair/tests/webhook-signature-if-absent.test.tspackages/corsair/tunnel/index.tspackages/corsair/webhooks/index.tspackages/notion/webhooks.test.tspackages/notion/webhooks/verification.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/corsair/tunnel/index.ts
- packages/corsair/webhooks/index.ts
- packages/notion/webhooks.test.ts
- packages/corsair/tests/process-webhook-status.test.ts
- packages/asana/webhooks/challenge.test.ts
- packages/asana/webhooks/challenge.ts
|
@greptile review |
|
@greptile @CodeRabbit review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/corsair/core/auth/key-manager.ts (1)
562-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the CAS retry loop into one helper. Four sites now repeat the same pattern: a five-attempt loop, a fresh read, an encrypted write guarded by the prior
configvalue, and a throw after the last attempt. The retry count, the immediate retry with no backoff, and the failure message are copied in each site. A shared helper keeps the retry policy in one place.
packages/corsair/core/auth/key-manager.ts#L562-L586: move the loop indoUpdateConfiginto awithCasRetry(fn)helper that owns the attempt count and the terminal error.packages/corsair/core/auth/key-manager.ts#L599-L622: call the same helper fromsetWebhookSignatureIfAbsentand keep only the signature-specific branch inside the callback.packages/corsair/core/auth/key-manager.ts#L631-L664: call the same helper fromissueNewDekand keep only the rotation logic inside the callback.packages/corsair/webhooks/tenant-links.ts#L74-L115: export the helper from the key-manager module or a shared module, then use it inwriteEncryptedAccountLinkField.Add a short randomized delay between attempts in the helper. Concurrent writers currently retry in lockstep, which raises the chance of exhausting all five attempts.
🤖 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 `@packages/corsair/core/auth/key-manager.ts` around lines 562 - 586, Extract the repeated five-attempt CAS retry policy into a shared withCasRetry(fn) helper, including fresh reads, immediate retries with a short randomized delay between attempts, and the existing terminal error. In packages/corsair/core/auth/key-manager.ts lines 562-586, replace doUpdateConfig’s loop with the helper; lines 599-622, keep only setWebhookSignatureIfAbsent’s signature-specific logic in its callback; and lines 631-664, keep only issueNewDek’s rotation logic in its callback. Export the helper for packages/corsair/webhooks/tenant-links.ts lines 74-115 and use it from writeEncryptedAccountLinkField.packages/corsair/tests/webhook-signature-if-absent.test.ts (1)
214-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the
console.errorspy even when an assertion fails.
error.mockRestore()runs only on the success path. If therejects.toThrow()ortoEqualassertion fails, the spy stays active and suppressesconsole.errorin later tests of this file. Restore the spy in the existingfinallyblock, or addafterEach(() => jest.restoreAllMocks()).♻️ Proposed change
expect(after.config).toEqual(before.config); - - error.mockRestore(); } finally { + error?.mockRestore(); cleanup(); }Declare
errorbefore thetryblock for this to compile.🤖 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 `@packages/corsair/tests/webhook-signature-if-absent.test.ts` around lines 214 - 228, Ensure the console.error spy created in the webhook-signature test is restored on both passing and failing assertion paths. Declare the error spy before the existing try block and move its mockRestore call into that block’s finally clause, preserving the current assertions and test behavior.packages/corsair/webhooks/tenant-links.ts (1)
98-110: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRemove the unnecessary
as anyfrom the CAS predicate.account.configalready matches the Kysely column type. SQLite serializes object values for both updates andWHEREpredicates, while PostgreSQL storesconfigasjsonb. Use.where('config', '=', account.config)directly.🤖 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 `@packages/corsair/webhooks/tenant-links.ts` around lines 98 - 110, Remove the unnecessary `as any` cast from the `config` predicate in the `updateTable('corsair_accounts')` chain, passing `account.config` directly to `.where('config', '=', ...)` while leaving the update and row-count handling unchanged.
🤖 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 `@packages/corsair/core/auth/key-manager.ts`:
- Around line 644-655: Update issueNewDek so it does not call
casWriteAccountConfig when row.dek is falsy but row.config contains existing
values; fail instead and preserve the stored configuration for operator
recovery. Keep the current re-encryption path for rows with a DEK, and allow
writing an empty configuration only when no existing config would be discarded.
---
Nitpick comments:
In `@packages/corsair/core/auth/key-manager.ts`:
- Around line 562-586: Extract the repeated five-attempt CAS retry policy into a
shared withCasRetry(fn) helper, including fresh reads, immediate retries with a
short randomized delay between attempts, and the existing terminal error. In
packages/corsair/core/auth/key-manager.ts lines 562-586, replace
doUpdateConfig’s loop with the helper; lines 599-622, keep only
setWebhookSignatureIfAbsent’s signature-specific logic in its callback; and
lines 631-664, keep only issueNewDek’s rotation logic in its callback. Export
the helper for packages/corsair/webhooks/tenant-links.ts lines 74-115 and use it
from writeEncryptedAccountLinkField.
In `@packages/corsair/tests/webhook-signature-if-absent.test.ts`:
- Around line 214-228: Ensure the console.error spy created in the
webhook-signature test is restored on both passing and failing assertion paths.
Declare the error spy before the existing try block and move its mockRestore
call into that block’s finally clause, preserving the current assertions and
test behavior.
In `@packages/corsair/webhooks/tenant-links.ts`:
- Around line 98-110: Remove the unnecessary `as any` cast from the `config`
predicate in the `updateTable('corsair_accounts')` chain, passing
`account.config` directly to `.where('config', '=', ...)` while leaving the
update and row-count handling unchanged.
🪄 Autofix
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: 5761bc86-ad73-41de-957d-bd2e6c7d19c2
📒 Files selected for processing (3)
packages/corsair/core/auth/key-manager.tspackages/corsair/tests/webhook-signature-if-absent.test.tspackages/corsair/webhooks/tenant-links.ts
1 participant
Description
Webhook handlers could return
{ success: false, statusCode: 401 }, but www still answered with HTTP 200. First-timewebhook_signaturewrites were also a non-atomic get-then-set across workers.This PR:
success/statusCode/errorfromprocessWebhookwebhookResponse.statusinstead of marking the envelope failedset_webhook_signature_if_absenton account key managersFixes #619
Checklist
Before submitting your PR, please verify the following:
pnpm lintand all checks passpnpm typecheckand there are no TypeScript errorspnpm buildand all packages build successfullypnpm testand all tests passScreenshots / Demos (if applicable)
pnpm --filter corsair exec jest tests/process-webhook-status.test.ts --runInBandpnpm --filter @corsair-dev/notion exec jest webhooks.test.ts --runInBandpnpm --filter @corsair-dev/asana exec jest webhooks/challenge.test.ts --runInBandSummary by CodeRabbit