Conversation
📝 WalkthroughWalkthroughAdds optional numeric Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Share Dialog
participant Client as Client
participant Server as Server
participant DB as Database
User->>UI: Configure share (passcode, maxViews)
UI->>Client: (optional) generateSharePasscode()
User->>UI: Create share
UI->>Client: encrypt payload + create(maxViews,...)
Client->>Server: create(sharedSecret)
Server->>Server: validate maxViews ≤ SHARE_MAX_VIEWS_LIMIT
Server->>DB: insert sharedSecret (includes maxViews)
DB-->>Server: inserted
Server-->>Client: return shareId
Client-->>UI: display link
sequenceDiagram
participant User as User
participant UI as Share View Page
participant Client as Client
participant Server as Server
participant DB as Database
User->>UI: Enter passcode to unlock
UI->>Client: accessSecret(shareId, passcode)
Client->>Server: accessSecret request
Server->>DB: fetch sharedSecret (views, maxViews)
DB-->>Server: sharedSecret
alt maxViews defined and views >= maxViews
Server-->>Client: return exhausted state
Client-->>UI: show "Link Used Up"
else Not exhausted
Server->>DB: increment views
Server-->>Client: return encrypted payload + metadata
Client-->>UI: decrypt & display content
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/SECURITY.md (1)
127-127:⚠️ Potential issue | 🟡 MinorStale client-side validation docs — still references 6-digit numeric passcodes.
Line 127 still documents
Regex /^\d{6}$/+maxLength={6}for passcodes, which conflicts with the new 8+ character alphanumeric share passcode requirements introduced in this PR. This section should distinguish between project passcodes (if still 6-digit) and share passcodes (now 8+ alphanumeric), or be updated accordingly.src/routes/share/$shareId.tsx (1)
112-116:⚠️ Potential issue | 🟠 MajorView limit is enforced client-side only — trivially bypassable.
recordViewis called after the client has already received and decrypted the data. A caller can simply skip therecordViewcall (or the request can fail) and the view counter is never incremented, effectively bypassing the view limit. This is inherent to the zero-knowledge design (as noted in the progress log), but it may be worth documenting this trade-off in the UI or security docs — users should understand that view limits are best-effort, not cryptographically enforced.One improvement: move the view recording to a mutation that returns the encrypted data (replacing the current
getquery), so the counter increments atomically with data retrieval. This would at least prevent repeated fetches without counting.convex/sharedSecrets.ts (1)
285-307:⚠️ Potential issue | 🟠 Major
recordViewis unauthenticated — enables DoS on view-limited links.Any unauthenticated client can call
recordViewwith a known share ID to inflate the view counter and exhaust a view-limited link without knowing the passcode. This is a denial-of-service vector against the newmaxViewsfeature.Consider at minimum adding rate limiting or requiring proof of successful decryption (e.g., a HMAC of the decrypted payload). Alternatively, combining the
getandrecordViewinto a single mutation that atomically returns the encrypted data and increments the counter would prevent the current client-side-only enforcement gap noted in$shareId.tsxas well.
🤖 Fix all issues with AI agents
In `@src/lib/utils.ts`:
- Around line 100-118: The generateSharePasscode function has modulo bias and an
insecure Math.random fallback; replace the fallback with a hard failure and
implement rejection sampling: require globalThis.crypto?.getRandomValues and
throw an error if missing, allocate a Uint8Array for the target length, then
fill the passcode by repeatedly reading bytes and only accepting bytes < 256 -
(256 % chars.length) so you can map acceptedByte % chars.length without bias;
update code paths that reference buffer/length/chars in generateSharePasscode to
use this rejection loop until the result reaches the desired length.
In `@src/routes/share/`$shareId.tsx:
- Line 215: Remove the debug console.log call that prints expiresAt — delete the
line containing console.log("****** expires At", { d: data?.expiresAt }); (the
temporary debug statement referencing data?.expiresAt) so no debug logging
remains in the $shareId route/component.
- Around line 29-32: The local interface SharedVariable is a duplicate of the
exported type in src/lib/types.ts; remove this local definition and import
SharedVariable from the shared module instead (update any references in this
file to use the imported type). Specifically, delete the local "interface
SharedVariable { name: string; value: string; }" and add an import for
SharedVariable from src/lib/types (so functions/components in this file use the
shared type rather than redefining it).
🧹 Nitpick comments (5)
src/lib/constants.ts (1)
48-51:META_DATA— consider whether this belongs inconstants.ts.This object contains social/repository links rather than application configuration or validation constants. It might be a better fit alongside
SITE_CONFIG(referenced insrc/utilities/site-config), which already holds similar metadata likesiteUrlandlinks.githubSecurity. Keeping all external URLs in one place avoids fragmentation.src/routes/share/$shareId.tsx (2)
34-54: Type guard only checks 2 of 11 asserted fields.
isSharePayloadasserts fields likepasscodeSalt,iv,authTag,payloadIv,payloadAuthTag, etc., but only verifies the presence ofencryptedPayloadandencryptedShareKey. If the server response shape changes or a field is missing, accesses at lines 84–108 would silently fail at runtime. Consider checking all fields that the decryption flow depends on.
305-315:maxViewsdisplay is hidden whenmaxViewsis falsy (including0).Line 311 uses
data?.maxViews && (...)which is falsy formaxViews === 0. While0shouldn't be a valid value based on backend validation (maxViews >= 1), using a stricter check is safer.Suggested tweak
- {data?.maxViews && ( + {data?.maxViews != null && (src/routes/d/shared.tsx (2)
261-266: Missing error handling for clipboard write.
navigator.clipboard.writeTextcan throw (e.g., when the page lacks focus or the Permissions API denies access). Other similar handlers in the file also lack this, but since this is new code, consider wrapping it.Proposed fix
async function handleCopyPasscode(id: string, passcode: string) { - await navigator.clipboard.writeText(passcode); - setCopiedPasscodeId(id); - setTimeout(() => setCopiedPasscodeId(null), 2000); - toast.success("Passcode copied"); + try { + await navigator.clipboard.writeText(passcode); + setCopiedPasscodeId(id); + setTimeout(() => setCopiedPasscodeId(null), 2000); + toast.success("Passcode copied"); + } catch { + toast.error("Failed to copy passcode"); + } }
690-708: "Set view limit" and "Remove view limit" dropdown items are clean.State initialization before opening the dialog and conditional rendering of "Remove" only when
maxViewsis set are both correct.Note:
share.maxViewson line 701 uses a truthy check, which would also hide the option ifmaxViewswere0. Backend validatesmaxViews >= 1, so this is safe in practice, butshare.maxViews != nullwould be more semantically precise.
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)
convex/sharedSecrets.ts (1)
276-316:⚠️ Potential issue | 🔴 CriticalView is consumed before passcode verification — allows unauthenticated view-limit exhaustion.
accessSecretincrementsviewsand returns the encrypted payload in a single unauthenticated call. Because passcode verification happens client-side after this mutation, any caller who knows (or brute-forces) the share ID can repeatedly invokeaccessSecretto exhaustmaxViewswithout ever supplying the passcode. For one-time links (maxViews === 1) a single spurious call permanently locks out the legitimate recipient.Consider one of:
- Server-side passcode verification — accept the passcode (or its hash) as an argument, verify it before incrementing
viewsand releasing the payload.- Split into two steps — keep the current unauthenticated metadata query (
get) and only release the payload + increment the view in a second mutation that requires passcode proof.Option 1 is the stronger fix and aligns with the PR goal of hardened security.
🤖 Fix all issues with AI agents
In `@src/routes/share/`$shareId.tsx:
- Around line 76-93: The current code calls accessSecret (which increments
views) before verifying the passcode, causing views to be consumed on wrong
passcodes; change the flow so the client performs a local "trial decrypt" of the
encrypted payload with the user-provided passcode and only calls accessSecret if
that trial decrypt succeeds. Specifically, locate the passcode submission
handler where accessSecret is awaited (and where isSharePayload and
setUnlockError are used), add a pre-check that attempts to decrypt the encrypted
data (using the same client-side decryption routine used after successful
access) and if decryption fails setUnlockError("Incorrect passcode.") and return
without calling accessSecret; only call accessSecret when the trial decrypt
passes so server-side views aren’t consumed by obvious wrong passcodes.
- Around line 309-323: The UI reads metadata from sharedSecret reactively and
after calling accessSecret the query may replace sharedSecret with an exhausted
shape lacking fields like isIndefinite, causing the expiry text to render
incorrectly; fix this by copying the metadata you need (e.g., isIndefinite,
expiresAt, maxViews) into a local ref or component state before invoking
accessSecret (or before the decryption/get query updates) and use those
preserved values when rendering the expiry text (the code paths around
sharedSecret, accessSecret, and formatRelativeTime should read from the saved
ref/state instead of directly from the reactive sharedSecret).
🧹 Nitpick comments (3)
convex/sharedSecrets.ts (1)
571-606:maxViews === 0accepted via the-1sentinel bypass.
args.maxViews === 0falls through to the< 1check and is correctly rejected. Good. However, consider the edge case at Line 596: settingmaxViewsequal to currentviewsimmediately exhausts the link. If that's intentional (as an alternative to disabling), it's fine — but a brief doc comment would prevent future confusion.Also, the comment says "Pass maxViews = -1 to remove limit," but
v.number()admits any float. Values like-0.5orNaN(from JSON) would bypass the-1check and then failNumber.isInteger. This is harmless (it throws), but explicitly documenting that only-1removes the limit and all other non-positive values are errors would strengthen the contract.Proposed tightening
handler: async (ctx, args) => { const { sharedSecret } = await checkSecretManagementAccess(ctx, args.id); - if (args.maxViews === -1) { + // Sentinel: -1 means "remove the view limit" + if (args.maxViews === -1) { await ctx.db.patch(args.id, { maxViews: undefined, }); return { success: true }; } if (!Number.isInteger(args.maxViews) || args.maxViews < 1) { throwError("maxViews must be a positive integer", "BAD_REQUEST", 400); } if (args.maxViews > SHARE_MAX_VIEWS_LIMIT) { throwError(`maxViews cannot exceed ${SHARE_MAX_VIEWS_LIMIT}`, "BAD_REQUEST", 400); } - if (args.maxViews < sharedSecret.views) { + if (args.maxViews < sharedSecret.views) { + // Note: maxViews === views is allowed and immediately exhausts the link. throwError("maxViews cannot be lower than current views", "BAD_REQUEST", 400); }src/routes/share/$shareId.tsx (2)
30-48: Type guard only checks two fields but asserts a much richer shape.
isSharePayloadnarrows to a type with 10 properties but only verifies the presence ofencryptedPayloadandencryptedShareKey. If the server ever returns a partial object containing those two keys (e.g., a future code path or schema change), the downstreamdecryptcalls will throw on missingpasscodeSalt,iv,authTag, etc.Consider checking all required fields:
Proposed fix
function isSharePayload(sharedSecret: unknown): sharedSecret is { encryptedPayload: string; encryptedShareKey: string; passcodeSalt: string; iv: string; authTag: string; payloadIv: string; payloadAuthTag: string; isIndefinite: boolean; expiresAt?: number; maxViews?: number; } { return Boolean( sharedSecret && typeof sharedSecret === "object" && "encryptedPayload" in sharedSecret && - "encryptedShareKey" in sharedSecret + "encryptedShareKey" in sharedSecret && + "passcodeSalt" in sharedSecret && + "iv" in sharedSecret && + "authTag" in sharedSecret && + "payloadIv" in sharedSecret && + "payloadAuthTag" in sharedSecret ); }
65-93: Missing"exhausted"guard at the top ofhandleUnlock.Line 66 checks for
expiredanddisabledbut notexhausted. If the reactivegetquery updates to an exhausted state between render and the user clicking "Unlock",handleUnlockproceeds to callaccessSecret— which harmlessly returns{ exhausted: true }and is caught on Line 87. So this isn't a bug, but adding"exhausted"to the early-return guard is more defensive and avoids the unnecessary mutation call.Proposed fix
- if (!sharedSecret || "expired" in sharedSecret || "disabled" in sharedSecret) return; + if ( + !sharedSecret || + "expired" in sharedSecret || + "disabled" in sharedSecret || + "exhausted" in sharedSecret + ) return;
| try { | ||
| const secret = await accessSecret({ id: shareId as Id<"sharedSecrets"> }); | ||
| if (!isSharePayload(secret)) { | ||
| if ("expired" in secret) { | ||
| setUnlockError("This link has expired."); | ||
| return; | ||
| } | ||
| if ("disabled" in secret) { | ||
| setUnlockError("This link has been disabled."); | ||
| return; | ||
| } | ||
| if ("exhausted" in secret) { | ||
| setUnlockError("This link has reached its view limit."); | ||
| return; | ||
| } | ||
| setUnlockError("Unable to access this link."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
A view is consumed even when the passcode is wrong.
accessSecret atomically increments views before the client attempts decryption. If the user mistypes the passcode, the view is already spent. For maxViews === 1 links this is especially punishing — a single typo permanently locks out the recipient.
This is the client-side manifestation of the server-side issue noted in convex/sharedSecrets.ts (the accessSecret mutation). The root fix belongs server-side (verify passcode before counting a view), but on the client side you could mitigate by running a local "trial decrypt" of the encrypted passcode before calling accessSecret, so obviously wrong passcodes never reach the mutation.
🤖 Prompt for AI Agents
In `@src/routes/share/`$shareId.tsx around lines 76 - 93, The current code calls
accessSecret (which increments views) before verifying the passcode, causing
views to be consumed on wrong passcodes; change the flow so the client performs
a local "trial decrypt" of the encrypted payload with the user-provided passcode
and only calls accessSecret if that trial decrypt succeeds. Specifically, locate
the passcode submission handler where accessSecret is awaited (and where
isSharePayload and setUnlockError are used), add a pre-check that attempts to
decrypt the encrypted data (using the same client-side decryption routine used
after successful access) and if decryption fails setUnlockError("Incorrect
passcode.") and return without calling accessSecret; only call accessSecret when
the trial decrypt passes so server-side views aren’t consumed by obvious wrong
passcodes.
There was a problem hiding this comment.
@coderabbitai but we are only calling this mutation once the code verifies on the client side
There was a problem hiding this comment.
@pantharshit007, I understand the confusion. You're right that there's client-side validation with getSharePasscodeError(passcode) before calling accessSecret — however, that validation only checks the format of the passcode (minimum/maximum length, alphanumeric pattern), not whether it's the correct passcode.
Here's the current flow in handleUnlock():
- ✅ Format validation:
getSharePasscodeError(passcode)checks if the passcode meets length/regex requirements - ✅ View consumed:
accessSecret()is called and increments theviewscount - ❌ Correctness verification: Decryption with
deriveKey()anddecrypt()— this is where we discover if the passcode is actually correct - If decryption fails, the catch block sets
"Invalid passcode"error
The problem is that step 2 happens before step 3. So if a user enters a well-formed but incorrect passcode (e.g., "WrongPass" instead of "CorrectPass"), the view count is already incremented before the decryption fails.
For a one-time link (maxViews === 1), this means one wrong guess locks out the legitimate recipient permanently.
The fix would be to verify the passcode is actually correct (by attempting decryption) before calling accessSecret. Would you like me to plan that change?
🧠 Learnings used
Learnt from: CR
Repo: pantharshit007/tijori PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T14:33:50.174Z
Learning: Applies to convex/**/*.ts : Never store plaintext secrets - only store encrypted values with IV and authTag
…ion and removing passcode hashes from client-side exposure
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| args: { | ||
| projectId: v.id("projects"), | ||
| passcode: v.string(), | ||
| }, | ||
| handler: async (ctx, args) => { | ||
| const userId = await getUserId(ctx); | ||
|
|
||
| const membership = await ctx.db | ||
| .query("projectMembers") | ||
| .withIndex("by_project_user", (q) => | ||
| q.eq("projectId", args.projectId).eq("userId", userId) | ||
| ) | ||
| .unique(); | ||
|
|
||
| if (!membership) { | ||
| throwError("Access denied: Not a member of this project", "FORBIDDEN", 403, { | ||
| user_id: userId, | ||
| project_id: args.projectId, | ||
| }); | ||
| } | ||
|
|
||
| const project = await ctx.db.get(args.projectId); | ||
| if (!project) { | ||
| throwError("Project not found", "NOT_FOUND", 404, { | ||
| user_id: userId, | ||
| project_id: args.projectId, | ||
| }); | ||
| } | ||
|
|
||
| const enteredHash = await hashPasscode(args.passcode, project.passcodeSalt); | ||
| const ok = enteredHash === project.passcodeHash; | ||
| return { ok }; |
There was a problem hiding this comment.
🔴 verifyPasscode sends plaintext passcode over the network, contradicting zero-knowledge claims
The verifyPasscode mutation accepts the raw plaintext passcode as an argument (passcode: v.string()) and hashes it server-side. This means the plaintext project passcode is transmitted to the Convex server on every unlock attempt.
Security Architecture Concern
At convex/projects.ts:846-848, the mutation accepts passcode: v.string() and at line 875 hashes it server-side: const enteredHash = await hashPasscode(args.passcode, project.passcodeSalt). The passcode is sent in plaintext over the WebSocket connection to Convex.
Previously (before this PR), passcode verification was done entirely client-side: the client hashed the passcode locally and compared against the stored hash. The old approach at src/routes/d/project/$projectId.tsx (LEFT side, lines 509-511) was:
const enteredHash = await cryptoHash(passcode, project.passcodeSalt);
if (enteredHash !== project.passcodeHash) { ... }
The new approach sends the plaintext passcode to the server, which contradicts the project's stated "zero-knowledge architecture" (from SECURITY.md). While the docs have been updated to acknowledge this ("Project passcodes are verified server-side (the passcode is sent for verification but never stored)"), this is a regression in the security model. The Convex server (and its logs, if any request logging is enabled) can now see plaintext project passcodes.
The passcode is also the input to PBKDF2 key derivation for encrypting all environment variables. If the server is compromised or logs are leaked, the attacker could derive the project encryption key.
Impact: Reduces the security guarantee from zero-knowledge to trust-the-server for project passcodes. This is a deliberate design choice acknowledged in docs, but worth flagging as it weakens the security model.
Prompt for agents
Consider reverting to client-side passcode verification to maintain the zero-knowledge architecture. The previous approach was: the server returns the passcodeHash to the client, the client hashes the entered passcode locally, and compares. The PR removed passcodeHash from client responses to prevent exposure, but the hash is already a one-way function — exposing the hash is far less risky than exposing the plaintext passcode.
Alternative: If server-side verification is desired, have the client hash the passcode first and send the hash to the server for comparison, rather than sending the plaintext. This preserves zero-knowledge while still moving verification to the server.
Was this helpful? React with 👍 or 👎 to provide feedback.
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)
docs/SECURITY.md (1)
3-3:⚠️ Potential issue | 🟡 MinorStale "Last Updated" date.
The document is being updated in this PR (February 2026), but the header still reads "Last Updated: January 26, 2026."
Proposed fix
-> Last Updated: January 26, 2026 +> Last Updated: February 7, 2026
🤖 Fix all issues with AI agents
In `@convex/projects.ts`:
- Around line 40-45: hashPasscode currently does a single SHA-256 which is too
fast for 6-digit passcodes; replace the one-shot digest with a slower KDF
(PBKDF2) or at minimum an HMAC-SHA256 to add computational cost. Update
hashPasscode to import the passcode as a raw key and call
crypto.subtle.deriveBits with PBKDF2 using a configurable iteration count (e.g.
CRYPTO.PBKDF2_ITERATIONS) and CRYPTO.HASH_ALGORITHM, then convert the derived
ArrayBuffer to base64 via arrayBufferToBase64; if you choose HMAC instead, use
crypto.subtle.importKey with "HMAC" and crypto.subtle.sign to compute
HMAC-SHA256(salt, passcode). Also ensure you add or reference constants
(CRYPTO.PBKDF2_ITERATIONS, CRYPTO.PBKDF2_DERIVED_KEY_LENGTH) and consider
compatibility with client-side verification (either update client to match the
stronger scheme or document the deliberate trade-off).
- Around line 845-878: Add per-user/per-project rate-limiting to verifyPasscode:
before hashing, look up a failure tracker (e.g., add fields to projectMembers
like failedAttempts:number and lockoutUntil:timestamp or create a
passcodeAttempts table keyed by projectId+userId) and if lockoutUntil > now
return { ok: false } or throw with FORBIDDEN; after each failed check increment
failedAttempts and set lockoutUntil to now+15min when failedAttempts >= 5, and
apply exponential backoff by awaiting a delay proportional to
2^(failedAttempts-1) seconds before returning; on successful verification reset
failedAttempts and clear lockoutUntil. Update logic around verifyPasscode,
projectMembers, hashPasscode and getUserId to read/update the tracker atomically
(use ctx.db.update/withIndex/unique patterns already used) so concurrent
attempts are handled correctly.
🧹 Nitpick comments (5)
docs/learning.md (1)
837-851: Potentially confusing juxtaposition of "no server-side verification" vs "server-side verification".Lines 837–838 state shared links were hardened "without introducing server-side passcode verification," while lines 849–850 state "Project passcodes are now verified on the backend." Although these refer to different passcode types (share vs. project), a reader skimming this section could easily misread the intent. Consider adding a brief qualifier, e.g., "without introducing server-side share passcode verification" to disambiguate.
src/lib/key-store.ts (1)
15-23: Expired-key eviction doesn't notify listeners.When
getKeylazily evicts an expired entry (Line 19), it doesn't callthis.notify(). Subscribers (e.g.,keyStoreVersioninshared.tsx) won't be informed that a key has disappeared, so UI elements gated onkeyStore.getKey(...)won't reactively update until the next unrelated re-render.Proposed fix
getKey(projectId: string): CryptoKey | null { const entry = this.keys.get(projectId); if (!entry) return null; if (Date.now() > entry.expiresAt) { this.keys.delete(projectId); + this.notify(); return null; } return entry.key; }convex/projects.ts (1)
233-235: Inconsistent sanitization approach and imprecise type cast.The
getquery usesObject.fromEntries(Object.entries(...).filter(...))withas typeof project, whilelistandlistOwneduse destructuring (const { passcodeHash: _, ...safeProject } = project). The destructuring approach is both more readable and more type-safe. Additionally, theas typeof projectcast is inaccurate — the resulting object no longer containspasscodeHash, but TypeScript will believe it does.Proposed fix: use destructuring consistently
- return { - ...(Object.fromEntries( - Object.entries(project).filter(([key]) => key !== "passcodeHash") - ) as typeof project), - role: membership.role, - ownerTier: owner?.tier ?? "free", - }; + const { passcodeHash: _passcodeHash, ...safeProject } = project; + return { + ...safeProject, + role: membership.role, + ownerTier: owner?.tier ?? "free", + };src/routes/d/shared.tsx (2)
110-118: State declarations scattered between hooks.The view-limit state variables (Lines 115–118) are declared after the
useEffectsubscription hook, breaking the grouping with the other state declarations (Lines 101–108). Move them up to sit with the rest of the state for readability.Suggested grouping
const [searchQuery, setSearchQuery] = useState(""); + const [keyStoreVersion, setKeyStoreVersion] = useState(0); const [projectFilter, setProjectFilter] = useState("all"); const [statusFilter, setStatusFilter] = useState("all"); const [revealedPasscodes, setRevealedPasscodes] = useState<Set<string>>(new Set()); const [decryptedPasscodes, setDecryptedPasscodes] = useState<Record<string, string>>({}); const [copiedPasscodeId, setCopiedPasscodeId] = useState<string | null>(null); const [selectedIds, setSelectedIds] = useState<Set<string>>(new Set()); - const [keyStoreVersion, setKeyStoreVersion] = useState(0); + const [viewLimitTarget, setViewLimitTarget] = useState<SharedSecret | null>(null); + const [viewLimitValue, setViewLimitValue] = useState(""); + const [viewLimitError, setViewLimitError] = useState<string | null>(null); + const [isViewLimitOpen, setIsViewLimitOpen] = useState(false); useEffect(() => { return keyStore.subscribe(() => { setKeyStoreVersion((prev) => prev + 1); }); }, []); - const [viewLimitTarget, setViewLimitTarget] = useState<SharedSecret | null>(null); - const [viewLimitValue, setViewLimitValue] = useState(""); - const [viewLimitError, setViewLimitError] = useState<string | null>(null); - const [isViewLimitOpen, setIsViewLimitOpen] = useState(false);
169-169:void keyStoreVersionis a workaround to force re-renders — consider a brief comment.The
void keyStoreVersion;silences the unused-variable lint, but the actual purpose (triggering re-renders sokeyStore.getKey()calls in JSX reflect current state) isn't obvious. A one-line comment would help future readers understand why this variable exists.Suggested comment
- void keyStoreVersion; + // Read keyStoreVersion so React re-renders when keys change (used by inline keyStore.getKey() calls) + void keyStoreVersion;
| async function hashPasscode(passcode: string, salt: string): Promise<string> { | ||
| const encoder = new TextEncoder(); | ||
| const combined = encoder.encode(salt + passcode); | ||
| const digest = await crypto.subtle.digest(CRYPTO.HASH_ALGORITHM, combined); | ||
| return arrayBufferToBase64(digest); | ||
| } |
There was a problem hiding this comment.
hashPasscode uses a single SHA-256 round — fast to brute-force for 6-digit codes.
This function hashes with one pass of SHA-256, which is extremely fast on modern hardware (~billions/sec on GPU). For a 6-digit numeric passcode, the entire keyspace (10⁶) can be exhausted in milliseconds. Since PBKDF2 is already used elsewhere in the codebase for key derivation, consider using it here too (or at minimum HMAC-SHA256 instead of plain SHA-256(salt ‖ passcode)) to add computational cost.
That said, this hash is also stored and used for client-side verification elsewhere, so changing it here alone would create a mismatch. If this is a deliberate trade-off (matching the existing client-side hash scheme), document it and consider strengthening the overall passcode hashing scheme in a follow-up.
🤖 Prompt for AI Agents
In `@convex/projects.ts` around lines 40 - 45, hashPasscode currently does a
single SHA-256 which is too fast for 6-digit passcodes; replace the one-shot
digest with a slower KDF (PBKDF2) or at minimum an HMAC-SHA256 to add
computational cost. Update hashPasscode to import the passcode as a raw key and
call crypto.subtle.deriveBits with PBKDF2 using a configurable iteration count
(e.g. CRYPTO.PBKDF2_ITERATIONS) and CRYPTO.HASH_ALGORITHM, then convert the
derived ArrayBuffer to base64 via arrayBufferToBase64; if you choose HMAC
instead, use crypto.subtle.importKey with "HMAC" and crypto.subtle.sign to
compute HMAC-SHA256(salt, passcode). Also ensure you add or reference constants
(CRYPTO.PBKDF2_ITERATIONS, CRYPTO.PBKDF2_DERIVED_KEY_LENGTH) and consider
compatibility with client-side verification (either update client to match the
stronger scheme or document the deliberate trade-off).
There was a problem hiding this comment.
ignoring sicne its now server side
There was a problem hiding this comment.
@pantharshit007, I understand your decision. Just to clarify: moving verification server-side does prevent clients from seeing the hash (which is great!), but it doesn't strengthen the hash itself against offline attacks if the database is compromised — single SHA-256 remains fast to brute-force in that scenario.
That said, the switch from 6-digit numeric to 8+ character alphanumeric passcodes is a significant security improvement (keyspace increases from ~1 million to ~218 trillion combinations), which does make brute-force attacks far less practical. That change addresses the core weakness much more effectively than the hashing algorithm alone would.
Happy to move forward with the current approach!
🧠 Learnings used
Learnt from: CR
Repo: pantharshit007/tijori PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T14:33:50.174Z
Learning: Applies to convex/**/*.ts : Always authenticate by checking `ctx.auth.getUserIdentity()` in Convex mutations and queries
Learnt from: CR
Repo: pantharshit007/tijori PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T14:33:50.174Z
Learning: Applies to convex/**/*.ts : Never log or expose decrypted secrets in Convex functions
Learnt from: CR
Repo: pantharshit007/tijori PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T14:33:50.174Z
Learning: Applies to convex/**/*.ts : Never store plaintext secrets - only store encrypted values with IV and authTag
Learnt from: CR
Repo: pantharshit007/tijori PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T14:33:50.174Z
Learning: Applies to convex/**/*.ts : Use compound indexes for permission checks like `by_project_user` in Convex queries
| export const verifyPasscode = mutation({ | ||
| args: { | ||
| projectId: v.id("projects"), | ||
| passcode: v.string(), | ||
| }, | ||
| handler: async (ctx, args) => { | ||
| const userId = await getUserId(ctx); | ||
|
|
||
| const membership = await ctx.db | ||
| .query("projectMembers") | ||
| .withIndex("by_project_user", (q) => | ||
| q.eq("projectId", args.projectId).eq("userId", userId) | ||
| ) | ||
| .unique(); | ||
|
|
||
| if (!membership) { | ||
| throwError("Access denied: Not a member of this project", "FORBIDDEN", 403, { | ||
| user_id: userId, | ||
| project_id: args.projectId, | ||
| }); | ||
| } | ||
|
|
||
| const project = await ctx.db.get(args.projectId); | ||
| if (!project) { | ||
| throwError("Project not found", "NOT_FOUND", 404, { | ||
| user_id: userId, | ||
| project_id: args.projectId, | ||
| }); | ||
| } | ||
|
|
||
| const enteredHash = await hashPasscode(args.passcode, project.passcodeSalt); | ||
| const ok = enteredHash === project.passcodeHash; | ||
| return { ok }; | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for rate-limiting, attempt tracking, and lockout mechanisms
rg -n "rateLimi|rateLimit|rate_limit|throttl|cooldown|lockout|attempt" --type=ts -g '!node_modules' | head -50Repository: pantharshit007/tijori
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check if verifyPasscode or related authentication functions have any delays/backoff
rg -A 10 "verifyPasscode\|checkAttempt\|trackAttempt" convex/ --type=ts | head -100Repository: pantharshit007/tijori
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Verify passcode format constraint
rg -B 5 -A 5 "passcode.*6.*digit\|6.*digit.*passcode\|maxLength.*6\|length.*6" --type=ts | head -100Repository: pantharshit007/tijori
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the verifyPasscode mutation details and surrounding code
cat -n convex/projects.ts | sed -n '840,880p'Repository: pantharshit007/tijori
Length of output: 1396
🏁 Script executed:
#!/bin/bash
# Look for passcode input field definition in client code
fd -e tsx -e ts | xargs rg -l "passcode" | grep -v node_modules | head -10Repository: pantharshit007/tijori
Length of output: 321
🏁 Script executed:
#!/bin/bash
# Check the projects schema definition
rg -A 20 "passcodeHash\|passcodeSalt" convex/schema.ts --type=tsRepository: pantharshit007/tijori
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Find the schema file
fd schema --type=ts convex/Repository: pantharshit007/tijori
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Check project settings page for passcode input constraints
cat -n src/routes/d/project/\$projectId.tsx | grep -B 10 -A 10 "passcode"Repository: pantharshit007/tijori
Length of output: 4759
🏁 Script executed:
#!/bin/bash
# Check the security documentation
cat -n src/routes/docs/security.tsx | head -150Repository: pantharshit007/tijori
Length of output: 8183
Add rate-limiting to verifyPasscode to prevent brute-force attacks.
The 6-digit passcode space (10⁶ combinations) can be exhaustively searched by an authenticated project member calling this mutation in a loop. Implement per-project/per-user attempt tracking with lockout:
- Track failed verification attempts (e.g., 5 failures → 15-minute cooldown)
- Add exponential backoff or delay on consecutive failures
Though the attacker must be an authenticated member, this is a security-critical app where insider threat mitigation is important.
🤖 Prompt for AI Agents
In `@convex/projects.ts` around lines 845 - 878, Add per-user/per-project
rate-limiting to verifyPasscode: before hashing, look up a failure tracker
(e.g., add fields to projectMembers like failedAttempts:number and
lockoutUntil:timestamp or create a passcodeAttempts table keyed by
projectId+userId) and if lockoutUntil > now return { ok: false } or throw with
FORBIDDEN; after each failed check increment failedAttempts and set lockoutUntil
to now+15min when failedAttempts >= 5, and apply exponential backoff by awaiting
a delay proportional to 2^(failedAttempts-1) seconds before returning; on
successful verification reset failedAttempts and clear lockoutUntil. Update
logic around verifyPasscode, projectMembers, hashPasscode and getUserId to
read/update the tracker atomically (use ctx.db.update/withIndex/unique patterns
already used) so concurrent attempts are handled correctly.
…shboard to use useSyncExternalStore for key store updates
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/lib/key-store.ts`:
- Around line 15-23: The getKey method currently deletes expired entries but
doesn’t inform subscribers, causing stale snapshots; modify getKey in the
key-store class so that when it detects an expired entry (the branch that calls
this.keys.delete(projectId)), it also calls this.notify() before returning null
so subscribers using getSnapshot() via useSyncExternalStore see the updated
keys.size and re-render; ensure notify() is only invoked when an actual deletion
occurs to avoid spurious notifications.
- Around line 42-44: getSnapshot() currently returns this.keys.size which
doesn't change when an existing projectId's key is replaced, so React's
useSyncExternalStore may not re-render; change snapshot to use a monotonic
version counter updated in notify() (e.g., add a private version number field,
increment it inside notify(), and have getSnapshot() return that version) and
ensure setKey/setKeys call notify() after mutating this.keys so subscribers see
the updated snapshot; reference getSnapshot, notify, setKey, setKeys, and
this.keys when making the changes.
In `@src/routes/d/shared.tsx`:
- Around line 267-272: The navigator.clipboard.writeText call in
handleCopyPasscode is unguarded and can reject; wrap the clipboard call in a
try/catch and guard availability (check navigator.clipboard && typeof
navigator.clipboard.writeText === "function") before calling it, only call
setCopiedPasscodeId and toast.success on successful write, and call toast.error
(or an appropriate fallback notification) inside the catch to surface permission
or availability failures; mirror the same defensive pattern used for
handleCopyLink so failures don't produce unhandled promise rejections.
- Around line 300-303: The parsing logic using Number.parseInt allows trailing
non-numeric characters (e.g., "10abc") to succeed; update the validation around
the variables parsed and trimmed in the view limit flow to only accept purely
numeric input: first ensure trimmed matches a stricter numeric pattern (e.g.,
/^\d+$/) or confirm that parsing round-trips (the parsed integer string equals
trimmed after normalizing), then convert to a number and check parsed is >= 1;
on failure call setViewLimitError("Max views must be a positive number") and
return.
🧹 Nitpick comments (5)
src/lib/key-store.ts (1)
35-40: Add explicit return types on the new public methods.The coding guidelines prefer explicit return types for exported functions. Since
keyStoreis exported and these methods are part of its public API (consumed byuseSyncExternalStore), annotating them improves clarity and catches accidental signature changes.Proposed fix
- subscribe(listener: () => void) { + subscribe(listener: () => void): () => void { this.listeners.add(listener); return () => { this.listeners.delete(listener); }; } - getSnapshot() { + getSnapshot(): number { return this.keys.size; }As per coding guidelines: "Prefer explicit return types for exported functions".
src/routes/d/shared.tsx (4)
109-112: Subscribe callback recreated every render causes unnecessary resubscriptions.
useSyncExternalStorewill re-subscribe whenever thesubscribeargument changes identity. The inline arrow(listener) => keyStore.subscribe(listener)is a new function each render, so React unsubscribes and resubscribes on every render cycle.Stabilize by binding the method once outside the component or using
useCallback:Proposed fix (bind outside component)
import { keyStore } from "@/lib/key-store"; + +const subscribeKeyStore = (listener: () => void) => keyStore.subscribe(listener); +const getKeyStoreSnapshot = () => keyStore.getSnapshot();Then inside the component:
- const keyStoreVersion = useSyncExternalStore( - (listener) => keyStore.subscribe(listener), - () => keyStore.getSnapshot() - ); + const keyStoreVersion = useSyncExternalStore(subscribeKeyStore, getKeyStoreSnapshot);
167-167:void keyStoreVersion— consider a brief comment for maintainability.This suppresses the "unused variable" warning, but the intent (force re-render on key-store changes) isn't immediately obvious. A one-line comment would help future readers:
+ // Reading keyStoreVersion forces re-render when key store changes void keyStoreVersion;
320-323: Prefercatch (err: unknown)overcatch (err: any).Strict TypeScript mode discourages
any. Usingunknownensures you go throughgetErrorMessage(which already handlesunknown) without losing type safety. This applies to both new catch blocks.Proposed fix
- } catch (err: any) { + } catch (err: unknown) { console.error("Failed to update view limit:", err); toast.error(getErrorMessage(err, "Failed to update view limit")); }- } catch (err: any) { + } catch (err: unknown) { console.error("Failed to remove view limit:", err); toast.error(getErrorMessage(err, "Failed to remove view limit")); }As per coding guidelines: "Strict TypeScript mode enabled - No implicit any, strict null checks".
Also applies to: 330-333
583-612: Buttontitleand tooltip content are duplicated.The
titleattribute (lines 588–594) and the<TooltipContent>(lines 605–610) render the exact same text. The tooltip already provides the accessible hover hint; thetitleattr will produce a double-tooltip in many browsers (native + custom). Consider removing one — typically the nativetitleif you rely on the custom<Tooltip>component for consistent styling.
| getSnapshot() { | ||
| return this.keys.size; | ||
| } |
There was a problem hiding this comment.
getSnapshot() based on keys.size won't detect key replacements.
If setKey is called for a projectId that already exists (e.g., re-deriving a key after re-entering a password), the map size stays the same. useSyncExternalStore compares snapshots with Object.is, so React will skip the re-render even though notify() fires — the snapshot value is identical.
A simple monotonic counter bumped in notify() is the standard pattern for external-store snapshots:
Proposed fix
class ProjectKeyStore {
private keys: Map<string, { key: CryptoKey; expiresAt: number }> = new Map();
private readonly ttlMs = 60 * 60 * 1000;
private listeners: Set<() => void> = new Set();
+ private version = 0;
// ...
getSnapshot(): number {
- return this.keys.size;
+ return this.version;
}
private notify(): void {
+ this.version++;
for (const listener of this.listeners) {
listener();
}
}
}🤖 Prompt for AI Agents
In `@src/lib/key-store.ts` around lines 42 - 44, getSnapshot() currently returns
this.keys.size which doesn't change when an existing projectId's key is
replaced, so React's useSyncExternalStore may not re-render; change snapshot to
use a monotonic version counter updated in notify() (e.g., add a private version
number field, increment it inside notify(), and have getSnapshot() return that
version) and ensure setKey/setKeys call notify() after mutating this.keys so
subscribers see the updated snapshot; reference getSnapshot, notify, setKey,
setKeys, and this.keys when making the changes.
There was a problem hiding this comment.
@coderabbitai but there won't be any situation where the user re-enters the password, since once unlocked there's only the option of locking it again.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all usages of setKey to understand the call patterns
rg -n 'keyStore\.setKey' --type ts --type tsx -A 3 -B 3
# Also search for the unlock and lock patterns
rg -n '(unlock|lock).*project' --type ts --type tsx -C 2 -g '!**/*.test.*' | head -100
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Search for all usages of setKey to understand the call patterns
rg -n 'keyStore\.setKey' -g '*.ts' -g '*.tsx' -A 3 -B 3
# Also search for the unlock and lock patterns in the project routes
rg -n 'setKey|removeKey' -g '*.ts' -g '*.tsx' -C 3
# Look specifically at the project unlock flow
rg -n 'const handleUnlock' -g '*.tsx' -A 20
Length of output: 2056
🏁 Script executed:
#!/bin/bash
# Look for the complete unlock flow
rg -n 'handleUnlock' -g '*.tsx' -A 30
# Also check if derivedKey is set multiple times
rg -n 'setDerivedKey' -g '*.tsx' -C 3
# Check the SharedDashboard component that uses useSyncExternalStore
rg -n 'useSyncExternalStore' -g '*.tsx' -C 5
Length of output: 14030
@pantharshit007 You're absolutely right about the single-project flow! Once a project is unlocked, the only option is to lock it—you won't unlock it again without locking first.
However, the issue is more subtle. The getSnapshot() method serves all subscribers across the entire app, not just one project. Looking at line 109-112 in src/routes/d/shared.tsx, the SharedDashboard uses useSyncExternalStore to track which project keys are available (so it knows which passcodes can be decrypted).
Consider this scenario with multiple projects:
- User unlocks Project A → map size goes from 0 to 1
- User unlocks Project B → map size goes from 1 to 2
- User locks Project A → map size goes from 2 to 1
- User unlocks Project A again → map size stays at 1 (no change!)
Even though the set of available keys changed in step 4, the size-based snapshot returns the same value, so React won't re-render the SharedDashboard. The UI would show stale information about which passcodes can be decrypted.
A version counter handles this cleanly—every mutation (add/remove/replace) increments the version, guaranteeing useSyncExternalStore detects the change. It's also the standard pattern for external stores in React's documentation.
Would you like to keep the size-based approach since it works for your current single-project unlock flow, or adopt the version counter for robustness across the multi-project scenario?
🧠 Learnings used
Learnt from: CR
Repo: pantharshit007/tijori PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T14:33:50.174Z
Learning: Applies to src/**/*.{ts,tsx} : Clear encryption keys from memory on logout/navigation
| const parsed = Number.parseInt(trimmed, 10); | ||
| if (!Number.isFinite(parsed) || parsed < 1) { | ||
| setViewLimitError("Max views must be a positive number"); | ||
| return; |
There was a problem hiding this comment.
Number.parseInt silently accepts trailing non-numeric characters.
Number.parseInt("10abc", 10) returns 10 and passes the Number.isFinite check, so a user typing "10abc" would successfully set a view limit of 10. While the <input type="number"> mitigates this in browsers (it won't allow non-numeric input in most UAs), programmatic or paste scenarios could still hit this path.
A stricter parse avoids surprises:
Proposed fix
- const parsed = Number.parseInt(trimmed, 10);
- if (!Number.isFinite(parsed) || parsed < 1) {
+ const parsed = Number(trimmed);
+ if (!Number.isInteger(parsed) || parsed < 1) {🤖 Prompt for AI Agents
In `@src/routes/d/shared.tsx` around lines 300 - 303, The parsing logic using
Number.parseInt allows trailing non-numeric characters (e.g., "10abc") to
succeed; update the validation around the variables parsed and trimmed in the
view limit flow to only accept purely numeric input: first ensure trimmed
matches a stricter numeric pattern (e.g., /^\d+$/) or confirm that parsing
round-trips (the parsed integer string equals trimmed after normalizing), then
convert to a number and check parsed is >= 1; on failure call
setViewLimitError("Max views must be a positive number") and return.
| } | ||
|
|
||
| const enteredHash = await hashPasscode(args.passcode, project.passcodeSalt); | ||
| const ok = enteredHash === project.passcodeHash; |
There was a problem hiding this comment.
🟡 Non-constant-time hash comparison in verifyPasscode enables timing attacks
The verifyPasscode mutation uses JavaScript's === operator to compare passcode hashes, which is not constant-time and can leak information about the stored hash through timing side-channels.
Root Cause
At convex/projects.ts:876:
const ok = enteredHash === project.passcodeHash;The === operator short-circuits on the first differing character, meaning comparisons that match more prefix characters take slightly longer. Over many requests, an attacker could statistically determine the correct hash byte-by-byte.
While the practical exploitability depends on network jitter and the Convex runtime environment, this is a well-known security anti-pattern for a security-focused application. The fix is to use a constant-time comparison function that always compares all bytes regardless of where they differ.
Impact: An attacker with project membership could potentially recover the passcode hash through repeated timing measurements, though practical exploitation is difficult in a serverless environment.
| const ok = enteredHash === project.passcodeHash; | |
| const enteredHash = await hashPasscode(args.passcode, project.passcodeSalt); | |
| // Constant-time comparison to prevent timing attacks | |
| const a = new TextEncoder().encode(enteredHash); | |
| const b = new TextEncoder().encode(project.passcodeHash); | |
| let ok = a.length === b.length; | |
| const len = Math.max(a.length, b.length); | |
| for (let i = 0; i < len; i++) { | |
| ok = ok && (a[i] === b[i]); | |
| } | |
| return { ok }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
…ipboard error handling in SharedDashboard
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/d/shared.tsx (1)
461-463:⚠️ Potential issue | 🟡 MinorMissing
titleattribute on severalButtoncomponents.As per coding guidelines, Button components from
@/components/ui/buttonshould have atitleattribute. The following buttons are missing one:
- Line 461: "Bulk Actions" button
- Line 653: Actions dropdown trigger (MoreHorizontal icon button)
- Line 771: "Load More" button
Proposed fix
- <Button variant="outline" size="sm" className="gap-2"> + <Button variant="outline" size="sm" className="gap-2" title="Bulk actions"> Bulk Actions- <Button variant="ghost" size="icon" className="h-8 w-8"> + <Button variant="ghost" size="icon" className="h-8 w-8" title="Share actions">- <Button variant="outline" onClick={() => loadMore(10)}> + <Button variant="outline" onClick={() => loadMore(10)} title="Load more shared secrets">As per coding guidelines:
src/**/*.tsx: "Add title attribute to Button components from@/components/ui/button"Also applies to: 653-655, 771-773
🤖 Fix all issues with AI agents
In `@src/lib/key-store.ts`:
- Around line 56-62: The sign-out click handler for user logout fails to clear
in-memory encryption keys; update the signOut onClick handler that calls
signOut(...) so it invokes keyStore.clear() before redirecting (e.g., call
keyStore.clear() inside the signOut callback or immediately before calling
signOut), ensuring keyStore.clear() is executed prior to setting
window.location.href or completing the signOut flow; reference the existing
keyStore and signOut call in the sign-out click handler and add the clear call
accordingly.
🧹 Nitpick comments (1)
src/routes/d/shared.tsx (1)
167-167: Add a brief comment explaining thevoidusage.
void keyStoreVersionsilently consumes the variable to trigger re-renders viauseSyncExternalStore. This pattern isn't immediately obvious to other developers.Suggested improvement
+ // Read keyStoreVersion to trigger re-renders when project keys change void keyStoreVersion;
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/d.tsx (1)
76-88:⚠️ Potential issue | 🟠 MajorAdd
titleattribute to the Button component and configure UserButton's sign-out flow to clear encryption keys.The Button at line 76 is missing the
titleattribute required by the coding guidelines (other Buttons in the file like the Tutorial button use this). Additionally, the UserButton component at line 121 has noonSignOutcallback orafterSignOutUrlconfigured, meaning when users sign out via that widget,keyStore.clear()won't be called—leaving encryption keys in memory. The pattern is already established inapp-sidebar.tsxwherehandleLogoutproperly callskeyStore.clear()before sign-out. Wire the same key-clearing logic to the UserButton's sign-out flow.
🧹 Nitpick comments (2)
src/routes/d.tsx (2)
89-95: Missingtitleattribute on Button.Per coding guidelines, Button components should have a
titleattribute. This applies to the "Return to Landing Page" button. Though this line isn't part of the current diff, it's adjacent to the changed code.Proposed fix
<Button variant="link" className="w-full text-muted-foreground" + title="Return to Landing Page" onClick={() => (window.location.href = "/")} >As per coding guidelines:
src/**/*.tsx: Add title attribute to Button components from@/components/ui/button.
146-146: Missingtitleattribute on "Go back to Home" Button.Proposed fix
- <Button onClick={() => (window.location.href = "/")}>Go back to Home</Button> + <Button title="Go back to Home" onClick={() => (window.location.href = "/")}>Go back to Home</Button>As per coding guidelines:
src/**/*.tsx: Add title attribute to Button components from@/components/ui/button.
| await ctx.db.patch(args.id, { | ||
| views: sharedSecret.views + 1, | ||
| }); |
There was a problem hiding this comment.
🔴 accessSecret consumes a view before client-side passcode verification, exhausting view-limited links on wrong passcode
The accessSecret mutation increments the view counter and returns the encrypted payload before the client attempts to decrypt with the user-entered passcode. If the user enters a wrong passcode, decryption fails on the client (line 127-129 of src/routes/share/$shareId.tsx), but the view has already been permanently consumed server-side.
Root Cause and Impact
The flow in src/routes/share/$shareId.tsx:77 calls accessSecret which at convex/sharedSecrets.ts:299-301 does:
await ctx.db.patch(args.id, {
views: sharedSecret.views + 1,
});…and returns the encrypted payload. Only after that does the client try decryption with the user's passcode at src/routes/share/$shareId.tsx:95-121. If decryption fails (wrong passcode), the catch block at line 128-129 shows an error, but the view was already consumed.
Impact: For a one-time link (maxViews: 1), a single wrong passcode attempt permanently exhausts the link. The next attempt — even with the correct passcode — will receive { exhausted: true } from the server, making the shared secret permanently inaccessible. This fundamentally breaks the one-time link security feature introduced in this PR.
Prompt for agents
The view count increment in the accessSecret mutation (convex/sharedSecrets.ts lines 299-301) needs to be deferred until after the client has successfully decrypted the payload with the correct passcode. There are two main approaches:
1. Split into two steps: Have accessSecret return the encrypted payload WITHOUT incrementing the view count. Add a separate confirmView mutation that the client calls AFTER successful decryption (in src/routes/share/$shareId.tsx, after line 124 where setDecryptedVariables is called). The confirmView mutation would then atomically check limits and increment the counter. This approach has a minor weakness: a malicious client could skip calling confirmView.
2. Server-side passcode verification for shared links: Accept the share passcode in the accessSecret mutation args, derive the key server-side, attempt to decrypt the ShareKey to verify correctness, and only increment the view count and return the payload if the passcode is valid. This is more secure but requires moving some decryption logic to the server.
Approach 1 is simpler and matches the existing zero-knowledge architecture. In convex/sharedSecrets.ts, remove the view increment from accessSecret (lines 299-301) and create a new confirmView mutation that takes the secret ID, checks limits, and increments the view count. In src/routes/share/$shareId.tsx, call confirmView after successful decryption at line 124.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
this is only called once the client decrypts and verifes the passcode
1 participant
Improve the security side of things and added one time and view based restriction on password sharing
Summary
Summary by CodeRabbit
New Features
Documentation
Chores
Checklist
Additional Information