fix: apply all the deepsec recommendations for security fixes#88
Merged
fix: apply all the deepsec recommendations for security fixes#88
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces a DeepSec security scanning workspace and makes targeted infrastructure and security improvements: adding deepsec configuration files, documentation, and per-project setup guides; pinning Bun action and Docker image to specific versions; and updating CSV hash fingerprinting and secret validation logic. ChangesDeepSec Workspace Integration
Security & Infrastructure Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🧹 Nitpick comments (1)
apps/web/server/utils/withSecret.ts (1)
11-14: 💤 Low valueConsider hashing to fixed length to prevent timing leakage on secret length.
The
a.length === b.lengthcheck is necessary (sincetimingSafeEqualthrows on length mismatch), but it can leak information about the expected secret's length through response timing. For defense-in-depth, hash both values to a fixed length before comparison.🔐 Optional: constant-time length comparison via hashing
+import { createHash, timingSafeEqual } from 'node:crypto'; -import { timingSafeEqual } from 'node:crypto'; import { defineEventHandler } from 'h3'; export function withSecret( handler: () => void, ): ReturnType<typeof defineEventHandler> { return defineEventHandler((event) => { event.res.status = 202; const secret = event.req.headers.get('x-revalidate-secret') ?? ''; const expected = process.env.REVALIDATE_SECRET ?? ''; - const a = Buffer.from(secret); - const b = Buffer.from(expected); - const equal = a.length === b.length && timingSafeEqual(a, b); + const a = createHash('sha256').update(secret).digest(); + const b = createHash('sha256').update(expected).digest(); + const equal = timingSafeEqual(a, b); if (equal && expected.length > 0) { handler(); } return { accepted: true }; }); }🤖 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 `@apps/web/server/utils/withSecret.ts` around lines 11 - 14, The current compare uses Buffer.from(secret)/Buffer.from(expected) and a.length === b.length with timingSafeEqual, which leaks the expected secret length; instead, in withSecret compute fixed-length hashes (e.g., SHA-256) of both secret and expected using crypto.createHash and convert those digests to Buffers, then call timingSafeEqual on the two fixed-size hash buffers (remove the a.length === b.length pre-check), and keep the existing expected.length > 0 guard or replace it with a check against the raw expected being non-empty before hashing; update references to a, b and equal to use the hashed buffers and the new timingSafeEqual comparison.
🤖 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 @.deepsec/AGENTS.md:
- Around line 21-23: Update the install command in the text that reads "pnpm
install" to "bun install" so it matches README.md and the pinned Bun CI action;
specifically replace the literal string "pnpm install" in the .deepsec/AGENTS.md
copy ("The deepsec skill is at `node_modules/deepsec/SKILL.md`...") with "bun
install" and, if helpful, add a short parenthetical note clarifying that the
pnpm-workspace.yaml remains for workspace metadata only.
In @.deepsec/README.md:
- Line 74: Update the incorrect GitHub URL string
"https://github.com/vercel/deepsec/tree/main/docs" in the README to point to the
correct organization by replacing it with
"https://github.com/vercel-labs/deepsec/tree/main/docs" so links to the deepsec
docs resolve to the vercel-labs repository.
In `@apps/ch-stream/Dockerfile`:
- Line 1: The FROM line in the Dockerfile pins oven/bun:1.3.13 to an incorrect
digest; update the image digest to match your target architecture by replacing
the current sha256:87416c9... with the correct digest for your platform (use
sha256:bb35eafd10b2e969809384850ff0474ba36a491239d715864bc87787b4cdf0a4 for
amd64 or sha256:7e06b148ea46613dc8434fd11fafa49cc98e764367ead42e9f424356fce0fdfa
for arm64) so the FROM instruction references a valid manifest.
In `@apps/web/scripts/ingest-hmrc-csv.ts`:
- Around line 119-125: The change to build the slug input via
JSON.stringify([orgName, townCity ?? '', county ?? '', typeRating, route]) will
change all existing slug hashes (including rows where townCity or county were
null) and thus invalidate existing slugId URLs and CDN edge caches; before
merging, ensure the deployment plan: (1) run the ingestion and immediately
trigger a CDN/edge purge for all HMRC detail routes so stale cached old-hash
responses are removed, (2) implement a temporary redirect mapping (old-hash →
new-hash) or a 301 redirect layer populated during the ingestion swap to
preserve links/SEO, and (3) update the comment in the getHmrcBySlugId handler to
record the date and nature of this one-time slugId recalculation so future
maintainers know the hash scheme changed; reference the slug generation site
(the input variable built with JSON.stringify and the null-coercion '??' on
townCity/county) and the getHmrcBySlugId handler when applying these changes.
---
Nitpick comments:
In `@apps/web/server/utils/withSecret.ts`:
- Around line 11-14: The current compare uses
Buffer.from(secret)/Buffer.from(expected) and a.length === b.length with
timingSafeEqual, which leaks the expected secret length; instead, in withSecret
compute fixed-length hashes (e.g., SHA-256) of both secret and expected using
crypto.createHash and convert those digests to Buffers, then call
timingSafeEqual on the two fixed-size hash buffers (remove the a.length ===
b.length pre-check), and keep the existing expected.length > 0 guard or replace
it with a check against the raw expected being non-empty before hashing; update
references to a, b and equal to use the hashed buffers and the new
timingSafeEqual comparison.
🪄 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: f4acce12-b2d9-476a-9dc5-3b9331b18e0e
⛔ Files ignored due to path filters (1)
.deepsec/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.deepsec/.gitignore.deepsec/AGENTS.md.deepsec/README.md.deepsec/data/learn-tanstack-start/INFO.md.deepsec/data/learn-tanstack-start/SETUP.md.deepsec/deepsec.config.ts.deepsec/package.json.deepsec/pnpm-workspace.yaml.github/actions/run-phase5-sweep/action.yamlapps/ch-stream/Dockerfileapps/web/scripts/ingest-hmrc-csv.tsapps/web/server/utils/withSecret.ts
We do not want to handle the | bug because the practical possibility of this happening is 0. HMRC won't allow company names to include | in their name. But even if they did you would need 2 companies to have the same name with the | in exactly the same spot. In which case it's fine skipping the hash computes as they both can and should point to the same slug.
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores