feat: Add @encrypted enhancer
#1922
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Sequence DiagramsequenceDiagram
participant Client
participant ZenStack
participant Database
Client->>ZenStack: Write data with @encrypted field
ZenStack->>ZenStack: Encrypt field value
ZenStack->>Database: Store encrypted value
Client->>ZenStack: Read data
ZenStack->>Database: Retrieve encrypted value
ZenStack->>ZenStack: Decrypt field value
ZenStack->>Client: Return decrypted data
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
CodeRabbit Configuration File (
|
| * | ||
| * ZenStack uses the Web Crypto API to encrypt and decrypt the field. | ||
| */ | ||
| attribute @encrypted(secret: String) @@@targetField([StringField]) |
There was a problem hiding this comment.
@ymc9 Is it currently possible to use a ENV var for setting the secret used in the encryption?
Or is there a better way to pass a secret? This shouldn't be hardcoded in the schema
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -0,0 +1,159 @@ | |||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | |||
There was a problem hiding this comment.
Since edge and node implementations are identical, this file can be a symlink
| const key = await getKey(this.options.encryption!.encryptionKey); | ||
|
|
||
| // Convert base64 back to bytes | ||
| const bytes = Uint8Array.from(atob(data), (c) => c.charCodeAt(0)); |
There was a problem hiding this comment.
Is it the same as const bytes = encoder.encode(atob(data))?
| decrypt: (model: string, field: FieldInfo, cipher: string) => Promise<string>; | ||
| }; | ||
|
|
||
| export type SimpleEncryption = { encryptionKey: string }; |
There was a problem hiding this comment.
Since we don't need to represent the key in plain text anywhere, is it better to define the key as a UInt8Array instead? So that we don't need to imply any encoding form or limit it to be a utf-8 string.
There was a problem hiding this comment.
If we do so, we can also validate if the given key is 32-byte long.
|
|
||
| private async preprocessWritePayload(model: string, action: PrismaWriteActionType, args: any) { | ||
| const visitor = new NestedWriteVisitor(this.options.modelMeta, { | ||
| field: async (field, _action, data, context) => { |
There was a problem hiding this comment.
Shall we add a nullish check to data here?
| // and can break validation rules like `@length` | ||
| if (hasPassword && kinds.includes('password')) { | ||
| // @password proxy | ||
| result = withPassword(result, options); | ||
| } | ||
|
|
||
| if (hasEncrypted && kinds.includes('encrypted')) { | ||
| if (!options.encryption) { |
There was a problem hiding this comment.
Shall we validate the shape of options.encryption? Here or inside the EncryptedHandler constructor.
|
|
||
| const shouldDecrypt = fieldInfo.attributes?.find((attr) => attr.name === '@encrypted'); | ||
| if (shouldDecrypt) { | ||
| entityData[field] = await this.decrypt(fieldInfo, entityData[field]); |
There was a problem hiding this comment.
If decryption fails, should we return the original cipher text? I'm thinking this will allow easier adoption: the @encrypted attribute can be added and deployed and then a background script is run to migrate the existing plain-text data.
ymc9
changed the title
@encrypted enhancer@encrypted enhancer
![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
🧹 Nitpick comments (6)
packages/runtime/src/enhancements/node/encrypted.ts (2)
57-80: Consider handling encryption errors more gracefully.
If an error occurs in the “await crypto.subtle.encrypt(...)” call (e.g., invalid key length), the caller receives a raw error. Depending on your architecture, you might consider catching and throwing a more descriptive error.
107-107: Use optional chaining for readability.
Static analysis suggests changing “if (args && args.data && actionsOfInterest.includes(action))” to optional chaining:- if (args && args.data && actionsOfInterest.includes(action)) { + if (args?.data && actionsOfInterest.includes(action)) {🧰 Tools
🪛 Biome (1.9.4)
[error] 107-107: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/runtime/src/enhancements/edge/encrypted.ts (2)
17-32: Identical logic to node environment is acceptable.
If the edge environment is truly the same, you might symlink as suggested before. Otherwise, duplicating logic is fine if separate platform-specific changes are expected.
107-107: Consider optional chaining for args.
Similar to the node implementation, adopting optional chaining is a minor readability improvement.- if (args && args.data && actionsOfInterest.includes(action)) { + if (args?.data && actionsOfInterest.includes(action)) {🧰 Tools
🪛 Biome (1.9.4)
[error] 107-107: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/runtime/src/types.ts (1)
181-181: Brief note: immutable encryption key.
When using a simple string-based key, confirm that future expansions won’t require key rotation logic or length checks.packages/schema/src/res/stdlib.zmodel (1)
556-562: Allow configuring encryption secrets.Currently, the new @Encrypted() attribute does not provide a mechanism for specifying an encryption secret (or mentioning how it is fetched). Storing secrets directly in code or schema is generally discouraged. Consider adding a parameter (e.g., secret: String?) to allow retrieving the key from an environment variable using env("KEY"), or any other configuration system, to enable safer key management.
Would you like me to propose a code snippet that extends @Encrypted() to accept a secret parameter?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/runtime/src/enhancements/edge/encrypted.ts(1 hunks)packages/runtime/src/enhancements/node/create-enhancement.ts(3 hunks)packages/runtime/src/enhancements/node/encrypted.ts(1 hunks)packages/runtime/src/types.ts(4 hunks)packages/schema/src/res/stdlib.zmodel(1 hunks)tests/integration/tests/enhancements/with-encrypted/with-encrypted.test.ts(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
tests/integration/tests/enhancements/with-encrypted/with-encrypted.test.ts
26-26: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Biome (1.9.4)
packages/runtime/src/enhancements/edge/encrypted.ts
[error] 107-107: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/runtime/src/enhancements/node/encrypted.ts
[error] 107-107: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (15)
packages/runtime/src/enhancements/node/encrypted.ts (4)
17-32: Looks good: Properly wires encryption enhancement into the Prisma proxy.
No issues found in this block.
53-55: Explicit check for custom encryption.
This type guard cleanly distinguishes between custom and simple encryption. Nicely done.
81-102: Consider adding a fallback strategy if decryption fails.
Should decryption fail (e.g., corrupted or invalid cipher), returning the original cipher text might simplify adoption, matching one of your colleague’s earlier comments.
145-145: Add a nullish check to data before encrypting.
If “data” is null or undefined, encryption calls will fail. A quick check prevents runtime errors, echoing a past suggestion.
packages/runtime/src/enhancements/edge/encrypted.ts (2)
53-55: Good check for custom vs. simple encryption.
This type guard remains consistent with the node version, ensuring a parallel architecture.
81-102: Offer a fallback for decryption step.
When an invalid cipher string is encountered, consider reusing logic from the node version about fallback to the original string.
tests/integration/tests/enhancements/with-encrypted/with-encrypted.test.ts (2)
47-50: Great test verifying the encryption boundary.
Verifies that the stored value is encrypted for unauthorized read attempts yet decrypted for legitimate lookups.
98-101: Custom encryption logic test is well-structured.
This scenario effectively demonstrates custom encryption features. Nicely done.
packages/runtime/src/types.ts (3)
138-141: Encryption property inclusion is coherent.
The optional encryption field has a clear definition, making it straightforward to integrate.
154-154: Including 'encrypted' in EnhancementKind is consistent.
This ensures the new enhancement is recognized when building enhancements for the Prisma client.
176-180: Validate if a Uint8Array-based key is more suitable.
A prior comment suggested using a typed array instead of string. If you need stronger key validation, consider adopting that approach.
packages/runtime/src/enhancements/node/create-enhancement.ts (4)
17-17: Import for withEncrypted is correct.
Cleanly pulls in the new encryption functionality.
24-24: Adding 'encrypted' to ALL_ENHANCEMENTS is correct.
This ensures the new enhancement is part of the default pipeline.
104-104: Efficient detection of @Encrypted usage.
Simple check for fields with the “@Encrypted” attribute. Nothing to add.
132-139: Mandatory encryption options for @Encrypted.
Throwing an error if missing is appropriate, preventing incomplete configurations.
| }`); | ||
|
|
||
| const sudoDb = enhance(undefined, { kinds: [] }); | ||
| const db = enhance(undefined, { encryption: { encryptionKey: 'c558Gq0YQK2QcqtkMF9BGXHCQn4dMF8w' } }); |
There was a problem hiding this comment.
Avoid committing encryption keys in code.
A static scanner flagged “c558Gq0YQK2QcqtkMF9BGXHCQn4dMF8w” as a potential key. Consider referencing it via environment variables to avoid security risks.
🧰 Tools
🪛 Gitleaks (8.21.2)
26-26: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Resolves #1643