fix: Target variables #170
Conversation
WalkthroughThis pull request introduces several changes across multiple files, primarily focusing on enhancing the handling of sensitive job variables in the Changes
Possibly related PRs
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
packages/job-dispatch/src/job-variables-deployment/utils.ts (2)
44-54: Add JSDoc documentation for better maintainability.The function implementation looks good, but could benefit from documentation describing its purpose, parameters, and return value.
Consider adding JSDoc:
+/** + * Retrieves a target variable value for a given target and key. + * @param tx - Database transaction + * @param targetId - ID of the target + * @param key - Variable key to look up + * @returns Promise resolving to the target variable or null if not found + */ export const getTargetVariableValue = (tx: Tx, targetId: string, key: string) =>
44-54: Consider adding explicit return type for better type safety.The function follows the codebase patterns well and has proper error handling through
takeFirstOrNull.Consider adding explicit return type:
-export const getTargetVariableValue = (tx: Tx, targetId: string, key: string) => +export const getTargetVariableValue = ( + tx: Tx, + targetId: string, + key: string +): Promise<SCHEMA.TargetVariable | null> =>packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts (2)
111-119: Consider adding JSDoc documentation for the function signature.The function's return type includes a union of
DeploymentVariableValue | TargetVariable. Adding documentation would help clarify when each type is returned and the significance of thesensitiveflag.+/** + * Determines the value for a release variable by checking target variables first, + * then falling back to deployment variable values. + * @param tx - Database transaction + * @param variableKey - Key of the variable to look up + * @param variableId - ID of the deployment variable + * @param defaultValueId - ID of the default value, if any + * @param jobTarget - Target configuration + * @returns Object containing the resolved value (either target or deployment), + * whether it was a direct match, and if the value is sensitive + */ export const determineReleaseVariableValue = async (
Line range hint
134-173: Consider more explicit handling of missing values.When no deployment variable values exist and no target variable is found, the function could provide more context about why no value was available.
Consider enhancing the error handling:
const deploymentVariableValues = await utils.getVariableValues( tx, variableId, ); - if (deploymentVariableValues.length === 0) return null; + if (deploymentVariableValues.length === 0) { + console.warn( + `No values found for variable ${variableId} and no target variable for ${variableKey}` + ); + return null; + }packages/job-dispatch/src/__test__/job-variables-deployment.test.ts (1)
562-654: Consider making the test name more specific.The test effectively verifies target variable prioritization, but the name could be more specific to indicate it's testing the non-sensitive case, e.g., "should prioritize non-sensitive target variable if it exists".
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
- apps/webservice/package.json (1 hunks)
- apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (3 hunks)
- packages/db/drizzle/0023_melted_nightshade.sql (1 hunks)
- packages/db/drizzle/meta/_journal.json (1 hunks)
- packages/db/src/schema/job.ts (2 hunks)
- packages/db/src/schema/target.ts (1 hunks)
- packages/job-dispatch/src/test/job-variables-deployment.test.ts (9 hunks)
- packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts (4 hunks)
- packages/job-dispatch/src/job-variables-deployment/utils.ts (1 hunks)
🔇 Additional comments (13)
packages/job-dispatch/src/job-variables-deployment/utils.ts (1)
44-54: Verify sensitive data handling implementation.Given that this PR involves handling sensitive variables (per AI summary), we should verify how this function integrates with the sensitive data handling mechanisms.
Let's check the schema and related implementations:
✅ Verification successful
Sensitive data handling is properly implemented
The codebase correctly handles sensitive variables:
- In
target.ts, sensitive variables are encrypted usingvariablesAES256().encrypt()before storage- The
getTargetVariableValuefunction is used withindetermineReleaseVariableValuewhich properly preserves the sensitive flag when returning target variables- The sensitive flag is propagated through to job variables in
determineVariablesForReleaseJob🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check schema definition for sensitive column rg -A 5 "targetVariable.*=.*defineTable" # Check for sensitive data handling in related files rg -l "sensitive.*targetVariable|targetVariable.*sensitive" # Look for encryption/decryption usage rg "@ctrlplane/secrets"Length of output: 734
Script:
#!/bin/bash # Check the implementation in target.ts that uses variablesAES256 cat packages/job-dispatch/src/target.ts # Check the implementation in job-variables-deployment.ts cat packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts # Check the schema definition for target variables rg -A 10 "targetVariable.*=.*{" packages/job-dispatch/srcLength of output: 12835
packages/db/src/schema/job.ts (2)
3-3: LGTM: Import addition is correct and well-organized.The
booleantype import is properly added to support the new sensitive field.
96-96: LGTM: Well-designed schema change for sensitive data handling.The
sensitivefield is well-designed with appropriate constraints:
- NotNull constraint prevents ambiguity
- Default false is a secure choice, requiring explicit opt-in
- Boolean type is suitable for this flag
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (3)
20-20: LGTM! Good separation of security concerns.The addition of
@ctrlplane/secretsmodule shows good architectural design by isolating cryptographic operations.
76-76: LGTM! Clean and concise syntax.The simplified object spread syntax improves code readability while maintaining the same functionality.
Line range hint
20-64: Verify consistent handling of sensitive variables across the codebase.Since this introduces sensitive variable handling, we should ensure consistent implementation across all related endpoints.
Let's verify the implementation:
✅ Verification successful
Let me gather more information about the write operations and variable handling to ensure complete verification.
Let me do one final check focusing specifically on job variable mutations and their encryption handling.
Based on the gathered information, I can now provide a final response:
Sensitive variable handling is consistently implemented across the codebase
The verification shows:
- Proper encryption using
variablesAES256for sensitive values in both read ([jobId]/route.ts) and write operations (job-dispatch/src/target.ts)- Consistent implementation in job creation through
job-dispatch/src/job-creation.ts- No direct exposure of sensitive values in error handling patterns
- Centralized encryption service in
packages/secrets🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent handling of sensitive variables across the codebase # Check for other endpoints that might need similar sensitive data handling echo "Checking for other API endpoints handling job variables..." rg -l "from.*job.*variable.*" --type ts # Check for proper encryption usage echo "Checking for encryption usage..." rg "variablesAES256" --type ts -A 3 # Look for potential sensitive data exposure in error handling echo "Checking error handling patterns..." rg "catch.*console\.(log|error)" --type ts -A 3Length of output: 3236
Script:
#!/bin/bash # Check for job variable write operations and their encryption handling echo "Checking job variable write operations..." ast-grep --pattern 'insert($$$).into(jobVariable)' # Check for other sensitive variable handling patterns echo "Checking other sensitive variable patterns..." rg "sensitive.*=.*true" --type ts -A 3 # Check for variable encryption in deployment related files echo "Checking deployment variable handling..." rg "encrypt.*value" --type ts -A 3Length of output: 661
Script:
#!/bin/bash # Check for all job variable related database operations echo "Checking job variable database operations..." rg "db\.(insert|update).*jobVariable" --type ts -A 5 # Check for any direct variable value assignments echo "Checking variable value assignments..." ast-grep --pattern 'value = $$$' # Look for potential variable handling in job creation echo "Checking job creation endpoints..." rg "POST.*jobs" --type ts -A 10Length of output: 2961
apps/webservice/package.json (1)
23-23: LGTM! Verify workspace package existence.The addition of
@ctrlplane/secretsfollows the project's workspace dependency pattern and aligns with the PR's objective of enhancing sensitive variable handling.Let's verify the workspace package exists:
✅ Verification successful
Dependency verified and properly linked
The
@ctrlplane/secretspackage exists atpackages/secrets/package.jsonand is correctly referenced as a workspace dependency in multiple packages, including the webservice app.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that @ctrlplane/secrets package exists in the workspace # Expected: Find package.json for the secrets package # Test 1: Search for the secrets package directory fd --type f "package.json" --exec grep -l "\"name\": \"@ctrlplane/secrets\"" # Test 2: Verify the package is properly linked in the workspace rg -l "@ctrlplane/secrets.*workspace:\\*" --type jsonLength of output: 223
packages/db/drizzle/meta/_journal.json (2)
165-171: LGTM! The new migration entry is well-structured.The entry follows the established format and maintains consistency with existing migrations.
165-171: Verify migration sequence integrity.Let's ensure there are no gaps in the migration sequence and that the corresponding SQL file exists.
✅ Verification successful
Migration sequence integrity verified successfully
Both the SQL file
0023_melted_nightshade.sqlexists in the expected location and there are exactly 24 migrations (0-23) with no gaps in the sequence.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify migration sequence integrity and SQL file existence # Test 1: Check if the SQL file exists fd --type f "0023_melted_nightshade.sql" packages/db/drizzle # Test 2: Verify no gaps in migration sequence by checking if we have exactly 24 migrations (0-23) jq '.entries | length' packages/db/drizzle/meta/_journal.jsonLength of output: 174
packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts (2)
158-158: Verify assumption about deployment variable sensitivity.The code assumes deployment variables are never sensitive (hardcoded
false). Is this intentional? If deployment variables can be sensitive, we should retrieve this information from the schema.Let's check if deployment variables have sensitivity information:
#!/bin/bash # Check if deployment variables have sensitivity information in schema ast-grep --pattern 'interface DeploymentVariableValue { $$$ sensitive?: boolean; $$$ }'Also applies to: 165-165, 171-171
120-131: 🛠️ Refactor suggestionConsider explicit error handling for getTargetVariableValue.
While the null case is handled, we should consider explicitly handling potential errors from the target variable lookup to prevent silent failures.
Let's verify the error handling in the utility function:
Consider wrapping the call in a try-catch:
- const targetVariableValue = await utils.getTargetVariableValue( - tx, - jobTarget.id, - variableKey, - ); + let targetVariableValue; + try { + targetVariableValue = await utils.getTargetVariableValue( + tx, + jobTarget.id, + variableKey, + ); + } catch (error) { + console.error(`Failed to fetch target variable: ${error}`); + targetVariableValue = null; + }packages/db/src/schema/target.ts (1)
241-241: LGTM! Type export aligns with sensitive variable handling.The new
TargetVariabletype export provides proper type inference for thetargetVariabletable model, which includes thesensitiveflag. This addition supports the enhanced handling of sensitive job variables mentioned in the PR objectives.packages/job-dispatch/src/__test__/job-variables-deployment.test.ts (1)
17-17: LGTM: Mock setup is correct.The
getTargetVariableValuemock is properly added to the utils module mocks.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation