feat: Implement comprehensive account and credential management system#10
feat: Implement comprehensive account and credential management system#10
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds comprehensive credential UI (list/detail/edit), encrypted credential key/value and password flows, deterministic tag colors, anti‑autofill protections and permission/default‑container system, many ORPC endpoints/hooks, Prisma schema migrations (key‑value pairs, plans, unique constraints), utilities, Docker config, and wide shared UI updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser
participant RSC as Accounts Detail (RSC)
participant Client as CredentialDetailView (Client)
participant ORPC as oRPC Credential Router
participant DB as Prisma/DB
participant Crypto as Encryption Utils
User->>Browser: open /dashboard/accounts/:id
Browser->>RSC: request page
RSC->>ORPC: server fetch credential + platforms (includes tags & metadata with encryption fields)
ORPC->>DB: read credential + metadata (+ encrypted ids)
DB-->>ORPC: credential data
ORPC-->>RSC: credential + platforms
RSC-->>Browser: render page + hydrate Client with initialData
Browser->>Client: mounts, triggers client hooks
Client->>ORPC: useCredential(id) (client-safe include)
ORPC->>DB: read client-safe fields
DB-->>ORPC: data
ORPC-->>Client: hydrated data
User->>Client: click "reveal password"
Client->>ORPC: getCredentialPassword(id)
ORPC->>DB: fetch encrypted value
ORPC->>Crypto: decryptData (server-side)
Crypto-->>ORPC: plaintext
ORPC-->>Client: decrypted password
User->>Client: save new password
Client->>Crypto: generateEncryptionKey + encryptData (client)
Crypto-->>Client: encrypted payload
Client->>ORPC: updateCredentialPassword(payload)
ORPC->>DB: transaction (create EncryptedData, credential history, update credential)
DB-->>ORPC: ok
ORPC-->>Client: success
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 5
🧹 Nitpick comments (13)
app/(dashboard)/dashboard/accounts/page.tsx (1)
32-49: Log and trace fetch failures (don’t silently swallow).Add a console/error logger (or Sentry) in catch to aid debugging/observability.
} catch { - return { + console.error("AccountsPage:getInitialData failed to fetch initial data") + return { credentials: { credentials: [], total: 0, hasMore: false, page: 1, limit: 50, }, platforms: { platforms: [], total: 0, hasMore: false, page: 1, limit: 100, }, } }schemas/utils/base-key-value-pair.ts (1)
3-11: Optional stricter variant for non-empty values.Encryption paths require non-empty value; export a stricter schema to avoid runtime checks downstream.
export const baseKeyValuePairSchema = z.object({ id: z.string().optional(), key: z.string().min(1, "Key is required"), value: z.string(), createdAt: z.date().optional(), updatedAt: z.date().optional(), encrypted: z.boolean().optional(), isEncrypting: z.boolean().optional(), }) +// Use when value must be present (e.g., before encryption) +export const requiredValueKeyValuePairSchema = baseKeyValuePairSchema.extend({ + value: z.string().min(1, "Value is required"), +})components/shared/key-value-pair-form.tsx (2)
151-163: Add accessible label to the remove button.Improves a11y and discoverability.
{localPairs.length > 1 && ( <Button type="button" variant="ghost" size="icon" onClick={() => handleRemovePair(index)} disabled={disabled} - className="text-muted-foreground hover:text-destructive flex-shrink-0" + className="text-muted-foreground hover:text-destructive flex-shrink-0" + aria-label="Remove key-value pair" > <Icons.trash className="size-4" /> </Button> )}
170-183: Add accessible label to the add button.Minor a11y polish.
<div className="border-border border-t p-3 sm:p-4"> <Button type="button" variant="outline" size="sm" onClick={handleAddPair} disabled={disabled} className="text-muted-foreground hover:text-foreground flex w-full items-center gap-2" + aria-label="Add another key-value pair" > <Icons.add className="size-4" /> Add Another </Button> </div>components/app/dashboard-add-credential-dialog.tsx (2)
538-538: Typo: fix password placeholder.Small UI nit.
- placeholder="En.ter password" + placeholder="Enter password"
441-467: Use computed selectedPlatform to simplify selectedItem.Reduces repeated finds and re-renders.
- selectedItem={ - platforms.find( - (p) => p.id === credentialForm.watch("platformId") - ) - ? { - value: credentialForm.watch("platformId"), - label: - platforms.find( - (p) => - p.id === credentialForm.watch("platformId") - )?.name || "", - logo: getPlaceholderImage( - platforms.find( - (p) => - p.id === credentialForm.watch("platformId") - )?.name || "", - getLogoDevUrlWithToken( - platforms.find( - (p) => - p.id === - credentialForm.watch("platformId") - )?.logo || null - ) - ), - } - : null - } + selectedItem={ + selectedPlatform + ? { + value: selectedPlatform.id, + label: selectedPlatform.name, + logo: getPlaceholderImage( + selectedPlatform.name, + getLogoDevUrlWithToken(selectedPlatform.logo) + ), + } + : null + }Also applies to: 141-143
components/app/dashboard-credential-key-value-pairs.tsx (3)
232-234: Consider enabling duplicate‑key validation in UI.Preventing duplicate keys at the source reduces backend conflicts and user confusion.
- validateDuplicateKeys={false} + validateDuplicateKeys
46-67: Trim payload and de‑dupe keys before save.Sanitize editingData: trim, drop empties, and collapse duplicates (last write wins or block). This avoids silent server‑side errors.
Example insertion before building existing/new arrays:
+ // Normalize and de-dupe by key (case-insensitive) + const norm = editingData + .map(p => ({ ...p, key: p.key?.trim() ?? "", value: p.value?.trim() ?? "" })) + .filter(p => p.key || p.value) + const seen = new Set<string>() + const normalized = [] + for (let i = norm.length - 1; i >= 0; i--) { + const k = norm[i].key.toLowerCase() + if (!seen.has(k)) { seen.add(k); normalized.unshift(norm[i]) } + }Then use normalized instead of editingData.
45-67: Minor: remove unused dep in useMemo.credentialId isn’t used in displayData; drop from deps.
- }, [keyValuePairs, editingData, credentialId]) + }, [keyValuePairs, editingData])components/shared/encrypted-key-value-display.tsx (2)
83-97: Duplicate-key guard silently ignores input.Early-returning on duplicates with no feedback confuses users. Surface an inline error, tooltip, or expose an onDuplicateKeyAttempt callback so the parent can toast.
Minimal option:
- if (validateDuplicateKeys && newKey.trim()) { + if (validateDuplicateKeys && newKey.trim()) { const isDuplicate = value.some( (pair, i) => i !== index && pair.key.trim().toLowerCase() === newKey.trim().toLowerCase() ) - if (isDuplicate) { - return - } + if (isDuplicate) return // TODO: show inline error/tooltip or notify parent }
118-161: Avoid caching decrypted secrets; enforce single-use fetch.gcTime: 0 is good, but decryptedValues persists in state indefinitely. Consider optional auto-clear (e.g., 30–60s timer) or clear on blur to reduce exposure.
entities/credential/credential-key-value/entity.ts (2)
116-135: Parallelize batch encryption.Encrypt pairs concurrently to reduce latency for larger inputs.
- const encryptedPairs: GenericEncryptedKeyValuePairDto[] = [] - - for (const pair of validPairs) { - const encryptedPair = await this.encryptKeyValuePair(pair) - encryptedPairs.push(encryptedPair) - } - - return encryptedPairs + return Promise.all(validPairs.map((p) => this.encryptKeyValuePair(p)))
67-76: Prefer undefined over empty string for missing IDs.Returning id: "" can leak into UI/logic; use undefined to signify absence.
- return { - id: credential.id || "", + return { + id: credential.id, key: credential.key, value: "", }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/(dashboard)/dashboard/accounts/page.tsx(1 hunks)components/app/auth-login-form.tsx(3 hunks)components/app/auth-register-form.tsx(5 hunks)components/app/dashboard-add-credential-dialog.tsx(8 hunks)components/app/dashboard-add-secret-dialog.tsx(3 hunks)components/app/dashboard-create-container-dialog.tsx(1 hunks)components/app/dashboard-credential-key-value-pairs.tsx(1 hunks)components/shared/encrypted-key-value-display.tsx(1 hunks)components/shared/key-value-pair-form.tsx(1 hunks)entities/credential/credential-key-value/entity.ts(1 hunks)lib/utils/card-expiry-helpers.ts(0 hunks)schemas/credential/credential-metadata.ts(1 hunks)schemas/utils/base-key-value-pair.ts(1 hunks)schemas/utils/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- lib/utils/card-expiry-helpers.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- components/app/dashboard-add-secret-dialog.tsx
- schemas/credential/credential-metadata.ts
- components/app/dashboard-create-container-dialog.tsx
- components/app/auth-login-form.tsx
- components/app/auth-register-form.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
components/shared/key-value-pair-form.tsx (2)
schemas/utils/base-key-value-pair.ts (1)
BaseKeyValuePair(13-13)components/shared/icons.tsx (1)
Icons(134-252)
components/app/dashboard-credential-key-value-pairs.tsx (7)
types/index.d.ts (1)
KeyValuePair(56-59)schemas/utils/base-key-value-pair.ts (1)
BaseKeyValuePair(13-13)hooks/use-toast.ts (1)
useToast(87-186)orpc/hooks/use-credentials.ts (2)
useCredentialKeyValuePairs(68-78)useUpdateCredentialKeyValuePairs(282-315)lib/utils/index.ts (1)
handleErrors(139-212)components/app/dashboard-credential-key-value-pairs-skeleton.tsx (1)
DashboardCredentialKeyValuePairsSkeleton(10-89)components/shared/encrypted-key-value-display.tsx (1)
EncryptedKeyValueDisplay(32-264)
app/(dashboard)/dashboard/accounts/page.tsx (2)
orpc/context.ts (1)
createContext(7-24)orpc/client/server.ts (1)
createServerClient(11-17)
entities/credential/credential-key-value/entity.ts (5)
entities/credential/credential-key-value/query.ts (2)
CredentialKeyValuePairSimpleDbData(9-12)CredentialKeyValuePairDbData(4-7)entities/encryption/entity.ts (1)
EncryptedDataEntity(6-30)schemas/encryption/encryption.ts (1)
GenericEncryptedKeyValuePairDto(31-33)schemas/utils/base-key-value-pair.ts (1)
BaseKeyValuePair(13-13)lib/encryption.ts (3)
generateEncryptionKey(14-28)encryptData(37-108)exportKey(193-200)
components/shared/encrypted-key-value-display.tsx (3)
schemas/utils/base-key-value-pair.ts (1)
BaseKeyValuePair(13-13)orpc/hooks/use-credentials.ts (1)
credentialKeys(21-28)components/shared/icons.tsx (1)
Icons(134-252)
components/app/dashboard-add-credential-dialog.tsx (12)
hooks/use-prevent-auto-save.ts (1)
usePreventAutoSave(8-42)hooks/use-aggressive-form-blocker.ts (1)
useAggressiveFormBlocker(8-75)orpc/hooks/use-platforms.ts (1)
usePlatforms(19-29)orpc/hooks/use-tags.ts (1)
useTags(16-22)orpc/hooks/use-credentials.ts (1)
useCreateCredentialWithMetadata(154-177)schemas/credential/credential.ts (3)
credentialDtoSchema(17-29)accountStatusEnum(13-13)AccountStatusInfer(15-15)lib/utils/password-helpers.ts (1)
checkPasswordStrength(34-75)entities/credential/credential-key-value/entity.ts (1)
CredentialKeyValuePairEntity(17-136)lib/utils/index.ts (3)
getPlaceholderImage(227-236)getLogoDevUrlWithToken(242-248)cn(30-32)components/shared/key-value-pair-form.tsx (1)
KeyValuePairForm(25-187)config/converter.tsx (1)
statusConfig(38-57)components/shared/container-selector.tsx (1)
ContainerSelector(199-339)
🔇 Additional comments (7)
schemas/utils/base-key-value-pair.ts (1)
13-13: LGTM.Type export matches schema.
schemas/utils/index.ts (1)
1-1: LGTM.Barrel re-export is correct.
components/app/dashboard-add-credential-dialog.tsx (4)
821-827: Remove unused event param and preventDefault in submit path.This was flagged earlier; the button is type="button" and the form already blocks submit.
- <Button - type="button" - disabled={createCredentialWithMetadataMutation.isPending} - onClick={(e) => { - e.preventDefault() - e.stopPropagation() - onSubmit(e) - }} - > + <Button + type="button" + disabled={createCredentialWithMetadataMutation.isPending} + onClick={() => onSubmit()} + >- async function onSubmit(e?: React.FormEvent) { - if (e) { - e.preventDefault() - e.stopPropagation() - } + async function onSubmit() {
587-605: Server-side safeguard for duplicate tags (schema + atomic create).Client dedupe helps UX, but enforce uniqueness and atomic creation in backend to fully resolve duplicates.
- Add a composite unique index on Tag (name, userId, containerId).
- Make tag creation idempotent/atomic (upserts or createMany with skipDuplicates) inside a transaction.
588-596: Deduplicate tags client‑side to avoid duplicate creation.Normalize by name (case‑insensitive) before saving to the form.
- <TagSelector<TagDto> + <TagSelector<TagDto> selectedTags={credentialForm.watch("tags") || []} availableTags={availableTags.map((tag) => ({ name: tag.name, containerId: tag.containerId || undefined, color: tag.color || undefined, }))} - onChange={(tags: TagDto[]) => - credentialForm.setValue("tags", tags) - } + onChange={(tags: TagDto[]) => { + const seen = new Set<string>() + const unique = [] + for (const t of tags) { + const k = t.name.trim().toLowerCase() + if (!seen.has(k)) { + seen.add(k) + unique.push(t) + } + } + credentialForm.setValue("tags", unique) + }} getValue={(tag: TagDto) => tag.name} getLabel={(tag: TagDto) => tag.name} createTag={(inputValue: string): TagDto => ({ name: inputValue, containerId: undefined, color: undefined, })} />
204-224: Keep form onSubmit minimal; metadata encryption & validation confirmed.
- Optional: if you have an inline onSubmit that calls e.stopPropagation() and returns false, replace with e.preventDefault() only to reduce event noise.
- Verified: CredentialKeyValuePairEntity.encryptKeyValuePairs returns generic encrypted pairs (valueEncryption present); metadataForm.setValue(...) is called before metadataForm.trigger(); credentialKeyValuePairDtoSchema extends the generic encrypted schema and accepts the encrypted shape; server createWithMetadata consumes valueEncryption — flow is correct.
components/app/dashboard-credential-key-value-pairs.tsx (1)
163-198: Stop exposing KV data on window; gate to dev and remove plaintext.displayData (may include plaintext) is attached to window; this is a serious leakage vector. Gate to development only and only expose safe actions/flags. This was flagged earlier and still persists.
Apply:
- useEffect(() => { - if (typeof window !== "undefined") { - // Expose save/cancel handlers on window for toolbar integration - // TODO: Replace with proper React context when implementing save toolbar - const windowWithCredentials = window as Window & { - credentialKeyValuePairs?: { - save: () => Promise<{ success: boolean; error?: string }> - cancel: () => void - hasChanges: boolean - data: KeyValuePair[] - } - } - - windowWithCredentials.credentialKeyValuePairs = { - save: handleSave, - cancel: handleCancel, - hasChanges, - data: displayData, - } - } - - return () => { - if (typeof window !== "undefined") { - const windowWithCredentials = window as Window & { - credentialKeyValuePairs?: { - save: () => Promise<{ success: boolean; error?: string }> - cancel: () => void - hasChanges: boolean - data: KeyValuePair[] - } - } - delete windowWithCredentials.credentialKeyValuePairs - } - } - }, [handleSave, handleCancel, hasChanges, displayData]) + useEffect(() => { + if (process.env.NODE_ENV === "development" && typeof window !== "undefined") { + const w = window as Window & { + credentialKeyValuePairs?: { + save: () => Promise<{ success: boolean; error?: string }> + cancel: () => void + hasChanges: boolean + } + } + w.credentialKeyValuePairs = { save: handleSave, cancel: handleCancel, hasChanges } + return () => { delete w.credentialKeyValuePairs } + } + }, [handleSave, handleCancel, hasChanges])
| const key = await generateEncryptionKey() | ||
| const encryptResult = await encryptData(pair.value, key) | ||
| const keyString = await exportKey(key as CryptoKey) | ||
|
|
||
| return { | ||
| id: pair.id, | ||
| key: pair.key, | ||
| valueEncryption: { | ||
| encryptedValue: encryptResult.encryptedData, | ||
| iv: encryptResult.iv, | ||
| encryptionKey: keyString, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Server crash in Node: exportKey used with string key.
generateEncryptionKey returns a base64 string in Node; exportKey throws in Node. This path will throw during server-side encryption.
Fix:
- const key = await generateEncryptionKey()
- const encryptResult = await encryptData(pair.value, key)
- const keyString = await exportKey(key as CryptoKey)
+ const key = await generateEncryptionKey()
+ const encryptResult = await encryptData(pair.value, key)
+ const keyString =
+ typeof key === "string" ? key : await exportKey(key as CryptoKey)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const key = await generateEncryptionKey() | |
| const encryptResult = await encryptData(pair.value, key) | |
| const keyString = await exportKey(key as CryptoKey) | |
| return { | |
| id: pair.id, | |
| key: pair.key, | |
| valueEncryption: { | |
| encryptedValue: encryptResult.encryptedData, | |
| iv: encryptResult.iv, | |
| encryptionKey: keyString, | |
| }, | |
| } | |
| const key = await generateEncryptionKey() | |
| const encryptResult = await encryptData(pair.value, key) | |
| const keyString = | |
| typeof key === "string" ? key : await exportKey(key as CryptoKey) | |
| return { | |
| id: pair.id, | |
| key: pair.key, | |
| valueEncryption: { | |
| encryptedValue: encryptResult.encryptedData, | |
| iv: encryptResult.iv, | |
| encryptionKey: keyString, | |
| }, | |
| } |
🤖 Prompt for AI Agents
In entities/credential/credential-key-value/entity.ts around lines 101 to 113,
the code assumes generateEncryptionKey returns a CryptoKey and always calls
exportKey, but in Node generateEncryptionKey returns a base64 string so
exportKey throws; change the logic to handle both types by checking if key is a
string and, if so, use it directly as keyString, otherwise call exportKey(key as
CryptoKey); ensure encryptData is called with the appropriate key type and keep
the returned valueEncryption.encryptionKey set to the base64 string in either
case.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
components/shared/container-selector.tsx (3)
249-263: Replace validateEntityForContainer import with ContainerEntity method; fold filters to one pass.Removes a brittle import and reduces work.
Apply:
- const compatibleContainers = useMemo( - () => - containers - .filter((container) => - validateEntityForContainer(container.type, entityType) - ) - .filter( - (container) => - container.name.toLowerCase().includes(searchQuery.toLowerCase()) || - container.description - ?.toLowerCase() - .includes(searchQuery.toLowerCase()) - ), - [containers, entityType, searchQuery] - ) + const compatibleContainers = useMemo(() => { + const q = searchQuery.trim().toLowerCase() + return containers.filter((container) => { + if ( + !ContainerEntity.validateEntityForContainer(container.type, entityType) + ) + return false + const name = container.name.toLowerCase() + const desc = container.description?.toLowerCase() ?? "" + return name.includes(q) || desc.includes(q) + }) + }, [containers, entityType, searchQuery])
63-64: Surface upgrade context when selection is disabled.Currently all options are disabled with no hint. Add a title to explain why.
Apply:
<Button key={container.id} @@ - disabled={container.id === currentContainerId || !canSelect} + disabled={container.id === currentContainerId || !canSelect} + title={ + !canSelect + ? `${getUpgradeMessage(Feature.CONTAINERS).title} ${getUpgradeMessage(Feature.CONTAINERS).description}` + : undefined + } >
68-69: Use design-token color that exists."text-success" may not exist in your theme; prior code used emerald.
Apply:
- <Icons.check className="text-success ml-auto size-3" /> + <Icons.check className="ml-auto size-3 text-emerald-600" />lib/permissions/hooks/use-user-permissions.ts (1)
27-30: Gate “selection” on update, not create (policy check).Selection feels closer to modify than create. If intentional, ignore.
Optional change:
- canSelectContainers: config.features.containers.create, // Allow selection if can create + canSelectContainers: config.features.containers.update, // Selection aligns with modify capabilitylib/permissions/utils.ts (1)
139-151: Hoist hierarchy map to module scope.Avoids reallocating per call.
Apply:
-function isPermissionLevelSufficient( - userLevel: PermissionLevel, - requiredLevel: PermissionLevel -): boolean { - const levelHierarchy = { +const LEVEL_HIERARCHY: Record<PermissionLevel, number> = { [PermissionLevel.NONE]: 0, [PermissionLevel.READ]: 1, [PermissionLevel.WRITE]: 2, [PermissionLevel.ADMIN]: 3, - } - - return levelHierarchy[userLevel] >= levelHierarchy[requiredLevel] +} + +function isPermissionLevelSufficient( + userLevel: PermissionLevel, + requiredLevel: PermissionLevel +): boolean { + return LEVEL_HIERARCHY[userLevel] >= LEVEL_HIERARCHY[requiredLevel] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
components/shared/container-selector.tsx(1 hunks)hooks/use-prevent-auto-save.ts(1 hunks)lib/encryption.ts(6 hunks)lib/permissions/hooks/use-user-permissions.ts(1 hunks)lib/permissions/index.ts(1 hunks)lib/permissions/types.ts(1 hunks)lib/permissions/utils.ts(1 hunks)middleware/permissions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- middleware/permissions.ts
- hooks/use-prevent-auto-save.ts
- lib/permissions/index.ts
- lib/encryption.ts
- lib/permissions/types.ts
🧰 Additional context used
🧬 Code graph analysis (3)
lib/permissions/utils.ts (2)
lib/permissions/index.ts (8)
getPermissionConfig(4-4)PermissionConfig(13-13)hasPermission(5-5)Feature(17-17)PermissionLevel(17-17)getUpgradeMessage(6-6)canPerformAction(7-7)Action(17-17)lib/permissions/types.ts (1)
PermissionConfig(28-43)
components/shared/container-selector.tsx (6)
lib/permissions/utils.ts (1)
getUpgradeMessage(167-192)lib/permissions/types.ts (1)
UserPermissionFlags(8-26)lib/permissions/hooks/use-user-permissions.ts (1)
useUserPermissions(11-40)orpc/hooks/use-containers.ts (1)
useContainers(37-45)entities/utils/container/entity.ts (2)
ContainerEntity(7-108)validateEntityForContainer(29-43)components/app/dashboard-create-container-dialog.tsx (1)
DashboardCreateContainerDialog(64-231)
lib/permissions/hooks/use-user-permissions.ts (2)
orpc/hooks/use-users.ts (1)
useCurrentUser(16-23)lib/permissions/utils.ts (1)
getPermissionConfig(14-45)
🔇 Additional comments (5)
components/shared/container-selector.tsx (1)
42-74: Nice DRY extraction.ContainerList/CreateContainerButton/ContainerSelectorContent remove prior duplication.
Also applies to: 76-127, 129-189
lib/permissions/hooks/use-user-permissions.ts (2)
14-20: Good: flags derived from centralized config.Keeps UI in sync with middleware.
Also applies to: 26-35
42-42: Export verified — no action required.
lib/permissions/types.ts exports UseUserPermissionsReturn (line 54); the export used in lib/permissions/hooks/use-user-permissions.ts is valid.lib/permissions/utils.ts (2)
114-122: Fix included: Feature.CREDENTIALS mapping added.Unblocks credential permission checks for Pro.
14-26: No serialization risk — keep Infinity. No JSON.stringify or DB-write usages of the permission config were found; getPermissionConfig is only used in in-app permission checks and the UI. Relevant locations: lib/permissions/utils.ts (maxCount: Infinity at line 25; comparison at line 231), lib/permissions/hooks/use-user-permissions.ts (maxContainers at line 34).
|
42 comments should be resolved :) ... |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/app/dashboard-add-credential-dialog.tsx (1)
292-338: Key-value metadata persists across new credentials
When we reset the form (both in the “create another” path and when closing the dialog) we never clearplainKeyValuePairs, so the additional-info fields from the previous credential show up—and will be resubmitted—in the next credential. We need to wipe that local state alongside the form resets.metadataForm.reset({ recoveryEmail: "", phoneNumber: "", keyValuePairs: [], has2FA: false, }) - setSensitiveData({ identifier: "", password: "" }) - setPasswordStrength(null) - setShowAdvanced(false) + setSensitiveData({ identifier: "", password: "" }) + setPlainKeyValuePairs([]) + setPasswordStrength(null) + setShowAdvanced(false)if (!open) { credentialForm.reset() metadataForm.reset() setSensitiveData({ identifier: "", password: "" }) setCreateMore(false) + setPlainKeyValuePairs([]) setShowAdvanced(false) setPasswordStrength(null) }
🧹 Nitpick comments (1)
prisma/schema/utils.prisma (1)
42-56: Update the docstrings to match the new schemaThe comments still describe
Tag.colorandContainer.iconas optional, but both columns are now required. Please refresh the JSDoc so future readers (and generated docs) aren’t misled.Also applies to: 73-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
components/app/dashboard-add-card-dialog.tsx(3 hunks)components/app/dashboard-add-card-form.tsx(9 hunks)components/app/dashboard-add-credential-dialog.tsx(8 hunks)components/app/dashboard-add-secret-dialog.tsx(3 hunks)components/app/dashboard-credential-client.tsx(1 hunks)components/app/dashboard-credential-detail-skeleton.tsx(1 hunks)components/app/dashboard-credential-detail-view.tsx(1 hunks)components/app/dashboard-credential-key-value-pairs-skeleton.tsx(1 hunks)components/app/dashboard-credential-key-value-pairs.tsx(1 hunks)components/app/dashboard-credential-password-field.tsx(1 hunks)components/app/dashboard-credential-sidebar.tsx(1 hunks)components/layout/breadcrumb-resource-select.tsx(1 hunks)components/layout/dashboard-dynamic-breadcrumb.tsx(2 hunks)components/shared/coming-soon.tsx(4 hunks)components/shared/container-selector.tsx(1 hunks)components/shared/dashboard-view-controls.tsx(1 hunks)components/shared/encrypted-key-value-display.tsx(1 hunks)components/shared/icons.tsx(6 hunks)components/shared/key-value-pair-form.tsx(1 hunks)config/consts.ts(1 hunks)entities/credential/credential-metadata/query.ts(1 hunks)entities/credential/credential/query.ts(1 hunks)lib/utils/color-helpers.ts(1 hunks)lib/utils/tag-helpers.ts(2 hunks)orpc/routers/credential.ts(11 hunks)prisma/schema/utils.prisma(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- components/shared/dashboard-view-controls.tsx
- components/app/dashboard-credential-sidebar.tsx
- components/app/dashboard-add-secret-dialog.tsx
- components/app/dashboard-add-card-dialog.tsx
- components/app/dashboard-credential-client.tsx
- components/app/dashboard-credential-key-value-pairs-skeleton.tsx
- lib/utils/color-helpers.ts
🧰 Additional context used
🧬 Code graph analysis (15)
components/app/dashboard-credential-password-field.tsx (6)
schemas/credential/dto.ts (1)
CredentialOutput(59-59)hooks/use-toast.ts (1)
useToast(87-186)orpc/hooks/use-credentials.ts (2)
useUpdateCredentialPassword(318-344)useCredentialPassword(44-52)lib/encryption.ts (3)
generateEncryptionKey(14-28)encryptData(37-108)exportKey(194-201)lib/utils/index.ts (1)
handleErrors(139-212)components/shared/icons.tsx (1)
Icons(139-262)
components/app/dashboard-credential-key-value-pairs.tsx (7)
types/index.d.ts (1)
KeyValuePair(54-57)schemas/utils/base-key-value-pair.ts (1)
BaseKeyValuePair(13-13)hooks/use-toast.ts (1)
useToast(87-186)orpc/hooks/use-credentials.ts (2)
useCredentialKeyValuePairs(68-78)useUpdateCredentialKeyValuePairs(282-315)lib/utils/index.ts (1)
handleErrors(139-212)components/app/dashboard-credential-key-value-pairs-skeleton.tsx (1)
DashboardCredentialKeyValuePairsSkeleton(10-79)components/shared/encrypted-key-value-display.tsx (1)
EncryptedKeyValueDisplay(32-256)
components/layout/breadcrumb-resource-select.tsx (12)
lib/utils/breadcrumb-helpers.ts (1)
ResourceType(31-31)orpc/hooks/use-credentials.ts (1)
useCredentials(118-128)orpc/hooks/use-cards.ts (1)
useCards(39-49)orpc/hooks/use-secrets.ts (1)
useSecrets(39-49)orpc/hooks/use-platforms.ts (1)
usePlatforms(19-29)components/ui/skeleton.tsx (1)
Skeleton(13-13)components/shared/icons.tsx (1)
Icons(139-262)components/ui/breadcrumb.tsx (1)
BreadcrumbPage(106-106)components/ui/popover.tsx (3)
Popover(48-48)PopoverTrigger(48-48)PopoverContent(48-48)components/ui/button.tsx (1)
Button(59-59)lib/utils/index.ts (2)
getPlaceholderImage(227-236)getLogoDevUrlWithToken(242-248)components/ui/command.tsx (6)
Command(168-168)CommandInput(170-170)CommandList(171-171)CommandEmpty(172-172)CommandGroup(173-173)CommandItem(174-174)
components/app/dashboard-add-card-form.tsx (1)
lib/utils/color-helpers.ts (1)
generateTagColor(11-29)
components/shared/container-selector.tsx (7)
components/shared/icons.tsx (1)
Icons(139-262)lib/permissions/utils.ts (1)
getUpgradeMessage(167-192)lib/permissions/types.ts (1)
UserPermissionFlags(8-26)lib/permissions/hooks/use-user-permissions.ts (1)
useUserPermissions(11-40)orpc/hooks/use-containers.ts (1)
useContainers(37-45)entities/utils/container/entity.ts (2)
ContainerEntity(7-108)validateEntityForContainer(29-43)components/app/dashboard-create-container-dialog.tsx (1)
DashboardCreateContainerDialog(64-231)
components/shared/encrypted-key-value-display.tsx (2)
schemas/utils/base-key-value-pair.ts (1)
BaseKeyValuePair(13-13)orpc/hooks/use-credentials.ts (1)
credentialKeys(21-28)
components/app/dashboard-add-credential-dialog.tsx (11)
hooks/use-aggressive-form-blocker.ts (1)
useAggressiveFormBlocker(8-75)hooks/use-prevent-auto-save.ts (1)
usePreventAutoSave(8-54)orpc/hooks/use-platforms.ts (1)
usePlatforms(19-29)orpc/hooks/use-credentials.ts (1)
useCreateCredentialWithMetadata(154-177)orpc/hooks/use-tags.ts (1)
useTags(16-22)schemas/credential/credential.ts (4)
CredentialDto(31-31)credentialDtoSchema(17-29)accountStatusEnum(13-13)AccountStatusInfer(15-15)entities/credential/credential-key-value/entity.ts (1)
CredentialKeyValuePairEntity(17-136)lib/utils/index.ts (4)
handleErrors(139-212)getPlaceholderImage(227-236)getLogoDevUrlWithToken(242-248)cn(30-32)components/ui/combobox-responsive.tsx (1)
ComboboxResponsive(43-134)components/shared/key-value-pair-form.tsx (1)
KeyValuePairForm(25-185)config/converter.tsx (1)
statusConfig(38-57)
lib/utils/tag-helpers.ts (3)
schemas/utils/tag.ts (1)
TagDto(9-9)prisma/client/database-client.ts (2)
PrismaTransactionClient(6-9)getDatabaseClient(19-23)lib/utils/color-helpers.ts (1)
generateTagColor(11-29)
components/shared/icons.tsx (1)
schemas/utils/utils.ts (2)
RecentItemType(16-16)RecentItemTypeEnum(15-15)
components/shared/coming-soon.tsx (1)
components/shared/icons.tsx (1)
Icons(139-262)
components/layout/dashboard-dynamic-breadcrumb.tsx (3)
lib/utils/breadcrumb-helpers.ts (2)
isIndividualResourcePage(2-12)getResourceType(15-28)components/layout/breadcrumb-resource-select.tsx (1)
BreadcrumbResourceSelect(38-229)lib/utils/index.ts (1)
capitalizeFirstLetter(54-56)
components/shared/key-value-pair-form.tsx (2)
schemas/utils/base-key-value-pair.ts (1)
BaseKeyValuePair(13-13)components/shared/icons.tsx (1)
Icons(139-262)
components/app/dashboard-credential-detail-view.tsx (7)
schemas/credential/dto.ts (2)
CredentialOutput(59-59)UpdateCredentialInput(52-52)schemas/utils/dto.ts (1)
ListPlatformsOutput(109-109)hooks/use-toast.ts (1)
useToast(87-186)orpc/hooks/use-credentials.ts (5)
useCredential(31-41)useUpdateCredentialWithSecuritySettings(234-279)useUpdateCredential(180-231)useDeleteCredential(347-386)useCredentialSecuritySettings(55-65)schemas/credential/credential.ts (2)
CredentialFormDto(89-89)credentialFormDtoSchema(77-87)lib/utils/index.ts (1)
handleErrors(139-212)components/shared/floating-save-toolbar.tsx (1)
FloatingSaveToolbar(14-45)
entities/credential/credential/query.ts (1)
entities/credential/credential-metadata/query.ts (1)
CredentialMetadataQuery(18-49)
orpc/routers/credential.ts (10)
schemas/credential/dto.ts (4)
getCredentialInputSchema(14-14)updateCredentialPasswordInputSchema(40-47)credentialOutputSchema(28-28)CredentialOutput(59-59)entities/credential/credential/query.ts (1)
CredentialQuery(17-44)entities/credential/credential/entity.ts (1)
CredentialEntity(17-136)schemas/credential/credential-key-value.ts (1)
credentialKeyValuePairWithValueRoSchema(46-52)lib/encryption.ts (2)
decryptData(111-191)encryptData(37-108)middleware/permissions.ts (1)
requirePermission(46-104)lib/utils/tag-helpers.ts (1)
createTagsAndGetConnections(21-96)lib/utils/encryption-helpers.ts (1)
createEncryptedData(20-50)schemas/credential/credential.ts (1)
credentialFormDtoSchema(77-87)entities/credential/credential-metadata/query.ts (1)
CredentialMetadataQuery(18-49)
🔇 Additional comments (8)
components/app/dashboard-credential-password-field.tsx (1)
67-78: Stop sending client-side encryption keys to the server.
savePasswordChangesstill exports the generated key and posts it inpasswordEncryption.encryptionKey. That revives the critical issue we already discussed: the server ends up persisting the very key needed to decrypt the password, so the encryption adds no protection. Drop the client-generated key from the payload and align with the server-side fix (server-owned keys or KMS-wrapped secrets). Once the API contract is updated, prune theexportKeycall and theencryptionKeyfield here.components/shared/container-selector.tsx (1)
15-16: Reuse the canonical ContainerEntity validator instead of the utils alias.We already import
ContainerEntity, so wiring throughContainerEntity.validateEntityForContainerkeeps this component on the official path and avoids the duplicate helper flagged earlier.-import { validateEntityForContainer } from "@/lib/utils" ... - validateEntityForContainer(container.type, entityType) + ContainerEntity.validateEntityForContainer( + container.type, + entityType + )Also applies to: 245-251
orpc/routers/credential.ts (4)
438-444: Enforce WRITE permission on every mutating credential routeOnly
createCredentialappliesrequirePermissionfor WRITE access.updateCredential,updateCredentialPassword,updateCredentialWithSecuritySettings,updateCredentialKeyValuePairs,createCredentialWithMetadata, anddeleteCredentialcurrently bypass that guard, so a user with read-only plan can still mutate credentials. Please apply the same.use(requirePermission({ feature: Feature.CREDENTIALS, level: PermissionLevel.WRITE }))middleware to each of these procedures.export const updateCredential = authProcedure + .use(({ context, next }) => + requirePermission({ + feature: Feature.CREDENTIALS, + level: PermissionLevel.WRITE, + })({ context, next }) + ) .input(updateCredentialInputSchema) @@ -export const deleteCredential = authProcedure +export const deleteCredential = authProcedure + .use(({ context, next }) => + requirePermission({ + feature: Feature.CREDENTIALS, + level: PermissionLevel.WRITE, + })({ context, next }) + ) .input(deleteCredentialInputSchema) @@ -export const updateCredentialPassword = authProcedure +export const updateCredentialPassword = authProcedure + .use(({ context, next }) => + requirePermission({ + feature: Feature.CREDENTIALS, + level: PermissionLevel.WRITE, + })({ context, next }) + ) @@ -export const updateCredentialWithSecuritySettings = authProcedure +export const updateCredentialWithSecuritySettings = authProcedure + .use(({ context, next }) => + requirePermission({ + feature: Feature.CREDENTIALS, + level: PermissionLevel.WRITE, + })({ context, next }) + ) @@ -export const updateCredentialKeyValuePairs = authProcedure +export const updateCredentialKeyValuePairs = authProcedure + .use(({ context, next }) => + requirePermission({ + feature: Feature.CREDENTIALS, + level: PermissionLevel.WRITE, + })({ context, next }) + ) @@ -export const createCredentialWithMetadata = authProcedure +export const createCredentialWithMetadata = authProcedure + .use(({ context, next }) => + requirePermission({ + feature: Feature.CREDENTIALS, + level: PermissionLevel.WRITE, + })({ context, next }) + )Also applies to: 538-543, 600-605, 786-790, 930-936
563-577: Pass the transaction client tocreateEncryptedDataInside the password-update transaction,
createEncryptedDatais executed on the global client. If any later step in the transaction fails, the new encrypted row is still committed, leaving orphaned password blobs. Passtxthrough so the insert participates in the same transaction.- const newPasswordEncryptionResult = await createEncryptedData({ - encryptedValue: passwordEncryption.encryptedValue, - encryptionKey: passwordEncryption.encryptionKey, - iv: passwordEncryption.iv, - }) + const newPasswordEncryptionResult = await createEncryptedData( + { + encryptedValue: passwordEncryption.encryptedValue, + encryptionKey: passwordEncryption.encryptionKey, + iv: passwordEncryption.iv, + }, + tx + )
722-729: Keep security-setting encryption inside the outer transaction
upsertSecuritySettingcreates encrypted rows without the transaction client, so failures later in the tx leak orphaned encrypted records. ThreadtxintocreateEncryptedData.- const encryptedDataResult = await createEncryptedData({ - encryptedValue: encryptionResult.encryptedData, - encryptionKey: encryptionKey, - iv: encryptionResult.iv, - }) + const encryptedDataResult = await createEncryptedData( + { + encryptedValue: encryptionResult.encryptedData, + encryptionKey: encryptionKey, + iv: encryptionResult.iv, + }, + tx + )
1010-1064: Ensure key-value pair encryption is transactionalWithin the
$transaction, calls tocreateEncryptedDatastill execute on the global client, so they commit even if the transaction rolls back, leaving dangling encrypted blobs. Please passtxinto each call.- const encryptedDataResult = await createEncryptedData({ - encryptedValue: encryptionResult.encryptedData, - encryptionKey: encryptionKey, - iv: encryptionResult.iv, - }) + const encryptedDataResult = await createEncryptedData( + { + encryptedValue: encryptionResult.encryptedData, + encryptionKey: encryptionKey, + iv: encryptionResult.iv, + }, + tx + ) @@ - const encryptedDataResult = await createEncryptedData({ - encryptedValue: encryptionResult.encryptedData, - encryptionKey: encryptionKey, - iv: encryptionResult.iv, - }) + const encryptedDataResult = await createEncryptedData( + { + encryptedValue: encryptionResult.encryptedData, + encryptionKey: encryptionKey, + iv: encryptionResult.iv, + }, + tx + )entities/credential/credential-metadata/query.ts (1)
36-48: Nice separation of server vs. client includes.Splitting
getInclude/getClientSafeIncludekeeps encryption metadata on the server path only—exactly what we need to avoid leaking keys.components/app/dashboard-credential-key-value-pairs.tsx (1)
161-195: Do not leak key-value data via the global window.
window.credentialKeyValuePairs.data = displayDataexposes plaintext edits (including secrets for newly added pairs) to every script on the page. This was previously flagged and is still present. Gate the bridge toNODE_ENV === "development"(or drop it entirely) and never attach the raw data array outside dev tooling; only expose inert callbacks/flags.- useEffect(() => { - if (typeof window !== "undefined") { + useEffect(() => { + if ( + typeof window !== "undefined" && + process.env.NODE_ENV === "development" + ) { @@ - windowWithCredentials.credentialKeyValuePairs = { - save: handleSave, - cancel: handleCancel, - hasChanges, - data: displayData, - } + windowWithCredentials.credentialKeyValuePairs = { + save: handleSave, + cancel: handleCancel, + hasChanges, + } } @@ - if (typeof window !== "undefined") { + if ( + typeof window !== "undefined" && + process.env.NODE_ENV === "development" + ) { const windowWithCredentials = window as Window & { credentialKeyValuePairs?: { save: () => Promise<{ success: boolean; error?: string }> cancel: () => void hasChanges: boolean - data: KeyValuePair[] } } delete windowWithCredentials.credentialKeyValuePairs } } - }, [handleSave, handleCancel, hasChanges, displayData]) + }, [handleSave, handleCancel, hasChanges])
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/utils/tag-helpers.ts (1)
84-96: Inconsistent case-sensitivity: findMany after createMany should usemode: "insensitive".Lines 46-53 use
mode: "insensitive"for the initial lookup, but lines 90-96 fetch newly created tags using a case-sensitive query. If the database has a case-insensitive unique constraint or if tags are stored with different casing, this query might miss records.Apply this diff to make the retrieval consistent:
newTags = await client.tag.findMany({ where: { - name: { in: tagsToCreate.map((tag) => tag.name) }, + name: { + in: tagsToCreate.map((tag) => tag.name), + mode: "insensitive" + }, userId, containerId: containerId || null, }, })Alternatively, if the concern about Prisma support for
mode: "insensitive"withinis valid (see previous comment), consider querying by IDs returned fromcreateManyor using a follow-up case-insensitive lookup strategy for the newly created tags.
🧹 Nitpick comments (2)
components/shared/encrypted-key-value-display.tsx (1)
138-140: Consider brief cache window for decrypted values.Setting
gcTime: 0prevents any re-use if the user clicks the eye icon multiple times in quick succession, forcing redundant fetches. A brief cache (e.g.,gcTime: 30000for 30 seconds) would improve UX while maintaining security—React Query will still evict on unmount or after the window.queryFn: () => orpc.credentials.getKeyValuePairValue.call({ credentialId, keyValuePairId: pair.id!, }), staleTime: 0, // Always fetch fresh for security - gcTime: 0, // Don't cache values + gcTime: 30000, // Brief 30s cache for re-clicks })components/app/dashboard-add-credential-dialog.tsx (1)
393-406: Strengthen honeypot inputs with ARIA and data attributes.The hidden honeypot inputs lack
aria-hidden="true"and could benefit from explicit data attributes to mark them for testing/debugging. This prevents screen readers from announcing them and makes intent clearer.<div style={{ display: "none" }}> <input type="text" name="username" tabIndex={-1} autoComplete="username" + aria-hidden="true" + data-honeypot="true" /> <input type="password" name="password" tabIndex={-1} autoComplete="current-password" + aria-hidden="true" + data-honeypot="true" /> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
components/app/dashboard-add-credential-dialog.tsx(8 hunks)components/app/dashboard-credential-key-value-pairs.tsx(1 hunks)components/layout/breadcrumb-resource-select.tsx(1 hunks)components/shared/encrypted-key-value-display.tsx(1 hunks)lib/utils/tag-helpers.ts(2 hunks)orpc/hooks/use-credentials.ts(3 hunks)orpc/hooks/use-platforms.ts(2 hunks)orpc/hooks/use-users.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/app/dashboard-credential-key-value-pairs.tsx
- components/layout/breadcrumb-resource-select.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
orpc/hooks/use-platforms.ts (2)
schemas/utils/dto.ts (2)
ListPlatformsInput(106-106)ListPlatformsOutput(109-109)orpc/client/utils.ts (1)
orpc(5-7)
components/shared/encrypted-key-value-display.tsx (3)
schemas/utils/base-key-value-pair.ts (1)
BaseKeyValuePair(13-13)orpc/hooks/use-credentials.ts (1)
credentialKeys(21-28)components/shared/icons.tsx (1)
Icons(139-262)
lib/utils/tag-helpers.ts (3)
schemas/utils/tag.ts (1)
TagDto(9-9)prisma/client/database-client.ts (2)
PrismaTransactionClient(6-9)getDatabaseClient(19-23)lib/utils/color-helpers.ts (1)
generateTagColor(11-29)
components/app/dashboard-add-credential-dialog.tsx (13)
hooks/use-aggressive-form-blocker.ts (1)
useAggressiveFormBlocker(8-75)hooks/use-prevent-auto-save.ts (1)
usePreventAutoSave(8-54)orpc/hooks/use-platforms.ts (1)
usePlatforms(18-28)orpc/hooks/use-credentials.ts (1)
useCreateCredentialWithMetadata(154-177)orpc/hooks/use-tags.ts (1)
useTags(16-22)schemas/utils/base-key-value-pair.ts (1)
BaseKeyValuePair(13-13)schemas/credential/credential.ts (4)
CredentialDto(31-31)credentialDtoSchema(17-29)accountStatusEnum(13-13)AccountStatusInfer(15-15)entities/credential/credential-key-value/entity.ts (1)
CredentialKeyValuePairEntity(17-136)lib/utils/index.ts (3)
getPlaceholderImage(227-236)getLogoDevUrlWithToken(242-248)cn(30-32)components/ui/combobox-responsive.tsx (1)
ComboboxResponsive(43-134)schemas/utils/tag.ts (1)
TagDto(9-9)components/shared/key-value-pair-form.tsx (1)
KeyValuePairForm(25-185)config/converter.tsx (1)
statusConfig(38-57)
orpc/hooks/use-credentials.ts (2)
schemas/credential/dto.ts (4)
CredentialOutput(59-59)ListCredentialsInput(57-57)ListCredentialsOutput(63-63)UpdateCredentialPasswordInput(53-55)orpc/client/utils.ts (1)
orpc(5-7)
orpc/hooks/use-users.ts (2)
orpc/client/utils.ts (1)
orpc(5-7)orpc/hooks/use-containers.ts (1)
containerKeys(15-25)
🔇 Additional comments (21)
orpc/hooks/use-platforms.ts (1)
18-27: LGTM! Clean implementation of optional query options.The extension to accept
UseQueryOptionsfollows best practices: theOmittype correctly prevents callers from overridingqueryKeyorqueryFn, and the spread operator preserves existing behavior while allowing fine-grained control over caching, refetching, and other query settings.orpc/hooks/use-users.ts (2)
17-25: LGTM! Sensible defaults for user data caching.The 5-minute
staleTimeandretry: 1configuration appropriately balance freshness with performance for current user data. The query key factory integration is clean.
75-91: Invalidation key is correct (past review comment no longer applies).The invalidation at line 84 correctly uses
containerKeys.all(without parentheses) becausecontainerKeys.allis defined as a constant array["containers"] as const, not a function. The past review comment suggestingcontainerKeys.all()is now incorrect.The mutation implementation looks good: proper error logging, success-conditional invalidation, and correct cache key targeting.
orpc/hooks/use-credentials.ts (7)
31-41: LGTM! Consistent with the options pattern.The extension to accept optional
UseQueryOptionsfollows the same type-safe pattern asuse-platforms.ts, preserving query key and function integrity.
44-52: Good security posture: no caching for passwords.The
staleTime: 0andgcTime: 0configuration ensures passwords are never cached, which is appropriate for this sensitive data. Theenabledparameter allows controlled fetching.
55-65: Security-sensitive hooks correctly implement conditional caching.The tiered approach is sound:
- Security settings: 5-minute cache (less sensitive, can be cached)
- Key-value pairs (keys only): 2-minute cache (metadata, less risky)
- Key-value pairs with values: no cache (sensitive data)
- Individual key-value pair values: no cache (sensitive data)
Each hook appropriately uses
enabledto prevent unintended fetches of sensitive data.Also applies to: 68-78, 81-92, 95-115
118-128: LGTM! Consistent with the options pattern.The extension preserves the existing
placeholderDatabehavior while allowing additional query configuration.
234-279: LGTM! Proper optimistic update with targeted invalidation.The mutation correctly:
- Implements optimistic updates with rollback on error
- Invalidates both the credential list and the specific security settings query
- Follows the established pattern from
useUpdateCredential
282-315: LGTM! Comprehensive cache invalidation for key-value pairs.The invalidation strategy correctly targets all related query keys:
key-value-pairs(keys only)key-value-pairs-with-values(with sensitive values)key-value-pair-value(individual values)- Main credential detail
This ensures all cached variants are refreshed after an update.
318-344: Password invalidation key is now correct (past review comment resolved).Line 332 now correctly uses
[...credentialKeys.detail(variables.id), "password"], which matches the fetch key at line 46. The past review comment flagging a mismatch with["credential-password", id]has been addressed.The mutation properly invalidates:
- The credential detail
- The password-specific query
- The credential lists
components/shared/encrypted-key-value-display.tsx (1)
158-175: LGTM – Display logic now correct.The updated logic correctly prioritizes explicit
pair.valuefirst, preventing the masked-display bug reported in the past review. Users can now edit values reliably.components/app/dashboard-add-credential-dialog.tsx (3)
700-720: LGTM – Key-value pair encryption now correctly persisted.The plainKeyValuePairs → encrypt → convertGenericToCredential flow ensures additionalInfos are correctly shaped and encrypted before submission, resolving the past reported issue.
81-82: LGTM – Aggressive anti-autofill strategy appropriate for vault forms.The combination of
useAggressiveFormBlocker,usePreventAutoSave, hidden honeypots,data-form-isolatedattributes, and explicitautoCompleteoverrides provides defense-in-depth against browser/extension password managers. This is appropriate for a credential vault where autofill could compromise security.Also applies to: 383-391
577-595: Duplicate tag creation issue: add DB uniqueness and atomic creation.The reported duplicate-tag issue stems from a race condition in tag creation (lib/utils/tag-helpers.ts uses findMany → createMany without atomicity). The Tag model lacks a composite unique constraint on
(name, userId, containerId), so concurrent requests can create duplicates.Fix:
- Add
@@unique([name, userId, containerId])to the Tag model inprisma/schema/utils.prismaand run a migration.- Update
lib/utils/tag-helpers.tsto usecreateManywithskipDuplicates: true(if your DB supports it), or perform per-tag upserts inside a transaction.Run the following script to confirm the Tag model lacks the uniqueness constraint:
#!/bin/bash # Check Prisma schema for Tag uniqueness constraint rg -nP 'model\s+Tag\s*\{' -A20 prisma/schemalib/utils/tag-helpers.ts (7)
1-9: LGTM! Clean import reorganization.The imports now properly include transaction support (
getDatabaseClient,PrismaTransactionClient) and deterministic color generation (generateTagColor), aligning with the PR's goal of comprehensive credential management.
10-26: Function overloads correctly typed for transaction support.The three overloads properly handle the optional
txparameter with correct TypeScript typing. This enables both standalone usage and transactional usage from callers likecard.ts.
33-44: Normalization logic correctly deduplicates and preserves original case.The implementation properly:
- Trims and lowercases for the map key (deduplication)
- Preserves the original trimmed case in the tag value
- Filters out empty strings
55-73: Deduplication and filtering logic is sound.The code correctly:
- Deduplicates by lowercase key
- Prefers container-specific tags over null-container tags
- Filters out tags that already exist
79-79: Deterministic color generation aligns with PR goals.Using
generateTagColorinstead of random colors ensures consistent tag appearance across sessions, improving UX.
99-99: LGTM! Correct aggregation of existing and new tags.Using
existingTagsByName.values()ensures deduplicated existing tags are combined with newly created tags.
46-53: Verifymode: "insensitive"+in(Postgres, @prisma/client ^6.9.0)Repo uses @prisma/client ^6.9.0 with a PostgreSQL datasource; Prisma docs state
mode: 'insensitive'is supported for Postgres string filters, but there are reported TypeScript/runtime issues whenmodeis used inside some nested/list filters. Ifname: { in: normalizedNames, mode: "insensitive" }fails to type-check or run, either run a quick local test or replace with an OR-of-equals fallback (map each name to{ name: { equals: name, mode: "insensitive" } }) and adjust the containerId OR as needed. File: lib/utils/tag-helpers.ts (lines 46–53). (prismagraphql.com)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
orpc/routers/container.ts (1)
149-191: Make updateContainer tag updates transactional to avoid orphan Tag rows.createTagsAndGetConnections here runs outside a transaction; if the subsequent update fails, newly created Tag rows remain. Wrap the whole update in a tx and pass tx to the tag helper.
- .handler(async ({ input, context }): Promise<ContainerOutput> => { + .handler(async ({ input, context }): Promise<ContainerOutput> => { const { id, ...updateData } = input - // Verify container ownership - const existingContainer = await database.container.findFirst({ + // Verify container ownership + const existingContainer = await database.container.findFirst({ where: { id, userId: context.user.id, ...(context.permissions?.canOnlyAccessDefaultContainers && { isDefault: true, }), }, }) if (!existingContainer) { throw new ORPCError("NOT_FOUND") } - // Process the update data - const updatePayload: Prisma.ContainerUpdateInput = {} + // Process the update data + const updatePayload: Prisma.ContainerUpdateInput = {} if (updateData.name !== undefined) updatePayload.name = updateData.name if (updateData.icon !== undefined) updatePayload.icon = updateData.icon if (updateData.description !== undefined) updatePayload.description = updateData.description if (updateData.type !== undefined) updatePayload.type = updateData.type - // Handle tags if provided - if (updateData.tags !== undefined) { - const tagConnections = await createTagsAndGetConnections( - updateData.tags, - context.user.id, - undefined - ) - updatePayload.tags = tagConnections - } - - const updatedContainer = await database.container.update({ - where: { id }, - data: updatePayload, - }) + const updatedContainer = await database.$transaction(async (tx) => { + // Handle tags if provided + if (updateData.tags !== undefined) { + const tagConnections = await createTagsAndGetConnections( + updateData.tags, + context.user.id, + undefined, + tx + ) + updatePayload.tags = tagConnections + } + return tx.container.update({ + where: { id }, + data: updatePayload, + }) + }) return ContainerEntity.getSimpleRo(updatedContainer) })orpc/routers/credential.ts (4)
317-344: List returns data from all containers; restrict to default containers for Normal users.Add a conditional where clause to honor default‑only access. Also prefer a safe include for list responses.
const where = { userId: context.user.id, + ...(context.permissions?.canOnlyAccessDefaultContainers && { + OR: [{ container: { isDefault: true } }, { containerId: null }], + }), ...(containerId && { containerId }), ...(platformId && { platformId }), @@ - include: { ...CredentialQuery.getInclude() }, + include: { ...CredentialQuery.getClientSafeInclude() },Also applies to: 349-351
47-57: Read endpoint should respect default‑only access.Gate ownership lookups by container.isDefault for Normal users.
const credential = await database.credential.findFirst({ where: { id: input.id, userId: context.user.id, + ...(context.permissions?.canOnlyAccessDefaultContainers && { + container: { isDefault: true }, + }), }, })Apply similar filtering to getPassword, getCredentialSecuritySettings, getCredentialKeyValuePairs, getCredentialKeyValuePairsWithValues, getCredentialKeyValuePairValue.
Also applies to: 63-70
437-456: Make updateCredential atomic; pass tx to encryption to avoid orphaned rows.createEncryptedData runs outside a transaction; if the update throws (e.g., P2002), the encryptedData row leaks.
- .handler(async ({ input, context }): Promise<CredentialOutput> => { + .handler(async ({ input, context }): Promise<CredentialOutput> => { const { id, ...updateData } = input - // Verify credential ownership - const existingCredential = await database.credential.findFirst({ + // Verify credential ownership + const existingCredential = await database.credential.findFirst({ where: { id, userId: context.user.id, + ...(context.permissions?.canOnlyAccessDefaultContainers && { + container: { isDefault: true }, + }), }, }) if (!existingCredential) { throw new ORPCError("NOT_FOUND") } - // Process the update data - const updatePayload: Prisma.CredentialUpdateInput = {} + // Process the update data + const updatePayload: Prisma.CredentialUpdateInput = {} @@ - // Handle password encryption updates if provided - if (updateData.passwordEncryption) { - const passwordEncryptionResult = await createEncryptedData({ - encryptedValue: updateData.passwordEncryption.encryptedValue, - encryptionKey: updateData.passwordEncryption.encryptionKey, - iv: updateData.passwordEncryption.iv, - }) - if (!passwordEncryptionResult.success || !passwordEncryptionResult.encryptedData) { - throw new ORPCError("INTERNAL_SERVER_ERROR") - } - updatePayload.passwordEncryption = { - connect: { id: passwordEncryptionResult.encryptedData.id }, - } - } - try { - const updatedCredential = await database.credential.update({ - where: { id }, - data: updatePayload, - }) + try { + const updatedCredential = await database.$transaction(async (tx) => { + if (updateData.passwordEncryption) { + const passwordEncryptionResult = await createEncryptedData( + { + encryptedValue: updateData.passwordEncryption.encryptedValue, + encryptionKey: updateData.passwordEncryption.encryptionKey, + iv: updateData.passwordEncryption.iv, + }, + tx + ) + if (!passwordEncryptionResult.success || !passwordEncryptionResult.encryptedData) { + throw new ORPCError("INTERNAL_SERVER_ERROR") + } + updatePayload.passwordEncryption = { + connect: { id: passwordEncryptionResult.encryptedData.id }, + } + } + return tx.credential.update({ where: { id }, data: updatePayload }) + }) return CredentialEntity.getSimpleRo(updatedCredential) } catch (error) {Also applies to: 508-527
772-789: Delete should also respect default‑only access.Restrict ownership check by container.isDefault for Normal users.
const existingCredential = await database.credential.findFirst({ where: { id: input.id, userId: context.user.id, + ...(context.permissions?.canOnlyAccessDefaultContainers && { + container: { isDefault: true }, + }), }, })
🧹 Nitpick comments (6)
prisma/seed/credentials.ts (1)
165-201: Consider removing redundant container check.The
if (accountsContainer)condition at line 166 is redundant because the code already guards against missingaccountsContainerat line 56 with acontinue. While defensive, the condition will always be true at this point.Apply this diff to simplify:
- // AWS credential - goes to default Accounts container - if (accountsContainer) { + // AWS credential - goes to default Accounts container // Prepare encrypted data for AWS password const awsPasswordEncrypted = await encryptDataSync( awsPassword, SEED_ENCRYPTION_CONFIG.MASTER_KEY, SEED_ENCRYPTION_CONFIG.CREDENTIAL_PASSWORD_IV ) encryptedDataToCreate.push({ id: awsPasswordEncId, encryptedValue: awsPasswordEncrypted, encryptionKey: SEED_ENCRYPTION_CONFIG.MASTER_KEY, iv: SEED_ENCRYPTION_CONFIG.CREDENTIAL_PASSWORD_IV, }) // Prepare AWS credential credentialsToCreate.push({ id: awsCredId, identifier: `${user.name.replace(/\s+/g, ".").toLowerCase()}@company.com`, passwordEncryptionId: awsPasswordEncId, status: AccountStatus.ACTIVE, description: "AWS account", updatedAt: new Date(), createdAt: new Date(), platformId: awsPlatform.id, userId: user.id, containerId: accountsContainer.id, }) // Store tag connections for AWS credential if (workTag) { credentialTagConnections.push({ credentialId: awsCredId, tagIds: [workTag.id], }) } - }components/app/dashboard-add-credential-dialog.tsx (2)
430-456: Consider extracting platform selection logic for clarity.The nested
platforms.findcalls to constructselectedItemare repeated three times within the object literal, making the code harder to read and maintain.You can extract this logic:
+ const selectedPlatformItem = selectedPlatform ? { + value: selectedPlatform.id, + label: selectedPlatform.name, + logo: getPlaceholderImage( + selectedPlatform.name, + getLogoDevUrlWithToken(selectedPlatform.logo || null) + ), + } : null + <ComboboxResponsive items={platforms.map((platform) => ({ value: platform.id, label: platform.name, logo: getPlaceholderImage( platform.name, getLogoDevUrlWithToken(platform.logo || null) ), }))} - selectedItem={ - platforms.find( - (p) => p.id === credentialForm.watch("platformId") - ) - ? { - value: credentialForm.watch("platformId"), - label: - platforms.find( - (p) => - p.id === credentialForm.watch("platformId") - )?.name || "", - logo: getPlaceholderImage( - platforms.find( - (p) => - p.id === credentialForm.watch("platformId") - )?.name || "", - getLogoDevUrlWithToken( - platforms.find( - (p) => - p.id === - credentialForm.watch("platformId") - )?.logo || null - ) - ), - } - : null - } + selectedItem={selectedPlatformItem} onSelect={(item) =>
696-716: KeyValuePairForm integration works but has subtle conversion flow.The
onChangehandler (lines 698-707) converts pairs viaconvertFromKeyValueManagerimmediately, creating placeholder DTO shapes with empty encryption fields. The actual encryption happens later inonSubmit(lines 196-210), which overwrites these placeholders.This works correctly but creates two conversion points that future maintainers might find confusing. The flow is:
- User edits →
setPlainKeyValuePairs+ set placeholder DTOs to form- Submit → encrypt
plainKeyValuePairs→ convert to credential DTOs → overwrite formConsider adding a comment explaining why both
plainKeyValuePairsstate and form state are maintained, or simplifying to only updateplainKeyValuePairsstate here and defer all DTO conversion to submit time.prisma/schema/credential.prisma (1)
134-145: Enforce one-to-one between key/value rows and their encrypted payloads.Each
CredentialKeyValuePairshould own exactly one encrypted blob. Without a uniqueness check onvalueEncryptionId, nothing stops multiple rows from pointing at the same encrypted payload, which complicates deletes/rotations and risks stale data. Let’s tighten the schema by marking the column as unique.- valueEncryptionId String + valueEncryptionId String @uniqueorpc/routers/credential.ts (2)
72-106: Reads: prefer client‑safe includes where decryption isn’t needed.
- getCredentialSecuritySettings, getPassword must include encryption; OK.
- For getCredentialKeyValuePairs (keys‑only), switch to CredentialMetadataQuery.getClientSafeInclude to avoid loading encryption keys unnecessarily.
- include: CredentialQuery.getInclude(), // Note: Uses full include but only returns safe fields + include: CredentialQuery.getClientSafeInclude(),This also applies to getCredentialKeyValuePairsWithValues only if you decrypt via a separate fetch for the selected kv; otherwise keep as-is.
Also applies to: 269-315, 107-127, 154-166, 215-235
954-967: Dedupe incoming keyValuePairs by key to reduce redundant writes.If the payload contains duplicate keys, the loop repeats updates. Dedupe first, keeping the last value.
- const { credentialId, keyValuePairs } = input + const { credentialId } = input + const kvByKey = new Map<string, { id?: string; key: string; value?: string }>() + for (const kv of input.keyValuePairs) { + const key = kv.key.trim() + if (!key) continue + kvByKey.set(key, { ...kv, key }) + } + const keyValuePairs = Array.from(kvByKey.values())Also applies to: 1020-1110
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
components/app/dashboard-add-credential-dialog.tsx(8 hunks)components/shared/container-selector.tsx(1 hunks)components/shared/encrypted-key-value-display.tsx(1 hunks)lib/utils/default-containers.ts(1 hunks)orpc/routers/container.ts(14 hunks)orpc/routers/credential.ts(12 hunks)orpc/routers/user.ts(3 hunks)prisma/schema/credential.prisma(4 hunks)prisma/schema/utils.prisma(7 hunks)prisma/seed/containers.ts(1 hunks)prisma/seed/credentials.ts(7 hunks)prisma/seed/users.ts(5 hunks)schemas/user/user.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- orpc/routers/user.ts
- prisma/schema/utils.prisma
- components/shared/encrypted-key-value-display.tsx
- lib/utils/default-containers.ts
- components/shared/container-selector.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
orpc/routers/container.ts (5)
middleware/permissions.ts (2)
requireDefaultContainerAccess(147-185)requireContainerPermission(109-127)lib/utils/tag-helpers.ts (1)
createTagsAndGetConnections(21-103)entities/utils/container/entity.ts (1)
ContainerEntity(7-108)lib/utils/encryption-helpers.ts (1)
createEncryptedData(20-50)schemas/utils/utils.ts (1)
EntityTypeSchema(67-67)
components/app/dashboard-add-credential-dialog.tsx (9)
hooks/use-aggressive-form-blocker.ts (1)
useAggressiveFormBlocker(8-75)hooks/use-prevent-auto-save.ts (1)
usePreventAutoSave(8-54)schemas/utils/base-key-value-pair.ts (1)
BaseKeyValuePair(13-13)schemas/credential/credential.ts (4)
CredentialDto(31-31)credentialDtoSchema(17-29)accountStatusEnum(13-13)AccountStatusInfer(15-15)entities/credential/credential-key-value/entity.ts (1)
CredentialKeyValuePairEntity(17-136)lib/utils/index.ts (4)
handleErrors(139-212)getPlaceholderImage(227-236)getLogoDevUrlWithToken(242-248)cn(30-32)schemas/utils/tag.ts (1)
TagDto(9-9)components/shared/key-value-pair-form.tsx (1)
KeyValuePairForm(25-185)config/converter.tsx (1)
statusConfig(38-57)
orpc/routers/credential.ts (10)
schemas/credential/dto.ts (4)
getCredentialInputSchema(14-14)updateCredentialPasswordInputSchema(40-47)credentialOutputSchema(28-28)CredentialOutput(59-59)entities/credential/credential/query.ts (1)
CredentialQuery(17-44)entities/credential/credential/entity.ts (1)
CredentialEntity(17-136)schemas/credential/credential-key-value.ts (1)
credentialKeyValuePairWithValueRoSchema(46-52)lib/encryption.ts (2)
decryptData(111-191)encryptData(37-108)middleware/permissions.ts (1)
requirePermission(46-104)lib/utils/tag-helpers.ts (1)
createTagsAndGetConnections(21-103)lib/utils/encryption-helpers.ts (1)
createEncryptedData(20-50)schemas/credential/credential.ts (1)
credentialFormDtoSchema(77-87)entities/credential/credential-metadata/query.ts (1)
CredentialMetadataQuery(18-49)
🔇 Additional comments (28)
schemas/user/user.ts (1)
1-1: Past review suggestions successfully implemented.The schema now includes:
- Email format validation with
.email()- Flexible
createdAthandling that accepts bothDateobjects (server-side) and ISO datetime strings (serialized/client-side)- Proper Prisma enum integration for the
planfieldThe implementation is robust and handles serialization boundaries correctly.
Also applies to: 32-41
prisma/seed/users.ts (3)
5-47: LGTM! Helper properly implements default containers for seeding.The seeder-specific helper correctly creates the three default containers (Accounts, Cards, Environment Variables) with appropriate types and flags. The
skipDuplicates: truemakes this idempotent as requested in the past review.Note:
createManyreturns{ count: number }, not the actual container records, but since the caller doesn't use the return value this is fine.
52-86: LGTM! Plan assignments provide good test coverage.The mix of NORMAL and PRO plans across seed users provides appropriate test data for both user tiers.
111-123: LGTM! Robust error handling for default container creation.The try/catch with per-user logging ensures that seed failures are visible while allowing other users to be seeded successfully.
prisma/seed/credentials.ts (4)
50-61: LGTM! Guard properly prevents credential seeding without default container.The guard correctly checks for the Accounts container and skips seeding with a clear warning if missing, preventing NOT NULL constraint violations as requested in the past review.
86-119: LGTM! Google credential correctly targets default Accounts container.
122-163: LGTM! GitHub credential properly handles multi-space names.The identifier generation now uses
/\s+/gto remove all whitespace globally, addressing the past review comment. Credential correctly targets the Accounts container.
204-239: LGTM! Bulk operations properly structured.Transaction ensures atomicity for data creation, and tag connections are correctly handled separately since
createManydoesn't support relation connections.prisma/seed/containers.ts (3)
3-11: LGTM! Clear separation between default and additional containers.The scope change to Pro users only is appropriate, as default containers are now created per user in
users.ts. Logging and variable names accurately reflect that these are additional/custom containers.
13-41: LGTM! Pro user containers properly configured.Work and Finance containers are correctly marked as non-default (
isDefault: false) and use the flexible MIXED type appropriate for custom Pro user containers.
43-51: LGTM! Idempotent seeding with skipDuplicates.The
skipDuplicates: trueoption properly addresses the past review comment, making the seed script idempotent and safe to re-run.components/app/dashboard-add-credential-dialog.tsx (4)
196-210: LGTM! additionalInfos persistence fix correctly implemented.The fix for the reported bug ("additionalInfos are not being created") has been properly implemented. The code now:
- Encrypts the plainKeyValuePairs
- Converts them to CredentialKeyValuePairDto using
convertGenericToCredential- Sets the credential-shaped DTOs to the form
This ensures the keyValuePairs match the expected schema and will be persisted correctly.
803-812: LGTM! onSubmit signature correctly simplified.The
onSubmitfunction is now called without event parameters (line 806), and the function signature (line 166) no longer accepts unnecessaryReact.FormEventparameters. This correctly reflects that it's invoked from atype="button"click handler, not a form submission.
81-82: Anti-autofill implementation is thorough.The aggressive anti-autofill measures are comprehensive:
useAggressiveFormBlockerandusePreventAutoSavehooks- Hidden honeypot inputs (lines 389-402)
data-testid="vault-form"anddata-form-isolatedattributes- Password manager ignore attributes (
data-lpignore,data-1p-ignore,data-bwignore)autoComplete="off"andautoComplete="new-password"This multi-layered approach effectively prevents browser and password manager interference with credential vault forms.
Also applies to: 383-402
573-591: Remove schema change – composite unique constraint already declared. Verify your backend tag-creation logic uses an atomic upsert orcreateMany…skipDuplicates(in your tag-helpers module or wherever you persist tags) to prevent duplicates at the database layer.prisma/schema/credential.prisma (1)
51-51: Verify no existing Credential duplicates before applying the composite unique constraint.Ensure DATABASE_URL is set, then run:
psql "$DATABASE_URL" <<'SQL' SELECT identifier, platform_id, user_id, COUNT(*) AS duplicate_count FROM credential GROUP BY 1,2,3 HAVING COUNT(*) > 1; SQLIf any rows are returned, dedupe them before applying
@@unique([identifier, platformId, userId]).orpc/routers/container.ts (8)
45-56: Default-only access enforced on getContainer — good.Switch to authWithDefaultAccessProcedure and conditional isDefault filter look correct. This addresses prior gating concerns.
67-79: List respects default-only access — good.Conditional isDefault constraint under canOnlyAccessDefaultContainers is correctly applied.
109-123: Create enforces plan restriction early — good.Guarding Normal users and chaining permission checks via authWithDefaultAccessProcedure is correct.
125-142: Tags created inside a transaction — good.Passing tx to createTagsAndGetConnections prevents partial writes.
201-216: Delete respects default-only access — good.Ownership lookup includes isDefault gating; permission level ADMIN enforced.
231-258: Create-with-secrets is properly transactional and passes tx to encryption + tags — good.Plan guard, tx usage, and helper calls are correct.
Also applies to: 271-303
337-359: Default-container lookup by entity type — good.Schema usage and mapping via ContainerEntity.getDefaultContainerTypeForEntity look correct.
361-377: User default containers endpoint — good.Filters and ordering are appropriate.
orpc/routers/credential.ts (4)
382-388: Duplicate tags on create — fixed.createTagsAndGetConnections is now used with tx and normalized lookups; this should resolve duplicate tag creation.
Also applies to: 825-833
865-909: “additionalInfos not being created” — fixed.Metadata and encrypted key‑value pairs are created within the same transaction and pass tx to createEncryptedData.
Please confirm the client uses createCredentialWithMetadata when additional infos are present; otherwise, those fields will be ignored by createCredential.
1-6: Consistency: enforce WRITE permission is now applied across mutations — good.WRITE checks are consistently chained on create/update/delete and related mutations.
Also applies to: 31-36
826-841: All createEncryptedData calls inside tx — good.Passing tx ensures no orphaned encryptedData on failures.
Also applies to: 880-907, 1039-1046
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
lib/permissions/utils.ts (2)
180-184: Confusing upgrade message for CREDENTIALS feature.The message states "Credential management is available" and "You can create and manage credentials with your current plan." This doesn't follow the upgrade-prompt pattern of other features (which encourage users to upgrade). Since both NORMAL and PRO users have WRITE access to CREDENTIALS, this message will be shown to both—but it reads more like a confirmation than an upgrade prompt.
Consider either:
- Removing this case entirely and relying on the default message (since both plans have access), or
- Clarifying when this message would actually be shown (e.g., if there's a future plan tier that doesn't have CREDENTIALS access)
Apply this diff if you prefer to remove the confusing case:
const messages: Record<Feature, { title: string; description: string }> = { [Feature.CONTAINERS]: { title: "You can only use Containers on a Pro plan", description: "and above. Upgrade to Pro to continue.", }, [Feature.EXPORTS]: { title: "Data exports require a Pro plan", description: "Upgrade to export your data in various formats.", }, - [Feature.CREDENTIALS]: { - title: "Credential management is available", - description: - "You can create and manage credentials with your current plan.", - }, }
14-45: Consider adding CREDENTIALS and EXPORTS to getPermissionConfig for consistency.Currently,
getPermissionConfigonly returns configuration for thecontainersfeature, butgetUserPermissionLevelhandles CONTAINERS, EXPORTS, and CREDENTIALS. If other parts of the codebase callgetPermissionConfigexpecting feature-specific limits or flags for CREDENTIALS or EXPORTS, they'll find only the containers field.Extend
PermissionConfigandgetPermissionConfigto include CREDENTIALS and EXPORTS configurations (or document thatgetPermissionConfigis containers-only). For example:export function getPermissionConfig(plan: UserPlan): PermissionConfig { switch (plan) { case UserPlan.PRO: return { plan, features: { containers: { create: true, update: true, delete: true, unlimited: true, maxCount: Infinity, }, + credentials: { + create: true, + update: true, + delete: true, + }, + exports: { + enabled: true, + }, }, } case UserPlan.NORMAL: default: return { plan, features: { containers: { create: false, update: false, delete: false, unlimited: false, maxCount: 3, }, + credentials: { + create: true, + update: true, + delete: true, + }, + exports: { + enabled: false, + }, }, } } }You'll also need to update
lib/permissions/types.tsto reflect these new fields inPermissionConfig.orpc/routers/credential.ts (2)
1066-1067: Consider validation or sanitization for key names.Line 1066 skips empty keys (
if (!kvPair.key.trim()) continue), which is good, but there's no validation for:
- Maximum key length
- Disallowed characters (e.g., control characters, special symbols that might break UI)
- Reserved key names beyond
passwordProtectionandaccessLoggingAdd validation at the input schema level or within the handler:
for (const kvPair of keyValuePairs) { - if (!kvPair.key.trim()) continue + const trimmedKey = kvPair.key.trim() + if (!trimmedKey) continue + if (trimmedKey.length > 100) { + throw new ORPCError("BAD_REQUEST", { + message: "Key name cannot exceed 100 characters", + }) + } + if (/[^\w\s\-_.]/.test(trimmedKey)) { + throw new ORPCError("BAD_REQUEST", { + message: "Key name contains invalid characters", + }) + }Alternatively, define these constraints in the input schema at lines 1001-1007.
920-952: Consider extracting encrypted key-value creation into a helper.The loop at lines 922-952 creates encrypted key-value pairs and is duplicated in
updateCredentialKeyValuePairs(lines 1119-1152). Extracting this logic into a shared helper (similar toupsertSecuritySetting) would reduce duplication and improve maintainability.Create a helper in
lib/utils/encryption-helpers.ts:export async function createEncryptedKeyValuePair( tx: PrismaTransactionClient, metadataId: string, key: string, value: string ): Promise<void> { const crypto = await import("crypto") const encryptionKey = crypto.randomBytes(32).toString("base64") const encryptionResult = await encryptData(value, encryptionKey) const encryptedDataResult = await createEncryptedData( { encryptedValue: encryptionResult.encryptedData, encryptionKey: encryptionKey, iv: encryptionResult.iv, }, tx ) if (!encryptedDataResult.success || !encryptedDataResult.encryptedData) { throw new ORPCError("INTERNAL_SERVER_ERROR", { message: "Failed to encrypt key-value pair", }) } await tx.credentialKeyValuePair.create({ data: { key, valueEncryptionId: encryptedDataResult.encryptedData.id, credentialMetadataId: metadataId, }, }) }Then replace the loops in both procedures with calls to this helper.
components/app/dashboard-add-credential-dialog.tsx (1)
696-708: KeyValuePairForm integration could be optimized, but functions correctly.The
onChangehandler (lines 698-708) immediately converts pairs viaconvertFromKeyValueManager, which creates placeholder DTOs with empty encryption fields. These placeholders are then replaced during submission (lines 196-210) with properly encrypted data.While functional, this creates unnecessary intermediate state updates. Consider simplifying by storing only the plain pairs in state and performing the conversion once during submission.
Simplify by removing the intermediate DTO conversion:
<KeyValuePairForm value={plainKeyValuePairs} - onChange={(pairs: BaseKeyValuePair[]) => { - setPlainKeyValuePairs(pairs) - metadataForm.setValue( - "keyValuePairs", - pairs.map((pair) => - CredentialKeyValuePairEntity.convertFromKeyValueManager( - pair - ) - ) - ) - }} + onChange={(pairs: BaseKeyValuePair[]) => setPlainKeyValuePairs(pairs)} label="Additional Information" placeholder={{ key: "Enter key (e.g., Security Question)", value: "Enter value (e.g., Mother's maiden name)", }} validateDuplicateKeys={true} showPasswordField={true} />The metadataForm.setValue for keyValuePairs is unnecessary here since the properly encrypted pairs are set during submission at lines 196-210.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/app/dashboard-add-credential-dialog.tsx(8 hunks)lib/permissions/utils.ts(1 hunks)orpc/routers/credential.ts(13 hunks)prisma/seed/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- prisma/seed/README.md
🧰 Additional context used
🧬 Code graph analysis (3)
orpc/routers/credential.ts (10)
middleware/permissions.ts (2)
requireDefaultContainerAccess(147-185)requirePermission(46-104)schemas/credential/dto.ts (2)
getCredentialInputSchema(14-14)updateCredentialPasswordInputSchema(40-47)entities/credential/credential/query.ts (1)
CredentialQuery(17-44)entities/credential/credential/entity.ts (1)
CredentialEntity(17-136)schemas/credential/credential-key-value.ts (1)
credentialKeyValuePairWithValueRoSchema(46-52)lib/encryption.ts (2)
decryptData(111-191)encryptData(37-108)lib/utils/tag-helpers.ts (1)
createTagsAndGetConnections(21-103)lib/utils/encryption-helpers.ts (1)
createEncryptedData(20-50)schemas/credential/credential.ts (1)
credentialFormDtoSchema(77-87)entities/credential/credential-metadata/query.ts (1)
CredentialMetadataQuery(18-49)
lib/permissions/utils.ts (1)
lib/permissions/types.ts (1)
PermissionConfig(28-43)
components/app/dashboard-add-credential-dialog.tsx (12)
hooks/use-aggressive-form-blocker.ts (1)
useAggressiveFormBlocker(8-75)hooks/use-prevent-auto-save.ts (1)
usePreventAutoSave(8-54)orpc/hooks/use-platforms.ts (1)
usePlatforms(18-28)orpc/hooks/use-credentials.ts (1)
useCreateCredentialWithMetadata(154-177)orpc/hooks/use-tags.ts (1)
useTags(16-22)entities/credential/credential-key-value/entity.ts (1)
CredentialKeyValuePairEntity(17-136)lib/utils/index.ts (3)
getPlaceholderImage(227-236)getLogoDevUrlWithToken(242-248)cn(30-32)components/ui/combobox-responsive.tsx (1)
ComboboxResponsive(43-134)components/shared/tag-selector.tsx (1)
TagSelector(35-217)components/shared/key-value-pair-form.tsx (1)
KeyValuePairForm(25-185)config/converter.tsx (1)
statusConfig(38-57)components/shared/container-selector.tsx (1)
ContainerSelector(196-350)
🔇 Additional comments (20)
lib/permissions/utils.ts (1)
114-122: LGTM: Feature.CREDENTIALS permission mapping correctly implemented.Both PRO and NORMAL users are granted WRITE access to credentials, which aligns with the fix from past reviews and allows credential creation for all authenticated users.
orpc/routers/credential.ts (14)
49-51: LGTM: Default-access procedure correctly introduced.The
authWithDefaultAccessProcedurecombines authentication with default-container access checks, properly addressing the past review concern about enforcing plan restrictions.
366-402: LGTM: createCredential properly enforces permissions and default-container restrictions.The procedure correctly:
- Uses
authWithDefaultAccessProcedureto enforce default-container access for NORMAL users- Chains
requirePermissionmiddleware for Feature.CREDENTIALS / PermissionLevel.WRITE- Validates that NORMAL users can only use default containers (lines 385-402)
This addresses all past review concerns about permission checks and default-container access.
404-458: LGTM: Transaction handling and conflict detection properly implemented.The create flow correctly:
- Wraps all operations in
database.$transaction- Passes
txtocreateTagsAndGetConnectionsandcreateEncryptedData- Handles
PrismaClientKnownRequestErrorwith code P2002 for unique constraint violations (lines 447-458)This addresses past review concerns about atomicity and duplicate handling.
78-111: LGTM: Security settings retrieval properly decrypts on server.
getCredentialSecuritySettingscorrectly fetches the credential with full include, verifies ownership, and delegates decryption toCredentialEntity.getSecuritySettings. This keeps encryption keys server-side.
113-158: LGTM: Key-value pairs endpoint correctly filters sensitive keys.
getCredentialKeyValuePairsproperly:
- Verifies credential ownership
- Filters out
passwordProtectionandaccessLoggingto prevent exposing internal security settings (lines 146-149)- Returns only safe metadata (id, key, timestamps) without values
160-219: LGTM: Decryption endpoint with proper error handling.
getCredentialKeyValuePairsWithValuescorrectly:
- Filters sensitive keys (lines 183-186)
- Decrypts each value server-side
- Gracefully handles decryption failures by returning empty strings (lines 204-214) rather than exposing raw encrypted data
221-273: LGTM: Single key-value pair retrieval with ownership verification.
getCredentialKeyValuePairValueproperly:
- Verifies credential ownership
- Validates the key-value pair exists
- Decrypts server-side
- Returns a clear error on decryption failure (lines 267-272)
275-321: LGTM: Password retrieval with proper security and audit tracking.
getCredentialPasswordcorrectly:
- Verifies credential ownership
- Decrypts password server-side
- Updates
lastViewedtimestamp for audit purposes (lines 309-312)- Handles decryption failures gracefully (lines 315-320)
462-552: LGTM: Update procedure with permission checks and conflict handling.
updateCredentialcorrectly:
- Enforces WRITE permission via middleware (lines 463-468)
- Wraps update in try-catch (lines 533-551)
- Handles unique constraint violations with CONFLICT error (lines 541-549)
This addresses past review concerns.
554-623: LGTM: Password update with transactional history tracking.
updateCredentialPasswordcorrectly:
- Enforces WRITE permission (lines 556-561)
- Wraps operations in
database.$transaction- Passes
txtocreateEncryptedData(line 591)- Creates history entry before updating credential (lines 604-611)
This addresses past review concerns about transaction handling.
625-794: LGTM: Security settings update fully transactional.
updateCredentialWithSecuritySettingsand helperupsertSecuritySettingcorrectly:
- Enforce WRITE permission (lines 627-632)
- Execute all operations within a single transaction (lines 658-720)
- Pass
txtocreateEncryptedDatain helper (line 760)- Clean up old encrypted data on updates (lines 779-783)
This addresses past review concerns about transactional encryption operations.
827-987: LGTM: createCredentialWithMetadata properly secured and transactional.The procedure correctly:
- Uses
authWithDefaultAccessProcedureandrequirePermissionmiddleware (lines 827-833)- Validates default-container restriction for NORMAL users (lines 850-866)
- Wraps all operations in transaction (lines 869-956)
- Passes
txtocreateTagsAndGetConnectionsandcreateEncryptedData(lines 874, 884, 930)- Handles unique constraint violations (lines 965-974)
This addresses all past review concerns about permissions, default-container access, and atomicity.
990-1158: LGTM: Key-value pairs update with comprehensive transaction handling.
updateCredentialKeyValuePairscorrectly:
- Enforces WRITE permission (lines 992-997)
- Wraps all operations in transaction (lines 1028-1155)
- Passes
txtocreateEncryptedDatafor all new/updated pairs (lines 1089, 1133)- Properly cleans up old encrypted data on updates (lines 1109-1112)
- Filters out sensitive security settings (lines 1044-1046)
This addresses past review concerns about transactional encryption.
797-824: LGTM: Delete procedure with permission enforcement.
deleteCredentialcorrectly enforces WRITE permission via middleware (lines 798-803) and verifies ownership before deletion.components/app/dashboard-add-credential-dialog.tsx (5)
1-90: LGTM: Clean imports and setup.The imports are well-organized, anti-autofill hooks are correctly integrated, and data fetching with
usePlatformsanduseTagsis properly initialized.
166-324: Encryption and submission flow implemented correctly.The credential submission logic properly:
- Validates required fields before encryption
- Encrypts password and key-value pairs with separate keys
- Converts encrypted generic pairs to
CredentialKeyValuePairDto(lines 202-204) ✓- Includes
keyValuePairsinmetadataDtowith array validation (lines 253-258) ✓- Handles success/error cases with appropriate user feedback
The fixes for the reported "additionalInfos not persisted" bug are present and correct.
383-402: Anti-autofill measures correctly implemented.The form properly prevents default submission (lines 383-387) and includes hidden honeypot inputs (lines 389-402) to discourage password manager interference. Combined with
useAggressiveFormBlockerandusePreventAutoSave, this provides robust protection against unwanted autofill behavior.
338-819: Well-structured responsive dialog with comprehensive features.The UI implementation is excellent:
- Clear two-column layout with main form and sidebar
- Proper accessibility with labels, tooltips, and ARIA attributes
- Responsive design with mobile-first grid layouts
- Platform selection with logo rendering
- Password strength indicator
- Advanced settings in collapsible section
- Status management with visual badge and popover
- Container selector integration
- Footer with "create more" workflow support
The component provides a polished, professional credential creation experience.
573-591: Duplicate tag prevention confirmedThe Prisma schema includes
@@unique([name, userId, containerId]), andlib/utils/tag-helpers.tsusesskipDuplicates: true, so the backend now atomically prevents duplicate tags. No changes needed in this component.
1 participant
🚀 Feature Overview
This PR introduces a complete account and credential management system with a redesigned dashboard interface, comprehensive CRUD operations, and enhanced user experience features.
✨ Key Features
📋 Account Management
🔐 Credential Management System
🎨 Enhanced UI/UX
🏗️ Technical Improvements
🗂️ Major Changes
New Pages
accounts/page.tsx- Account listing with advanced filteringaccounts/[id]/page.tsx- Individual account detail viewNew Component Architecture
Database & Schema Updates
📊 Impact
🎯 Technical Highlights
Preview: The Vercel deployment showcases the new account management interface with all features live and interactive.
Summary by CodeRabbit
New Features
Improvements
Security
UI/UX
Documentation