fix: batch - password toggle, copilot button, feature cards#836
fix: batch - password toggle, copilot button, feature cards#836saurabhhhcodes wants to merge 1 commit into
Conversation
🚀 Thank You for Contributing to Late-MeetPlease ensure that:
Thank you for contributing 💙 |
github-actions
Bot
added
gssoc
|
👋 Thank you @saurabhhhcodes for your contribution to Late-Meet!
Please review any automated suggestions or code review comments that may appear below! We will review your PR as soon as possible! Please consider starring the repository ⭐ to show your support! |
📝 WalkthroughWalkthroughModified ChangesVault Passphrase Verification Token
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/options.html (1)
412-412:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd accessible names to the remaining feature toggles.
Lines 412/473/500/533/559 render checkboxes without a programmatic label (
aria-label/aria-labelledby), so assistive tech can announce unnamed controls.Minimal fix example
-<input type="checkbox" id="late-joiner-toggle" checked /> +<input type="checkbox" id="late-joiner-toggle" checked aria-label="Late Joiner Briefing" /> -<input type="checkbox" id="topic-toggle" checked /> +<input type="checkbox" id="topic-toggle" checked aria-label="Topic Detection" /> -<input type="checkbox" id="decision-toggle" checked /> +<input type="checkbox" id="decision-toggle" checked aria-label="Decision Detection" /> -<input type="checkbox" id="action-toggle" checked /> +<input type="checkbox" id="action-toggle" checked aria-label="Action Item Extraction" /> -<input type="checkbox" id="sentiment-toggle" checked /> +<input type="checkbox" id="sentiment-toggle" checked aria-label="Sentiment Analysis" />Also applies to: 473-473, 500-500, 533-533, 559-559
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/options.html` at line 412, The checkbox input elements with IDs late-joiner-toggle (line 412), and the similar checkboxes at lines 473, 500, 533, and 559 lack accessible names for assistive technologies. Add an aria-label attribute to each of these input type="checkbox" elements with a descriptive label that identifies what feature or setting the checkbox controls, ensuring the label clearly describes the purpose of each toggle for screen reader users.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/content.ts`:
- Around line 620-625: The button is being removed after sending the
OPEN_SIDE_PANEL message without checking if the operation was actually
successful. The background handler returns { success: true } even when the side
panel fails to open, causing the button to be removed unnecessarily. In
src/content.ts, after awaiting the chrome.runtime.sendMessage response, check
the success property of the returned object before removing the button.
Additionally, in src/background.ts, modify the OPEN_SIDE_PANEL handler to return
{ success: false } when chrome.sidePanel.open throws an error, instead of always
returning success: true.
---
Outside diff comments:
In `@src/options.html`:
- Line 412: The checkbox input elements with IDs late-joiner-toggle (line 412),
and the similar checkboxes at lines 473, 500, 533, and 559 lack accessible names
for assistive technologies. Add an aria-label attribute to each of these input
type="checkbox" elements with a descriptive label that identifies what feature
or setting the checkbox controls, ensuring the label clearly describes the
purpose of each toggle for screen reader users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1824f6e-41ae-4497-86b3-df53248df293
📒 Files selected for processing (5)
src/content.tssrc/options.csssrc/options.htmlsrc/options.tssrc/popup.ts
| await chrome.runtime.sendMessage({ type: "OPEN_SIDE_PANEL" }); | ||
| if (openPanelTimer) { | ||
| clearTimeout(openPanelTimer); | ||
| openPanelTimer = null; | ||
| } | ||
| btn.remove(); |
There was a problem hiding this comment.
Don’t remove the floating button unless side-panel open is truly successful.
On Line 625, the button is removed after any resolved OPEN_SIDE_PANEL response. In src/background.ts (OPEN_SIDE_PANEL handler), failures are caught but still answered with { success: true }, so users can lose the button even when open fails.
Suggested contract-safe fix
- await chrome.runtime.sendMessage({ type: "OPEN_SIDE_PANEL" });
+ const response = await chrome.runtime.sendMessage({ type: "OPEN_SIDE_PANEL" });
+ if (!response?.success) {
+ throw new Error("Failed to open side panel");
+ }
if (openPanelTimer) {
clearTimeout(openPanelTimer);
openPanelTimer = null;
}
btn.remove();// src/background.ts (paired change outside this hunk):
// return { success: false } when chrome.sidePanel.open throws,
// instead of always returning success: true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/content.ts` around lines 620 - 625, The button is being removed after
sending the OPEN_SIDE_PANEL message without checking if the operation was
actually successful. The background handler returns { success: true } even when
the side panel fails to open, causing the button to be removed unnecessarily. In
src/content.ts, after awaiting the chrome.runtime.sendMessage response, check
the success property of the returned object before removing the button.
Additionally, in src/background.ts, modify the OPEN_SIDE_PANEL handler to return
{ success: false } when chrome.sidePanel.open throws an error, instead of always
returning success: true.
|
Don’t remove the floating button unless side-panel open is truly successful. On Line 625, the button is removed after any resolved OPEN_SIDE_PANEL response. In src/background.ts (OPEN_SIDE_PANEL handler), failures are caught but still answered with { success: true }, so users can lose the button even when open fails. Suggested contract-safe fix
// src/background.ts (paired change outside this hunk): In |
…i123#837) - Store an encrypted verification token during first-time vault setup - Always verify the passphrase by decrypting the verification token, even when no credentials are stored yet - Fix AES_GCM references to use AES_ALGORITHM constant - Previously, unlockCredentials() accepted any passphrase when the vault was initialized (salt stored) but no credentials existed yet
github-actions
Bot
added
security-review
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/credentials.ts`:
- Around line 174-190: The setup flow in credentials.ts assigns derivedKey
before the verification token is fully encrypted and persisted, which can leave
the vault marked unlocked on a failed setup. Move the derivedKey assignment in
the setup routine so it happens only after crypto.subtle.encrypt and
chrome.storage.local.set both succeed, keeping isUnlocked() and auto-lock state
consistent when using deriveKeyFromPassphrase and the vault verification storage
path.
- Around line 149-157: The vault verification flow in credentials.ts currently
allows unlock to succeed when there is no token and no encrypted credential to
verify against. Update the verification logic around the sample-credential
decrypt path so it fails closed whenever nothing is available to check, and
ensure the unlock path only proceeds after a real successful decrypt. Also, in
the legacy decrypt branch, backfill VAULT_VERIFICATION_KEY after a successful
decrypt so migrated vaults no longer depend on sample credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a4903e3-45b6-4a12-8651-bd6b89cc2975
📒 Files selected for processing (1)
src/utils/credentials.ts
| } else { | ||
| const sampleKey = CREDENTIAL_KEYS.find((k) => encryptedCreds[k]); | ||
| if (sampleKey && encryptedCreds[sampleKey]) { | ||
| const combined = base64ToArrayBuffer(encryptedCreds[sampleKey]); | ||
| const iv = new Uint8Array(combined.slice(0, IV_LENGTH)); | ||
| const ciphertext = combined.slice(IV_LENGTH); | ||
| await crypto.subtle.decrypt({ name: AES_ALGORITHM, iv }, key, ciphertext); | ||
| } | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when there is nothing to verify.
Line 149 falls back to decrypting a sample credential, but if no token and no encrypted credential exists, the block completes and Line 161 unlocks with any passphrase. Also consider backfilling VAULT_VERIFICATION_KEY after a successful legacy credential decrypt so migrated vaults stop depending on sample credentials.
🛡️ Proposed fix
} else {
const sampleKey = CREDENTIAL_KEYS.find((k) => encryptedCreds[k]);
if (sampleKey && encryptedCreds[sampleKey]) {
const combined = base64ToArrayBuffer(encryptedCreds[sampleKey]);
const iv = new Uint8Array(combined.slice(0, IV_LENGTH));
const ciphertext = combined.slice(IV_LENGTH);
await crypto.subtle.decrypt({ name: AES_ALGORITHM, iv }, key, ciphertext);
+ await chrome.storage.local.set({
+ [VAULT_VERIFICATION_KEY]: await createVaultVerificationToken(key),
+ });
+ } else {
+ return false;
}
}📝 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.
| } else { | |
| const sampleKey = CREDENTIAL_KEYS.find((k) => encryptedCreds[k]); | |
| if (sampleKey && encryptedCreds[sampleKey]) { | |
| const combined = base64ToArrayBuffer(encryptedCreds[sampleKey]); | |
| const iv = new Uint8Array(combined.slice(0, IV_LENGTH)); | |
| const ciphertext = combined.slice(IV_LENGTH); | |
| await crypto.subtle.decrypt({ name: AES_ALGORITHM, iv }, key, ciphertext); | |
| } | |
| } catch { | |
| return false; | |
| } | |
| } else { | |
| const sampleKey = CREDENTIAL_KEYS.find((k) => encryptedCreds[k]); | |
| if (sampleKey && encryptedCreds[sampleKey]) { | |
| const combined = base64ToArrayBuffer(encryptedCreds[sampleKey]); | |
| const iv = new Uint8Array(combined.slice(0, IV_LENGTH)); | |
| const ciphertext = combined.slice(IV_LENGTH); | |
| await crypto.subtle.decrypt({ name: AES_ALGORITHM, iv }, key, ciphertext); | |
| await chrome.storage.local.set({ | |
| [VAULT_VERIFICATION_KEY]: await createVaultVerificationToken(key), | |
| }); | |
| } else { | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/credentials.ts` around lines 149 - 157, The vault verification flow
in credentials.ts currently allows unlock to succeed when there is no token and
no encrypted credential to verify against. Update the verification logic around
the sample-credential decrypt path so it fails closed whenever nothing is
available to check, and ensure the unlock path only proceeds after a real
successful decrypt. Also, in the legacy decrypt branch, backfill
VAULT_VERIFICATION_KEY after a successful decrypt so migrated vaults no longer
depend on sample credentials.
| derivedKey = await deriveKeyFromPassphrase(passphrase, salt.buffer); | ||
|
|
||
| const verificationIv = crypto.getRandomValues(new Uint8Array(IV_LENGTH)); | ||
| const verificationPlaintext = new TextEncoder().encode("vault-verification-token"); | ||
| const verificationCiphertext = await crypto.subtle.encrypt( | ||
| { name: AES_ALGORITHM, iv: verificationIv }, | ||
| derivedKey, | ||
| verificationPlaintext, | ||
| ); | ||
| const verificationCombined = new Uint8Array(verificationIv.length + verificationCiphertext.byteLength); | ||
| verificationCombined.set(verificationIv); | ||
| verificationCombined.set(new Uint8Array(verificationCiphertext), verificationIv.length); | ||
|
|
||
| await chrome.storage.local.set({ | ||
| [SALT_STORAGE_KEY]: arrayBufferToBase64(salt.buffer), | ||
| [VAULT_VERIFICATION_KEY]: arrayBufferToBase64(verificationCombined.buffer), | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Assign derivedKey only after setup is persisted.
Line 174 publishes the unlocked key before token encryption/storage completes. If crypto.subtle.encrypt or chrome.storage.local.set rejects, callers see a failed setup path while isUnlocked() can still be true and no auto-lock timer is reset.
🔒 Proposed fix
const salt = crypto.getRandomValues(new Uint8Array(SALT_LENGTH));
- derivedKey = await deriveKeyFromPassphrase(passphrase, salt.buffer);
+ const key = await deriveKeyFromPassphrase(passphrase, salt.buffer);
const verificationIv = crypto.getRandomValues(new Uint8Array(IV_LENGTH));
const verificationPlaintext = new TextEncoder().encode("vault-verification-token");
const verificationCiphertext = await crypto.subtle.encrypt(
{ name: AES_ALGORITHM, iv: verificationIv },
- derivedKey,
+ key,
verificationPlaintext,
);
@@
await chrome.storage.local.set({
[SALT_STORAGE_KEY]: arrayBufferToBase64(salt.buffer),
[VAULT_VERIFICATION_KEY]: arrayBufferToBase64(verificationCombined.buffer),
});
+ derivedKey = key;
resetAutoLockTimer();📝 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.
| derivedKey = await deriveKeyFromPassphrase(passphrase, salt.buffer); | |
| const verificationIv = crypto.getRandomValues(new Uint8Array(IV_LENGTH)); | |
| const verificationPlaintext = new TextEncoder().encode("vault-verification-token"); | |
| const verificationCiphertext = await crypto.subtle.encrypt( | |
| { name: AES_ALGORITHM, iv: verificationIv }, | |
| derivedKey, | |
| verificationPlaintext, | |
| ); | |
| const verificationCombined = new Uint8Array(verificationIv.length + verificationCiphertext.byteLength); | |
| verificationCombined.set(verificationIv); | |
| verificationCombined.set(new Uint8Array(verificationCiphertext), verificationIv.length); | |
| await chrome.storage.local.set({ | |
| [SALT_STORAGE_KEY]: arrayBufferToBase64(salt.buffer), | |
| [VAULT_VERIFICATION_KEY]: arrayBufferToBase64(verificationCombined.buffer), | |
| }); | |
| const salt = crypto.getRandomValues(new Uint8Array(SALT_LENGTH)); | |
| const key = await deriveKeyFromPassphrase(passphrase, salt.buffer); | |
| const verificationIv = crypto.getRandomValues(new Uint8Array(IV_LENGTH)); | |
| const verificationPlaintext = new TextEncoder().encode("vault-verification-token"); | |
| const verificationCiphertext = await crypto.subtle.encrypt( | |
| { name: AES_ALGORITHM, iv: verificationIv }, | |
| key, | |
| verificationPlaintext, | |
| ); | |
| const verificationCombined = new Uint8Array(verificationIv.length + verificationCiphertext.byteLength); | |
| verificationCombined.set(verificationIv); | |
| verificationCombined.set(new Uint8Array(verificationCiphertext), verificationIv.length); | |
| await chrome.storage.local.set({ | |
| [SALT_STORAGE_KEY]: arrayBufferToBase64(salt.buffer), | |
| [VAULT_VERIFICATION_KEY]: arrayBufferToBase64(verificationCombined.buffer), | |
| }); | |
| derivedKey = key; | |
| resetAutoLockTimer(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/credentials.ts` around lines 174 - 190, The setup flow in
credentials.ts assigns derivedKey before the verification token is fully
encrypted and persisted, which can leave the vault marked unlocked on a failed
setup. Move the derivedKey assignment in the setup routine so it happens only
after crypto.subtle.encrypt and chrome.storage.local.set both succeed, keeping
isUnlocked() and auto-lock state consistent when using deriveKeyFromPassphrase
and the vault verification storage path.
2 participants
Fixes:
Summary by CodeRabbit