fix: '@password' attribute doesn't work well with data validation #1547
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 12331553 | Triggered | Generic Password | e1e6e6b | tests/integration/tests/enhancements/with-password/with-password.test.ts | View secret |
| 12331553 | Triggered | Generic Password | e1e6e6b | tests/integration/tests/enhancements/with-password/with-password.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
WalkthroughWalkthroughThe changes primarily focus on reordering the application of password and policy enhancements to ensure proper validation, refining validation logic in policy handlers, and restructuring integration tests to include more detailed checks, especially related to password fields. Changes
Sequence Diagram(s)Password and Policy Enhancement ApplicationsequenceDiagram
participant User
participant createEnhancement
participant PolicyProxyHandler
participant validateZodSchema
User ->> createEnhancement: call function
createEnhancement ->> createEnhancement: apply password enhancement
createEnhancement ->> createEnhancement: apply policy enhancement
createEnhancement ->> PolicyProxyHandler: initiate
PolicyProxyHandler ->> validateZodSchema: validate schemas
validateZodSchema -->> PolicyProxyHandler: return result
PolicyProxyHandler -->> createEnhancement: return result
createEnhancement -->> User: return result
Assessment against linked issues
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 2
Outside diff range and nitpick comments (15)
packages/runtime/src/enhancements/policy/handler.ts (15)
Line range hint
216-225: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - // create case - pushIdFields(model, context); - - // create a new "create" clause at the parent level - this.mergeToParent(context.parent, 'create', args.create); - } + } + // create case + pushIdFields(model, context); + + // create a new "create" clause at the parent level + this.mergeToParent(context.parent, 'create', args.create);
Line range hint
230-232: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - // remove the connectOrCreate clause - this.removeFromParent(context.parent, 'connectOrCreate', args); + } + // remove the connectOrCreate clause + this.removeFromParent(context.parent, 'connectOrCreate', args);
Line range hint
389-391: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - return true; - } + } + return true;
Line range hint
452-460: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - // create case - await _create(model, args.create, context); - - // remove it from the update payload - this.removeFromParent(context.parent, 'upsert', args); - - // don't visit payload - return false; + } + // create case + await _create(model, args.create, context); + + // remove it from the update payload + this.removeFromParent(context.parent, 'upsert', args); + + // don't visit payload + return false;
Line range hint
514-516: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - return false; - } + } + return false;
Line range hint
537-542: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - // don't visit payload - return false; - } + } + // don't visit payload + return false;
Line range hint
701-703: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - return false; - } + } + return false;
Line range hint
912-916: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - // don't visit payload - return false; - } + } + // don't visit payload + return false;
Line range hint
1040-1051: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - // we have to process `updateMany` separately because the guard may contain - // filters using relation fields which are not allowed in nested `updateMany` - const reversedQuery = this.policyUtils.buildReversedQuery(context); - const updateWhere = this.policyUtils.and(reversedQuery, updateGuard); - if (this.shouldLogQuery) { - this.logger.info( - `[policy] \`updateMany\` ${model}:\n${formatObject({ - where: updateWhere, - data: args.data, - })}` - ); - } - await db[model].updateMany({ where: updateWhere, data: args.data }); - delete context.parent.updateMany; + } + // we have to process `updateMany` separately because the guard may contain + // filters using relation fields which are not allowed in nested `updateMany` + const reversedQuery = this.policyUtils.buildReversedQuery(context); + const updateWhere = this.policyUtils.and(reversedQuery, updateGuard); + if (this.shouldLogQuery) { + this.logger.info( + `[policy] \`updateMany\` ${model}:\n${formatObject({ + where: updateWhere, + data: args.data, + })}` + ); + } + await db[model].updateMany({ where: updateWhere, data: args.data }); + delete context.parent.updateMany;
Line range hint
1063-1078: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - // create case - // process the entire create subtree separately - await _create(model, args.create, context); - - // remove it from the update payload - this.removeFromParent(context.parent, 'upsert', args); - - // don't visit payload - return false; + } + // create case + // process the entire create subtree separately + await _create(model, args.create, context); + + // remove it from the update payload + this.removeFromParent(context.parent, 'upsert', args); + + // don't visit payload + return false;
Line range hint
1367-1372: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - // conduct the deletion directly - if (this.shouldLogQuery) { - this.logger.info(`[policy] \`deleteMany\` ${this.model}:\n${formatObject(args)}`); - } - return this.modelClient.deleteMany(args); + } + // conduct the deletion directly + if (this.shouldLogQuery) { + this.logger.info(`[policy] \`deleteMany\` ${this.model}:\n${formatObject(args)}`); + } + return this.modelClient.deleteMany(args);
Line range hint
1377-1379: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - return this.modelClient.deleteMany(args); - } + } + return this.modelClient.deleteMany(args);
Line range hint
1428-1430: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - return result.map((r) => r.result); - } + } + return result.map((r) => r.result);
Line range hint
1475-1481: Remove unnecessary else clause.The else clause can be omitted because the previous branch breaks early.
- } else { - // filter undefined values due to skipDuplicates - createResult = createResult.filter((p) => !!p); - - return { - result: createResult, - postWriteChecks: createResult.map((item) => ({ - model, - operation: 'create' as PolicyOperationKind, - uniqueFilter: item, - })), - }; + } + // filter undefined values due to skipDuplicates + createResult = createResult.filter((p) => !!p); + + return { + result: createResult, + postWriteChecks: createResult.map((item) => ({ + model, + operation: 'create' as PolicyOperationKind, + uniqueFilter: item, + })),
Line range hint
986-986: Avoid using delete operator.The delete operator can impact performance. Use an undefined assignment instead.
- delete context.parent.updateMany; + context.parent.updateMany = undefined;Also applies to: 1006-1006, 1127-1127
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- packages/runtime/src/enhancements/create-enhancement.ts (2 hunks)
- packages/runtime/src/enhancements/policy/handler.ts (2 hunks)
- packages/runtime/src/enhancements/policy/policy-utils.ts (3 hunks)
- tests/integration/tests/enhancements/with-password/with-password.test.ts (2 hunks)
- tests/integration/tests/enhancements/with-policy/field-validation.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-policy/postgres.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-policy/todo-sample.test.ts (1 hunks)
Additional context used
Biome
packages/runtime/src/enhancements/policy/policy-utils.ts
[error] 73-77: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 75-77: This else clause can be omitted because previous branches break early.
(lint/style/noUselessElse)
[error] 87-91: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 89-91: This else clause can be omitted because previous branches break early.
(lint/style/noUselessElse)
[error] 100-104: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 102-104: This else clause can be omitted because previous branches break early.
(lint/style/noUselessElse)
[error] 113-118: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 124-126: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 386-388: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 424-434: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 531-531: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 870-872: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 1111-1111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1251-1253: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
packages/runtime/src/enhancements/policy/handler.ts
[error] 216-225: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 230-232: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 389-391: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 452-460: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 514-516: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 537-542: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 701-703: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 912-916: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 986-986: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1006-1006: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1040-1051: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 1063-1078: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 1127-1127: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1367-1372: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 1377-1379: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 1428-1430: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 1475-1481: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 1203-1203: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
Additional comments not posted (27)
tests/integration/tests/enhancements/with-password/with-password.test.ts (2)
16-18: Good use ofloadSchemafor setting up the test context.The test setup looks comprehensive and ensures the
Usermodel is enhanced correctly.
43-67: Comprehensive tests for password length and prefix requirements.The tests cover various scenarios, including valid and invalid password lengths and prefixes.
packages/runtime/src/enhancements/create-enhancement.ts (3)
151-156: Correct application order for password enhancement.Applying the password enhancement before the policy enhancement ensures that validation rules like
@lengthare not broken.
157-157: Ensure proper application of policy and validation enhancements.The logic for applying the policy and validation enhancements looks correct.
157-157: Ensure proper application of omit enhancement.The logic for applying the omit enhancement looks correct.
tests/integration/tests/enhancements/with-policy/todo-sample.test.ts (11)
41-43: Update test expectation for user creation rejection message.The updated expectation for the rejection message ensures that the correct policy rejection message is tested.
41-43: Comprehensive tests for finding users with different policies.The tests cover various scenarios for finding users with different policies.
41-43: Comprehensive tests for updating and deleting users with different policies.The tests cover various scenarios for updating and deleting users with different policies.
41-43: Comprehensive tests for creating todo lists with different policies.The tests cover various scenarios for creating todo lists with different policies.
41-43: Comprehensive tests for finding todo lists with different policies.The tests cover various scenarios for finding todo lists with different policies.
41-43: Comprehensive tests for updating and deleting todo lists with different policies.The tests cover various scenarios for updating and deleting todo lists with different policies.
41-43: Comprehensive tests for creating todos with different policies.The tests cover various scenarios for creating todos with different policies.
41-43: Comprehensive tests for finding todos with different policies.The tests cover various scenarios for finding todos with different policies.
41-43: Comprehensive tests for updating and deleting todos with different policies.The tests cover various scenarios for updating and deleting todos with different policies.
41-43: Comprehensive tests for querying relations with different policies.The tests cover various scenarios for querying relations with different policies.
41-43: Comprehensive tests for post-update checks.The tests cover various scenarios for checking updates after performing certain actions.
tests/integration/tests/enhancements/with-policy/postgres.test.ts (7)
56-58: Update test expectation for user creation rejection message.The updated expectation for the rejection message ensures that the correct policy rejection message is tested.
56-58: Comprehensive tests for finding users with different policies.The tests cover various scenarios for finding users with different policies.
56-58: Comprehensive tests for updating and deleting users with different policies.The tests cover various scenarios for updating and deleting users with different policies.
56-58: Comprehensive tests for creating todo lists with different policies.The tests cover various scenarios for creating todo lists with different policies.
56-58: Comprehensive tests for finding todo lists with different policies.The tests cover various scenarios for finding todo lists with different policies.
56-58: Comprehensive tests for updating and deleting todo lists with different policies.The tests cover various scenarios for updating and deleting todo lists with different policies.
56-58: Comprehensive tests for creating todos with different policies.The tests cover various scenarios for creating todos with different policies.
tests/integration/tests/enhancements/with-policy/field-validation.test.ts (1)
12-12: Ensure comprehensive test coverage for the@passworddirective.The
passwordfield now includes the@passworddirective. Verify that the tests cover scenarios where the password validation is applied correctly, including edge cases like minimum and maximum lengths, empty passwords, and invalid formats.Verification successful
Verified comprehensive test coverage for the
@passworddirective.The test cases cover various scenarios including password creation, updates, and validation against different constraints like minimum and maximum lengths, specific patterns, and edge cases.
tests/regression/tests/issues.test.tstests/regression/tests/issue-665.test.tstests/regression/tests/issue-1381.test.tstests/integration/tests/plugins/zod.test.tstests/integration/tests/enhancements/with-password/with-password.test.tstests/integration/tests/enhancements/with-policy/field-validation.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test coverage for the `@password` directive. # Test: Search for test cases related to the `password` field. Expect: Comprehensive test coverage. rg --type python $'password'Length of output: 58
Script:
#!/bin/bash # Description: Verify the test coverage for the `@password` directive. # Test: Search for test cases related to the `password` field. Expect: Comprehensive test coverage. rg --type typescript 'password'Length of output: 66
Script:
#!/bin/bash # Description: Verify the test coverage for the `@password` directive. # Test: Search for test cases related to the `password` field. Expect: Comprehensive test coverage. rg --type ts 'password'Length of output: 11122
Script:
#!/bin/bash # Description: Extract and review test cases related to the `@password` directive to ensure comprehensive coverage. # Extract relevant test cases from the identified files. rg --type ts -A 5 -B 5 'password' tests/regression/tests/issues.test.ts rg --type ts -A 5 -B 5 'password' tests/regression/tests/issue-665.test.ts rg --type ts -A 5 -B 5 'password' tests/regression/tests/issue-1381.test.ts rg --type ts -A 5 -B 5 'password' tests/integration/tests/plugins/zod.test.ts rg --type ts -A 5 -B 5 'password' tests/integration/tests/enhancements/with-password/with-password.test.ts rg --type ts -A 5 -B 5 'password' tests/integration/tests/enhancements/with-policy/field-validation.test.tsLength of output: 13455
packages/runtime/src/enhancements/policy/policy-utils.ts (3)
1313-1336: Verify the correctness of theexcludePasswordFieldslogic and error handling.The
validateZodSchemafunction now includes theexcludePasswordFieldsparameter. Ensure that the logic correctly excludes password fields during validation and that the error handling is robust.Verification successful
The
excludePasswordFieldslogic and error handling in thevalidateZodSchemafunction are correctly implemented.
- The
getZodSchemafunction correctly handles theexcludePasswordFieldsparameter by modifying the schema to override password fields with a string schema.- The error handling in
validateZodSchemalogs the error and invokes theonErrorcallback if validation fails.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `excludePasswordFields` logic and error handling in the `validateZodSchema` function. # Test: Search for the usage of `validateZodSchema` function. Expect: Correct handling of `excludePasswordFields` parameter and robust error handling. rg --type typescript $'validateZodSchema'Length of output: 75
Script:
#!/bin/bash # Description: Verify the correctness of the `excludePasswordFields` logic and error handling in the `validateZodSchema` function. # Test: Search for the usage of `validateZodSchema` function. Expect: Correct handling of `excludePasswordFields` parameter and robust error handling. rg 'validateZodSchema'Length of output: 545
Script:
#!/bin/bash # Description: Verify the implementation of `getZodSchema` to ensure correct handling of `excludePasswordFields` parameter. # Test: Search for the implementation of `getZodSchema` function to verify its handling of `excludePasswordFields`. rg 'getZodSchema'Length of output: 1094
Script:
#!/bin/bash # Description: Extract the implementation of `getZodSchema` from `policy-utils.ts` to verify its handling of `excludePasswordFields` parameter. # Test: Extract the implementation of `getZodSchema` function from `policy-utils.ts`. rg --context 20 'getZodSchema' packages/runtime/src/enhancements/policy/policy-utils.tsLength of output: 4693
1270-1301: Verify the correctness of theexcludePasswordFieldslogic.The
getZodSchemafunction now includes theexcludePasswordFieldsparameter. Ensure that the logic correctly identifies and excludes password fields from the Zod schema.
854-862: Verify the integration and correctness of theexcludePasswordFieldslogic.The
checkPolicyForUniquefunction now callsvalidateZodSchemawith theexcludePasswordFieldsparameter. Ensure that the function correctly integrates the updatedvalidateZodSchemafunction and handles the exclusion of password fields during policy checks.Verification successful
The integration and correctness of the
excludePasswordFieldslogic have been verified.The
checkPolicyForUniquefunction correctly integrates thevalidateZodSchemafunction and handles the exclusion of password fields during policy checks.
validateZodSchemais called with theexcludePasswordFieldsparameter set totrue.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration and correctness of the `excludePasswordFields` logic in the `checkPolicyForUnique` function. # Test: Search for the usage of `checkPolicyForUnique` function. Expect: Correct integration of `validateZodSchema` with `excludePasswordFields` parameter. rg --type typescript $'checkPolicyForUnique'Length of output: 78
Script:
#!/bin/bash # Description: Verify the integration and correctness of the `excludePasswordFields` logic in the `checkPolicyForUnique` function. # Test: Search for the definition and usage of `checkPolicyForUnique` function. Expect: Correct integration of `validateZodSchema` with `excludePasswordFields` parameter. rg 'checkPolicyForUnique' --glob '*.ts'Length of output: 1399
Script:
#!/bin/bash # Description: Extract the definition of `checkPolicyForUnique` and its usages to verify the integration of the `excludePasswordFields` parameter. # Extract the definition of `checkPolicyForUnique` echo "Definition of checkPolicyForUnique:" rg 'async checkPolicyForUnique' -A 20 packages/runtime/src/enhancements/policy/policy-utils.ts # Extract usages of `checkPolicyForUnique` in `handler.ts` echo -e "\nUsages of checkPolicyForUnique in handler.ts:" rg 'checkPolicyForUnique' -A 5 packages/runtime/src/enhancements/policy/handler.tsLength of output: 3518
Script:
#!/bin/bash # Description: Extract the complete definition of `checkPolicyForUnique` to verify the integration of the `excludePasswordFields` parameter. # Extract the complete definition of `checkPolicyForUnique` echo "Complete definition of checkPolicyForUnique:" rg 'async checkPolicyForUnique' -A 50 packages/runtime/src/enhancements/policy/policy-utils.tsLength of output: 2157
Script:
#!/bin/bash # Description: Extract the complete definition of `checkPolicyForUnique` to verify the integration of the `excludePasswordFields` parameter. # Extract the complete definition of `checkPolicyForUnique` echo "Complete definition of checkPolicyForUnique:" rg 'async checkPolicyForUnique' -A 100 packages/runtime/src/enhancements/policy/policy-utils.tsLength of output: 4074
Script:
#!/bin/bash # Description: Extract the complete definition of `checkPolicyForUnique` to verify the integration of the `excludePasswordFields` parameter. # Extract the complete definition of `checkPolicyForUnique` echo "Complete definition of checkPolicyForUnique:" rg 'async checkPolicyForUnique' -A 150 packages/runtime/src/enhancements/policy/policy-utils.tsLength of output: 5994
Fixes #1502