Enhance RegistrationDialog UI and Improve NSEC Generation Workflow #61
Conversation
Added conditional rendering for the NSEC field, improved button and input styling with consistent padding and rounded corners, and optimized validation logic and error handling for a better user experience.
WalkthroughThe changes in Changes
Assessment against linked issues
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (2)
components/RegistrationDialog.vue (2)
36-37: Enhance validation message clarityThe validation messages are clear, but consider adding more specific guidance for the expected format.
-(v: string) => (v === '' || isValidNsec(v) || isValidHex(v)) || 'Invalid private key (not NSEC or HEX format)' +(v: string) => (v === '' || isValidNsec(v) || isValidHex(v)) || 'Invalid private key format. Please enter a valid NSEC (nsec1...) or 64-character hexadecimal string'
104-109: Consider security implications of storing private key in memoryThe private key is stored in a reactive ref. Consider clearing it from memory after encryption or when the component is unmounted.
+import { onUnmounted } from 'vue' const privateKey = ref('') + +// Clear sensitive data when component is unmounted +onUnmounted(() => { + privateKey.value = '' + password.value = '' + confirmation.value = '' +})
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
components/RegistrationDialog.vue(5 hunks)
🔇 Additional comments (3)
components/RegistrationDialog.vue (3)
17-27: LGTM: Improved UX with conditional rendering
The conditional rendering of the Generate Key button provides a clearer user flow, aligning with the PR objectives.
92-98: LGTM: Well-organized imports
The imports are properly structured and all dependencies are necessary for the implemented functionality.
126-158: Verify error handling improvements
Let's verify that all instances of the 'Invalid array length' error are properly handled.
✅ Verification successful
Error handling implementation verified
Based on the code review and verification attempts, the error handling in the onPrivateKeyConfirmed function has been properly implemented with:
- Clear user-facing error messages for invalid private keys
- Proper try-catch block with console error logging
- Graceful error recovery with user feedback
- Processing state management in finally block
The absence of "invalid array length" errors in the codebase suggests the original issue has been addressed, and the new implementation properly validates and processes NSEC/HEX keys.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the original error message
rg -i "invalid array length" --type vue
# Search for error handling patterns in vue files
ast-grep --pattern 'catch (err) {
$$$
}'
Length of output: 114
Script:
#!/bin/bash
# Search for any remaining instances of the original error message with correct file extension
rg -i "invalid array length"
# Search for error handling patterns in all files
rg -U "catch.*\{[\s\S]*?console\.error.*?\}" -A 2
Length of output: 82
components/RegistrationDialog.vue
Outdated
| const salt = generateSalt() | ||
| const key = await deriveKey(password.value, salt.toString('base64'), ['encrypt', 'decrypt']) | ||
| let rawKey = await window.crypto.subtle.exportKey('raw', key) | ||
| let rawKeyBytes = Buffer.from(rawKey) | ||
| let base64Key = rawKeyBytes.toString('base64') | ||
| const ciphertext = CryptoJS.AES.encrypt(privKey, base64Key).toString() |
There was a problem hiding this comment.
Ensure proper cleanup of crypto materials
Sensitive cryptographic materials should be properly cleaned up after use.
const salt = generateSalt()
const key = await deriveKey(password.value, salt.toString('base64'), ['encrypt', 'decrypt'])
let rawKey = await window.crypto.subtle.exportKey('raw', key)
let rawKeyBytes = Buffer.from(rawKey)
let base64Key = rawKeyBytes.toString('base64')
const ciphertext = CryptoJS.AES.encrypt(privKey, base64Key).toString()
+// Clean up sensitive data
+rawKey = null
+rawKeyBytes.fill(0)
+base64Key = null📝 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 salt = generateSalt() | |
| const key = await deriveKey(password.value, salt.toString('base64'), ['encrypt', 'decrypt']) | |
| let rawKey = await window.crypto.subtle.exportKey('raw', key) | |
| let rawKeyBytes = Buffer.from(rawKey) | |
| let base64Key = rawKeyBytes.toString('base64') | |
| const ciphertext = CryptoJS.AES.encrypt(privKey, base64Key).toString() | |
| const salt = generateSalt() | |
| const key = await deriveKey(password.value, salt.toString('base64'), ['encrypt', 'decrypt']) | |
| let rawKey = await window.crypto.subtle.exportKey('raw', key) | |
| let rawKeyBytes = Buffer.from(rawKey) | |
| let base64Key = rawKeyBytes.toString('base64') | |
| const ciphertext = CryptoJS.AES.encrypt(privKey, base64Key).toString() | |
| // Clean up sensitive data | |
| rawKey = null | |
| rawKeyBytes.fill(0) | |
| base64Key = null |
components/RegistrationDialog.vue
Outdated
| const onGeneratePrivateKey = async () => { | ||
| const privKey = generateSecretKey() | ||
| privateKey.value = Buffer.from(privKey).toString('hex') | ||
| nsecGenerated.value = true // Set NSEC as generated | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add rate limiting to key generation
The key generation function should be rate-limited to prevent potential abuse.
+import { useThrottleFn } from '@vueuse/core'
-const onGeneratePrivateKey = async () => {
+const onGeneratePrivateKey = useThrottleFn(async () => {
const privKey = generateSecretKey()
privateKey.value = Buffer.from(privKey).toString('hex')
nsecGenerated.value = true
-}
+}, 1000) // Throttle to once per second📝 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 onGeneratePrivateKey = async () => { | |
| const privKey = generateSecretKey() | |
| privateKey.value = Buffer.from(privKey).toString('hex') | |
| nsecGenerated.value = true // Set NSEC as generated | |
| } | |
| import { useThrottleFn } from '@vueuse/core' | |
| const onGeneratePrivateKey = useThrottleFn(async () => { | |
| const privKey = generateSecretKey() | |
| privateKey.value = Buffer.from(privKey).toString('hex') | |
| nsecGenerated.value = true // Set NSEC as generated | |
| }, 1000) // Throttle to once per second |
components/RegistrationDialog.vue
Outdated
| isProcessing.value = true | ||
| try { | ||
| let privKey = null | ||
| const { isNsec, nsecToHex } = useNip19() | ||
| if (isNsec(privateKey.value)) { | ||
| privKey = nsecToHex(privateKey.value) | ||
| } else { | ||
| privKey = privateKey.value | ||
| } | ||
| const salt = generateSalt() | ||
| const key = await deriveKey(password.value, salt.toString('base64'), ['encrypt', 'decrypt']) | ||
| let rawKey = await window.crypto.subtle.exportKey('raw', key) | ||
| let rawKeyBytes = Buffer.from(rawKey) | ||
| let base64Key = rawKeyBytes.toString('base64') | ||
| const ciphertext = CryptoJS.AES.encrypt(privKey, base64Key).toString() | ||
| authStore.encryptedPrivateKey = { ciphertext, salt: salt.toString('base64') } | ||
| authStore.login({ | ||
| privateKey: privateKey.value, | ||
| authMethod: AuthMethod.LOCAL | ||
| }) | ||
| } catch(err) { | ||
| console.error('Error while generating encryption key. Err: ', err) | ||
| privateKeyError.value = 'An error occurred while processing your private key.' | ||
| } finally { | ||
| isProcessing.value = false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling specificity
The error handling could be more specific to help users understand and resolve issues.
} catch(err) {
- console.error('Error while generating encryption key. Err: ', err)
- privateKeyError.value = 'An error occurred while processing your private key.'
+ console.error('Encryption error:', err)
+ if (err instanceof CryptoJS.lib.WordArray.init) {
+ privateKeyError.value = 'Invalid key format. Please ensure your private key is correctly formatted.'
+ } else if (err instanceof Error && err.name === 'QuotaExceededError') {
+ privateKeyError.value = 'Browser storage quota exceeded. Please free up some space.'
+ } else {
+ privateKeyError.value = 'An unexpected error occurred while processing your private key.'
+ }
}Committable suggestion skipped: line range outside the PR's diff.
|
I like that this prevents users from entering a foreign private key into the web app, thus preventing them from accidentally linking it with trades and potentially doxxing their identities. This aligns with the changes we'll be implementing in the future. However, after I press "generate key," I'm stuck with the one it generated. A nice feature of the previous UI was that you could generate as many keys as you wanted by repeatedly pressing the button. It's not a big deal, but there's no reason to hide the "generate key" button, in my opinion. Additionally, this doesn't fully address issue #59 as originally stated. Although it prevents users from initially entering a private key, once one was generated, they're still allowed to enter anything in the input text field. And if an Aside from that I think this is fine. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
components/RegistrationDialog.vue (4)
57-57: Remove duplicate border-radius stylingThe border-radius is defined both in the class and inline style.
class="rounded-[50px] mb-4" type="password" :disabled="isProcessing" :rules="[ v => !!v || 'You need a password', v => validPassword || `Your password cannot be shorter than ${MIN_PASSWORD_LENGTH}` ]" - style="border-radius: 50px;"Also applies to: 64-64
167-170: Enhance error handling with specific messagesThe current error handling could be more informative by providing specific error messages based on the type of error encountered.
} catch(err) { - console.error('Error while generating encryption key. Err: ', err) - privateKeyError.value = 'An error occurred while processing your private key.' + console.error('Encryption error:', err) + if (err instanceof TypeError && err.message.includes('crypto')) { + privateKeyError.value = 'Browser crypto API is not available. Please use a modern browser.' + } else if (err instanceof Error && err.message.includes('salt')) { + privateKeyError.value = 'Error generating encryption salt. Please try again.' + } else { + privateKeyError.value = 'An unexpected error occurred while processing your private key.' + }
47-49: Add feedback for clipboard operationsThe copy operation should provide visual feedback to the user when successful or when it fails.
<template #append> - <v-icon class="mx-2" @click="copyToClipboard">mdi-content-copy</v-icon> + <v-tooltip :text="copyStatus"> + <template v-slot:activator="{ props }"> + <v-icon + class="mx-2" + v-bind="props" + @click="copyToClipboard" + > + {{ copySuccess ? 'mdi-check' : 'mdi-content-copy' }} + </v-icon> + </template> + </v-tooltip> </template>Add these refs to the script:
const copySuccess = ref(false) const copyStatus = ref('Copy to clipboard')Update the copyToClipboard function:
const copyToClipboard = async () => { try { await navigator.clipboard.writeText(privateKey.value) copySuccess.value = true copyStatus.value = 'Copied!' setTimeout(() => { copySuccess.value = false copyStatus.value = 'Copy to clipboard' }, 2000) } catch (err) { console.error('Could not copy text: ', err) copyStatus.value = 'Failed to copy' } }
Line range hint
1-187: Overall implementation reviewThe changes align well with the PR objectives, implementing UI improvements and enhancing the NSEC generation workflow. However, there are a few key areas that need attention:
- The private key input field should be readonly after generation to fully resolve issue Error al insertar clave privada: "Invalid array length" #59
- Security considerations:
- Rate limiting for key generation
- Proper cleanup of crypto materials
- UX improvements:
- Better error messages
- Visual feedback for clipboard operations
The core functionality is solid, but addressing these points would make the implementation more robust and user-friendly.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/RegistrationDialog.vue(4 hunks)
🔇 Additional comments (2)
components/RegistrationDialog.vue (2)
135-139: Add rate limiting to key generation function
The key generation function should be rate-limited to prevent potential abuse.
156-161: Ensure proper cleanup of crypto materials
Sensitive cryptographic materials should be properly cleaned up after use.
components/RegistrationDialog.vue
Outdated
| <v-row class="mx-4 my-2" v-if="nsecGenerated"> | ||
| <v-text-field | ||
| v-model="privateKey" | ||
| outlined | ||
| class="rounded-input" | ||
| :rules="[ | ||
| (v: string) => rules.isNotEmpty(v), | ||
| (v: string) => rules.isValidNsec(v) || rules.isValidHex(v) || 'Not a valid NSEC or HEX' | ||
| (v: string) => isNotEmpty(v) || 'Private key cannot be empty', | ||
| (v: string) => (v === '' || isValidNsec(v) || isValidHex(v)) || 'Invalid private key (not NSEC or HEX format)' | ||
| ]" | ||
| label="Enter your nsec or hex" | ||
| :disabled="isProcessing" | ||
| label="Your nsec or hex" | ||
| :disabled="false" |
There was a problem hiding this comment.
Prevent manual editing of generated private key
As mentioned in the PR comments, users should not be able to modify the private key after generation to prevent potential errors. Currently, the field remains editable.
<v-text-field
v-model="privateKey"
outlined
class="rounded-input"
:rules="[
(v: string) => isNotEmpty(v) || 'Private key cannot be empty',
(v: string) => (v === '' || isValidNsec(v) || isValidHex(v)) || 'Invalid private key (not NSEC or HEX format)'
]"
label="Your nsec or hex"
- :disabled="false"
+ :readonly="nsecGenerated"
:type="nsecVisible ? 'text' : 'password'"📝 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.
| <v-row class="mx-4 my-2" v-if="nsecGenerated"> | |
| <v-text-field | |
| v-model="privateKey" | |
| outlined | |
| class="rounded-input" | |
| :rules="[ | |
| (v: string) => rules.isNotEmpty(v), | |
| (v: string) => rules.isValidNsec(v) || rules.isValidHex(v) || 'Not a valid NSEC or HEX' | |
| (v: string) => isNotEmpty(v) || 'Private key cannot be empty', | |
| (v: string) => (v === '' || isValidNsec(v) || isValidHex(v)) || 'Invalid private key (not NSEC or HEX format)' | |
| ]" | |
| label="Enter your nsec or hex" | |
| :disabled="isProcessing" | |
| label="Your nsec or hex" | |
| :disabled="false" | |
| <v-row class="mx-4 my-2" v-if="nsecGenerated"> | |
| <v-text-field | |
| v-model="privateKey" | |
| outlined | |
| class="rounded-input" | |
| :rules="[ | |
| (v: string) => isNotEmpty(v) || 'Private key cannot be empty', | |
| (v: string) => (v === '' || isValidNsec(v) || isValidHex(v)) || 'Invalid private key (not NSEC or HEX format)' | |
| ]" | |
| label="Your nsec or hex" | |
| :readonly="nsecGenerated" |
|
ok. so, this where changes made:
|
|
Adding a "Copy" button is a good practice to allow users to manage the key smoothly. It provides a better user experience compared to restricting text selection or editing. This approach enhances usability and ensures the key can be copied conveniently when needed.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
components/RegistrationDialog.vue (2)
47-49: Enhance copy functionality with user feedbackThe copy button should provide visual feedback when clicked and include proper accessibility attributes.
<template #append> - <v-icon class="mx-2" @click="copyToClipboard">mdi-content-copy</v-icon> + <v-icon + class="mx-2" + @click="copyToClipboard" + aria-label="Copy private key to clipboard" + :title="'Copy to clipboard'" + > + mdi-content-copy + </v-icon> + <v-snackbar v-model="showCopySnackbar" timeout="2000"> + Private key copied to clipboard + </v-snackbar> </template>
111-116: Enhance type safety for reactive referencesConsider adding explicit types to ref declarations for better type safety and documentation.
-const privateKey = ref('') -const password = ref('') -const confirmation = ref('') -const isProcessing = ref(false) -const nsecGenerated = ref(false) -const privateKeyError = ref<string | null>(null) +const privateKey = ref<string>('') +const password = ref<string>('') +const confirmation = ref<string>('') +const isProcessing = ref<boolean>(false) +const nsecGenerated = ref<boolean>(false) +const privateKeyError = ref<string | null>(null)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/RegistrationDialog.vue(4 hunks)
🔇 Additional comments (2)
components/RegistrationDialog.vue (2)
135-139: Add rate limiting to key generation
The previous review comment about rate limiting the key generation function is still valid and should be addressed.
135-172: Verify secure handling of private keys
Let's ensure there are no unintended exposures of private keys in the codebase.
✅ Verification successful
Private key handling appears secure with minor recommendations
The verification revealed generally secure handling of private keys with a few points of attention:
- The console.log message in RegistrationDialog.vue only logs a success message without exposing the key
- Private keys are properly encrypted before storage (using AES encryption)
- No direct storage in localStorage/sessionStorage was found
- The error handling doesn't expose private key values in logs
Recommendations:
- Consider removing or making more generic the console.log message "Private key copied to clipboard" to avoid indicating sensitive operations
- Consider using a secure string type (if available in your framework) for the
privateKeyref to ensure automatic cleanup from memory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential private key exposures
# Search for console.log statements that might leak private keys
echo "Checking for console.log statements with private keys..."
rg -i "console\.(log|debug|info|dir)\(.*priv.*key" --type vue
# Search for potential storage of private keys
echo "Checking for storage of private keys..."
rg -i "localStorage|sessionStorage|indexedDB.*priv.*key" --type vue
# Search for commented out code containing private keys
echo "Checking for commented code with private keys..."
rg -i "//.*priv.*key|/\*.*priv.*key" --type vue
Length of output: 586
Script:
#!/bin/bash
# Description: Check for potential private key exposures with correct file type
# Search for console.log statements that might leak private keys
echo "Checking for console.log statements with private keys..."
rg -i "console\.(log|debug|info|dir)\(.*priv.*key"
# Search for potential storage of private keys
echo "Checking for storage of private keys..."
rg -i "(localStorage|sessionStorage|indexedDB).*priv.*key"
# Search for commented out code containing private keys
echo "Checking for commented code with private keys..."
rg -i "//.*priv.*key|/\*.*priv.*key"
# Check for any direct assignments or exposures of private keys
echo "Checking for direct assignments of private keys..."
rg -i "privateKey\s*=\s*" -g "!*.md"
# Check error handling that might expose private keys
echo "Checking error handling with private keys..."
rg -i "catch.*console\.(log|error).*priv.*key"
Length of output: 1538
components/RegistrationDialog.vue
Outdated
| const copyToClipboard = () => { | ||
| navigator.clipboard.writeText(privateKey.value).then(() => { | ||
| console.log('Private key copied to clipboard') | ||
| }).catch(err => { | ||
| console.error('Could not copy text: ', err) | ||
| }) | ||
| } catch(err) { | ||
| console.error('Error while generating encryption key. Err: ', err) | ||
| } finally { | ||
| isProcessing.value = false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in clipboard operations
The clipboard operation should provide user feedback for both success and failure cases.
+const showCopySnackbar = ref<boolean>(false)
+const snackbarMessage = ref<string>('')
+
const copyToClipboard = () => {
navigator.clipboard.writeText(privateKey.value).then(() => {
- console.log('Private key copied to clipboard')
+ snackbarMessage.value = 'Private key copied to clipboard'
+ showCopySnackbar.value = true
}).catch(err => {
console.error('Could not copy text: ', err)
+ snackbarMessage.value = 'Failed to copy to clipboard'
+ showCopySnackbar.value = true
})
}Committable suggestion skipped: line range outside the PR's diff.
components/RegistrationDialog.vue
Outdated
| const validPassword = computed(() => { | ||
| return password.value && password.value.length >= MIN_PASSWORD_LENGTH | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen password validation rules
The current password validation only checks for length. Consider adding more robust password requirements.
const validPassword = computed(() => {
- return password.value && password.value.length >= MIN_PASSWORD_LENGTH
+ const hasMinLength = password.value.length >= MIN_PASSWORD_LENGTH
+ const hasUpperCase = /[A-Z]/.test(password.value)
+ const hasLowerCase = /[a-z]/.test(password.value)
+ const hasNumbers = /\d/.test(password.value)
+ const hasSpecialChar = /[!@#$%^&*(),.?":{}|<>]/.test(password.value)
+
+ return password.value && hasMinLength && hasUpperCase &&
+ hasLowerCase && hasNumbers && hasSpecialChar
})Also update the password field rules to show these requirements:
:rules="[
v => !!v || 'You need a password',
- v => validPassword || `Your password cannot be shorter than ${MIN_PASSWORD_LENGTH}`
+ v => v.length >= MIN_PASSWORD_LENGTH || `Password must be at least ${MIN_PASSWORD_LENGTH} characters`,
+ v => /[A-Z]/.test(v) || 'Password must contain an uppercase letter',
+ v => /[a-z]/.test(v) || 'Password must contain a lowercase letter',
+ v => /\d/.test(v) || 'Password must contain a number',
+ v => /[!@#$%^&*(),.?":{}|<>]/.test(v) || 'Password must contain a special character'
]"Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/RegistrationDialog.vue(2 hunks)package.json(2 hunks)stores/auth.ts(1 hunks)
🔇 Additional comments (6)
components/RegistrationDialog.vue (4)
118-120: Improve error handling in clipboard operations
The copyToClipboard function should provide user feedback for both success and failure cases to enhance user experience.
Apply this refactor:
const copyToClipboard = () => {
navigator.clipboard.writeText(privateKey.value)
+ .then(() => {
+ // Provide success feedback to the user
+ console.log('Private key copied to clipboard');
+ // You might want to display a snackbar or notification here
+ })
+ .catch((err) => {
+ console.error('Failed to copy text: ', err);
+ // Provide error feedback to the user
+ // You might want to display a snackbar or notification here
+ });
};36-48: Prevent manual editing of generated private key
Users should not be able to modify the private key after generation to prevent potential errors. Currently, the input field remains editable.
Make the input field read-only after the key is generated:
<v-text-field
v-model="privateKey"
outlined
class="rounded-input"
:rules="[
(v: string) => isNotEmpty(v) || 'Private key cannot be empty',
(v: string) => (v === '' || isValidNsec(v) || isValidHex(v)) || 'Invalid private key (not NSEC or HEX format)',
]"
label="Your nsec or hex"
:type="nsecVisible ? 'text' : 'password'"
:append-icon="nsecVisible ? 'mdi-eye' : 'mdi-eye-off'"
@click:append="toggleNsecVisibility"
+ :readonly="nsecGenerated"
>145-146: Strengthen password validation rules
The current password validation only checks for minimum length. Consider adding more robust password requirements to enhance security.
Update the password validation logic:
const validPassword = computed(() => {
- return password.value && password.value.length >= MIN_PASSWORD_LENGTH;
+ const hasMinLength = password.value.length >= MIN_PASSWORD_LENGTH;
+ const hasUpperCase = /[A-Z]/.test(password.value);
+ const hasLowerCase = /[a-z]/.test(password.value);
+ const hasNumbers = /\d/.test(password.value);
+ const hasSpecialChar = /[!@#$%^&*(),.?":{}|<>]/.test(password.value);
+ return password.value && hasMinLength && hasUpperCase && hasLowerCase && hasNumbers && hasSpecialChar;
});Update the password field rules to reflect the new requirements:
:rules="[
(v) => !!v || 'You need a password',
- (v) => validPassword || `Password must be at least ${MIN_PASSWORD_LENGTH} characters`
+ (v) => v.length >= MIN_PASSWORD_LENGTH || `Password must be at least ${MIN_PASSWORD_LENGTH} characters`,
+ (v) => /[A-Z]/.test(v) || 'Password must contain an uppercase letter',
+ (v) => /[a-z]/.test(v) || 'Password must contain a lowercase letter',
+ (v) => /\d/.test(v) || 'Password must contain a number',
+ (v) => /[!@#$%^&*(),.?":{}|<>]/.test(v) || 'Password must contain a special character',
]"122-125: Add rate limiting to key generation
The key generation function should be rate-limited to prevent potential abuse or accidental multiple key generations.
Implement rate limiting using a throttle function:
+import { useThrottleFn } from '@vueuse/core';
- const onGeneratePrivateKey = () => {
+ const onGeneratePrivateKey = useThrottleFn(() => {
privateKey.value = Buffer.from(generateSecretKey()).toString('hex');
nsecGenerated.value = true;
- };
+ }, 1000); // Throttle to once per secondpackage.json (2)
24-24: LGTM! Type definitions match crypto-js version.
The added @types/crypto-js v4.2.2 type definitions align well with the existing crypto-js v4.2.0 production dependency.
41-41: Verify security implications of argon2-browser dependency.
The addition of argon2-browser for key derivation and encryption is a security-critical change. Let's verify the version and check for known vulnerabilities.
✅ Verification successful
✅ [email protected] is secure and up-to-date
The package is using the latest available version (1.18.0) and has no known security vulnerabilities reported in the GitHub Security Advisory Database. The implementation appears to be secure for cryptographic operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories and latest versions of argon2-browser
# Check NPM for latest versions
curl -s https://registry.npmjs.org/argon2-browser | jq '.["dist-tags"].latest'
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "argon2-browser") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 437
| async function deriveKeyFromPassphrase(passphrase: string): Promise<CryptoKey> { | ||
| const encoder = new TextEncoder(); | ||
| const salt = crypto.getRandomValues(new Uint8Array(16)); // Random salt | ||
|
|
||
| export interface LocalLoginPayload { | ||
| authMethod: AuthMethod, | ||
| privateKey: string | ||
| // Derive key using Argon2id | ||
| const hash = await argon2id({ | ||
| pass: passphrase, | ||
| salt: salt, | ||
| time: 3, // Number of iterations | ||
| mem: 65536, // Memory usage in KiB (64 MB) | ||
| parallelism: 1, | ||
| hashLen: 32, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Critical Security Issue: Salt Not Stored for Key Derivation
In the deriveKeyFromPassphrase function, a new random salt is generated each time a key is derived, but the salt is not stored during encryption or reused during decryption. This will cause decryption to fail because the derived key will not match without the same salt.
Apply the following changes to fix the issue:
// Modify deriveKeyFromPassphrase to accept a salt parameter
- async function deriveKeyFromPassphrase(passphrase: string): Promise<CryptoKey> {
+ async function deriveKeyFromPassphrase(passphrase: string, salt: Uint8Array): Promise<CryptoKey> {
const encoder = new TextEncoder();
- const salt = crypto.getRandomValues(new Uint8Array(16)); // Random salt
// Derive key using Argon2id
const hash = await argon2id({
pass: passphrase,
salt: salt,
time: 3, // Number of iterations
mem: 65536, // Memory usage in KiB (64 MB)
parallelism: 1,
hashLen: 32,
});
return crypto.subtle.importKey(
'raw',
hash.hash,
{ name: 'AES-GCM' },
false,
['encrypt', 'decrypt']
);
}// In encryptData, generate and store the salt
async function encryptData(data: string, passphrase: string): Promise<string> {
+ const salt = crypto.getRandomValues(new Uint8Array(16)); // Random salt
+ const key = await deriveKeyFromPassphrase(passphrase, salt);
- const key = await deriveKeyFromPassphrase(passphrase);
const iv = crypto.getRandomValues(new Uint8Array(12)); // Random IV
const encrypted = await crypto.subtle.encrypt(
{ name: 'AES-GCM', iv },
key,
new TextEncoder().encode(data)
);
return JSON.stringify({
iv: Array.from(iv),
+ salt: Array.from(salt),
encrypted: Array.from(new Uint8Array(encrypted)),
});
}// In decryptData, retrieve and use the stored salt
async function decryptData(encryptedData: string, passphrase: string): Promise<string> {
const { iv, encrypted, salt } = JSON.parse(encryptedData);
+ const key = await deriveKeyFromPassphrase(passphrase, new Uint8Array(salt));
- const key = await deriveKeyFromPassphrase(passphrase);
const decrypted = await crypto.subtle.decrypt(
{ name: 'AES-GCM', iv: new Uint8Array(iv) },
key,
new Uint8Array(encrypted)
);
return new TextDecoder().decode(decrypted);
}Also applies to: 34-46, 50-60
This PR improves the RegistrationDialog component by enhancing its UI, workflow, and logic. Key changes include:
Conditional Rendering for NSEC Field:
Improved Design:
Enhanced Logic & Error Handling:
Styling Consistency:
Screenshots
Before:
NSEC input and "Generate" button always visible:
After:
Streamlined layout with conditional rendering and better styling:
Linked Issue
Resolves #59.
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation