chore(lint): repair remaining biome violations to green the lint CI - #10
Merged
Merged
Conversation
The Lint job has been red on main since PR #7. Apply biome's safe auto-fixes across the repo (formatting, brace style, dot notation, import protocol) and fix the two remaining noMisusedPromises errors in organisation-usage-panel.tsx by wrapping ReactNode props in Boolean() before the truthiness check (same render semantics, no Promise in a conditional position). biome check . now exits 0 locally (843 warnings remain, non-blocking). Verified: tsc --noEmit clean, @documenso/lib 421/421 tests pass.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (41)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request applies formatting, import-order, and unused-import cleanup across application, server, library, test, and script files. It also adds four entries to the Vietnamese translation map. Runtime behavior remains unchanged. ChangesApplication UI formatting
Server routes, authentication, and jobs
Library and domain maintenance
Scripts and translations
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Change: Other Merge Risk: ⚪ Minimal · up to The reviewed rendering behavior is unchanged, and no actionable current-head risk remains. This change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request contains numerous refactorings, code cleanups, and formatting improvements across the application components and utility modules. The review feedback suggests optimizing the webhook payload parsing logic to handle non-object JSON inputs more robustly and recommends using timing-safe comparison for signature verification to enhance security.
Comment on lines
+79
to
+133
| const rawBody = await c.req.text(); | ||
|
|
||
| let eventName = ''; | ||
| let eventName = ''; | ||
|
|
||
| try { | ||
| eventName = (JSON.parse(rawBody).event || '').toString().toLowerCase(); | ||
| } catch { | ||
| eventName = ''; | ||
| } | ||
| try { | ||
| eventName = (JSON.parse(rawBody).event || '').toString().toLowerCase(); | ||
| } catch { | ||
| eventName = ''; | ||
| } | ||
|
|
||
| const isPingEvent = | ||
| eventName === 'ping' || eventName === 'test' || eventName === 'endpoint.test'; | ||
|
|
||
| // Fail-closed: the webhook handler performs privileged mutations | ||
| // (organisation deletion, member role grants), so it must never run | ||
| // without a configured shared secret. Only the connectivity ping is | ||
| // allowed through so the Developer Portal health check still reports | ||
| // that the endpoint is reachable but unconfigured. | ||
| if (!webhookSecret) { | ||
| if (isPingEvent) { | ||
| return c.json( | ||
| { | ||
| success: false, | ||
| message: | ||
| 'Webhook secret is not configured; events will be rejected. Set CROVE_DOS_WEBHOOK_SECRET.', | ||
| eventId: 'unconfigured', | ||
| }, | ||
| 200, | ||
| ); | ||
| } | ||
| const isPingEvent = eventName === 'ping' || eventName === 'test' || eventName === 'endpoint.test'; | ||
|
|
||
| // Fail-closed: the webhook handler performs privileged mutations | ||
| // (organisation deletion, member role grants), so it must never run | ||
| // without a configured shared secret. Only the connectivity ping is | ||
| // allowed through so the Developer Portal health check still reports | ||
| // that the endpoint is reachable but unconfigured. | ||
| if (!webhookSecret) { | ||
| if (isPingEvent) { | ||
| return c.json( | ||
| { | ||
| success: false, | ||
| message: 'Webhook endpoint is not configured to process events', | ||
| message: 'Webhook secret is not configured; events will be rejected. Set CROVE_DOS_WEBHOOK_SECRET.', | ||
| eventId: 'unconfigured', | ||
| }, | ||
| 503, | ||
| 200, | ||
| ); | ||
| } | ||
|
|
||
| const isValid = verifyDosWebhookSignature({ | ||
| rawBody, | ||
| signatureHeader, | ||
| secret: webhookSecret, | ||
| }); | ||
| return c.json( | ||
| { | ||
| success: false, | ||
| message: 'Webhook endpoint is not configured to process events', | ||
| }, | ||
| 503, | ||
| ); | ||
| } | ||
|
|
||
| const isValid = verifyDosWebhookSignature({ | ||
| rawBody, | ||
| signatureHeader, | ||
| secret: webhookSecret, | ||
| }); | ||
|
|
||
| if (!isValid) { | ||
| return c.json({ success: false, message: 'Invalid webhook signature' }, 401); | ||
| } | ||
|
|
||
| try { | ||
| const payload = JSON.parse(rawBody); | ||
| const eventId = | ||
| payload.id || | ||
| payload.event_id || | ||
| payload.eventId || | ||
| crypto.createHash('sha256').update(rawBody).digest('hex').slice(0, 32); |
There was a problem hiding this comment.
The webhook payload is currently parsed twice: once to extract the eventName (lines 83-87) and again to extract the payload (line 128). Additionally, if the payload is valid JSON but not an object (e.g., null or an array), accessing properties on it directly will throw a TypeError and result in a 500 Internal Server Error instead of a 400 Bad Request.\n\nWe can optimize this by parsing the JSON once at the beginning, validating that it is a non-null object, and reusing the parsed payload throughout the handler.
const rawBody = await c.req.text();\n\n let payload: any = null;\n try {\n payload = JSON.parse(rawBody);\n } catch {\n return c.json({ success: false, message: 'Invalid JSON payload' }, 400);\n }\n\n if (!payload || typeof payload !== 'object') {\n return c.json({ success: false, message: 'Invalid JSON payload' }, 400);\n }\n\n const eventName = (payload.event || '').toString().toLowerCase();\n const isPingEvent = eventName === 'ping' || eventName === 'test' || eventName === 'endpoint.test';\n\n // Fail-closed: the webhook handler performs privileged mutations\n // (organisation deletion, member role grants), so it must never run\n // without a configured shared secret. Only the connectivity ping is\n // allowed through so the Developer Portal health check still reports\n // that the endpoint is reachable but unconfigured.\n if (!webhookSecret) {\n if (isPingEvent) {\n return c.json(\n {\n success: false,\n message: 'Webhook secret is not configured; events will be rejected. Set CROVE_DOS_WEBHOOK_SECRET.',\n eventId: 'unconfigured',\n },\n 200,\n );\n }\n\n return c.json(\n {\n success: false,\n message: 'Webhook endpoint is not configured to process events',\n },\n 503,\n );\n }\n\n const isValid = verifyDosWebhookSignature({\n rawBody,\n signatureHeader,\n secret: webhookSecret,\n });\n\n if (!isValid) {\n return c.json({ success: false, message: 'Invalid webhook signature' }, 401);\n }\n\n try {\n const eventId =\n payload.id ||\n payload.event_id ||\n payload.eventId ||\n crypto.createHash('sha256').update(rawBody).digest('hex').slice(0, 32);| .createHmac('sha256', secret) | ||
| .update(rawBody, 'utf8') | ||
| .digest('hex'); | ||
| const computedHash = crypto.createHmac('sha256', secret).update(rawBody, 'utf8').digest('hex'); |
There was a problem hiding this comment.
For verifying webhook signatures, it is highly recommended to use a timing-safe comparison (like crypto.timingSafeEqual) instead of standard string comparison (e.g., ===). Standard string comparison is vulnerable to timing attacks, which can allow an attacker to iteratively guess the correct signature.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The Lint CI job has been failing on main since PR #7 (50 errors at last run on PR #9), so real lint regressions are invisible. PR #8 and #9 merged with lint red.
What
biome check . --write(safe fixes only): formatting, brace style, dot notation,node:import protocol across 41 files. Zero behavior change - spot-checked the largest diffs (dos-webhook.ts, generate-vi-po.mjs) are pure reformat/reindent.lint/nursery/noMisusedPromiseserrors inorganisation-usage-panel.tsx: ReactNode props (subtext,action) in a truthiness conditional trip the rule under React 19 types. Wrapped inBoolean()- identical render semantics, no Promise in a conditional position.Verification
npx biome check .exits 0 locally (0 errors; 843 warnings + 35 infos remain, non-blocking)tsc --noEmitin apps/remix: cleannpm run test -w @documenso/lib: 421/421 passSummary by CodeRabbit
New Features
Improvements
Maintenance