← Back to PRs

#19543: security: add encrypted localStorage wrapper using Web Crypto API

by Mozzzaic open 2026-02-17 23:04 View on GitHub →
app: web-ui size: M
## Summary - Add `secure-storage.ts` module with AES-GCM encryption for localStorage - PBKDF2 key derivation from origin + user-agent fingerprint - Random IV per encryption (no ciphertext reuse) - Graceful migration: unencrypted values are returned as-is and re-encrypted on next write - API: `secureSet(key, value)`, `secureGet(key)`, `secureRemove(key)` ## Security Impact Provides encryption layer for sensitive data (private keys, auth tokens) stored in browser localStorage. Mitigates exposure via DevTools or XSS attacks. ## Test plan - [x] Encrypt/decrypt round-trip - [x] enc: prefix verification - [x] Graceful migration from unencrypted values - [x] Unicode support - [x] Random IV produces different ciphertexts <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR introduces `secure-storage.ts`, an AES-GCM encrypted wrapper for `localStorage` intended to protect sensitive values like private keys and auth tokens. The cryptographic implementation has several serious issues that prevent it from delivering its stated security guarantees. **Critical issues:** - **Key material is fully public:** The PBKDF2 "password" is `location.origin + navigator.userAgent`—both are observable in every HTTP request and fully reconstructable by any attacker who can read `localStorage`. Because no secret is involved, the encryption provides no confidentiality. An attacker with XSS or DevTools access can derive the exact key in one PBKDF2 call. - **User-agent in key material causes silent data loss:** `navigator.userAgent` changes silently on browser updates, OS updates, or across devices. After any such change, a different key is derived, all previously encrypted values become undecryptable, and `secureGet` returns the raw `enc:…` ciphertext blob to callers without error—silently corrupting any credentials or tokens stored here. - **`secureGet` catch block returns ciphertext as a migration value:** The catch in `secureGet` is only reached when a value with the `enc:` prefix fails to decrypt (the legitimate plaintext-migration path is handled inside `decrypt()` itself without throwing). When decryption fails due to key mismatch or tampering, callers silently receive the raw encrypted string as if it were valid data. - **`btoa(String.fromCharCode(...new Uint8Array(ciphertext)))` throws for values over ~48 KB:** Spreading all ciphertext bytes as separate function arguments exceeds the JS engine argument limit, producing a hard `RangeError`. **Additional issues:** - Module-level `cachedKey` is never reset between tests; the test suite passes coincidentally by reusing a stale key rather than exercising real cross-load behaviour. - The `enc:` prefix is not cryptographically bound to the ciphertext (not included as AES-GCM AAD), allowing prefix-stripping or prefix-injection attacks. <h3>Confidence Score: 1/5</h3> - Not safe to merge: the encryption scheme provides no real confidentiality, and using user-agent as key material will silently destroy stored data on browser updates. - Two independent critical issues each independently disqualify this PR: (1) the "password" fed to PBKDF2 is fully public so the scheme is effectively obfuscation rather than encryption, and (2) user-agent in key material is a silent data-loss time bomb triggered by routine browser updates. The `secureGet` fallback additionally hides failures from callers, and the `btoa` spread pattern will crash on any non-trivial payload. The module is not yet imported by any other code, so merging it now would ship broken security infrastructure that other PRs could build on. - Both files need attention: `secure-storage.ts` has fundamental cryptographic design issues, and `secure-storage.test.ts` does not cover the failure modes that would catch them. <sub>Last reviewed commit: 3759d64</sub> <!-- greptile_other_comments_section --> <!-- /greptile_comment -->

Most Similar PRs