feat: migrate to new core #297
Conversation
…ddresses, saving apikey in secure storage
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @ignaciosantise's task in 1m 44s —— View job Code Review CompleteReviewed PR #297: Migration to new merchant authentication system Issue 1: Cryptographically weak PIN hashing algorithmID: usesettingsst-weak-pin-hash-a7b3 The PIN hashing function uses a simple bitwise hash that is not cryptographically secure. This custom hash function can be easily reversed or brute-forced, undermining the security of PIN protection for merchant credentials. function hashPin(pin: string): string {
let hash = 0;
for (let i = 0; i < pin.length; i++) {
const char = pin.charCodeAt(i);
hash = (hash << 5) - hash + char;
hash = hash & hash;
}
return hash.toString(16);
}Recommendation: Use a proper cryptographic hash function like SHA-256 or bcrypt for PIN hashing: import { Crypto } from 'expo-crypto';
async function hashPin(pin: string): Promise<string> {
return await Crypto.digestStringAsync(
Crypto.CryptoDigestAlgorithm.SHA256,
pin
);
}Exploit Scenario: An attacker with access to the device's storage could extract the PIN hash and reverse-engineer the 4-digit PIN through brute force or rainbow tables, gaining unauthorized access to merchant credentials. Issue 2: Insecure web fallback storage for sensitive credentialsID: securestorag-localstorage-fallback-c4d2 The web implementation stores sensitive data (API keys, PIN hashes) in browser localStorage, which is not encrypted and accessible to any JavaScript running on the page. This completely negates the security benefits of using secure storage on native platforms. Recommendation: For web platform, implement proper security measures: // Option 1: Warn users that web platform is not secure for sensitive data
if (isBrowser && key === SECURE_STORAGE_KEYS.MERCHANT_API_KEY) {
console.warn('Web platform does not support secure credential storage');
}
// Option 2: Use Web Crypto API for encryption before localStorage
// Option 3: Disable merchant API key storage on web entirelyExploit Scenario: On web platform, XSS attacks or browser extensions can read merchant API keys and PIN hashes directly from localStorage, bypassing all security measures. Issue 3: Missing API key validation in header constructionID: payment-missing-apikey-validation-f8a1 The Recommendation: Add proper validation: if (!merchantApiKey || merchantApiKey.trim().length === 0) {
throw new Error("Merchant API key is not configured");
}Issue 4: Async hash function not awaited in PIN verificationID: usesettingsst-async-hash-timing-2b9f If the PIN hashing is changed to use async cryptographic functions (as recommended), the current synchronous Recommendation: When implementing crypto hashing, ensure all hash calls are properly awaited: const hashedPin = await hashPin(pin);
const isValid = storedPinHash !== null && hashedPin === storedPinHash;Issue 5: Hard-coded SDK version in API headersID: payment-hardcoded-sdk-version-3e7c SDK version "1.0.0" is hard-coded in API headers, requiring code changes for version updates. Recommendation: Import version from package.json or app config: import { version } from '../../package.json';
// In headers:
"Sdk-Version": version,Issue 6: Missing input sanitization for merchant IDID: settings-merchant-id-no-sanitization-9a2c Merchant ID input is trimmed but not sanitized. Special characters or injection attempts are not validated. Recommendation: Add input validation pattern: const MERCHANT_ID_PATTERN = /^[a-zA-Z0-9_-]+$/;
if (!MERCHANT_ID_PATTERN.test(trimmedMerchantId)) {
showErrorToast("Merchant ID contains invalid characters");
return;
}Issue 7: Clipboard operation without error handlingID: scan-clipboard-no-error-handling-5d1f The QR code copy-to-clipboard operation doesn't handle potential failures. Recommendation: Add try-catch for clipboard operations: const handleCopyPaymentUrl = async () => {
try {
await Clipboard.setStringAsync(qrUri);
showSuccessToast("Payment URL copied");
} catch (error) {
showErrorToast("Failed to copy to clipboard");
}
};Automated Checks✅ No workflow security issues detected Summary7 issues found: 2 HIGH severity (weak PIN hashing, insecure web storage), 2 MEDIUM severity (missing validation, async timing), 3 LOW severity. Critical security concerns with PIN hashing and web storage fallback need immediate attention before merchant credentials are stored in production. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| </ThemedText> | ||
| <View style={styles.merchantInputRow}> | ||
| <TextInput | ||
| value={merchantIdInput} |
There was a problem hiding this comment.
🤖 Auto Review Issue: Missing input sanitization for merchant ID
Severity: LOW
Category: security
Tool: Claude Auto Review
Recommendation: Add input validation pattern:
const MERCHANT_ID_PATTERN = /^[a-zA-Z0-9_-]+$/;
if (!MERCHANT_ID_PATTERN.test(trimmedMerchantId)) {
showErrorToast("Merchant ID contains invalid characters");
return;
}| }; | ||
|
|
||
| const handleCopyPaymentUrl = async () => { | ||
| await Clipboard.setStringAsync(qrUri); |
There was a problem hiding this comment.
🤖 Auto Review Issue: Clipboard operation without error handling
Severity: LOW
Category: code_quality
Tool: Claude Auto Review
Recommendation: Add try-catch for clipboard operations:
const handleCopyPaymentUrl = async () => {
try {
await Clipboard.setStringAsync(qrUri);
showSuccessToast("Payment URL copied");
} catch (error) {
showErrorToast("Failed to copy to clipboard");
}
};There was a problem hiding this comment.
Pull request overview
This PR migrates the POS app to a new merchant authentication system, transitioning from public payment endpoints to authenticated merchant-specific APIs. The refactor introduces secure credential storage, simplifies the settings UI by removing merchant address verification, and enhances the payment flow with new API request formats.
Key Changes:
- Merchant authentication now requires both a merchant ID and API key, stored securely via expo-secure-store with credentials passed as headers to new
/merchant/paymentendpoints - Settings UI redesigned with separate inputs for merchant ID and API key, removing the previous merchant address lookup and confirmation flow
- PIN and API key credentials migrated from Zustand storage to platform-specific secure storage (expo-secure-store for native, localStorage for web)
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
dapps/pos-app/utils/types.ts |
Updated payment request structure to use referenceId and structured amount object with unit |
dapps/pos-app/utils/secure-storage.web.ts |
Added web implementation of secure storage using localStorage with prefix |
dapps/pos-app/utils/secure-storage.ts |
Added native implementation using expo-secure-store for encrypted credential storage |
dapps/pos-app/store/useSettingsStore.ts |
Refactored to move PIN and API key to secure storage; added migration logic from v6 to v8 |
dapps/pos-app/package.json |
Added expo-secure-store dependency |
dapps/pos-app/package-lock.json |
Lockfile updates for expo-secure-store and type dependencies |
dapps/pos-app/hooks/use-merchant-flow.ts |
Completely refactored to handle separate merchant ID and API key flows with PIN protection |
dapps/pos-app/hooks/use-biometric-auth.ts |
Updated to use async isPinSet method from settings store |
dapps/pos-app/components/qr-code.tsx |
Made QR code pressable with optional onPress handler |
dapps/pos-app/components/pin-modal.tsx |
Minor cleanup of imports and biometric icon styling |
dapps/pos-app/components/merchant-confirm-modal.tsx |
Removed unused merchant confirmation modal component |
dapps/pos-app/components/merchant-address-row.tsx |
Removed unused merchant address display component |
dapps/pos-app/app/settings.tsx |
Redesigned with separate merchant ID and API key sections, removed address verification |
dapps/pos-app/app/scan.tsx |
Updated to use new payment request format and added QR code copy-to-clipboard feature |
dapps/pos-app/app.json |
Added expo-secure-store configuration with biometric permissions |
dapps/pos-app/api/payment.ts |
Added merchant authentication headers function and updated endpoints to /merchant/payment routes |
Files not reviewed (1)
- dapps/pos-app/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@claude review |
|
Claude finished @ignaciosantise's task in 2m 1s —— View job Code Review CompleteReviewed PR #297: Migration to new merchant authentication system Issues FoundIssue 1: Cryptographically weak PIN hashing algorithmID: usesettingsst-weak-pin-hash-a7b3 The PIN hashing function (line 10-18) uses a simple bitwise hash ( While the comment acknowledges "not cryptographically secure, but sufficient for local protection", the PIN protects access to merchant API keys stored in secure storage. An attacker with device access could extract the PIN hash and reverse-engineer the 4-digit PIN through brute force. Recommendation: Use a proper cryptographic hash function: import * as Crypto from 'expo-crypto';
async function hashPin(pin: string): Promise<string> {
return await Crypto.digestStringAsync(
Crypto.CryptoDigestAlgorithm.SHA256,
pin
);
}Note: This requires updating all Exploit Scenario: An attacker with access to device storage extracts the PIN hash from secure storage and uses a rainbow table or brute force to recover the 4-digit PIN, gaining unauthorized access to merchant credentials. Issue 2: Sensitive data logged in console outputID: securestorag-console-log-sensitive-8f2a Error handlers in secure storage log errors to console (lines 13, 22, 31), which may expose sensitive data in error messages or stack traces. This is also flagged by GitHub Advanced Security (code-scanning alerts #22, #23, #24). Recommendation: Remove or sanitize console.error calls: console.error(`Error getting secure item for key type`);
// Don't log the actual error object which may contain sensitive dataIssue 3: Missing error handling for secure storage operations in settings storeID: usesettingsst-setapikey-no-errorhandling-9c4f The Copilot reviewer also flagged this at line 89. Recommendation: Add try-catch with user feedback: setMerchantApiKey: async (apiKey: string | null) => {
try {
if (apiKey) {
await secureStorage.setItem(
SECURE_STORAGE_KEYS.MERCHANT_API_KEY,
apiKey,
);
} else {
await secureStorage.removeItem(SECURE_STORAGE_KEYS.MERCHANT_API_KEY);
}
} catch (error) {
console.error("Failed to store merchant API key");
throw new Error("Failed to save credentials securely");
}
},Issue 4: Hard-coded SDK version in API headersID: payment-hardcoded-sdk-version-3e7c SDK version "1.0.0" is hard-coded, requiring code changes for version updates. Recommendation: Import from package.json: import packageJson from '../../package.json';
// In headers:
"Sdk-Version": packageJson.version,Issue 5: Inconsistent prefix usage in web secure storage fallbackID: securestorag-prefix-inconsistency-2d8b The fallback storage (lines 41, 55, 70) uses prefixed keys via Recommendation: Ensure consistent key usage across both storage mechanisms. Response to Previous ReviewsValidating issues from previous reviews: GitHub Advanced Security (Clear-text logging): ✅ Confirmed - Issue #2 above addresses this Copilot Review:
Previous Claude Review:
Automated Checks✅ No workflow security issues detected (no changes to .github/workflows/) Summary5 issues found: 1 HIGH severity (weak PIN hashing), 2 MEDIUM severity (console logging sensitive data, missing error handling), 2 LOW severity (hard-coded version, prefix inconsistency). Key Security Concern: The weak PIN hashing algorithm (Issue #1) is the most critical finding. While the author dismissed this in a previous review, it represents a genuine security vulnerability that allows PIN recovery through brute force. The 4-digit PIN space (10,000 combinations) can be exhausted in milliseconds with the current hash function. Web Platform Note: The author appropriately added a security warning to the web storage implementation, noting it's for development/testing only. Author Response Context: The author has addressed several previous review comments and provided clarifications on design decisions (UUID formatting, feature deferrals). Some security recommendations were dismissed but warrant reconsideration. |
| try { | ||
| return await SecureStore.getItemAsync(key); | ||
| } catch (error) { | ||
| console.error(`Error getting secure item`, error); |
There was a problem hiding this comment.
🤖 Auto Review Issue: Sensitive data logged in console output
Severity: MEDIUM
Category: security
Tool: Claude Auto Review
Recommendation: Remove or sanitize console.error calls:
console.error(`Error getting secure item for key type`);
// Don't log the actual error object which may contain sensitive data| const value = storage.getItem(prefixedKey); | ||
| return value ?? null; | ||
| } | ||
| return fallbackStorage.get(prefixedKey) ?? null; |
There was a problem hiding this comment.
🤖 Auto Review Issue: Inconsistent prefix usage in web secure storage fallback
Severity: LOW
Category: code_quality
Tool: Claude Auto Review
Recommendation: Ensure consistent key usage across both storage mechanisms.
This pull request introduces significant improvements to the merchant authentication and payment flow in the POS app, refactoring how merchant credentials are managed and simplifying the settings UI. It also enhances the payment initiation and status APIs for merchant authentication, removes redundant merchant confirmation components, and adds a convenient QR code copy feature for payment URLs.
Merchant Authentication & API Integration
payment.tsto require merchant-specific headers, including API key and merchant ID, retrieved securely viaexpo-secure-store. The payment endpoints now use/merchant/paymentroutes, improving security and traceability. [1] [2] [3]Settings UI & Merchant Credential Management
MerchantAddressRowandMerchantConfirmModalcomponents, cleaning up unused code. [1] [2]Payment Initiation & QR Code Improvements
/merchant/paymentendpoint. Added a feature to copy the payment URL to the clipboard by tapping the QR code, with a success toast confirmation. [1] [2] [3]Secure Storage Integration
expo-secure-storetoapp.jsonto support secure credential storage and biometric authentication.Minor Codebase Improvements