Harden project security, deletion, and quality gates - #68
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Medium
Line 440 in 332bbee
continueTeamDelete resumes team deletion across multiple transactions without rechecking getTeamSubscriptionState. If a checkout or webhook activates the team's paid subscription after the initial guard in deleteTeam but before a continuation runs, deleteTeamBatch still deletes the team record — leaving an active paid subscription with no team. Recheck getTeamSubscriptionState at the start of deleteTeamBatch (or set a deleting flag on the team and block subscription activation once deletion starts).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @convex/teams.ts around line 440:
`continueTeamDelete` resumes team deletion across multiple transactions without rechecking `getTeamSubscriptionState`. If a checkout or webhook activates the team's paid subscription after the initial guard in `deleteTeam` but before a continuation runs, `deleteTeamBatch` still deletes the team record — leaving an active paid subscription with no team. Recheck `getTeamSubscriptionState` at the start of `deleteTeamBatch` (or set a `deleting` flag on the team and block subscription activation once deletion starts).
| }, [reconcileTeamSubscription, team]); | ||
|
|
||
| if (context === undefined || shouldCanonicalize) { | ||
| if (context === undefined || team === undefined || shouldCanonicalize) { |
There was a problem hiding this comment.
🟠 High dashboard/-settings.tsx:113
When resolveContext returns null for an unknown or unauthorized team, context is null and team (derived from context?.team) is undefined. The first early-return guard checks team === undefined, so it matches this completed not-found state and renders "Loading..." forever — the subsequent context === null || !team branch that should show "Team not found" never runs. Move the team === undefined check into the not-found branch instead of the loading branch.
| if (context === undefined || team === undefined || shouldCanonicalize) { | |
| if (context === undefined || shouldCanonicalize) { |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @app/routes/dashboard/-settings.tsx around line 113:
When `resolveContext` returns `null` for an unknown or unauthorized team, `context` is `null` and `team` (derived from `context?.team`) is `undefined`. The first early-return guard checks `team === undefined`, so it matches this completed not-found state and renders "Loading..." forever — the subsequent `context === null || !team` branch that should show "Team not found" never runs. Move the `team === undefined` check into the not-found branch instead of the loading branch.
| ? uploadableProjectIds.has(routeProjectId) | ||
| : false; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🟡 Medium dashboard/-layout.tsx:88
When the same drop error is triggered twice in succession, the second occurrence disappears almost immediately instead of staying visible for 4 seconds. setDropError receives the same string value that is already in state, so React skips the re-render and the useEffect keyed on dropError never restarts. The original 4-second timeout continues counting down from the first occurrence and clears the second error as little as 0.1 seconds after it appears. Consider keying the timer on a counter or error id so repeated identical errors reset the dismissal window.
Also found in 1 other location(s)
app/routes/dashboard/-project.tsx:275
handleCreateFolderreusesdndError, whose dismissal timer only restarts when the string value changes. If folder creation fails, the user retries within three seconds, and the retry produces the same error message, React keeps the same state value and the existing timer is not reset; the newly surfaced failure can therefore disappear almost immediately instead of remaining visible for a full dismissal interval.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @app/routes/dashboard/-layout.tsx around line 88:
When the same drop error is triggered twice in succession, the second occurrence disappears almost immediately instead of staying visible for 4 seconds. `setDropError` receives the same string value that is already in state, so React skips the re-render and the `useEffect` keyed on `dropError` never restarts. The original 4-second timeout continues counting down from the first occurrence and clears the second error as little as 0.1 seconds after it appears. Consider keying the timer on a counter or error id so repeated identical errors reset the dismissal window.
Also found in 1 other location(s):
- app/routes/dashboard/-project.tsx:275 -- `handleCreateFolder` reuses `dndError`, whose dismissal timer only restarts when the string value changes. If folder creation fails, the user retries within three seconds, and the retry produces the same error message, React keeps the same state value and the existing timer is not reset; the newly surfaced failure can therefore disappear almost immediately instead of remaining visible for a full dismissal interval.
1 participant
Summary
Why
The audit found four material risks: deleted database rows could leave paid external assets behind, subject-only identity comparisons could collide across issuers, common routes performed redundant or amplified reads, and the repository had no enforced quality gate. This change addresses those findings while keeping the identity rollout backwards-compatible with existing data.
Deployment impact
Deploy the widened schema and dual-read code first, then run the documented resumable identity migration in
docs/deployment.md. Legacy identity fields remain in place for rollback and should only be narrowed in a later deployment after every migration reportsdone.Validation
bun install --frozen-lockfilebun run check(52 unit tests and 44 Convex tests)bun run buildNote
Harden project security, deletion, and quality gates with canonical identity and batched cleanup
tokenIdentifier-based identity system across teams, members, invites, videos, comments, and share links, replacing legacy Clerk subject comparisons in all authorization guards (requireTeamAccess,requireProjectAccess,requireVideoAccess) and mutation handlers.userIdentity/ownerIdentity/uploadedByIdentity/createdByIdentityfields on existing records using the legacy issuer+subject format.deleteTeamBatch+continueTeamDelete) and adds adeleteVideoAssetsinternal action with exponential backoff retries to clean up S3 and Mux resources on video deletion.getThreaded, flattens them locally for counts and player markers, and removes thecomments:listquery; also replaceswindow.alertcalls with inline error notices throughout the dashboard.strictNullChecksin tsconfig.json and adds a GitHub Actions CI workflow that runsbun run checkon every push and pull request.by_video_and_timestampcomments index is removed from the schema and is no longer available at runtime.📊 Macroscope summarized 332bbee. 33 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.