Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Encrypt user form data on client side #380

Merged
merged 12 commits into from
Feb 8, 2025

Conversation

atrakh
Copy link
Contributor

@atrakh atrakh commented Feb 7, 2025

What changed?

  • Adds a set of encryption methods to encrypt user data before it is sent to the database (Convex)
  • Adds a placeholder UI on the /settings/data page to help demonstrate how the feature works. The forms in the UI are basic and should be updated in the future to properly support intended use cases.
  • Adds a list query to retrieve all userFormData for rendering

Why?

Users of namesake should have there data protected -- external parties and operators of namesake should have a minimal amount of PII (Personally Identifiable Information) available to them

How was this change made?

This PR implements a client-side encryption key, stored in IndexedDB in the browser, that is used to encrypt and decrypt data between the client and server.

I initially experimented with a two-key system (implementing a Key Encrypting Key), but found that implementation complicated and likely to not be completely secure.

This simpler implementation is more streamlined, reducing the risk of vulnerability. However, there are some things to consider regarding security:

  • To reduce the risk of XSS or other attacks where browser storage can be compromised, KEK could be implemented with a non-extractable key
  • Other XSS prevention measure should exist in the application and be tested before 1.0

This implementation does not currently implement convenient portability of the key. If the user logs in from another device, a new key will be generated and data will not load. I recommend we merge this PR as a proof-of-concept and add the portability feature (e.g. allowing the user to download their key and re-enter it later) as a follow-up.

How was this tested?

I added a basic test-suite to test round-trips -- encryption methods and IndexedDB are mocked. I also used the placeholder UI to test manually and confirm encrypted data is stored in Convex.

Anything else?

Migrations! If, after launch, the key-generation / encryption patterns are changed in the future, we'll need to maintain the existing implementation and migrate the database to indicate which implementation was used to encrypt the data for backwards compatability

Copy link

changeset-bot bot commented Feb 7, 2025

🦋 Changeset detected

Latest commit: 8ad679d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
namesake Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added frontend CSS, HTML, and JS changes backend Schema, query, and mutation changes testing Unit and e2e tests labels Feb 7, 2025
@atrakh atrakh changed the title Encrypt form data on client side feat: encrypt uesr form data on client side Feb 7, 2025
@atrakh atrakh changed the title feat: encrypt uesr form data on client side feat: encrypt user form data on client side Feb 7, 2025
@evadecker
Copy link
Member

This is great! Thank you so much for this extremely helpful contribution. It goes a long way toward getting Namesake ready for launch. And thank you for the detailed and thoughtful PR comment; it helps a lot in understanding what's going on and limitations to follow up on.

I'm filing a few issues here which address issues you mention in your PR comment:

Appreciate if you have any thoughts or color to add to either of those tickets! Feel free to comment with extra context or opinions.

For this PR:

  1. Can you add a changeset to this PR which explains the nature of the changes, written in user-facing language? pnpm changesetminor → enter explanation
  2. The test failure is from Biome, and should be automatically fixable if you run pnpm check:fix
  3. If the Data Encryption Key is deleted manually, or unavailable, we get infinite loaders. Could we present a basic user-facing error banner if decryption fails?
CleanShot.2025-02-06.at.22.26.01.mp4

Copy link
Member

@evadecker evadecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small comments and questions, but otherwise very excited to get this merged!

src/components/settings/UserDataForm/UserDataForm.tsx Outdated Show resolved Hide resolved
src/components/settings/UserDataForm/UserDataForm.tsx Outdated Show resolved Hide resolved
src/utils/encryption.test.ts Outdated Show resolved Hide resolved
@atrakh atrakh requested a review from evadecker February 7, 2025 21:46
@atrakh
Copy link
Contributor Author

atrakh commented Feb 7, 2025

Already! pushed up changes to address the review, apart from a couple comments where the explanation warrants keeping the code as is (IMO - let me know if you think we should still make changes)

Here's a screenshot of the basic error handling for failure to decrypt. This should be made better when portable keys are nted

Screenshot 2025-02-07 at 1 45 59 PM

Copy link
Member

@evadecker evadecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks so much for your contribution, @atrakh!

@evadecker evadecker changed the title feat: encrypt user form data on client side feat: Encrypt user form data on client side Feb 8, 2025
@evadecker evadecker merged commit 0882e88 into namesakefyi:main Feb 8, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Schema, query, and mutation changes frontend CSS, HTML, and JS changes testing Unit and e2e tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants