feat: extract crypto helpers and add comprehensive AES-256-GCM test s…#79
Conversation
|
@trivikramkalagi91-commits is attempting to deploy a commit to the Deekshith Gowda HS's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
More reviews will be available in 53 minutes and 5 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR refactors inline AES-256-GCM crypto logic into a dedicated ChangesCrypto Helpers Extraction and Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
hi @deekshithgowda85 i am a gssoc contributor pls review the PR i tried it to understand the issue and worked on it using AI. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
__tests__/crypto.test.ts (1)
46-59: ⚡ Quick winStrengthen format contract assertions for IV/tag lengths.
Line 46-59 currently checks “hex-like and non-empty” only. Add exact length checks (IV=24 hex chars, tag=32 hex chars) to lock the AES-256-GCM contract from
lib/crypto.ts.Suggested test hardening
const [iv, tag, enc] = parts; expect(iv.length).toBeGreaterThan(0); expect(tag.length).toBeGreaterThan(0); expect(enc.length).toBeGreaterThan(0); + expect(iv).toHaveLength(24); // 12-byte IV in hex + expect(tag).toHaveLength(32); // 16-byte GCM auth tag in hex🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/crypto.test.ts` around lines 46 - 59, The test should assert exact hex-lengths to lock the AES-256-GCM contract: after calling encrypt(...) and splitting the ciphertext into parts, add assertions that the IV hex string length equals 24 and the tag hex string length equals 32 (leave the encrypted payload check as non-empty) — update the test that references encrypt to replace the current non-empty IV/tag checks with exact length assertions while keeping the existing hex-regex validations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jest.config.js`:
- Around line 1-10: Replace the CommonJS require by using ES module
imports/exports: import createDefaultPreset from "ts-jest" (or import {
createDefaultPreset } from "ts-jest" depending on the package export) and
replace module.exports with export default, then keep the existing usage of
createDefaultPreset() and tsJestTransformCfg (the transform assignment)
unchanged so the config still sets testEnvironment and transform; update the
file to use ESM syntax so createDefaultPreset, tsJestTransformCfg and the
exported Jest config no longer use require().
In `@lib/crypto.ts`:
- Around line 3-5: Replace the insecure fallback in getKey so the app fails
closed: in the getKey function check process.env.NEXTAUTH_SECRET (referencing
getKey and the createHash call) and if it's undefined/empty throw a clear error
(e.g., "NEXTAUTH_SECRET must be set") instead of using
"default-dev-secret-32-chars-long"; when present, continue to derive the Buffer
via createHash("sha256").update(raw).digest(); ensure tests set NEXTAUTH_SECRET
explicitly.
- Around line 18-20: The current destructuring const [ivHex, tagHex, dataHex] =
ciphertext.split(":") allows extra segments to be ignored; change the logic so
you first assign the result to a variable (e.g., const parts =
ciphertext.split(":")) and validate parts.length === 3 before destructuring or
using parts[0..2]; if the length is not exactly 3, throw the existing "Invalid
ciphertext format" error. Update the code path around the ivHex/tagHex/dataHex
validation to use parts to ensure tampered payloads like
`${validCiphertext}:junk` are rejected.
---
Nitpick comments:
In `@__tests__/crypto.test.ts`:
- Around line 46-59: The test should assert exact hex-lengths to lock the
AES-256-GCM contract: after calling encrypt(...) and splitting the ciphertext
into parts, add assertions that the IV hex string length equals 24 and the tag
hex string length equals 32 (leave the encrypted payload check as non-empty) —
update the test that references encrypt to replace the current non-empty IV/tag
checks with exact length assertions while keeping the existing hex-regex
validations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31e65a9c-74bb-4cd5-8596-b3830ef1786c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
__tests__/crypto.test.tsjest.config.jslib/crypto.tslib/env-store.tspackage.json
| const { createDefaultPreset } = require("ts-jest"); | ||
|
|
||
| const tsJestTransformCfg = createDefaultPreset().transform; | ||
|
|
||
| /** @type {import("jest").Config} **/ | ||
| module.exports = { | ||
| testEnvironment: "node", | ||
| transform: { | ||
| ...tsJestTransformCfg, | ||
| }, |
There was a problem hiding this comment.
Replace require usage to unblock lint CI.
Line 1 violates the enforced ESLint rule and is currently breaking the pipeline. Switch to a ts-jest preset config that does not require require().
Suggested fix
-const { createDefaultPreset } = require("ts-jest");
-
-const tsJestTransformCfg = createDefaultPreset().transform;
-
/** `@type` {import("jest").Config} **/
module.exports = {
+ preset: "ts-jest",
testEnvironment: "node",
- transform: {
- ...tsJestTransformCfg,
- },
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { createDefaultPreset } = require("ts-jest"); | |
| const tsJestTransformCfg = createDefaultPreset().transform; | |
| /** @type {import("jest").Config} **/ | |
| module.exports = { | |
| testEnvironment: "node", | |
| transform: { | |
| ...tsJestTransformCfg, | |
| }, | |
| /** `@type` {import("jest").Config} **/ | |
| module.exports = { | |
| preset: "ts-jest", | |
| testEnvironment: "node", | |
| }; |
🧰 Tools
🪛 ESLint
[error] 1-1: A require() style import is forbidden.
(@typescript-eslint/no-require-imports)
🪛 GitHub Actions: CI / 0_Lint and Build.txt
[error] 1-1: ESLint error: A require() style import is forbidden. (@typescript-eslint/no-require-imports)
🪛 GitHub Actions: CI / Lint and Build
[error] 1-1: ESLint (@typescript-eslint/no-require-imports): A require() style import is forbidden.
🪛 GitHub Check: Lint and Build
[failure] 1-1:
A require() style import is forbidden
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jest.config.js` around lines 1 - 10, Replace the CommonJS require by using ES
module imports/exports: import createDefaultPreset from "ts-jest" (or import {
createDefaultPreset } from "ts-jest" depending on the package export) and
replace module.exports with export default, then keep the existing usage of
createDefaultPreset() and tsJestTransformCfg (the transform assignment)
unchanged so the config still sets testEnvironment and transform; update the
file to use ESM syntax so createDefaultPreset, tsJestTransformCfg and the
exported Jest config no longer use require().
| export function getKey(): Buffer { | ||
| const raw = process.env.NEXTAUTH_SECRET ?? "default-dev-secret-32-chars-long"; | ||
| return createHash("sha256").update(raw).digest(); |
There was a problem hiding this comment.
Fail closed when NEXTAUTH_SECRET is missing.
Falling back to a hard-coded secret makes every misconfigured deployment encrypt env vars with the same known key, so DB-backed secrets are effectively recoverable. Require an explicit secret here and let tests set one deliberately.
Suggested fix
export function getKey(): Buffer {
- const raw = process.env.NEXTAUTH_SECRET ?? "default-dev-secret-32-chars-long";
+ const raw = process.env.NEXTAUTH_SECRET;
+ if (!raw) {
+ throw new Error("NEXTAUTH_SECRET must be set");
+ }
return createHash("sha256").update(raw).digest();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function getKey(): Buffer { | |
| const raw = process.env.NEXTAUTH_SECRET ?? "default-dev-secret-32-chars-long"; | |
| return createHash("sha256").update(raw).digest(); | |
| export function getKey(): Buffer { | |
| const raw = process.env.NEXTAUTH_SECRET; | |
| if (!raw) { | |
| throw new Error("NEXTAUTH_SECRET must be set"); | |
| } | |
| return createHash("sha256").update(raw).digest(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crypto.ts` around lines 3 - 5, Replace the insecure fallback in getKey so
the app fails closed: in the getKey function check process.env.NEXTAUTH_SECRET
(referencing getKey and the createHash call) and if it's undefined/empty throw a
clear error (e.g., "NEXTAUTH_SECRET must be set") instead of using
"default-dev-secret-32-chars-long"; when present, continue to derive the Buffer
via createHash("sha256").update(raw).digest(); ensure tests set NEXTAUTH_SECRET
explicitly.
| const [ivHex, tagHex, dataHex] = ciphertext.split(":"); | ||
| if (!ivHex || !tagHex || dataHex === undefined) { | ||
| throw new Error("Invalid ciphertext format"); |
There was a problem hiding this comment.
Reject ciphertexts that contain extra : segments.
split(":") + destructuring ignores any fourth segment, so a tampered payload like ${validCiphertext}:junk still decrypts successfully. Enforce exactly three parts before creating the decipher.
Suggested fix
export function decrypt(ciphertext: string): string {
- const [ivHex, tagHex, dataHex] = ciphertext.split(":");
- if (!ivHex || !tagHex || dataHex === undefined) {
+ const parts = ciphertext.split(":");
+ if (parts.length !== 3) {
+ throw new Error("Invalid ciphertext format");
+ }
+ const [ivHex, tagHex, dataHex] = parts;
+ if (!ivHex || !tagHex || dataHex === undefined) {
throw new Error("Invalid ciphertext format");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [ivHex, tagHex, dataHex] = ciphertext.split(":"); | |
| if (!ivHex || !tagHex || dataHex === undefined) { | |
| throw new Error("Invalid ciphertext format"); | |
| const parts = ciphertext.split(":"); | |
| if (parts.length !== 3) { | |
| throw new Error("Invalid ciphertext format"); | |
| } | |
| const [ivHex, tagHex, dataHex] = parts; | |
| if (!ivHex || !tagHex || dataHex === undefined) { | |
| throw new Error("Invalid ciphertext format"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crypto.ts` around lines 18 - 20, The current destructuring const [ivHex,
tagHex, dataHex] = ciphertext.split(":") allows extra segments to be ignored;
change the logic so you first assign the result to a variable (e.g., const parts
= ciphertext.split(":")) and validate parts.length === 3 before destructuring or
using parts[0..2]; if the length is not exactly 3, throw the existing "Invalid
ciphertext format" error. Update the code path around the ivHex/tagHex/dataHex
validation to use parts to ensure tampered payloads like
`${validCiphertext}:junk` are rejected.
deekshithgowda85
added
gssoc:approved
|
thank you |
2 participants
Pull Request
Summary
This PR resolves the issue by extracting the internal cryptographic utilities from
lib/env-store.tsinto a modular, dedicated file and adding a comprehensive unit testing suite.Related Issue
Closes: #21
What Changed
encrypt,decrypt, andgetKeyfunctions out oflib/env-store.tsand into a new, reusable module at lib/crypto.ts."") evaluated as falsy and caused unexpected decryption validation failures.ts-jest, and@types/jestconfigurations, and hooked up the"test": "jest"execution script inpackage.json.iv:tag:ciphertextformatting rules, unique Initialization Vector generation, and strict anti-tampering behavior.Verification
npm testand all 9 tests pass flawlessly with 100% success locally.BELOW IS THE RESULT WHEN I RAN 'npm test':
PASS tests/crypto.test.ts
Crypto Utils
Round-Trip Data Integrity
√ should successfully encrypt and decrypt a standard string (4 ms)
√ should handle empty strings (1 ms)
√ should handle long strings
√ should handle special characters and unicode
Format and Delimiter Validation
√ should return a string separated by two colons into 3 parts (2 ms)
Initialization Vector (IV) Uniqueness
√ should generate a unique ciphertext for the same plaintext
Robust Error & Tamper Handling
√ should throw an error if the input string is malformed (13 ms)
√ should throw an error if the ciphertext is modified (11 ms)
√ should throw an error if the authentication tag is altered (1 ms)
Test Suites: 1 passed, 1 total
Tests: 9 passed, 9 total
Snapshots: 0 total
Time: 0.502 s, estimated 1 s
Ran all test suites.
NOTE: hi i accept that i worked using antigravity to solve this issue and also i tried to understand what it is actually instead of simply making a PR and what i understood basuc is tht adding a test cases which can be usable everywhere for example by making a seperate tool box as lib/crypto.ts which is not fixed for only one thing now it is seperated.
Summary by CodeRabbit
Tests
Chores