Adds support for encrypted license keys #335
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce cryptographic signature verification for license key payloads. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant Crypto
participant FileSystem
User->>WebApp: Submit license key
WebApp->>WebApp: Decode and parse payload (includes signature)
WebApp->>FileSystem: Read public key from path (env variable)
WebApp->>Crypto: Call verifySignature(data, signature, publicKeyPath)
Crypto->>FileSystem: Check and read public key file
Crypto-->>WebApp: Return verification result
alt Signature valid
WebApp-->>User: Accept license and display info
else Signature invalid or missing
WebApp-->>User: Reject license and log error
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public.pemis excluded by!**/*.pem
📒 Files selected for processing (6)
.env.development(1 hunks)Dockerfile(1 hunks)packages/crypto/src/index.ts(2 hunks)packages/web/src/app/[domain]/settings/license/page.tsx(1 hunks)packages/web/src/env.mjs(1 hunks)packages/web/src/features/entitlements/server.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (8)
packages/crypto/src/index.ts (1)
2-2: LGTM: Import added for file system operations.The
fsimport is necessary for reading the public key file in the newverifySignaturefunction.packages/web/src/env.mjs (1)
58-58: LGTM: Environment variable properly added to schema.The addition of
SOURCEBOT_PUBLIC_KEY_PATHas an optional string in the server schema is correctly implemented and appropriately placed.Dockerfile (2)
177-177: LGTM: Environment variable correctly configured for container.The
SOURCEBOT_PUBLIC_KEY_PATHenvironment variable is properly set to match the file location in the container.
185-185:✅ Verification successful
Verify public key file availability during Docker build.
The
public.pemfile is correctly added to the COPY command. Ensure that this file exists in the build context when building the Docker image.Check if the public key file exists in the project root for Docker builds:
🏁 Script executed:
#!/bin/bash # Description: Verify that public.pem exists for Docker builds # Expected: File should exist in build context or build will fail if [ -f "public.pem" ]; then echo "✅ public.pem found in build context" ls -la public.pem else echo "❌ public.pem not found in build context - Docker build will fail" echo "Files in root directory:" ls -la | head -10 fi # Also check if there are any .pem files that might be the intended file echo "Other .pem files in directory:" find . -maxdepth 2 -name "*.pem" 2>/dev/null || echo "No .pem files found"Length of output: 302
public.pem found in build context – no further action required
The verification confirms that
public.pemexists at the project root and will be successfully included by theCOPYinstruction in the Dockerfile. No additional changes needed.packages/web/src/app/[domain]/settings/license/page.tsx (1)
112-119: LGTM! Enhanced timestamp display improves user experience.The change from
toLocaleDateStringtotoLocaleStringwith timezone information provides users with more precise license expiry details, which is beneficial for license management.packages/web/src/features/entitlements/server.ts (3)
7-7: LGTM! Proper import of cryptographic verification function.The import of
verifySignaturefrom the crypto package aligns with the PR objective of adding encrypted license key support.
19-19: LGTM! Schema properly extended for signature verification.Adding the required
sigfield to the schema ensures that license keys must include a signature for validation.
31-35:⚠️ Potential issueCritical: JSON.stringify property order is not guaranteed.
The signature verification creates a JSON string for verification, but
JSON.stringifydoesn't guarantee property order across different JavaScript engines or versions. This could cause valid signatures to fail verification.Use a deterministic serialization approach:
- const dataToVerify = JSON.stringify({ - expiryDate: licenseData.expiryDate, - id: licenseData.id, - seats: licenseData.seats - }); + // Ensure deterministic serialization by manually constructing the string + const dataToVerify = `{"expiryDate":"${licenseData.expiryDate}","id":"${licenseData.id}","seats":${licenseData.seats}}`;Or consider using a deterministic JSON serialization library like
canonical-jsonorfast-json-stable-stringify.Likely an incorrect or invalid review comment.
|
Could you add a changelog entry? |
Summary by CodeRabbit
New Features
Improvements
Chores