feat(api/secrets): view secret value permission 2 #3190
Conversation
WalkthroughThe pull request introduces comprehensive updates across both backend and frontend codebases, primarily focused on secret management and permission handling. In the backend, several functions and schemas have been updated to use a refined permission model—shifting from generic actions to secret-specific actions under the new Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
backend/src/ee/services/permission/project-permission.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (7)
✨ Finishing Touches
🪧 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 (
|
![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
🔭 Outside diff range comments (2)
frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/SelectionPanel.tsx (1)
89-92:⚠️ Potential issueInconsistent permission check usage
While other permission checks have been updated to use
ProjectPermissionSecretActions, this check still usesProjectPermissionActions.Delete. This should be consistent with the other changes.Apply this change for consistency with other permission checks:
if ( permission.can( - ProjectPermissionActions.Delete, + ProjectPermissionSecretActions.Delete, subject(ProjectPermissionSub.SecretFolders, { environment: env.slug, secretPath }) ) )frontend/src/hooks/api/dashboard/queries.tsx (1)
121-136: 💡 Verification agent🧩 Analysis chain
Function signatures should be updated for consistency
The
useGetProjectSecretsQuickSearchfunction should also be updated to include theviewSecretValueparameter for consistency withuseGetProjectSecretsDetails.Also applies to: 298-359
🏁 Script executed:
#!/bin/bash # Description: Check if viewSecretValue parameter is used in other API endpoints that return secrets # Look for API endpoints that return secrets and check if they use viewSecretValue rg -A 3 -B 3 "api/v1/dashboard/secrets-deep-search" --type ts echo "---" # Check for other query functions that are similar to useGetProjectSecretsDetails rg "useGetProjectSecrets" --type tsLength of output: 3772
Action Required: Update Function Signature for Consistency
Please update the function signature for
useGetProjectSecretsQuickSearchto include theviewSecretValueparameter so that it matches the signature ofuseGetProjectSecretsDetails. The current implementation does not include this parameter, which is used in similar API endpoints (e.g., the endpoint atapi/v1/dashboard/secrets-deep-search). This adjustment is required both in the primary definition (lines 121-136) and in the related section (lines 298-359).
- File:
frontend/src/hooks/api/dashboard/queries.tsx- Affected Functions:
useGetProjectSecretsQuickSearchshould accept aviewSecretValueparameter similar touseGetProjectSecretsDetails.
🧹 Nitpick comments (14)
frontend/src/components/v2/Blur/Blur.tsx (1)
10-22: Improve accessibility for the role="button" usage.You have
role="button"without an associated onClick or keyDown handler. Screen readers might expect this element to be interactive. Consider adding appropriate event handlers or using a different role to more accurately reflect this element's behavior.backend/src/db/migrations/20250218020306_backfill-secret-permissions-with-readvalue.ts (2)
86-198: Consider transaction usage for large data updates.Chunked updates handle large datasets well, but there's still a risk of partial completion if the migration fails halfway. You might consider batching within a single or segmented transaction to ensure atomicity.
200-313: Completeness of rollback.The rollback logic mirrors the forward migration, which is good for consistency. Similar to the “up” function, consider adding transaction handling to ensure partial updates do not leave the data in an inconsistent state.
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretNoAccessListView.tsx (1)
38-38: Refactored blurring implementation using a dedicated component.The custom blur implementation has been replaced with the dedicated
Blurcomponent, which improves code maintainability and consistency across the application. This change simplifies the code while maintaining the same visual indication for secrets the user doesn't have permission to view.frontend/src/hooks/api/secretImports/queries.tsx (2)
166-190: Consider refactoring to reduce code duplication.The secret mapping logic is nearly identical between the
useGetImportedSecretsSingleEnvanduseGetImportedSecretsAllEnvshooks. Consider extracting this transformation logic into a shared utility function to improve maintainability.+// Add at the top of the file after imports +const transformImportedSecret = (encSecret: any) => ({ + id: encSecret.id, + env: encSecret.environment, + key: encSecret.secretKey, + value: encSecret.secretValue, + secretValueHidden: encSecret.secretValueHidden, + tags: encSecret.tags, + comment: encSecret.secretComment, + createdAt: encSecret.createdAt, + updatedAt: encSecret.updatedAt, + version: encSecret.version +}); +const transformImportedSecrets = (el: any) => ({ + environment: el.environment, + secretPath: el.secretPath, + environmentInfo: el.environmentInfo, + folderId: el.folderId, + secrets: el.secrets.map(transformImportedSecret) +}); // Then in useGetImportedSecretsSingleEnv - select: (data: TImportedSecrets[]) => { - return data.map((el) => ({ - environment: el.environment, - secretPath: el.secretPath, - environmentInfo: el.environmentInfo, - folderId: el.folderId, - secrets: el.secrets.map((encSecret) => { - return { - id: encSecret.id, - env: encSecret.environment, - key: encSecret.secretKey, - value: encSecret.secretValue, - secretValueHidden: encSecret.secretValueHidden, - tags: encSecret.tags, - comment: encSecret.secretComment, - createdAt: encSecret.createdAt, - updatedAt: encSecret.updatedAt, - version: encSecret.version - }; - }) - })); - } + select: (data: TImportedSecrets[]) => data.map(transformImportedSecrets) // And in useGetImportedSecretsAllEnvs - select: useCallback( - (data: Awaited<ReturnType<typeof fetchImportedSecrets>>) => - data.map((el) => ({ - environment: el.environment, - secretPath: el.secretPath, - environmentInfo: el.environmentInfo, - folderId: el.folderId, - secrets: el.secrets.map((encSecret) => { - return { - id: encSecret.id, - env: encSecret.environment, - key: encSecret.secretKey, - value: encSecret.secretValue, - secretValueHidden: encSecret.secretValueHidden, - tags: encSecret.tags, - comment: encSecret.secretComment, - createdAt: encSecret.createdAt, - updatedAt: encSecret.updatedAt, - version: encSecret.version - }; - }) - })), - [] - ) + select: useCallback( + (data: Awaited<ReturnType<typeof fetchImportedSecrets>>) => data.map(transformImportedSecrets), + [] + )
134-147: Ensure type safety for the API response data.The code is currently mapping fields from
encSecretwithout type checking. Consider using a more explicit type forencSecretor adding runtime validation to ensure the expected properties exist and are of the correct type.secrets: el.secrets.map((encSecret) => { + // Ensure all required properties exist + if (!encSecret.id || !encSecret.secretKey) { + console.warn('Missing required properties in secret data', encSecret); + } return { id: encSecret.id, env: encSecret.environment, key: encSecret.secretKey, value: encSecret.secretValue, secretValueHidden: encSecret.secretValueHidden, tags: encSecret.tags, comment: encSecret.secretComment, createdAt: encSecret.createdAt, updatedAt: encSecret.updatedAt, version: encSecret.version };Alternatively, define a proper TypeScript interface for the API response data:
interface SecretResponse { id: string; environment: string; secretKey: string; secretValue: string; secretValueHidden: boolean; tags: string[]; secretComment: string; createdAt: string; updatedAt: string; version: number; }backend/e2e-test/routes/v2/service-token.spec.ts (1)
499-499: Order of permissions changed (read, readValue, write).The reordering is non-problematic functionally, but consider keeping a consistent order (e.g., alphabetical or read→write→readValue) for readability across test cases.
backend/src/services/secret/secret-fns.ts (1)
368-375: Conditional decryption based onsecretValueHidden.The code neatly checks
secretValueHiddenbefore decrypting. Consider adding tests that verify the masked value is returned whensecretValueHiddenis true.frontend/src/pages/project/RoleDetailsBySlugPage/components/ProjectRoleModifySection.utils.tsx (2)
26-32: Consider clarifying the difference between 'read' and 'readValue'.
Defining bothread(for describing secret metadata) andreadValue(for full secret value retrieval) within the same schema can be confusing. It may be beneficial to renamereadtodescribeor similar to match the "Describe Secret" label and reduce ambiguity.- read: z.boolean().optional(), // describe secret + describe: z.boolean().optional(), // describe secret
517-519: Clarify the difference between 'Describe Secret' and 'Read Value' in the UI.
Renaming "Read" to "Describe Secret" and adding "Read Value" is meaningful, but users might still be confused about what “describe” entails. Consider amending labels to better convey that 'Describe Secret' is about viewing metadata vs. 'Read Value' for retrieving actual contents.backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts (1)
185-217: Consider refactoring repeated read-value checks to reduce duplication.
Both branches use the same pattern: computingcanReadValue, masking or decrypting the secret, and settingsecretValueHidden. Consolidating this logic into a helper function would improve maintainability.- // Repeated lines handling canReadValue, decrypting or masking secret + // Suggestion: Extract a shared helper for read-value logicAlso applies to: 232-279
backend/src/services/secret/secret-types.ts (2)
183-184: Consider Adding Documentation or Default ValuesThese properties (
viewSecretValueandthrowOnMissingReadValuePermission) appear to be pivotal to controlling read permissions. Consider clarifying their intended usage via comments or setting default values to make the code self-documenting.
415-415: Confirm Usage of the Newly AddedfindMethodExtending the
secretV2BridgeDALto includefindis useful for retrieving secrets, but make sure there's corresponding indexing or query optimization if these lookups may be frequent or run on large datasets.backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts (1)
106-114: Return Secrets with Tags from Bulk InsertNow using
secretDAL.findso that newly inserted secrets are returned with their tags. This approach is beneficial for returning complete data but watch out for performance overhead if the dataset is large.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (70)
backend/e2e-test/routes/v2/service-token.spec.ts(5 hunks)backend/e2e-test/vitest-environment-knex.ts(0 hunks)backend/src/db/migrations/20250218020306_backfill-secret-permissions-with-readvalue.ts(1 hunks)backend/src/ee/routes/v1/secret-approval-request-router.ts(3 hunks)backend/src/ee/routes/v1/secret-router.ts(2 hunks)backend/src/ee/routes/v1/secret-version-router.ts(2 hunks)backend/src/ee/routes/v1/snapshot-router.ts(3 hunks)backend/src/ee/services/permission/project-permission.ts(11 hunks)backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts(6 hunks)backend/src/ee/services/secret-replication/secret-replication-service.ts(1 hunks)backend/src/ee/services/secret-rotation/secret-rotation-service.ts(2 hunks)backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts(6 hunks)backend/src/lib/api-docs/constants.ts(3 hunks)backend/src/lib/errors/index.ts(1 hunks)backend/src/server/routes/sanitizedSchemas.ts(2 hunks)backend/src/server/routes/v1/dashboard-router.ts(12 hunks)backend/src/server/routes/v2/service-token-router.ts(1 hunks)backend/src/server/routes/v3/secret-router.ts(22 hunks)backend/src/services/external-migration/external-migration-fns.ts(1 hunks)backend/src/services/external-migration/external-migration-queue.ts(1 hunks)backend/src/services/integration-auth/integration-delete-secret.ts(1 hunks)backend/src/services/integration/integration-service.ts(3 hunks)backend/src/services/project/project-service.ts(2 hunks)backend/src/services/secret-import/secret-import-fns.ts(6 hunks)backend/src/services/secret-import/secret-import-service.ts(7 hunks)backend/src/services/secret-sync/secret-sync-queue.ts(1 hunks)backend/src/services/secret-sync/secret-sync-service.ts(3 hunks)backend/src/services/secret-tag/secret-tag-dal.ts(1 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts(9 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts(50 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts(7 hunks)backend/src/services/secret-v2-bridge/secret-version-dal.ts(3 hunks)backend/src/services/secret/secret-dal.ts(2 hunks)backend/src/services/secret/secret-fns.ts(7 hunks)backend/src/services/secret/secret-queue.ts(1 hunks)backend/src/services/secret/secret-service.ts(42 hunks)backend/src/services/secret/secret-types.ts(4 hunks)backend/src/services/secret/secret-version-dal.ts(3 hunks)backend/src/services/service-token/service-token-service.ts(2 hunks)backend/src/services/service-token/service-token-types.ts(1 hunks)frontend/src/components/secrets/SecretReferenceDetails/SecretReferenceDetails.tsx(4 hunks)frontend/src/components/v2/Blur/Blur.tsx(1 hunks)frontend/src/components/v2/Blur/index.tsx(1 hunks)frontend/src/components/v2/InfisicalSecretInput/InfisicalSecretInput.tsx(1 hunks)frontend/src/context/ProjectPermissionContext/types.ts(2 hunks)frontend/src/hooks/api/dashboard/queries.tsx(3 hunks)frontend/src/hooks/api/dashboard/types.ts(1 hunks)frontend/src/hooks/api/secretApprovalRequest/queries.tsx(1 hunks)frontend/src/hooks/api/secretImports/queries.tsx(2 hunks)frontend/src/hooks/api/secretSnapshots/queries.tsx(1 hunks)frontend/src/hooks/api/secrets/queries.tsx(5 hunks)frontend/src/hooks/api/secrets/types.ts(5 hunks)frontend/src/hooks/api/types.ts(2 hunks)frontend/src/pages/project/AccessControlPage/components/ServiceTokenTab/components/ServiceTokenSection/AddServiceTokenModal.tsx(2 hunks)frontend/src/pages/project/RoleDetailsBySlugPage/components/GeneralPermissionPolicies.tsx(1 hunks)frontend/src/pages/project/RoleDetailsBySlugPage/components/ProjectRoleModifySection.utils.tsx(5 hunks)frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx(2 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretEditRow.tsx(5 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretNoAccessOverviewTableRow.tsx(2 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretOverviewTableRow.tsx(1 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretRenameRow.tsx(2 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretSearchInput/components/QuickSearchSecretItem.tsx(2 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/SelectionPanel.tsx(3 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/components/MoveSecretsDialog/MoveSecretsDialog.tsx(2 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/SecretDashboardPage.tsx(3 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/ActionBar/ActionBar.tsx(3 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx(8 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretItem.tsx(10 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretNoAccessListView.tsx(2 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SnapshotView/SecretItem.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- backend/e2e-test/vitest-environment-knex.ts
✅ Files skipped from review due to trivial changes (4)
- frontend/src/components/v2/Blur/index.tsx
- backend/src/lib/errors/index.ts
- backend/src/services/secret-tag/secret-tag-dal.ts
- frontend/src/pages/project/RoleDetailsBySlugPage/components/GeneralPermissionPolicies.tsx
🧰 Additional context used
🧠 Learnings (1)
frontend/src/hooks/api/secretSnapshots/queries.tsx (1)
Learnt from: DanielHougaard
PR: Infisical/infisical#3128
File: frontend/src/hooks/api/secretSnapshots/queries.tsx:78-78
Timestamp: 2025-02-20T01:50:15.668Z
Learning: In the secret snapshot feature, the `secretValueHidden` property is always set to `false` as snapshots represent historical data that should always show the secret values. The property is only declared to satisfy TypeScript type requirements.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check TS and Lint
- GitHub Check: Check API Changes
- GitHub Check: Check Frontend Type and Lint check
- GitHub Check: Run integration test
🔇 Additional comments (299)
frontend/src/components/v2/Blur/Blur.tsx (1)
5-8: Ensure test coverage for optional tooltip usage.The interface includes an optional
tooltipTextwith no unit tests shown here. Consider verifying via testing that the tooltip behaves correctly when this prop is absent or present.backend/src/db/migrations/20250218020306_backfill-secret-permissions-with-readvalue.ts (4)
10-17: Enums for permissions look good.The updated enums for
ProjectPermissionSubandSecretActionsare consistent with the new readValue permission. No immediate issues spotted.
19-31: Validation approach is solid.Using Zod for parsing and ensuring the permission structures are valid helps prevent malformed data. Good practice!
32-53: Efficiently adding 'readValue' permission.The logic to add
SecretActions.ReadValueonly ifSecretActions.Readis present is clear and prevents duplication. This meets the migration's intent.
55-82: Clean removal of 'readValue' during rollback.The approach similarly reverses the permission changes. The checks handle edge cases when
readValuemay or may not exist, which is appropriate.frontend/src/hooks/api/dashboard/types.ts (1)
72-72: Validate default behavior of new 'viewSecretValue' property.By declaring
viewSecretValueas a non-optional boolean, ensure all call sites provide it. If certain usage scenarios don't supply this property, consider making it optional or providing a default in the code that constructs these DTOs.backend/src/server/routes/sanitizedSchemas.ts (1)
237-243: Good implementation of the SanitizedTagSchemaThe new schema properly follows the established pattern of using
pickto select specific fields from the base schema andextendto add additional fields. This approach is consistent with other sanitized schemas in the file and ensures type safety.frontend/src/hooks/api/secretSnapshots/queries.tsx (1)
78-78: Added secretValueHidden property to maintain consistency with visibility controlThe addition of this property aligns with the broader feature for controlling secret value visibility. Based on the retrieved learning, in the secret snapshot feature, this property is typically set to
falseas snapshots represent historical data that should always show secret values. This change ensures proper typing and consistent data structure across the application.frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretNoAccessOverviewTableRow.tsx (2)
5-6: Importing the new Blur componentGood addition of the new component import which will be used to improve the UI for hidden secret values.
29-29: Improved UI for "no access" state using dedicated componentReplacing the DIV with blur-sm class with a dedicated Blur component is a good improvement. This provides a more consistent UI treatment for inaccessible secret values across the application and likely includes additional functionality beyond just visual blurring.
backend/src/ee/services/secret-replication/secret-replication-service.ts (1)
268-268: Important permission flag added for secret value accessAdding the
viewSecretValue: trueparameter ensures the replication service has proper access to the actual secret values during the replication process. This is essential because the replication service needs to work with the actual values to replicate them correctly.This change is part of the broader implementation of granular permission control for secret values introduced in this PR.
backend/src/services/service-token/service-token-types.ts (1)
10-10: Added read value permission to service token capabilities.The service token permission types have been extended to include a new "readValue" permission type alongside the existing "read" and "write" permissions, providing more granular access control.
This change aligns with the principle of least privilege by allowing service tokens to be created with the ability to view that a secret exists without necessarily seeing its actual value.
backend/src/services/secret/secret-queue.ts (1)
405-406: Implementation of view permissions in import process.The
viewSecretValue: trueparameter has been added to ensure that integration import processes have full access to secret values. This is necessary for system-level operations that need to process the actual values of secrets.This complements the service token permissions change by showing how the new permission model gets used in practice within backend processes.
frontend/src/hooks/api/secretApprovalRequest/queries.tsx (1)
82-82: Added user permission status to decrypted secret objects.The
secretValueHiddenproperty from the encrypted secret is now preserved in the decrypted secret object, supporting UI decisions about whether to display the actual secret value.This connects the backend permission model to the frontend UI layer, ensuring permission checks are properly enforced during rendering.
frontend/src/pages/secret-manager/SecretDashboardPage/components/SnapshotView/SecretItem.tsx (3)
23-23: Added Blur component import.The Blur component is imported to support conditionally obscuring secret values based on user permissions.
124-132: Implemented conditional UI rendering based on view permissions.Secret values are now conditionally rendered based on the user's permissions:
- When
secretValueHiddenis true, a blurred element with explanatory tooltip is displayed- Otherwise, the actual secret value is shown in a SecretInput component
This UI implementation respects the backend permission model, preventing unauthorized users from viewing sensitive information.
The tooltip provides excellent user feedback by explaining why the value is hidden rather than simply obscuring it without explanation.
135-143: Consistent implementation of permission-based rendering for post-rollback values.Similar to the pre-rollback implementation, this code conditionally displays either a blurred element or the actual secret value based on user permissions.
The consistent implementation ensures that permission checks are applied in all relevant contexts, providing a uniform user experience.
frontend/src/components/v2/InfisicalSecretInput/InfisicalSecretInput.tsx (1)
122-123: Added view secret value permission control.The addition of
viewSecretValue: falseparameter to theuseGetProjectSecretshook ensures that this component doesn't attempt to fetch secret values when it doesn't need them, aligning with the new granular permission system.backend/src/server/routes/v2/service-token-router.ts (1)
97-97: Enhanced permission model with readValue capability.The addition of the "readValue" permission to the service token permission enum is a well-designed improvement that enables more granular access control. Now service tokens can be created with specific permissions to view secret values separately from basic read operations.
backend/src/services/secret-sync/secret-sync-queue.ts (1)
251-254: Added explicit viewSecretValue parameter for secret imports.The addition of
viewSecretValue: trueparameter to thefnSecretsV2FromImportsfunction call ensures that imported secrets will include their values, which is necessary for the secret synchronization process. This change correctly implements the granular permission model in the context of secret imports.frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretNoAccessListView.tsx (1)
1-2: Added import for the Blur component.The import of the Blur component is appropriately added to support the refactoring below.
backend/src/ee/routes/v1/secret-version-router.ts (2)
44-44: Minor formatting improvement.Added a blank line before the return statement for better readability, following good code style practices.
25-29:❓ Verification inconclusive
Schema updated to include secret visibility information.
The response schema for
secretVersionshas been extended to include asecretValueHiddenboolean property. This aligns with the "view secret value permission" feature being implemented in this PR.
🏁 Script executed:
#!/bin/bash # Let's verify that all code consuming this API properly handles the new secretValueHidden property # First, let's look for files that might display secret versions echo "Looking for files that consume secret versions data:" rg -l "secretVersions" --type ts --type tsxLength of output: 186
Attention: Schema Update Verification & Consumer Impact Check
The schema update in
backend/src/ee/routes/v1/secret-version-router.ts(lines 25–29) correctly extendssecretRawSchemato include thesecretValueHidden: z.boolean()property as expected for the "view secret value permission" feature. However, the initial ripgrep command produced an error regarding the TSX file type. Please run the updated script above to verify that all code consuming thesecretVersionsAPI (in both.tsand.tsxfiles) properly handles the newsecretValueHiddenproperty.
🏁 Script executed:
#!/bin/bash echo "Searching for references to secretVersions in *.ts files:" rg -l "secretVersions" -g "*.ts" echo "Searching for references to secretVersions in *.tsx files:" rg -l "secretVersions" -g "*.tsx"Length of output: 1105
Schema Update Confirmed & Consumer References Discovered
The update in
backend/src/ee/routes/v1/secret-version-router.tsproperly extends thesecretRawSchemawith thesecretValueHidden: z.boolean()property in line with the new "view secret value permission" feature. Our updated search across both.tsand.tsxfiles has returned several references tosecretVersions(e.g., in files likebackend/src/services/secret/secret-service.tsandfrontend/src/hooks/api/secrets/queries.tsx, among others).Please ensure that all these consumers handle the new
secretValueHiddenproperty appropriately where they process or display secret version data.backend/src/services/integration-auth/integration-delete-secret.ts (1)
71-72:✅ Verification successful
Added
viewSecretValue: trueparameter to support the view permission feature.This change ensures that the function
fnSecretsV2FromImportshas the necessary permissions to view secret values in the integration deletion flow. Since this is an integration-based operation that needs full access to secrets, setting this totrueis appropriate.
🏁 Script executed:
#!/bin/bash # Let's confirm the signature of fnSecretsV2FromImports to ensure it accepts this parameter echo "Checking the declaration of fnSecretsV2FromImports function:" rg -A 10 "export const fnSecretsV2FromImports" --type tsLength of output: 1070
Verified: Integration deletion secret permissions are correctly configured
- The
fnSecretsV2FromImportsfunction (inbackend/src/services/secret-import/secret-import-fns.ts) accepts bothhasSecretAccessandviewSecretValueparameters.- Setting
viewSecretValue: trueinbackend/src/services/integration-auth/integration-delete-secret.tsproperly enables full access to view secret values during the integration deletion flow.- No further changes are necessary.
frontend/src/hooks/api/types.ts (2)
49-50: Added new error type for permission handling.A new enum value
CustomForbiddenErroris added to theApiErrorTypesenum. This provides better error handling specifically for the "view secret value permission" feature.
73-78:❓ Verification inconclusive
Extended API error types to support new forbidden error case.
This adds the corresponding type definition for the new
CustomForbiddenError. Unlike the existingForbiddenErrorwhich includes detailed permission rules, this version is simpler with just a message. This is suitable for specific permission errors like those related to secret value visibility.
🏁 Script executed:
#!/bin/bash # Let's check if there's error handling code that uses this new error type echo "Searching for usage of the new CustomForbiddenError type:" rg "CustomForbiddenError" --type ts --type tsxLength of output: 199
Action Required: Verify Usage of
CustomForbiddenErrorin CodebaseThe new type definition for
CustomForbiddenError(located infrontend/src/hooks/api/types.tsat lines 73–78) is correctly added per the diff snippet. However, our initial search usingrg --type tsxreturned an error due to unrecognized file type for TSX files. To ensure this new type is properly used across the codebase, please re-run the search without relying on built-in type definitions. For example:rg "CustomForbiddenError" -g "*.ts" -g "*.tsx"This command will accurately list all usages of the new error type. Once verified manually, you can confirm that the type is integrated as expected.
frontend/src/hooks/api/secretImports/queries.tsx (2)
140-140: AddedsecretValueHiddenproperty to support secret visibility permissions.This change propagates the new
secretValueHiddenflag from the API response to the transformed secret object, which will be used by UI components to determine whether to show the actual secret value or a masked placeholder.
180-180: AddedsecretValueHiddenproperty to the mapped secret objects.Consistent with the change in the
useGetImportedSecretsSingleEnvhook, this adds the same property to secrets in theuseGetImportedSecretsAllEnvshook. This ensures a consistent approach to secret visibility across different parts of the application.backend/src/lib/api-docs/constants.ts (3)
669-669: Documentation enhancement for theviewSecretValueparameterThe addition of documentation for the
viewSecretValueparameter inSECRETS.ATTACH_TAGSimproves API clarity by clearly indicating the purpose of this parameter.
693-693: Documentation enhancement for theviewSecretValueparameterThe addition of documentation for the
viewSecretValueparameter inRAW_SECRETS.LISTimproves API clarity by clearly indicating the purpose of this parameter.
722-722: Documentation enhancement for theviewSecretValueparameterThe addition of documentation for the
viewSecretValueparameter inRAW_SECRETS.GETimproves API clarity by clearly indicating the purpose of this parameter.frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/SelectionPanel.tsx (3)
14-14: Improvement to permission model with finer-grained controlsGood addition of the
ProjectPermissionSecretActionsimport to use more specific permission controls for secret operations.
62-63: Updated permission check to use secret-specific actionProperly updated the permission check to use
ProjectPermissionSecretActions.Deleteinstead of the genericProjectPermissionActions.Deletefor better permission granularity.
114-115: Updated permission check to use secret-specific actionProperly updated the permission check to use
ProjectPermissionSecretActions.Deleteinstead of the genericProjectPermissionActions.Deletefor better permission granularity.frontend/src/hooks/api/dashboard/queries.tsx (3)
210-214: Enhancement: Added viewSecretValue parameterGood addition of the
viewSecretValueparameter to the function signature, which allows control over whether secret values are retrieved.
235-236: Properly included viewSecretValue in query keyCorrectly added the
viewSecretValueparameter to the query key, ensuring proper cache management when this parameter changes.
252-253: Properly passed viewSecretValue to API requestCorrectly included the
viewSecretValueparameter in the function call tofetchProjectSecretsDetails, ensuring the parameter is passed to the API.backend/src/ee/routes/v1/secret-approval-request-router.ts (5)
3-4: Schema dependency updateRemoved the
SecretTagsSchemaimport as it's no longer used in this file.
8-8: Improved schema importAdded the
SanitizedTagSchemaimport, which is likely a more secure or standardized schema for handling tags.
281-282: Enhanced tag schema with sanitizationUpdated the tag schema to use
SanitizedTagSchema.array().optional()instead of the previoustagSchema. This likely provides better validation and sanitization for security purposes.
300-301: Enhanced tag schema with sanitizationUpdated the tag schema to use
SanitizedTagSchema.array().optional()instead of the previoustagSchema. This likely provides better validation and sanitization for security purposes.
200-300:❓ Verification inconclusive
Verify tag schema consistency across the codebase
While the changes to use
SanitizedTagSchemaare good, it's important to ensure this pattern is consistently applied across all routes that handle tags.
🏁 Script executed:
#!/bin/bash # Description: Look for instances of SecretTagsSchema that might need to be updated to SanitizedTagSchema # Find files that still use SecretTagsSchema rg "SecretTagsSchema" --type ts # Find other router files that might need similar updates rg "tags:.*Schema" --type ts ./backend/src/ee/routesLength of output: 2313
Tag Schema Consistency Verification
The EE routes now consistently use
SanitizedTagSchema(as seen in bothsecret-approval-request-router.tsandsnapshot-router.ts). However, the search also revealed that some non-EE routes (for example, inbackend/src/server/routes/v1/secret-tag-router.ts) are still usingSecretTagsSchema. Please verify if this discrepancy is intentional or if those routes should also be updated to useSanitizedTagSchemafor consistency across the codebase.backend/src/services/secret-sync/secret-sync-service.ts (3)
6-6: Permission enum refinement looks good.The change from likely
ProjectPermissionActionsto the more specificProjectPermissionSecretActionsenhances the granularity of the permission model for secrets.
181-187: Appropriate permission check for secret value access.The permission check has been updated to use
ProjectPermissionSecretActions.ReadValuewhich correctly aligns with the granular permission model being implemented. This ensures proper authorization when accessing secret paths.
272-278: Consistent permission enforcement for secret values.The second instance of the permission check also properly uses
ProjectPermissionSecretActions.ReadValue, maintaining consistency throughout the codebase.frontend/src/pages/secret-manager/OverviewPage/components/SecretSearchInput/components/QuickSearchSecretItem.tsx (2)
113-137: Excellent UX improvement for permission feedback.The added tooltip provides clear feedback when users don't have permission to view a secret value. The disabled state on the button prevents user actions when permissions are insufficient.
The tooltip and disabled state work together to provide a better user experience by clearly communicating why an action isn't available.
171-188: Consistent permission feedback across actions.Good job implementing the same permission feedback pattern for viewing secret values as for copying them. This maintains UI consistency and provides clear user guidance.
frontend/src/pages/project/AccessControlPage/components/ServiceTokenTab/components/ServiceTokenSection/AddServiceTokenModal.tsx (3)
61-61: Successfully extended schema with new permission.Adding
readValueto the validation schema is the correct approach to support the new permission type.
298-302: Appropriate default permission settings.Setting
readValue: falseby default is a good security practice that follows the principle of least privilege. Users must explicitly grant this permission when needed.
304-317: Clear permission labeling and options.The updated permission labels clearly distinguish between "Describe Secret" and "Read Value" operations, making the permission model intuitive for users.
The label change from "Read (default)" to "Describe Secret (default)" makes the distinction between operations clearer and aligns with the backend permission model.
frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretOverviewTableRow.tsx (2)
224-224: Proper propagation of secret visibility state.Passing the
secretValueHiddenprop to the child component ensures that the permission restriction is properly handled in the UI.
225-231: Security-focused value handling.The conditional logic to provide an empty string when
secret?.secretValueHiddenis true prevents unauthorized access to secret values in the UI. This is a critical security measure that properly implements the permission model.frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretEditRow.tsx (5)
28-28: New Blur component import added to support secret visibility control.This import is added to handle the UI presentation for hidden secrets.
43-43: New prop added to control secret value visibility.Adding the
secretValueHiddenboolean property to the component's Props type is essential for implementing the view secret value permission feature.
63-63: Added the secretValueHidden prop to the component parameters.Correctly updated the component parameter list to include the new prop.
146-166: Conditional rendering based on permission to view secret values.Well-implemented conditional rendering that:
- Shows a blurred placeholder with explanatory tooltip when the user doesn't have permission
- Preserves the existing functionality when the user has permission
This implementation properly enforces the permission model for viewing secret values.
222-222: Copy button disabled when secret value is hidden.Good security practice to disable the copy functionality when the user doesn't have permission to view the secret value. This prevents unauthorized access to secret values through the clipboard.
frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx (2)
22-22: Import for more specific secret permission actions.Added import for
ProjectPermissionSecretActionsto support the granular permission model for secrets.
279-279: Updated permission check to use secret-specific action.Changed from general
ProjectPermissionActions.Createto the more specificProjectPermissionSecretActions.Createfor better permission model granularity. This change aligns with the new permission system that separates secret-related permissions from general actions.backend/src/ee/services/secret-rotation/secret-rotation-service.ts (2)
18-22: Updated imports to include the new secret-specific permission actions.Properly structured the imports to include
ProjectPermissionSecretActionsalongside the existing permission imports.
113-113: Updated permission check to use secret-specific action.Changed from
ProjectPermissionActions.EdittoProjectPermissionSecretActions.Editto align with the new permission model. This ensures that the secret rotation service correctly validates permissions using the appropriate action type.frontend/src/context/ProjectPermissionContext/types.ts (2)
10-16: Added new enum for secret-specific permission actions.Well-designed enum that provides granular control over secret-related permissions, including:
DescribeSecret- read metadata about secretsReadValue- new permission that specifically controls viewing the actual secret valueCreate,Edit,Delete- standard CRUD operationsThis is a good approach to implementing the view secret value permission feature, as it provides fine-grained control over different aspects of secret management.
149-149: Updated ProjectPermissionSet to use the secret-specific permission actions.This change ensures that secret-related permissions use the new
ProjectPermissionSecretActionsenum instead of the more generalProjectPermissionActions. This is a necessary adjustment to fully implement the new permission model throughout the system.backend/src/services/external-migration/external-migration-queue.ts (2)
30-30: Added find capability to secretDAL.This enhancement allows the service to query secrets by specified criteria, which is a good improvement to the data access capabilities.
32-32: Added find capability to secretTagDAL.Adding the find method to secretTagDAL enhances the service's ability to query and retrieve tags associated with secrets, consistent with the secretDAL enhancement.
backend/src/ee/routes/v1/secret-router.ts (2)
3-3: Updated permission model import to use more specific secret actions.The import is now using
ProjectPermissionSecretActionsinstead of the more generalProjectPermissionActions, which indicates a shift to a more granular permission model specifically for secrets.
12-12:✅ Verification successful
Updated schema to use secret-specific permission actions.
The schema now validates allowed actions against
ProjectPermissionSecretActionsenum instead of the generalProjectPermissionActions, ensuring that only valid secret-specific permissions can be assigned.
🏁 Script executed:
#!/bin/bash # Check to confirm what actions are available in the ProjectPermissionSecretActions enum ast-grep --pattern 'enum ProjectPermissionSecretActions { $$$ }'Length of output: 1212
Secret-specific permission actions schema update approved.
The updated schema now strictly validates allowed actions usingProjectPermissionSecretActions—which, based on the grep output, correctly includesDescribeSecret,ReadValue,Create,Edit, andDelete. This change enforces that only valid secret-specific permissions are accepted.
- Affected file:
backend/src/ee/routes/v1/secret-router.ts- Enum definitions in both the frontend and backend confirm consistency in allowed actions.
backend/src/services/external-migration/external-migration-fns.ts (2)
34-34: Added find capability to secretDAL.This enhancement allows for searching and querying secrets by criteria, consistent with similar changes in other files. The change keeps the data access layer functionality aligned across different components.
36-36: Added find capability to secretTagDAL.Adding the find method to secretTagDAL provides more flexibility for retrieving and filtering tags, maintaining consistency with the secretDAL enhancement and other related modules.
frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretRenameRow.tsx (2)
13-14: Updated permission imports to use secret-specific actions.The component now imports the more specialized
ProjectPermissionSecretActionsinstead of the generalProjectPermissionActions, aligning with the new granular permission model for secret operations.
54-55:❓ Verification inconclusive
Updated permission checks to use secret-specific actions.
The permission check logic now uses the more specific
ProjectPermissionSecretActions.DescribeSecretandProjectPermissionSecretActions.Editactions, replacing the more general permission checks. This provides more precise control over what actions users can perform on secrets.
🏁 Script executed:
#!/bin/bash # Find other instances of permission checks that might need similar updates rg -A 2 "permission.can\(ProjectPermissionActions" --type typescriptLength of output: 105
Permission Checks Update – Manual Verification Needed
The updated logic now correctly uses secret-specific actions (
ProjectPermissionSecretActions.DescribeSecretandProjectPermissionSecretActions.Edit) to enforce more granular permissions on secrets. However, our initial search for generic permission checks (usingProjectPermissionActions) encountered a file type error. Please manually verify that no legacy permission checks remain in TypeScript files.
- File reviewed:
frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretRenameRow.tsx(lines 54-55)- Recommendation: Manually check the repository for any remaining instances of
permission.can(ProjectPermissionActions...)in.ts/.tsxfiles.backend/src/services/integration/integration-service.ts (3)
5-9: Added appropriate permission import for secret values access control.The import statement now includes
ProjectPermissionSecretActionswhich is necessary for the more granular permission checks implemented in this file. This change aligns with the overall feature of controlling secret value visibility.
98-104: Enhanced permission check to specifically control secret value access.The permission check has been updated to use
ProjectPermissionSecretActions.ReadValueinstead of a generic read permission. This change provides more granular control over who can view actual secret values versus who can just see that secrets exist.
181-187: Updated permission check for secret value access during integration updates.Similar to the change in
createIntegration, this permission check now usesProjectPermissionSecretActions.ReadValueto specifically verify that the user has permission to view secret values rather than just read secret metadata.backend/src/services/secret-import/secret-import-service.ts (7)
8-12: Added appropriate permission imports for enhanced secret access control.The import statement has been updated to include
ProjectPermissionSecretActions, which is necessary for the more granular permission checks implemented throughout this file.
96-102: Updated permission check to use appropriate secret-specific action.The permission check now uses
ProjectPermissionSecretActions.DescribeSecretinstead of a more generic permission. This change allows for more granular control over secret access, differentiating between viewing metadata and actual secret values.
408-414: Updated permission check to use secret-specific action.Similar to previous changes, this permission check now uses
ProjectPermissionSecretActions.DescribeSecretto provide more granular control over secret access permissions.
602-609: Updated permission check to specifically control secret value access.The permission check in the filter function now uses
ProjectPermissionSecretActions.ReadValueto specifically verify that the user has permission to view the actual values of secrets, not just metadata.
646-663: Added viewSecretValue parameter to support secret visibility control.The
fnSecretsV2FromImportsfunction now includes aviewSecretValue: trueparameter, and the permission check usesProjectPermissionSecretActions.ReadValue. This change supports the ability to control whether secret values are visible or masked based on user permissions.
673-681: Updated permission check for secret value access in allowedImports filter.Similar to previous changes, this permission check now uses
ProjectPermissionSecretActions.ReadValuefor more granular control over who can access secret values.
690-695: Added secretValueHidden parameter to control secret value visibility.The
decryptSecretRawfunction call now includes asecretValueHidden: falseparameter, which indicates that the secret values should not be hidden when decrypted. This supports the new feature of controlling secret value visibility based on permissions.frontend/src/components/secrets/SecretReferenceDetails/SecretReferenceDetails.tsx (7)
1-8: Added necessary imports for error handling and UI elements.The imports have been updated to include FontAwesome icons for error display and eye icons for visibility toggling, which support the new error handling and visibility control features.
11-11: Added AxiosError import for API error handling.The addition of the AxiosError import allows for more specific error handling when API requests fail, supporting better user feedback.
14-14: Added notification import for user feedback.The import of createNotification enables the component to display user-friendly error messages when issues occur fetching the secret reference tree.
18-18: Updated imports to include error type definitions.Added imports for ApiErrorTypes and TApiErrors to support the enhanced error handling in the component.
95-100: Enhanced hook call to handle error states.The useGetSecretReferenceTree hook now captures isError and error states, which allows the component to provide better feedback when fetching the secret reference tree fails.
105-124: Added error handling with user notifications.This new useEffect hook provides better error handling by:
- Checking if the error is an AxiosError
- Displaying a specific notification for permission errors (CustomForbiddenError)
- Showing a generic error notification for other types of errors
This enhances the user experience by providing clear feedback about permission issues versus other types of errors.
145-154: Improved error state UI rendering.The component now conditionally renders an error message with an icon when isError is true, providing better visual feedback to users when there's a problem fetching the secret reference tree.
frontend/src/hooks/api/secrets/types.ts (5)
22-22: Added secretValueHidden flag to EncryptedSecret type.This new boolean property allows the application to track whether a secret's value should be hidden from the user based on their permissions, supporting the granular permission model introduced in this PR.
41-41: Added secretValueHidden flag to SecretV3RawSanitized type.Similar to the change in EncryptedSecret, this property enables controlling visibility of secret values in the sanitized version of secrets.
66-66: Added secretValueHidden flag to SecretV3Raw type.This property addition ensures consistency across all secret-related types, allowing the application to control visibility of secret values based on user permissions.
101-101: Added secretValueHidden flag to SecretVersions type.This change extends the visibility control to historical versions of secrets, ensuring that the permission model applies consistently to both current and previous secret values.
116-116: Added viewSecretValue parameter to TGetProjectSecretsKey type.This optional boolean parameter allows API requests to explicitly specify whether secret values should be returned or masked, supporting the new permission-based visibility control.
backend/src/services/service-token/service-token-service.ts (2)
8-12: Proper modularization of permission actionsThe import statement has been updated to include
ProjectPermissionSecretActionsfrom the project-permission module, which aligns with best practices for permission granularity.
74-76: Updated permission check to use secret-specific actionThe code now uses
ProjectPermissionSecretActions.Createinstead of the genericProjectPermissionActions.Createwhen validating permissions for secret creation. This change implements more granular permission control by using domain-specific permission actions.This permission model update ensures proper access control for secret-related operations, improving the security posture of the application by following the principle of least privilege.
frontend/src/pages/secret-manager/SecretDashboardPage/components/ActionBar/ActionBar.tsx (3)
24-24: LGTM - Added AxiosError import for error handlingThe added import enables proper type checking for API error responses.
58-58: LGTM - Enhanced error type importsImports now include
ApiErrorTypesandTApiErrorswhich support the improved error handling implementation.
156-220: Improved error handling for permission-related failuresThe implementation now properly handles the case when a user lacks the
readValuepermission while attempting to download secrets. This provides clear feedback to the user about permission issues instead of showing a generic error message.The error handling follows best practices by:
- Identifying specific error types (AxiosError)
- Checking for specific error conditions (ForbiddenError related to readValue permission)
- Providing appropriate user-facing messages
- Gracefully exiting the function when appropriate
- Using generic error handling as a fallback
This is a significant improvement to the UX when dealing with permission-related issues.
backend/src/services/secret/secret-dal.ts (2)
172-212: Well-implemented new function for retrieving secrets with tagsThe new
findManySecretsWithTagsfunction provides a clean way to retrieve multiple secrets with their associated tags in a single database query. This implementation supports the granular permission model being introduced elsewhere in the codebase.Key strengths of this implementation:
- Proper joining of related tables
- Appropriate error handling with specific error names
- Efficient data transformation using
sqlNestRelationships- Consistent error messaging with other DAL functions
488-489: LGTM - Export of new functionProperly exports the new
findManySecretsWithTagsfunction from the DAL factory.backend/src/services/project/project-service.ts (2)
14-18: Updated imports to support granular permission modelThe import statement now includes
ProjectPermissionSecretActionswhich supports the more fine-grained permission model being implemented throughout the system.
754-757: Permission check updated to use secret-specific actionThe permission check in
getProjectUpgradeStatushas been updated to useProjectPermissionSecretActions.DescribeSecretinstead of the genericProjectPermissionActions.Read. This provides more granular control over secret-related operations.This change is part of the broader initiative to enhance permission handling across the application by using domain-specific permission actions, which better follows the principle of least privilege.
frontend/src/pages/secret-manager/SecretDashboardPage/SecretDashboardPage.tsx (3)
107-114: Appropriate migration to more granular permission modelThe change from generic
ProjectPermissionActions.Readto the more specificProjectPermissionSecretActions.DescribeSecretis a good enhancement that provides finer-grained control over secret access. This separation of concerns helps enforce the principle of least privilege.
115-123: Good implementation of secret value viewing permissionThis new permission check specifically for reading secret values is an excellent security enhancement. It properly separates the permission to view metadata about a secret from the permission to view its actual value, which is a security best practice.
189-189: Successful integration of permission check with data fetchingThe addition of
viewSecretValue: canReadSecretValueparameter properly integrates the permission check with the data fetching mechanism, ensuring that secret values are only retrieved when the user has appropriate permissions.frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/components/MoveSecretsDialog/MoveSecretsDialog.tsx (2)
28-29: Clean update to imports for permission modelThe imports have been correctly updated to use the new secret-specific permission actions, which is consistent with the broader changes to the permission model across the application.
99-100: Proper migration to secret-specific permission checkThe change from
ProjectPermissionActions.DeletetoProjectPermissionSecretActions.Deletemaintains the same functionality while moving to the more specific permission model. This ensures consistent permission checking throughout the application.backend/src/services/secret/secret-version-dal.ts (3)
4-6: Appropriate import statements for new functionalityThe import statements have been correctly updated to include necessary types and utilities for the new function, including
SecretVersionsSchema,TSecretVersions, and additional utility functions likesqlNestRelationshipsandTFindOpt.
15-57: Well-implemented function for retrieving secret versionsThe
findBySecretIdfunction is thoroughly implemented with:
- Proper parameter handling including pagination and sorting
- Comprehensive query construction with appropriate table joins
- Well-structured data transformation using
sqlNestRelationships- Robust error handling that maintains consistency with other functions
The implementation follows best practices for database access and error management.
196-196: Proper export of new function in the returned objectThe new
findBySecretIdfunction is correctly added to the returned object, making it available for use by consumers of the factory.backend/src/ee/routes/v1/snapshot-router.ts (2)
3-3: Refined import statements for schema definitionsThe imports have been updated to remove
SecretTagsSchemaand addSanitizedTagSchema, reflecting the shift to use the sanitized tag schema throughout the application for consistency.Also applies to: 7-7
34-37: Enhanced schema with secret visibility controlThe response schema has been improved with:
- A new
secretValueHiddenboolean field that indicates whether the secret value is hidden- Updated
tagsfield to useSanitizedTagSchema.array()for consistent tag handlingThese changes properly support the new permission model that distinguishes between viewing secret metadata and secret values.
backend/src/services/secret-import/secret-import-fns.ts (8)
6-6: Import added for secret value hiding functionality.The import of
INFISICAL_SECRET_VALUE_HIDDEN_MASKis properly added to support the new feature of conditionally displaying or hiding secret values based on permissions.
35-41: Good addition of secretTags structure to support tag-based permissions.The addition of the
secretTagsarray to theTSecretImportSecretsV2type is well-structured with appropriate fields. This will allow for tag-based permission filtering, which aligns with the permission system enhancement.
49-49: Added secretValueHidden flag to support permission-based value visibility.This boolean flag is a good approach to indicate whether the secret value should be hidden from the user due to permission restrictions.
161-162: New parameter added to support permission-based secret visibility.The
hasSecretAccessparameter is already present, and nowviewSecretValueis added as a new parameter to control the visibility of secret values, which aligns well with the granular permission model.
168-168: Parameter type declaration added correctly.The
viewSecretValue: booleanparameter is properly typed in the function interface.
181-188: Updated stack type definition with new properties.The
stacktype has been correctly updated to include the newsecretValueHiddenandsecretTagsproperties, ensuring type safety throughout the function.
247-249: Conditional masking of secret values based on permission.This implementation correctly:
- Uses the
viewSecretValueflag to determine whether to show the actual decrypted value or a mask- Sets the
secretValueHiddenflag appropriately- Adds the
secretTagsto support tag-based permissionsThis is a solid implementation of permission-based secret value visibility.
287-288: Skip processing hidden secret values.This check prevents unnecessary processing of secret references when the secret value is hidden, which is an efficient approach. This also prevents potential information leakage by not attempting to expand references for secrets the user shouldn't see.
backend/src/services/secret-v2-bridge/secret-version-dal.ts (4)
4-4: Schema import updated to include SecretVersionsV2Schema.The import statement has been updated correctly to import the necessary schema for the new functionality.
6-6: Added necessary imports for SQL relationship handling.The import now includes
sqlNestRelationshipsandTFindOptwhich are needed for the newfindBySecretIdfunction to properly query and structure the results.
15-65: Well-implemented findBySecretId function with proper SQL joins and error handling.This function is well-structured with:
- Clear parameter definitions including pagination support
- Proper SQL joins to get related tag information
- Correct selection of columns with appropriate aliases
- Strong error handling with appropriate error types
- Proper use of the
sqlNestRelationshipsfunction to nest the resultsThe implementation follows the repository pattern consistently and aligns with the codebase's existing patterns.
179-180: Added the new function to the return object.The
findBySecretIdfunction is properly exported as part of the DAL factory, making it available for use by other services.frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretItem.tsx (9)
1-1: Properly enabled nested ternary for complex conditional rendering.The ESLint rule for no-nested-ternary has been disabled, which is appropriate given the complexity of the conditional rendering needed for handling secret visibility states.
48-49: Added necessary imports for permission model and UI blurring.The imports for
ProjectPermissionSecretActionsandBlurare correctly added to support the enhanced permission model and to provide visual indication of hidden secrets.
105-112: Form values updated to handle hidden secret values.The form's
defaultValuesandvaluesare correctly updated to handle the case when a secret value is hidden, ensuring the form behaves appropriately when the user lacks permissions to view the secret value.
133-151: Updated permission checks to use more granular secret actions.The permission checks now use
ProjectPermissionSecretActions.DescribeSecretandProjectPermissionSecretActions.Editinstead of the more generalProjectPermissionActions, which aligns with the more granular permission model being implemented.
153-154: Added secretValueHidden property extraction.The
secretValueHiddenproperty is correctly extracted from the secret object, making it clearer what's controlling the visibility logic.
286-288: Implemented conditional rendering based on secret visibility permissions.The code now correctly:
- Shows a blur component with helpful tooltip when the user doesn't have permission to see the secret value
- Uses the
InfisicalSecretInputcomponent otherwise- Ensures proper messaging to users about why they can't see certain values
This implementation provides a good user experience by clearly communicating permission limitations.
301-302: Correctly handling defaultValue in secret input.Setting
defaultValueto an empty string whensecretValueHiddenis true ensures the component doesn't accidentally reveal sensitive information.
310-311: Disabled copy functionality for hidden secrets.The copy secret button is now properly disabled when the secret value is hidden, preventing unauthorized access to sensitive information.
518-519: Disabled share functionality for hidden secrets.The share secret button is correctly disabled when
secretValueHiddenis true, preventing the sharing of secrets that the user doesn't have permission to view.frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx (7)
12-13: Added font awesome icon for warning display.The
faTriangleExclamationicon has been added to visually indicate permission-related warnings to users.
49-50: Imported ProjectPermissionSecretActions for granular permission control.The import allows the component to use the more specific secret-related permission actions.
127-135: Updated permission check to use secret-specific actions.The permission check for editing secrets has been updated to use
ProjectPermissionSecretActions.Editinstead of the more general action, aligning with the granular permission model.
137-146: Added permission check for reading secret values.A new permission check
cannotReadSecretValuehas been added to determine if the user has permission to view the actual secret value. This is properly implemented using theProjectPermissionSecretActions.ReadValueaction.
147-158: Updated read-only determination logic to include value reading permission.The
isReadOnlyvariable now correctly considers both edit permissions and read value permissions, ensuring that the UI reflects the user's actual capabilities.
287-338: Enhanced UI to provide feedback for permission restrictions.This section significantly improves the user experience by:
- Adding clear warning messages when users don't have permission to view secret values
- Using appropriate warning icons
- Including descriptive text explaining the permission issue
- Maintaining the form structure with proper FormControl components
- Disabling share functionality for hidden secrets
The implementation is thorough and user-friendly.
669-816: Updated version history display to respect secretValueHidden property.The version history section has been properly updated to:
- Include the
secretValueHiddenproperty in the map function parameters- Conditionally display masked or actual values based on permissions
- Prevent copying of hidden values by checking permissions in event handlers
- Use tooltips to explain why values are hidden
- Apply visual styling to distinguish hidden values
This comprehensive implementation ensures consistent permission enforcement throughout the user interface.
backend/e2e-test/routes/v2/service-token.spec.ts (4)
9-9: Add "readValue" permission to type declaration.This addition expands the permission set, allowing more granular control over secret value readability. No immediate issues detected; ensuring consistent usage across the codebase is recommended.
142-142: Include "readValue" in service token creation.Adding
"readValue"here aligns with the updated type declaration. Verify all downstream references handle this new permission correctly.
521-521: Reinforce "readValue" in unauthorized scenario.Same observation regarding permission order and consistency. The usage is valid, but verify all references to ensure consistent test coverage.
543-543: Non-write token lacks "write" permission.Using only
["read", "readValue"]is appropriate when disallowing secret creation/updates. Good clarity in permission scoping for read-only usage.backend/src/services/secret/secret-fns.ts (6)
16-16: Refactor import to specialized secret permissions.Switching to
ProjectPermissionSecretActionsindicates finer-grained control for secret operations. Ensure all references to older permission enums are updated for consistency.
54-54: Centralized mask constant introduced.Defining
INFISICAL_SECRET_VALUE_HIDDEN_MASKas a constant promotes clarity and reduces duplication. Great for consistent masking across the codebase.
195-195: Use ofProjectPermissionSecretActions.ReadValuefor environment paths.Replacing or adding the
ReadValueaction clarifies that reading the secret path is distinct from reading the secret value. Ensure permission logic is tested thoroughly (especially for partial read privileges).
349-349: New boolean field for secretValueHidden.Introducing
secretValueHiddenin the type signals a design shift toward partial or masked secret retrieval. Confirm all existing references to secrets handle this new field correctly.
393-393: ExposesecretValueHiddenin decrypted object.This field helps clients differentiate between truly hidden values and empty strings. Ensure logging or external outputs do not inadvertently reveal the real secret value.
1206-1225: New helper functionconditionallyHideSecretValue.Encapsulating the hiding logic is a clean approach. Having a dedicated function prevents accidental misuse of mask logic. Consider ensuring consistent usage site-wide for uniform security.
backend/src/ee/services/permission/project-permission.ts (9)
20-27: New secret-specific enumerations.Defining
ProjectPermissionSecretActionsclarifies the difference between reading secret metadata (DescribeSecret), reading values (ReadValue), and conventional create/edit/delete. This is a positive shift toward finer-grained permissions.
126-126: Incorporate secret actions into ProjectPermissionSet.Updating the permission set to handle
ProjectPermissionSecretActionsensures the new actions can be recognized in type checks and CASL rules. Looks consistent with the new enum.
472-472: V2 schema references new secret actions.Using
CASL_ACTION_SCHEMA_NATIVE_ENUM(ProjectPermissionSecretActions)ensures the schema accurately reflects enhanced secret permissions. Verify migration or fallback paths for older references.
565-575: Grant full secret access for admins.Adding the new secret actions (
DescribeSecret,ReadValue, etc.) for admins is consistent with an admin’s full privileges. Implementation appears aligned with the rest of the code.
635-639: Member permissions on secrets.This block parallels the admin’s permissions but is more restricted where appropriate. The explicit mention of new actions is well-handled.
811-812: Viewer role: separate "DescribeSecret" from "ReadValue".Granting both clarifies that a viewer can see minimal secret metadata while also reading the actual value. Consider ensuring that partial retrieval is still validated if needed.
854-855: Detect "read" vs. "readValue" in service token scopes.The variables
canReadandcanReadValuecomplement each other to differentiate reading secret metadata from reading the actual secret. This approach appears aligned with the new permission model.
887-893: Conditionally allow reading secret values.Checking if
subject === ProjectPermissionSub.Secrets && canReadValueproperly gates theReadValueaction. This logic is straightforward and easy to follow.
1002-1002: AddingsecretPoliciesto final data array.Including
secretPoliciesensures new secret checks (especiallyreadValue) flow into the backfill schema. Confirm no overlap or double application of older permission sets.frontend/src/pages/project/RoleDetailsBySlugPage/components/ProjectRoleModifySection.utils.tsx (3)
12-12: Adoption of new secret-specific actions is consistent.
ImportingProjectPermissionSecretActionsis a clean way to establish more granular permission checks for secret operations.
126-132: Granular secrets policy extension looks good.
Switching toSecretPolicyActionSchemafor[ProjectPermissionSub.Secrets]matches the new approach for fine-grained secret permissions.
295-315: Well-structured conditional block for Secrets.
The code correctly checks each secret permission (includingreadValue) and populatesformVal[subject]accordingly. This ensures reliable form data representation of secret-related permissions.backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts (4)
25-29: Granular permission imports pave the way for refined secret access control.
ImportingProjectPermissionSecretActionsalongside existing artifacts neatly supports more targeted permission checks for snapshots.
41-41: Importing mask constant is consistent with new read-value permission logic.
EnsuringINFISICAL_SECRET_VALUE_HIDDEN_MASKis available promotes secure placeholders whenever users lack read-value permission.
105-105: Strengthening folder-level access checks with 'DescribeSecret'.
Switching from a generic read check toProjectPermissionSecretActions.DescribeSecretensures users cannot view snapshot details without explicit permission. This is a more accurate reflection of the new secret-specific permission model.Also applies to: 142-142
178-183: Obtaining the folder’s full path is critical for permission checks.
Retrieving the entire folder path before evaluating access ensures that condition-based permissions (e.g., on secret paths) are accurately enforced.Also applies to: 220-226
frontend/src/hooks/api/secrets/queries.tsx (4)
29-35: Extending the query key to include 'viewSecretValue' is well-handled.
AddingviewSecretValueclarifies user intent when fetching secrets and aligns with backend logic for controlling value visibility.
52-61: Routing 'viewSecretValue' to the API request ensures proper control.
Consistently passing the parameter inparamsallows the backend to determine whether or not secret values should be revealed.
78-78: Tracking 'secretValueHidden' in the merged secrets is logical.
Storing the hidden state in the front-end data structure ensures UI components can accurately conceal or show secrets based on permissions.
118-119: Incorporating 'viewSecretValue' into 'useGetProjectSecrets' is coherent.
This aligns the hook's signature with the new parameter usage, ensuring that queries correctly account for secret visibility preferences.Also applies to: 135-140
backend/src/services/secret/secret-types.ts (2)
210-210: Ensure Consistency ofviewSecretValueUsageA second occurrence of
viewSecretValueis introduced. Verify that all function calls respect this flag and handle defaulting appropriately in case it’s missing or inconsistent with other definitions.
452-452: Validate Expanded DAL InterfaceSimilarly here, ensure you implement any necessary caching or concurrency considerations around the new
findoperation for better performance and maintainability.backend/src/server/routes/v1/dashboard-router.ts (15)
4-4: Importing Additional SchemasBringing in
ActionProjectType,SecretFoldersSchema, andSecretImportsSchemalooks consistent with the new logic. No issues spotted.
8-8: AddedProjectPermissionSecretActionsImportImporting
ProjectPermissionSecretActionsaligns with the new permission checks for secret values. This is good for fine-grained control.
19-19: Switch to Sanitized SchemasReplacing or adding schema imports with
SanitizedDynamicSecretSchema,SanitizedTagSchema, andsecretRawSchemahelps ensure consistent data validation and potentially avoids leaking sensitive data. Good update.
120-120: New Boolean FieldsecretValueHiddenThis property indicates obfuscation of the secret value. Ensure the client interprets this field correctly (e.g., rendering placeholders).
123-123: Optional Tags ArrayAllowing an optional
tagsarray is clear. This aligns with the changes for better tag-based filtering and classification.
292-292: DefaultingviewSecretValuetotrueBy default, all secret values will be revealed. Please confirm if this is intended from a security perspective, as it may heighten the risk of exposing sensitive information.
392-392:viewSecretValueDefaulted totruein SchemaSimilar concern here; default to
truemeans viewing secret values is the norm. Double-check necessity for read permissions in all relevant routes.
410-410: AdditionalsecretValueHiddenin Extended SchemaMaintaining the same
secretValueHiddenpattern across multiple endpoints is consistent. Ensure the front-end respects this in all data flows.
413-413: Optional Tags FieldNo issues with the optional array for tags here; consistent with the rest of the code.
595-613: PassingviewSecretValueandthrowOnMissingReadValuePermissionWhen calling
getSecretsRaw, you now provideviewSecretValueand disable throwing an error if read permission is missing. Verify that permission logic is robust enough to avoid inadvertently returning sensitive data.
853-855: DefaultingviewSecretValuetofalsefor “Secrets by Keys”This route takes a stricter default approach. This is safer from a security standpoint but be mindful of potential mismatch with other endpoints whose defaults are
true.
860-860:secretValueHiddenfor Secret DetailsEnsuring a boolean field that indicates secret concealment. This is consistent with broader code changes for secret masking.
863-863: Optional Tags for Secret DetailsTag usage remains consistent, enabling improved classification or search.
872-872: ExtractviewSecretValuefrom QueryIncluding
viewSecretValuein this route ensures consistent usage of the permission-based reveal logic. Implementation looks straightforward.
881-881: PropagatingviewSecretValueDownstreamPassing the parameter correctly ensures uniform secret-value visibility rules throughout the codebase.
backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts (5)
10-10: Imported Mask Constant
INFISICAL_SECRET_VALUE_HIDDEN_MASKimport is aligned with the new logic to obscure secret values. This is helpful for centralized handling.
298-307: Post-Update Retrieval of Secrets with TagsSimilar to bulk insert, after a bulk update, fetching secrets with tags keeps data consistent. This is a good inclusion for final data integrity.
539-540: Refined Forbidden Request Error MessageImproving clarity by naming the environment, path, and key in the error message helps identify permission issues more quickly. Good update.
558-559: Improved Error Message for Referenced SecretsSame improvement here. This reduces debugging overhead for missing or unauthorized references in different environments/folders.
647-676: Conditional Secret Value MaskingThe new
secretValueHiddenparameter gates whether the actual value or a masked placeholder is returned. This is a robust approach for enforcing partial visibility while still surfacing meta-info (e.g., presence of the secret).backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts (5)
61-61: Updated permission import to use secret-specific actions.The code now imports
ProjectPermissionSecretActionsfor more granular control over secret-related permissions, supporting the new "view secret value" permission feature.
90-97: EnhancedsecretTagDALinterface withfindmethod.The
secretTagDALinterface has been expanded to include afindmethod, enabling more flexible data retrieval in the secret tag management flow.
114-114: Addedfindcapability tosecretV2BridgeDALinterface.The interface for the V2 bridge DAL now includes a
findmethod, providing consistent capability with the tag DAL and enabling more flexible queries.
922-924: Updated permission check to use secret-specific action.The permission check now uses the more specific
ProjectPermissionSecretActions.ReadValueinstead of a generic read permission, which is aligned with the feature of controlling access to view secret values.
1372-1375: Updated permission action mapping for secret operations.The code now uses the more granular
ProjectPermissionSecretActionsenum for different secret operations (Create, Edit, Delete), providing better permission clarity and control.backend/src/server/routes/v3/secret-router.ts (20)
4-4: Updated imports to remove SecretTagsSchema.The import statements now directly use SecretsSchema and related imports without including the removed SecretTagsSchema.
20-20: Updated import to use SanitizedTagSchema.The import now references SanitizedTagSchema from sanitizedSchemas, aligning with the schema changes for better tag representation.
29-34: Added utility function for boolean query parameter conversion.This is a well-designed utility function that standardizes the conversion of string boolean values in query parameters, improving code consistency and readability.
80-82: Updated schema to use extend instead of merge.Changed from
.merge({ tags: ... })to.extend({ tags: ... })and updated to use SanitizedTagSchema for consistency.
136-136: Updated tag schema in response object.Changed to use SanitizedTagSchema.array() for consistent tag representation across the API.
238-242: Improved boolean parameter handling in query string definition.Refactored to use the new
convertStringBooleanutility function for consistent handling of boolean parameters, with default values aligned with expected behavior.
254-254: Added secretValueHidden flag to response schema.This is a key change that supports the "view secret value permission" feature by explicitly indicating when a secret value is hidden due to permissions.
256-256: Updated tags schema in response schema.Changed to use SanitizedTagSchema.array() for consistency with other changes.
267-267: Added secretValueHidden to imported secrets schema.Ensures consistent handling of secret value visibility across both direct and imported secrets.
380-382: Improved boolean parameter handling for single secret query.Updated to use the new
convertStringBooleanutility for consistent parameter handling in the single secret retrieval endpoint.
387-388: Updated response schema for single secret retrieval.Added secretValueHidden flag and updated tags schema for consistency with the listing endpoint.
420-420: Added viewSecretValue parameter to getSecretByNameRaw call.This passes the visibility parameter to the service layer, ensuring consistent permission handling.
627-629: Added secretValueHidden to update response schema.This ensures that the update operation properly indicates whether the secret value is hidden in the response.
908-908: Updated include_imports parameter definition.Changed to use the new
convertStringBooleanutility for consistent parameter handling.
1179-1179: Added secretValueHidden to PATCH response schema.Ensures that the PATCH endpoint properly indicates when a secret value is hidden due to permissions.
1349-1354: Updated DeleteSecret response schema structure.Improved the response schema to include secretValueHidden flag and use extend() for better type composition.
1666-1666: Added secretValueHidden to batch update response schema.Ensures consistent handling of secret value visibility in the batch update operation.
1783-1784: Added secretValueHidden to batch delete response schema.This completes the consistent handling of secret value visibility across all operations.
2047-2047: Updated batch/raw response schema with secretValueHidden.Ensures consistent handling of secret value visibility in the raw batch API responses.
2169-2173: Updated batch/raw delete response schema.Improved the structure to consistently include secretValueHidden flag in the response.
backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts (9)
4-4: Added import for ProjectPermissionSecretActions.This import supports the new granular permissions for secret management operations.
40-41: Added viewSecretValue and exception control to TGetSecretsDTO.Two important additions:
viewSecretValue: Controls whether to return secret values based on permissionsthrowOnMissingReadValuePermission: Controls error behavior when permissions are insufficientThese are crucial for implementing the "view secret value permission" feature.
54-57: Added new DTO type for missing value permission case.This type helps handle the special case when a user doesn't have permission to view secret values but can still access other secret data, creating a better typed interface for this scenario.
68-68: Added viewSecretValue to single secret DTO.This ensures consistent handling of secret value visibility permissions for both list and individual secret operations.
176-176: Enhanced secretDAL interface with find method.This extension to the interface supports more flexible queries in the bulk insert operation.
178-178: Enhanced secretTagDAL interface with find method.Added the find method to the tag DAL interface, providing consistent querying capabilities.
200-200: Enhanced secretDAL interface for bulk updates.Added the find method to support more flexible querying in bulk update operations.
202-202: Enhanced secretTagDAL interface for bulk updates.Added the find method to provide consistent querying capabilities in the bulk update flow.
344-344: Added filterByAction to folder mappings DTO.This parameter enables filtering secrets by specific permission actions, which is essential for implementing the granular "view secret value" permission model.
backend/src/services/secret/secret-service.ts (33)
9-9: Well-structured import for version tracking.
ImportingProjectVersionhelps differentiate logic by project version. No issues found here.
17-21: Clear separation of secret permissions.
IntroducingProjectPermissionSecretActionsandProjectPermissionSubis a solid move toward more granular permission handling. The import is consistent with the rest of the codebase.
213-213: Granular create permission usage.
Refined permission check usesProjectPermissionSecretActions.Create; looks correct and consistent.
331-331: Accurate edit permission usage.
Replaced or extended previous permission checks withProjectPermissionSecretActions.Edit. This is logical and well-aligned to the new model.
453-469: Conditional hiding of updated secret value.
Excellent approach to controlling visibility based onReadValuepermission.conditionallyHideSecretValueis invoked correctly, and the returned object merges hidden data when appropriate.
491-493: Appropriate delete permission check.
Switching toProjectPermissionSecretActions.Deletefits the new granular actions approach.
564-576: Hiding deleted secret's value.
Again usingProjectPermissionSecretActions.ReadValueto mask sensitive info. Clean, consistent pattern.
625-627: Read permission check for non-recursive scenario.
This ensures only authorized users can view the secret’s value.
650-655: Read permission check for imports.
Strictly gating import-based secret value access. Excellent.
707-709: Permission gate for single secret retrieval.
Ensures proper authorization for reading a single secret’s value.
757-762: Read permission check for imported fallback retrieval.
Reinforces security across imports. Good consistency.
775-775: Forcefully setting secretValueHidden = false.
Since the code already gates access above, returningsecretValueHidden: falsehere is consistent.
786-792: Consistently sets secretValueHidden false.
The inline comment clarifies that read permission was already checked. Well-documented.
814-816: Bulk create permission check.
Mirrors single-create approach withProjectPermissionSecretActions.Create.
902-904: Bulk update permission check.
GranularProjectPermissionSecretActions.Editensures consistent usage.
943-985: Transactional bulk update with hidden-value logic.
CombiningfnSecretBulkUpdateinside a transaction is good for atomicity. The final map to hide the secret value is correct.
1000-1002: Bulk delete permission.
Ensuring the user can delete secrets in a bulk operation. Straightforward.
1072-1080: Value hiding on bulk deletion.
Applies the same read-value check at deletion time, consistent with the rest of the service.
1303-1303: Defaulting throwOnMissingReadValuePermission.
Providing a default oftruemakes sense for strict security by default.
1314-1315: Introducing viewSecretValue parameter.
Enhances flexibility when retrieving secrets with partial or no value.
1353-1353: Decryption with forced visible state.
SettingsecretValueHidden: falseensures the raw data is processed, pending further checks.
1360-1366: Populating environment and workspace for imports.
This ensures consistent output structure across secrets and imports.
1421-1421: Skipping expansion for hidden secrets.
Quick early return prevents unnecessary reference expansions.
1438-1438: Skipping interpolation if value is hidden.
Same logic as above, properly short-circuits expansion.
1464-1464: Apply viewSecretValue in raw fetch.
Coordinates well with the bridging logic in v2.
1503-1503: Respect includeImports in getSecretByNameRaw.
Consistent with how other methods handle imports.
1534-1538: Returning decrypted secret with undefined metadata.
Aligns with the existing pattern to unify function returns.
1688-1698: Decrypting newly created secret.
Directly returning the decrypted secret withsecretValueHidden: falseis consistent after the user obtains permission.
1834-1834: Metadata inclusion in update params.
Ensures the update allows for extended metadata usage.
2401-2437: Masked secret versions retrieval.
Usage ofsecretValueHiddenifReadValueis denied. This securely handles historical data.
2462-2464: Attach Tags permission check.
Relies onEditpermission, consistent with previous usage.
2568-2570: Detach Tags permission check.
Likewise ensuresEditpermission for removing tags.
2786-2842: Move secrets with robust checks.
Enforcing relevant permissions for source/destination actions underProjectPermissionSecretActionsis thorough. Good job verifying keys and values before overwriting.backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (40)
14-18: Newly imported permissions look good.
This import aligns with the updated secret-based permission model.
80-86: Expanded folderDAL picks.
Adding these folderDAL methods seems consistent with the new multi-env flows.
264-272: Switched to secret-specific create action.
Replacing the generic action withProjectPermissionSecretActions.Createis consistent with the new permission model.
284-317: Transactional secret creation logic.
Using a transaction for bulk insert is correct and maintains atomicity.
411-411: Use of Edit action is correct.
Replacing the old action withProjectPermissionSecretActions.Editfor updates is consistent.
422-426: Tag retrieval check.
The logic to verify the existence of all referenced tags is valid.
430-439: Edit permission usage.
CheckingProjectPermissionSecretActions.Editbefore updating the secret is appropriate.
449-458: Ensuring new secret name has Edit permission.
This ensures that name changes are properly secured.
534-545: Computing secretValueHidden for updated secret.
Good call on verifyingReadValuepermission to decide if the secret value is masked.
546-556: Return updated secret with masked or unmasked value.
This consistent approach helps avoid accidental leaks.
602-602: Delete permission usage.
Switching toProjectPermissionSecretActions.Deleteis consistent with the new model.
645-653: Check readValue permission when deleting.
Masking or revealing the secret value upon deletion is correct.
655-669: Reshape deleted secret with proper visibility.
Returning the final shape with the hidden or revealed value is consistent.
697-700: Describing secrets permission.
UsingProjectPermissionSecretActions.DescribeSecretensures minimal data exposure.
748-750: Again verifying describe permission.
Ensures only authorized actors can enumerate secrets.
761-767: New filterByAction param for retrieving secrets.
Providing a default ofReadValueis logical for the secret retrieval context.
787-787: Conditional permission check.
Passing the customfilterByActionallows dynamic read vs. describe checks.
797-807: Local note about filterAction logic.
This helps differentiate between reading and merely describing secrets.
808-823: Reshape secret with hidden or revealed value.
Implementation matches the new permission-based approach.
852-855: Describe permission required.
Ensures users can’t list secrets without proper access.
908-915: Contextual permission check on environment + path.
Safeguarding the “getSecrets” operation viaDescribeSecretis appropriate.
953-1002: Granular filtering of secrets by describe/readValue checks.
This approach correctly masks secrets if the user lacks READ_VALUE.
1018-1033: Integrates personal secret logic.
Unmasking for the personal secret’s owner is correct.
1042-1042: Expanded readValue check.
Adheres to the updated secret-based permission structure.
1210-1210: ReadValue usage for expansions.
Ensures references are only expanded if the user can view them.
1309-1331: Conditional unmasking based on user’s permission.
This prevents secret exfiltration for unauthorized viewers.
1333-1345: Returning a properly shaped secret.
Ensures consistent structure for single-secret retrieval.
1413-1413: Create permission for single secret in upsert flow.
Consistent with the approach used in other methods.
1630-1638: Create permission in upsert mode.
Verifies the actor can create secrets that don’t previously exist.
1644-1652: Edit permission checks for updated tags.
Good that it re-verifies authorized tags even in bulk updates.
1696-1704: Create permission for new secret name.
Properly restricts collisions and ensures an actor can rename.
1814-1839: Masking secrets after bulk update.
ApplyingReadValuelogic across multiple secrets is consistent.
1901-1901: Delete permission for bulk removal.
Correctly referencesProjectPermissionSecretActions.Delete.
1941-1968: Deleted secrets visibility.
Ensures you only see the secret if you have readValue permission.
1987-1991: Locating folder path for secret versions.
This step helps build the path context for subsequent checks.
2007-2012: Fetching secret versions with offset & limit.
Pagination approach is standard and correct.
2013-2035: Mask older secret versions if user lacks readValue.
Ensures historical data is also protected.
2131-2137: Aggregated actions for source secrets.
Deleting, reading, or describing from the source folder is enforced properly.
2140-2147: Source permission checks in loop.
Good usage of CASL to confirm all necessary actions.
2224-2234: Verifying create/edit for destination secrets during move.
Applying the new enumerated actions ensures consistent handling.
DanielHougaard
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (1)
334-343:⚠️ Potential issuePotential leak of newly created secrets to users without readValue permission.
This method still hardcodes
falsefor thesecretValueHiddenflag, returning the unmasked secret value regardless of user permissions. This is inconsistent with other methods in the file (likeupdateSecretat line 535) that properly check theProjectPermissionSecretActions.ReadValuepermission.Apply this fix to address the issue:
- false + !CheckCanSecretsSubject(permission, ProjectPermissionSecretActions.ReadValue, { + environment, + secretPath, + secretName, + secretTags: tags?.map((el) => el.slug) + })
🧹 Nitpick comments (11)
backend/src/services/secret/secret-fns.ts (4)
54-54: Good introduction of a masked value constant.
DefiningINFISICAL_SECRET_VALUE_HIDDEN_MASKis a clear approach to obfuscate sensitive values. Ensure that any alternative or placeholder text is consistently used in logs or user-facing UI to avoid confusion.
344-344: ValidatesecretValueHiddenusage.
Declaring this boolean in the extended type helps convey when a secret is masked. Ensure it is always set (either true or false) at object creation to avoid runtime issues.Also applies to: 346-346
390-390: Clarify the purpose of exposing thesecretValueHiddenflag.
Including this property in the returned object can help the frontend or other consumers handle masked secrets. Make sure it’s not inadvertently displayed or logged in user-facing interfaces unless intended.
1203-1222: Refine the helper function for hiding ciphertext.
conditionallyHideSecretValueis well-scoped. For added clarity, consider documenting each returned property in JSDoc and verifying usage across different secret-handling flows for consistency.backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts (1)
142-145: Consistent usage ofDescribeSecretfor snapshot operations.
Applying the same permission check again at line 142 ensures both listing and subsequent actions are gated. This is good for consistency; be mindful of performance overhead if invoked multiple times for large snapshot queries.backend/src/ee/services/permission/permission-fns.ts (3)
1-4: File-wide ACL enhancements are valuable.
Disabling certain ESLint rules in the header is acceptable if necessary. Confirm that the justification remains valid so the code doesn’t become a “lint escape hatch.”
9-15: Refined type definitions for secret permissions.
Expanding the permission schema withProjectPermissionSecretActionsandSecretSubjectFieldsfosters clarity. Consider adding inline documentation for each field to assist future maintainers.
46-72: Enhanced boolean check withCheckCanSecretsSubject.
Allowing an OR condition between the new layered permissions and the olderDescribeAndReadValuekeeps backward compatibility. Consider logging or otherwise surfacing if an older permission path was used, to facilitate a smoother migration away from legacy permission sets.backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (3)
772-794: Improved permission checking logic, but inconsistent approach between conditions.The function correctly filters secrets based on user permissions, but uses different approaches for different permission types. For
ReadValueandDescribeSecret, it uses theCheckCanSecretsSubjectfunction, but for other actions it directly usespermission.can. This makes the code harder to maintain.Consider standardizing the approach by using
CheckCanSecretsSubjectfor all permission checks:if ( filterByAction === ProjectPermissionSecretActions.ReadValue || filterByAction === ProjectPermissionSecretActions.DescribeSecret ) { return CheckCanSecretsSubject(projectPermission, filterByAction, { environment: groupedFolderMappings[el.folderId][0].environment, secretPath: groupedFolderMappings[el.folderId][0].path, secretName: el.key, secretTags: el.tags.map((i) => i.slug) }); } - return projectPermission.can( - filterByAction, - subject(ProjectPermissionSub.Secrets, { - environment: groupedFolderMappings[el.folderId][0].environment, - secretPath: groupedFolderMappings[el.folderId][0].path, - secretName: el.key, - secretTags: el.tags.map((i) => i.slug) - }) - ); + return CheckCanSecretsSubject(projectPermission, filterByAction, { + environment: groupedFolderMappings[el.folderId][0].environment, + secretPath: groupedFolderMappings[el.folderId][0].path, + secretName: el.key, + secretTags: el.tags.map((i) => i.slug) + });
990-1013: Consider refactoring conditional personal secret logic for clarity.The
secretValueHiddenlogic includes anisPersonalSecretcheck at line 1012, but the check logic is defined several lines earlier. This makes the relationship between these two parts less obvious.Consider refactoring for better readability:
const isPersonalSecret = secret.userId === actorId && secret.type === SecretType.Personal; const secretValueHidden = !viewSecretValue || (!isPersonalSecret && !CheckCanSecretsSubject(permission, ProjectPermissionSecretActions.ReadValue, { environment, secretPath: groupedPaths[secret.folderId][0].path, secretName: secret.key, secretTags: secret.tags.map((i) => i.slug) })); return reshapeBridgeSecret( projectId, environment, groupedPaths[secret.folderId][0].path, { ...secret, value: secret.encryptedValue ? secretManagerDecryptor({ cipherTextBlob: secret.encryptedValue }).toString() : "", comment: secret.encryptedComment ? secretManagerDecryptor({ cipherTextBlob: secret.encryptedComment }).toString() : "" }, - secretValueHidden && !isPersonalSecret + secretValueHidden );
2084-2108: Inconsistent permission checking in moveSecrets.The function uses a mix of
CheckForbiddenErrorSecretsSubjectandForbiddenError.from(permission).throwUnlessCanfor checking permissions. A more consistent approach would improve maintainability.Consider using the
CheckForbiddenErrorSecretsSubjecthelper consistently for all checks:for (const sourceAction of sourceActions) { - if ( - sourceAction === ProjectPermissionSecretActions.DescribeSecret || - sourceAction === ProjectPermissionSecretActions.ReadValue - ) { - CheckForbiddenErrorSecretsSubject(permission, sourceAction, { - environment: sourceEnvironment, - secretPath: sourceSecretPath, - secretName: secret.key, - secretTags: secret.tags.map((el) => el.slug) - }); - } else { - ForbiddenError.from(permission).throwUnlessCan( - sourceAction, - subject(ProjectPermissionSub.Secrets, { - environment: sourceEnvironment, - secretPath: sourceSecretPath, - secretName: secret.key, - secretTags: secret.tags.map((el) => el.slug) - }) - ); - } + CheckForbiddenErrorSecretsSubject(permission, sourceAction, { + environment: sourceEnvironment, + secretPath: sourceSecretPath, + secretName: secret.key, + secretTags: secret.tags.map((el) => el.slug) + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
backend/src/ee/routes/v2/project-role-router.ts(2 hunks)backend/src/ee/services/permission/permission-fns.ts(1 hunks)backend/src/ee/services/permission/project-permission.ts(11 hunks)backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts(7 hunks)backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts(7 hunks)backend/src/services/integration/integration-service.ts(3 hunks)backend/src/services/project/project-service.ts(2 hunks)backend/src/services/secret-import/secret-import-service.ts(7 hunks)backend/src/services/secret-sync/secret-sync-service.ts(3 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts(45 hunks)backend/src/services/secret/secret-fns.ts(7 hunks)backend/src/services/secret/secret-service.ts(42 hunks)frontend/src/context/ProjectPermissionContext/types.ts(2 hunks)frontend/src/lib/fn/permission.ts(1 hunks)frontend/src/pages/project/AccessControlPage/components/ServiceTokenTab/components/ServiceTokenSection/AddServiceTokenModal.tsx(2 hunks)frontend/src/pages/project/RoleDetailsBySlugPage/components/GeneralPermissionPolicies.tsx(2 hunks)frontend/src/pages/project/RoleDetailsBySlugPage/components/ProjectRoleModifySection.utils.tsx(7 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretRenameRow.tsx(2 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/SecretDashboardPage.tsx(4 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx(8 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretItem.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/src/pages/project/RoleDetailsBySlugPage/components/GeneralPermissionPolicies.tsx
- frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretRenameRow.tsx
- frontend/src/pages/secret-manager/SecretDashboardPage/SecretDashboardPage.tsx
- backend/src/services/project/project-service.ts
- frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx
- backend/src/services/integration/integration-service.ts
- frontend/src/pages/project/AccessControlPage/components/ServiceTokenTab/components/ServiceTokenSection/AddServiceTokenModal.tsx
🔇 Additional comments (59)
frontend/src/pages/project/RoleDetailsBySlugPage/components/ProjectRoleModifySection.utils.tsx (3)
30-37: Well-defined schema for granular secret permissions.The new
SecretPolicyActionSchemaprovides a clear separation of permissions related to secrets, distinguishing between describing a secret and reading its value. This is a good security practice that follows the principle of least privilege.
301-325: Improved permission handling for secrets.The updated permission conversion logic now properly handles the new granular secret actions. This ensures that permissions are correctly processed when converting between the form representation and the API model.
527-556: Good UX for legacy permission transition.The tooltip on the legacy "Read" action clearly communicates to users that they should transition to using the more granular "Describe Secret" and "Read Value" actions. This provides a smooth migration path while maintaining backward compatibility.
backend/src/ee/routes/v2/project-role-router.ts (1)
96-99:❓ Verification inconclusive
Validates permission combinations to prevent security issues.
Adding the
checkForInvalidPermissionCombinationvalidation ensures that users can't create role permissions with potentially dangerous combinations. This is a good security practice to enforce constraints at the schema level.
🏁 Script executed:
#!/bin/bash # Check implementation of the checkForInvalidPermissionCombination function # to understand what combinations are being validated rg -A 15 "checkForInvalidPermissionCombination" --type typescriptLength of output: 100
#!/bin/bash # The previous command encountered an error due to an unrecognized file type. # Let's re-run the search for the `checkForInvalidPermissionCombination` function using the correct file type flag for TypeScript. rg -A 15 "checkForInvalidPermissionCombination" -t tsAction Required: Please re-run the above script to verify the implementation details of
checkForInvalidPermissionCombinationand confirm the security validations.frontend/src/context/ProjectPermissionContext/types.ts (2)
10-17: New enum for granular secret permission actions.The
ProjectPermissionSecretActionsenum clearly separates the actions related to secrets, with a meaningful distinction betweenDescribeAndReadValue(legacy),DescribeSecret, andReadValue. This will enable more precise permission control throughout the application.
150-151: Updated permission set type to use new secret actions.The
ProjectPermissionSetnow correctly usesProjectPermissionSecretActionsinstead of the generalProjectPermissionActionsfor secret-related permissions, ensuring type safety when checking permissions.backend/src/services/secret-import/secret-import-service.ts (6)
7-13: Using specialized permission checking functions.The addition of
CheckCanSecretsSubjectandCheckForbiddenErrorSecretsSubjectimports, along withProjectPermissionSecretActions, enables more precise permission checking for secret-related operations.
97-101: Updated permission check for source folder access.The permission check now correctly uses
ProjectPermissionSecretActions.DescribeSecretinstead of the general read permission, aligning with the new granular permission model.
598-602: Permission check now verifies ability to read secret values.The filter now specifically checks for
ProjectPermissionSecretActions.ReadValuepermission, ensuring users can only access secret values they're explicitly authorized to view.
643-643: Added viewSecretValue parameter to control secret visibility.The new parameter explicitly indicates that secret values should be visible when the user has appropriate permissions, improving the clarity of the code.
648-654: Consistent permission checking across the service.Both permission checks have been updated to use
CheckCanSecretsSubjectwithProjectPermissionSecretActions.ReadValue, ensuring uniform permission enforcement.Also applies to: 665-670
680-684: Updated decryption function call with visibility control.The
decryptSecretRawfunction now includes thesecretValueHidden: falseparameter, which is consistent with the permission changes and explicitly indicates that the secret value should be visible.frontend/src/lib/fn/permission.ts (1)
10-36: Well-structured permission checking functionThis function elegantly handles permission checking for secrets by evaluating both new granular permissions and the legacy permission model. The approach of checking both
canNewPermissionandcanOldPermissionensures backward compatibility.backend/src/services/secret-sync/secret-sync-service.ts (2)
182-185: Permission check correctly updated to new modelThe permission check has been updated to use the more granular
ProjectPermissionSecretActions.ReadValuewith the new utility function, replacing the previous generalReadpermission.
270-273: Consistent permission check updateThis permission check is consistently updated to match the new model, using
ProjectPermissionSecretActions.ReadValueinstead of the generalReadpermission.frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretItem.tsx (6)
48-50: Good import organization for new permission handlingThe new imports correctly bring in the necessary components for the enhanced permission model.
107-113: Form values properly handle hidden secretsThe form's defaultValues and values are correctly updated to show empty string when the secret is hidden due to permissions.
135-149: Permission check updated to use more granular modelThe
isReadOnlycheck now uses the newsecretsPermissionCanfunction withProjectPermissionSecretActions.DescribeSecret, providing more accurate permission control.
284-286: UI correctly shows blur effect for hidden secretsThe UI now properly shows a blur component with explanatory tooltip when the user doesn't have permission to view the secret value.
308-323: Copy button correctly disabled for hidden secretsThe copy button is now properly disabled when the secret value is hidden, preventing access to protected values.
516-529: Share secret button correctly disabled for hidden secretsThe share secret button is appropriately disabled when the secret value is hidden, preventing sharing of protected values.
backend/src/ee/services/permission/project-permission.ts (4)
20-27: Well-structured permission actions enumThe new
ProjectPermissionSecretActionsenum provides a more granular approach to secret permissions, maintaining backward compatibility withDescribeAndReadValuewhile introducing more specific actions likeDescribeSecretandReadValue.
566-576: Admin permissions properly updated for new action typesThe admin permissions correctly implement the new granular permission model, with a good comment explaining why
DescribeAndReadValueisn't added directly.
890-902: Service token permissions correctly implement new action typesThe service token permission builder now properly handles the new permission types, ensuring proper access control for service tokens.
959-966: Backfill function properly handles permission conversionThe
backfillPermissionV1SchemaToV2Schemafunction correctly updates the permission model to include the newReadValuepermission when converting from the old schema, ensuring backward compatibility.backend/src/services/secret/secret-fns.ts (3)
14-14: Imports look aligned with the new permission model.
No issues are apparent with addingCheckCanSecretsSubjectandProjectPermissionSecretActions. Ensure that all references to these new imports are consistently applied throughout the codebase.Also applies to: 16-16
194-197: Confirm environment alignment with permission checks.
UsingCheckCanSecretsSubjectto assertReadValuepermission is correct. Consider verifying thatenvironmentis properly validated or sanitized beforehand to prevent unexpected permission bypass.
365-373: Securely returning masked value is appropriate.
ReturningINFISICAL_SECRET_VALUE_HIDDEN_MASKifsecretValueHiddenis true prevents accidental exposure. Confirm that all downstream code handles the masked value gracefully (e.g., logs, config exports, etc.).backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts (4)
3-3: Re-importingForbiddenErroris consistent with CASL usage.
No issues. Continue to ensure that thrown errors are properly handled at higher levels to maintain a clear error flow.
14-14: Centralizing masked value references.
UsingINFISICAL_SECRET_VALUE_HIDDEN_MASKfromsecret-fns.tsaligns snapshot masking with the rest of the code. Keep the constant consistent across all modules.
25-25: Expanded permission imports improve clarity.
The addition ofCheckCanSecretsSubjectandCheckForbiddenErrorSecretsSubjectsupports finer-grained secret operations. Verify that all references toProjectPermissionSecretActionsmatch your intended permission granularity in the snapshot service as well.Also applies to: 27-31
105-108: EnsureDescribeSecretcheck is sufficient.
CheckForbiddenErrorSecretsSubjectwithProjectPermissionSecretActions.DescribeSecreteffectively restricts snapshot listing to authorized users. Confirm that any child folder or sub-path checks are enforced similarly if the code expects hierarchical permission propagation.backend/src/ee/services/permission/permission-fns.ts (3)
6-6: Good use ofBadRequestErrorandForbiddenRequestError.
Centralizing these errors clarifies the code’s intent. Avoid overloading with too many error types that could make exception handling unwieldy.
17-44: Safe fallback toDescribeAndReadValue.
CheckForbiddenErrorSecretsSubjectattempts the narrower permission (ReadValue or DescribeSecret), then falls back toDescribeAndReadValue. This is a logical approach. Ensure the fallback behavior is intended and tested thoroughly for partial roles.
74-105: Prevent conflicting or redundant secret permissions.
checkForInvalidPermissionCombinationproperly halts invalid role combinations. Ensure integrators realize thatDescribeAndReadValuealone supersedes the more granular actions.backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts (6)
61-61: Improved permission specificity with specialized secret actions enumThe import has been updated to use
ProjectPermissionSecretActionsinstead of the more generalProjectPermissionActions, providing better granularity for secret-specific operations.
80-80: Added specialized permission check function for secretsImporting
CheckForbiddenErrorSecretsSubjectto handle permission checks specifically for secret operations provides better encapsulation of permission logic.
92-98: Extended secretTagDAL interface with find methodThe interface for
secretTagDALhas been extended to include thefindmethod, enhancing the data access capabilities.
115-115: Extended secretV2BridgeDAL interface with find methodSimilar to the secretTagDAL change, this enhancement allows for more flexible querying of secrets in the V2 bridge.
922-926: Improved permission check for secret valuesReplaced generic permission check with the specialized
CheckForbiddenErrorSecretsSubjectfunction usingProjectPermissionSecretActions.ReadValueaction. This provides more granular control over who can view secret values.
1374-1376: Updated permission actions to use secret-specific actionsThe constants have been updated to use secret-specific permission actions (
ProjectPermissionSecretActions) instead of generic ones, maintaining consistency with the new permission model.backend/src/services/secret/secret-service.ts (13)
16-22: Enhanced permission handling with specialized permission functions and actionsAdded imports for
CheckCanSecretsSubjectandCheckForbiddenErrorSecretsSubjectalong withProjectPermissionSecretActionsto support the refined permission model for secret operations.
57-57: Added utility for conditional secret value hidingImporting
conditionallyHideSecretValuefunction to implement the pattern of hiding secret values when users lack appropriate permissions.
214-214: Consistently updated permission actions across all secret operationsPermission checks throughout the service now use secret-specific actions (
ProjectPermissionSecretActions.Create,.Edit,.Delete) instead of generic permission actions, providing more precise permission control.Also applies to: 332-332, 489-489, 808-809, 895-897, 1014-1015
455-466: Implemented conditional secret value hiding based on permissionsAdded logic to check if the user has permission to read secret values and conditionally hide them when returning the updated secret. This ensures sensitive data is only exposed to authorized users.
562-574: Applied consistent permission-based secret value hiding on delete operationsSimilar to the update operation, the delete operation now checks permissions and conditionally hides secret values in the returned data.
622-625: Updated permission check for reading secret valuesReplaced the general permission check with the specialized
CheckForbiddenErrorSecretsSubjectfunction for better control over secret value access.
647-650: Enhanced permission checks for imported secretsPermission checks for imported secrets now use
CheckCanSecretsSubjectwith theProjectPermissionSecretActions.ReadValueaction, ensuring consistent permission enforcement across both direct and imported secrets.Also applies to: 752-755
769-769: Added explicit secretValueHidden flag to returned secretsThe service now explicitly indicates whether a secret value is hidden due to permission restrictions, improving UI feedback and clarifying why certain values might not be visible.
Also applies to: 780-786
971-979: Applied consistent permission model to bulk update operationsThe bulk update operation now checks permissions and conditionally hides secret values in the returned data, maintaining consistency with single-secret operations.
1065-1073: Applied consistent permission model to bulk delete operationsSimilar to other operations, the bulk delete function now checks permissions and conditionally hides secret values in the returned data.
1249-1290: Enhanced permission checks for secret access listsThe
getSecretAccessListfunction now uses the specialized secret permission actions to accurately represent allowed operations on secrets, providing better insights into who can access what.
2419-2438: Applied permission checking to secret version retrievalAdded logic to check permissions and conditionally hide secret values in secret versions, ensuring consistent security across all secret-related operations.
2799-2854: Enhanced permission checking for secret move operationsThe
moveSecretsfunction now performs granular permission checks for both source and destination environments, ensuring users have appropriate permissions for both locations before allowing the move operation.backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (5)
942-986: Comprehensive filtering logic with balanced permission checks.The implementation effectively filters secrets based on multiple permission levels and provides appropriate handling for recursive secrets. The code includes clear comments that explain the logic, particularly for masking secret values rather than filtering them out when a user doesn't have ReadValue permissions.
1169-1185: Good security practice with sequential permission checks.The code effectively checks for permissions in sequence, first verifying the user can describe the secret, then checking if they can read its value. This follows the principle of least privilege and ensures permission checks are consistent.
1217-1236: Clear error handling for missing read permissions.The implementation properly throws a specific
ForbiddenRequestErrorwhen users attempt to access secret values without appropriate permissions, and includes a descriptive error message.
2077-2083: Explicit permission actions using readonly arrays.Good practice using
as constto create readonly arrays of required permission actions, making the security requirements explicit and clear.
2470-2482: Appropriate security check before accessing sensitive data.The code properly verifies the user has ReadValue permission before processing the secret reference tree, throwing a clear error message when permission is missing.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/src/server/routes/v3/secret-router.ts (1)
371-378:⚠️ Potential issueFix reference to undefined schema
There's a reference to
SecretTagsSchemawhich is no longer imported, causing build failures. This needs to be updated to useSanitizedTagSchemalike the other schema references.- tags: SecretTagsSchema.pick({ - id: true, - slug: true, - color: true - }) - .extend({ name: z.string() }) - .array() - .optional(), + tags: SanitizedTagSchema.array().optional(),🧰 Tools
🪛 GitHub Actions: Check Backend PR types and lint
[error] 371-371: error TS2552: Cannot find name 'SecretTagsSchema'. Did you mean 'SecretsSchema'?
🪛 GitHub Actions: Check API For Breaking Changes
[error] 371-371: ReferenceError: SecretTagsSchema is not defined
♻️ Duplicate comments (1)
backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (1)
338-348:⚠️ Potential issueFix potential secret value leak for unprivileged users.
Currently, this method hardcodes
falsefor thesecretValueHiddenflag, always showing the unmasked secret value to users who create secrets, even if they don't have permissions to read secret values.Apply this fix to align with other methods' permission checks:
- false + !CheckCanSecretsSubject(permission, ProjectPermissionSecretActions.ReadValue, { + environment, + secretPath, + secretName, + secretTags: (secret?.tags || []).map((el) => el.slug) + })
🧹 Nitpick comments (3)
backend/src/services/secret-v2-bridge/secret-version-dal.ts (1)
270-270: Don't forget to add unit tests for the new functionality.The new
findBySecretIdfunction has been correctly added to the factory's return object, ensuring it's accessible to other parts of the application. Consider adding unit tests to verify it works as expected with different inputs.backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (2)
197-206: Update permission check to use secret-specific action.This permission check is still using the general
ProjectPermissionActions.Readinstead of a more specificProjectPermissionSecretActionsenum value.Consider updating this check to use the newly introduced secret-specific permissions for consistency with the rest of the codebase:
ForbiddenError.from(permission).throwUnlessCan( - ProjectPermissionActions.Read, + ProjectPermissionSecretActions.ReadValue, subject(ProjectPermissionSub.Secrets, { environment: el.environment, secretPath: el.secretPath, secretName: el.secretKey, tags: referredSecretsGroupBySecretKey[el.secretKey][0]?.tags?.map((i) => i.slug) }) );
2034-2034: Update permission check to use secret-specific action.This line still uses the general
ProjectPermissionActions.Readinstead of a secret-specific action.For consistency with the rest of the codebase, consider whether this should use a
ProjectPermissionSecretActionsvalue:- ForbiddenError.from(permission).throwUnlessCan(ProjectPermissionActions.Read, ProjectPermissionSub.SecretRollback); + ForbiddenError.from(permission).throwUnlessCan(ProjectPermissionSecretActions.ReadValue, ProjectPermissionSub.SecretRollback);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts(7 hunks)backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts(7 hunks)backend/src/server/routes/sanitizedSchemas.ts(2 hunks)backend/src/server/routes/v3/secret-router.ts(22 hunks)backend/src/services/external-migration/external-migration-fns.ts(1 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts(9 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts(45 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts(7 hunks)backend/src/services/secret-v2-bridge/secret-version-dal.ts(6 hunks)backend/src/services/secret/secret-fns.ts(7 hunks)backend/src/services/secret/secret-queue.ts(1 hunks)backend/src/services/secret/secret-service.ts(41 hunks)backend/src/services/secret/secret-types.ts(4 hunks)frontend/src/hooks/api/secrets/types.ts(5 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- backend/src/services/secret/secret-queue.ts
- backend/src/server/routes/sanitizedSchemas.ts
- backend/src/services/external-migration/external-migration-fns.ts
- frontend/src/hooks/api/secrets/types.ts
- frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx
- backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts
- backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts
- backend/src/services/secret/secret-types.ts
- backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts
🧰 Additional context used
🪛 GitHub Actions: Check Backend PR types and lint
backend/src/server/routes/v3/secret-router.ts
[error] 371-371: error TS2552: Cannot find name 'SecretTagsSchema'. Did you mean 'SecretsSchema'?
🪛 GitHub Actions: Check API For Breaking Changes
backend/src/server/routes/v3/secret-router.ts
[error] 371-371: ReferenceError: SecretTagsSchema is not defined
🔇 Additional comments (62)
backend/src/server/routes/v3/secret-router.ts (20)
4-4: Updated imports for better schema organizationThe import statement has been updated to remove
SecretTagsSchemaand import the necessary schemas from the appropriate modules. This aligns with the pattern used in other files in the codebase.
20-20: Added sanitized schemas for improved type safetyImporting
SanitizedTagSchemaandsecretRawSchemafrom the sanitized schemas module provides better type safety and consistency in the API responses.
29-34: Well-implemented utility function for boolean conversionThe
convertStringBooleanfunction standardizes the conversion of string boolean values to actual booleans, which improves code readability and reduces duplication throughout the router.
80-82: Updated schema to use SanitizedTagSchemaThe schema has been correctly updated to use
SanitizedTagSchema.array()instead of the previous implementation, ensuring consistency with the rest of the codebase.
136-136: Consistent use of SanitizedTagSchemaGood consistency in using
SanitizedTagSchema.array()here as well.
238-241: Improved query parameter handling with utility functionThe query parameters now use the
convertStringBooleanutility function, which provides better type safety and default values. The addition ofviewSecretValueparameter allows for better control over secret value visibility.
254-254: Added secretValueHidden property and updated tags schemaThe addition of
secretValueHiddenboolean field indicates whether a secret value is hidden, supporting the new permission-based secret visibility feature. The tags field has been correctly updated to useSanitizedTagSchema.Also applies to: 256-256
267-268: Consistent schema extension for importsThe imported secrets schema is also updated with the
secretValueHiddenproperty for consistency.
317-317: Added viewSecretValue parameter to getSecretsRawThe
viewSecretValueparameter is now passed to the service layer, enabling fine-grained control over whether secret values are displayed or hidden.
422-424: Standardized boolean parameters with utility functionThe query parameters are consistently using the
convertStringBooleanutility function, including the newviewSecretValueparameter.
429-431: Updated schema to include secretValueHidden propertyThe schema has been properly updated to include the
secretValueHiddenproperty, which indicates whether a secret value is hidden based on permissions.
462-462: Passing viewSecretValue to service layerThe
viewSecretValueparameter is correctly passed to the service function, maintaining consistency with the rest of the codebase.
669-671: Consistent schema updates for secretValueHidden propertyThe update secret response schema is consistently updated to include the
secretValueHiddenproperty.
767-769: Consistent schema extension for delete operationsThe delete secret response schema is properly extended with the
secretValueHiddenproperty, maintaining consistency across all operations.
854-854: Updated tag schema in GET / endpointThe tag schema has been correctly updated to use
SanitizedTagSchema.array()in the GET endpoint.
950-950: Standardized include_imports parameterThe
include_importsparameter now uses theconvertStringBooleanutility function for consistency.
1221-1221: Added secretValueHidden to PATCH responseThe
secretValueHiddenproperty is consistently added to the PATCH response schema.
1708-1708: Consistent schema for batch operationsThe batch update operation response schema also includes the
secretValueHiddenproperty, maintaining consistency across endpoints.
1823-1827: Updated batch delete response schemaThe batch delete response schema has been properly extended to include the
secretValueHiddenproperty.
2089-2090: Consistent schema extensions for batch raw operationsThe batch raw operations consistently include the
secretValueHiddenproperty in their response schemas.Also applies to: 2212-2215
backend/src/services/secret-v2-bridge/secret-version-dal.ts (3)
16-66: Well-implemented newfindBySecretIdfunction.The new function retrieves secret versions based on a secretId with proper pagination, sorting, and error handling. The implementation correctly joins the necessary tables and processes the results using
sqlNestRelationshipsto structure the data.
191-201: Good addition of tag-related joins.These tag-related joins in the
findVersionsBySecretIdWithActorsfunction are consistent with the approach used in the newfindBySecretIdfunction, ensuring tags are properly included in both endpoints.
234-256: Properly structured the processing of query results.The migration to using
sqlNestRelationshipsfor processing the query results is a good approach as it helps maintain consistency with the newfindBySecretIdfunction implementation.backend/src/services/secret/secret-fns.ts (5)
14-16: Good use of granular permission handling.Switching from general permission actions to secret-specific actions through
ProjectPermissionSecretActionsprovides better control and security for secret operations.
54-54: Well-defined constant for hidden secret values.Using a dedicated constant for masked values improves code readability and maintenance.
194-197: Enhanced permission checks for secret access.The implementation now properly uses granular secret-specific permission checks for accessing secret paths, improving security.
345-347: Improved secret value protection based on permissions.The
decryptSecretRawfunction now conditionally reveals or masks secret values based on thesecretValueHiddenproperty, enhancing security by preventing unauthorized access to sensitive data.Also applies to: 365-372
1205-1223: Excellent abstraction withconditionallyHideSecretValue.This new utility function encapsulates the logic for determining whether to mask secret values, promoting code reuse and maintainability across the codebase. The implementation is clean and returns all necessary properties.
backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts (6)
3-31: Improved import organization and permission handling.The changes to imports properly reflect the shift to more granular secret-specific permissions through
ProjectPermissionSecretActionsand inclusion of the secret masking constant.
106-109: Enhanced security with specific permission checks.Replacing general permission checks with secret-specific checks using
CheckForbiddenErrorSecretsSubjectandProjectPermissionSecretActions.DescribeSecretimproves security by ensuring precise permission validation.
143-146: Consistent permission check implementation.The permission check is consistently applied across different methods, ensuring uniform security enforcement throughout the service.
181-186: Good addition of folder path calculation.Adding full folder path calculation for both bridge and non-bridge implementations ensures that permission checks have the necessary context, improving security.
Also applies to: 220-224
189-215: Enhanced secret value protection in snapshot data.The implementation now properly checks permissions for reading secret values and conditionally reveals or masks them. The addition of
secretValueHiddenin the returned data helps clients understand when values are hidden due to permission restrictions.
239-257: Consistent permission handling across implementations.The same permission-based approach for protecting secret values is consistently applied to both the bridge and non-bridge code paths, ensuring uniform security regardless of the implementation used.
backend/src/services/secret/secret-service.ts (19)
9-9: Well-structured imports reorganization.The import reorganization adds the necessary components for the upgraded permission model, including
ProjectVersion,CheckCanSecretsSubject,CheckForbiddenErrorSecretsSubject, andProjectPermissionSecretActions.Also applies to: 15-22
57-57: Good addition of the conditionallyHideSecretValue utility.The
conditionallyHideSecretValuefunction provides a centralized way to handle secret value visibility based on user permissions, ensuring consistency throughout the service.
215-215: Improved permission granularity with secret-specific actions.Replacing general permissions with secret-specific actions (
ProjectPermissionSecretActions) provides better granularity for permission control, making the authorization model more precise.Also applies to: 333-333, 490-490, 809-810, 896-898, 1014-1016
456-467: Properly handling secret value visibility in updateSecret.The implementation correctly checks if the user has permission to read the secret value and conditionally hides sensitive data in the response, enhancing security while maintaining functionality.
563-575: Properly handling secret value visibility in deleteSecret.Similar to updateSecret, this implementation correctly applies the secret value visibility rules to the delete operation response.
623-626: Enhanced permission validation with specialized checks.Replacing
ForbiddenError.from(permission).throwUnlessCanwithCheckForbiddenErrorSecretsSubjectprovides better error handling for permission checks specific to secrets.
648-651: Consistent permission checking for imported secrets.The code consistently applies the same permission checking logic for both direct secrets and imported secrets, ensuring proper secret value visibility control.
Also applies to: 753-756
702-705: Improved permission check in getSecretByName.The function now properly checks permissions for reading secret values and sets the appropriate visibility flag in the response.
Also applies to: 783-787
770-774: Secret value handling for imported secrets.Adding the
secretValueHiddenproperty to imported secrets ensures consistent handling of secret visibility across the application.
972-980: Proper permission check in updateManySecret.The implementation correctly applies permission checks and secret value hiding for bulk update operations, ensuring consistency with individual operations.
1066-1074: Proper permission check in deleteManySecret.Similar to other bulk operations, this implementation correctly handles permission checks and secret value hiding for bulk delete operations.
1303-1303: Added viewSecretValue parameter to getSecretsRaw.The new parameter allows API consumers to explicitly request whether secret values should be included in the response, enhancing flexibility while maintaining security.
Also applies to: 1320-1321
1482-1482: Added viewSecretValue parameter to getSecretByNameRaw.Consistent with getSecretsRaw, this parameter provides control over secret value visibility at the API level.
Also applies to: 1502-1502
1710-1715: Enhanced return value structure in createSecretRaw.The modified return structure properly applies the conditional secret value hiding pattern while maintaining backward compatibility.
2111-2111: Proper secret value visibility in createManySecretsRaw.Setting
secretValueHidden: falseensures consistency with the single secret creation operation where values are visible after creation.
2432-2451: Comprehensive permission check for secret versions.The
getSecretVersionsfunction now checks permission for each secret version and properly applies the secret value hiding logic based on permissions, environment, and secret path.
2813-2867: Improved permission model in moveSecrets.The function now checks permissions for both source and destination environments using the new secret-specific action model, providing more precise control over secret operations.
1235-1236: Setting default viewSecretValue to false for getSecretAccessList.Setting
viewSecretValue: falseby default when fetching the secret access list is a good security practice, as the list of permissions doesn't require the actual secret values.
1250-1277: Enhanced permission checking in getSecretAccessList.The improved permission checking logic properly distinguishes between DescribeSecret and ReadValue actions, providing more granular control over what users can see.
backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (9)
978-992: Clear handling of ReadValue permission access.Good implementation of permission checking that maintains access control while providing clear feedback. The code appropriately handles cases where a user might have general access to list secrets but not permission to view their values.
1344-1362: Comprehensive permission check for secret value access.This implementation correctly handles permission verification for viewing secret values, providing appropriate error messages when access is denied. The implementation properly checks for both general permission and tag-specific permissions.
544-563: Good permission-based implementation of secret value hiding.This code properly determines whether to hide secret values based on user permissions and passes that information to the reshaping function.
652-673: Well-structured permission check for secret deletion.Good implementation of checking ReadValue permissions during delete operations, ensuring users can't view secret values they shouldn't have access to when reviewing deletion results.
1518-1538: Proper permission check implementation for bulk created secrets.This implementation correctly determines visibility of secret values in bulk creation operations based on user permissions.
1852-1875: Good permission handling in bulk update operations.The implementation properly checks permissions for each updated secret and masks values appropriately based on user access rights.
1976-2001: Comprehensive permission checks for bulk delete operations.The implementation correctly handles permissions for bulk delete operations, appropriately masking secret values based on user permissions.
2162-2193: Strong permission validation for secret moves.This implementation thoroughly checks multiple required permissions before allowing secret moves, ensuring proper access control.
2563-2575: Important permission check before revealing reference tree.This check ensures users can't access secret reference trees without proper permissions to view the underlying values, preventing information leakage.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (2)
339-349:⚠️ Potential issueFix potential leak of newly created secrets to users without readValue permission.
Currently, this method hardcodes
falsefor thesecretValueHiddenflag, returning the unmasked secret value to all users even if they don't haveReadValuepermission. This is inconsistent with other methods in the service that properly check permissions before exposing secret values.Apply this diff to fix the issue:
- false + !CheckCanSecretsSubject(permission, ProjectPermissionSecretActions.ReadValue, { + environment, + secretPath, + secretName, + secretTags: tags?.map((el) => el.slug) + })
1158-1168:⚠️ Potential issueFix potential leak of secret values in getSecretById method.
Similar to the issue in
createSecret, this method also hardcodesfalsefor thesecretValueHiddenflag, potentially exposing secret values to users without proper permissions.Apply this diff to fix the issue:
return reshapeBridgeSecret( secret.projectId, folderWithPath.environmentSlug, folderWithPath.path, { ...secret, value: secretValue, comment: secretComment }, - false + !CheckCanSecretsSubject(permission, ProjectPermissionSecretActions.ReadValue, { + environment: folderWithPath.environmentSlug, + secretPath: folderWithPath.path, + secretName: secret.key, + secretTags: secret.tags.map((i) => i.slug) + }) );
🧹 Nitpick comments (3)
backend/package.json (1)
73-73: New Seed-Dev Script AddedThe new
"seed-dev": "knex --knexfile ./src/db/knexfile.ts --client pg seed:run"script is correctly added to support development environment database seeding without affecting production seeds.For better maintainability, consider adding a comment in the file or updating the README to document the purpose and usage of this new script.
backend/src/ee/services/permission/permission-fns.ts (2)
17-44: The CheckForbiddenErrorSecretsSubject function provides good fallback handlingThis function effectively checks granular secret permissions with a fallback to broader permissions. The function properly handles both specific subject fields and general subject checks.
One suggestion: consider adding error details or specific error messages to help with debugging when permissions fail.
} catch { + // Fallback to broader permission check when specific permission check fails if (subjectFields) {
74-104: The permission combination validation logic is complex but effectiveThe implementation correctly prevents invalid combinations of permissions, but the error message construction using nested ternary operators (lines 91-99) makes the code hard to read and maintain.
Consider refactoring this section to improve readability:
- throw new BadRequestError({ - message: `You have selected Full Read Access, and ${ - hasBothDescribeAndReadValue - ? "both Read Value and Describe Secret" - : hasReadValue - ? "Read Value" - : hasDescribeSecret - ? "Describe Secret" - : "" - }. You cannot select Read Value or Describe Secret if you have selected Full Read Access.` - }); + let conflictingPermissionText = ""; + if (hasBothDescribeAndReadValue) { + conflictingPermissionText = "both Read Value and Describe Secret"; + } else if (hasReadValue) { + conflictingPermissionText = "Read Value"; + } else if (hasDescribeSecret) { + conflictingPermissionText = "Describe Secret"; + } + + throw new BadRequestError({ + message: `You have selected Full Read Access, and ${conflictingPermissionText}. You cannot select Read Value or Describe Secret if you have selected Full Read Access.` + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/package.json(1 hunks)backend/src/ee/services/permission/permission-fns.ts(1 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts(50 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check API Changes
- GitHub Check: Run integration test
- GitHub Check: Check Frontend Type and Lint check
- GitHub Check: Check TS and Lint
🔇 Additional comments (9)
backend/src/ee/services/permission/permission-fns.ts (3)
1-16: Import statements have been properly updatedThe new imports correctly support the added functionality. The inclusion of
BadRequestErroris appropriate for the new validation logic, and the import of the new permission-related types enhances type safety throughout the file.
46-72: CheckCanSecretsSubject function properly handles both permission systemsThis function correctly checks both new granular permissions and the legacy broader permission. The boolean return value makes it easy to integrate with conditional logic in the calling code.
84-84:❓ Verification inconclusive
Verify early return logic
This early return at line 84 skips validation if neither
ReadValuenorDescribeSecretpermissions are present. Please verify this is the intended behavior, as it might allowDescribeAndReadValueto coexist with other non-checked permissions.
🏁 Script executed:
#!/bin/bash # Look for other usages of this function to understand expected behavior rg -A 10 "checkForInvalidPermissionCombination" --type typescriptLength of output: 100
Verify Early Return Logic on Permissions
The early return at line 84 skips further validation when both
ReadValueandDescribeSecretpermissions are missing, which may allow a combined permission (e.g.,DescribeAndReadValue) to bypass additional checks. The initial search usingrgwith the file type flagtypescriptproduced an error. Please re-run the search using the correct file type flag (for example, using-t ts) or otherwise manually verify that no additional invocations ofcheckForInvalidPermissionCombinationcontradict the intended behavior.backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (6)
952-1024: The enhanced permission checking implementation looks excellent.The updated filtering logic in
getSecretsis well-designed, with proper handling of permissions at both theDescribeSecretandReadValuelevels. The addition of theviewSecretValueandthrowOnMissingReadValuePermissionparameters provides flexible behavior options, and the conditional masking of secret values based on permissions is consistently implemented.
1069-1092: Well-implemented permission checks for imported secrets.The
hasSecretAccessfunction has been improved to properly check bothDescribeSecretandReadValuepermissions, with appropriate conditional logic based on theviewSecretValueparameter. This ensures consistent permission checking across both native and imported secrets.
1296-1327: Good implementation of secret value hiding and error handling.The added logic in
getSecretByNamecorrectly implements secret value hiding based on permissions and includes proper error handling when users attempt to access secret values they don't have permission to view.Also applies to: 1349-1382
2169-2199: Improved permission checking in moveSecrets operation.The refactored permission check approach in
moveSecretsis a significant improvement, systematically verifying all necessary permissions for both source and destination operations. The code now properly checksDescribeSecret,ReadValue,Delete,Create, andEditpermissions as appropriate.
13-20: Good refactoring to use specialized permission actions.The introduction of
ProjectPermissionSecretActionsfor specific secret-related actions, along with the helper functionsCheckCanSecretsSubjectandCheckForbiddenErrorSecretsSubject, greatly improves the clarity and consistency of permission checks throughout the service.
2571-2581: Proper restriction for secret reference tree access.This change appropriately restricts access to the secret reference tree by requiring
ReadValuepermission, displaying a clear error message when permission is denied.
backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts
Outdated
Show resolved
Hide resolved
frontend/src/pages/project/RoleDetailsBySlugPage/components/ProjectRoleModifySection.utils.tsx
Outdated
Show resolved
Hide resolved
This reverts commit bd66411.
DanielHougaard
force-pushed
the
revert-3189-revert-3128-daniel/view-secret-value-permission
branch
from
f357cf0 to
a8fc0e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (2)
340-349:⚠️ Potential issueMissing ReadValue permission check for newly created secrets.
Currently, this method hardcodes
falsefor thesecretValueHiddenflag, returning the unmasked secret value. If a user can create but not read secret values, they would still see the secret value, which may not be the intended behavior.Apply this fix:
- false + !permission.can( + ProjectPermissionSecretActions.ReadValue, + subject(ProjectPermissionSub.Secrets, { + environment, + secretPath, + secretName + }) + )
1158-1168:⚠️ Potential issueGetting secrets by ID always shows the secret value.
When getting a secret by ID, the
secretValueHiddenflag is hardcoded tofalse, which means the secret value is always shown. This might not be the intended behavior if the user only has permission to describe but not read the secret value.Apply this fix:
- false + !permission.can( + ProjectPermissionSecretActions.ReadValue, + subject(ProjectPermissionSub.Secrets, { + environment: folderWithPath.environmentSlug, + secretPath: folderWithPath.path, + secretName: secret.key, + secretTags: secret.tags?.map((i) => i.slug) + }) + )
🧹 Nitpick comments (5)
backend/src/server/routes/v1/dashboard-router.ts (1)
4-19: Comprehensive permission model for secret visibilityThe changes implement a consistent permission model across all secret-related endpoints, allowing for granular control over who can view secret values versus just metadata. This is achieved through:
- Adding the
viewSecretValueparameter to endpoint queries- Including
secretValueHiddenin response schemas- Using sanitized tag schemas for better security
- Gracefully handling permission issues with
throwOnMissingReadValuePermission: falseThis model enhances security by allowing users to have read access to secret metadata without necessarily seeing the actual values.
Consider documenting this permission model comprehensively in your API documentation to ensure developers understand the different levels of access control available.
Also applies to: 120-613, 692-864
backend/src/services/secret/secret-fns.ts (1)
1204-1223: Reusable function for secret-value masking.
Encapsulates the hiding logic. Consider adding unit tests for coverage.Do you want me to help create test cases for this function?
backend/src/ee/services/permission/permission-fns.ts (3)
1-1: ESLint rule disabled for nested ternaries.
You may consider refactoring the code to minimize nested ternaries.
17-45: Fallback permission check in catch block.
Consider catchingForbiddenErrorspecifically to avoid swallowing unrelated exceptions.try { ... -} catch { +} catch (err) { + if (err instanceof ForbiddenError) { + // fallback check + } else { + throw err; + } }
74-104: Checks for conflicting permission combinations.
You might simplify the nested conditionals for readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (70)
backend/e2e-test/vitest-environment-knex.ts(0 hunks)backend/package.json(1 hunks)backend/src/ee/routes/v1/secret-approval-request-router.ts(3 hunks)backend/src/ee/routes/v1/secret-router.ts(2 hunks)backend/src/ee/routes/v1/secret-version-router.ts(2 hunks)backend/src/ee/routes/v1/snapshot-router.ts(3 hunks)backend/src/ee/routes/v2/project-role-router.ts(2 hunks)backend/src/ee/services/permission/permission-fns.ts(1 hunks)backend/src/ee/services/permission/project-permission.ts(11 hunks)backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts(7 hunks)backend/src/ee/services/secret-replication/secret-replication-service.ts(1 hunks)backend/src/ee/services/secret-rotation/secret-rotation-service.ts(2 hunks)backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts(7 hunks)backend/src/lib/api-docs/constants.ts(3 hunks)backend/src/lib/errors/index.ts(1 hunks)backend/src/server/routes/sanitizedSchemas.ts(2 hunks)backend/src/server/routes/v1/dashboard-router.ts(12 hunks)backend/src/server/routes/v3/secret-router.ts(23 hunks)backend/src/services/external-migration/external-migration-fns.ts(1 hunks)backend/src/services/external-migration/external-migration-queue.ts(1 hunks)backend/src/services/integration-auth/integration-delete-secret.ts(1 hunks)backend/src/services/integration/integration-service.ts(3 hunks)backend/src/services/project/project-service.ts(2 hunks)backend/src/services/secret-import/secret-import-fns.ts(6 hunks)backend/src/services/secret-import/secret-import-service.ts(7 hunks)backend/src/services/secret-sync/secret-sync-queue.ts(1 hunks)backend/src/services/secret-sync/secret-sync-service.ts(3 hunks)backend/src/services/secret-tag/secret-tag-dal.ts(1 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts(9 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts(50 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts(7 hunks)backend/src/services/secret-v2-bridge/secret-version-dal.ts(6 hunks)backend/src/services/secret/secret-dal.ts(2 hunks)backend/src/services/secret/secret-fns.ts(7 hunks)backend/src/services/secret/secret-queue.ts(1 hunks)backend/src/services/secret/secret-service.ts(41 hunks)backend/src/services/secret/secret-types.ts(4 hunks)backend/src/services/secret/secret-version-dal.ts(3 hunks)backend/src/services/service-token/service-token-service.ts(2 hunks)frontend/src/components/secrets/SecretReferenceDetails/SecretReferenceDetails.tsx(4 hunks)frontend/src/components/v2/Blur/Blur.tsx(1 hunks)frontend/src/components/v2/Blur/index.tsx(1 hunks)frontend/src/components/v2/InfisicalSecretInput/InfisicalSecretInput.tsx(1 hunks)frontend/src/context/ProjectPermissionContext/types.ts(2 hunks)frontend/src/hooks/api/dashboard/queries.tsx(3 hunks)frontend/src/hooks/api/dashboard/types.ts(1 hunks)frontend/src/hooks/api/secretApprovalRequest/queries.tsx(1 hunks)frontend/src/hooks/api/secretImports/queries.tsx(2 hunks)frontend/src/hooks/api/secretSnapshots/queries.tsx(1 hunks)frontend/src/hooks/api/secrets/queries.tsx(5 hunks)frontend/src/hooks/api/secrets/types.ts(5 hunks)frontend/src/hooks/api/types.ts(2 hunks)frontend/src/lib/fn/permission.ts(1 hunks)frontend/src/pages/project/AccessControlPage/components/ServiceTokenTab/components/ServiceTokenSection/AddServiceTokenModal.tsx(2 hunks)frontend/src/pages/project/RoleDetailsBySlugPage/components/GeneralPermissionPolicies.tsx(2 hunks)frontend/src/pages/project/RoleDetailsBySlugPage/components/ProjectRoleModifySection.utils.tsx(7 hunks)frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx(2 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretEditRow.tsx(5 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretNoAccessOverviewTableRow.tsx(2 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretOverviewTableRow.tsx(1 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretRenameRow.tsx(2 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretSearchInput/components/QuickSearchSecretItem.tsx(2 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/SelectionPanel.tsx(3 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/components/MoveSecretsDialog/MoveSecretsDialog.tsx(2 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/SecretDashboardPage.tsx(4 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/ActionBar/ActionBar.tsx(3 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx(10 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretItem.tsx(9 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretNoAccessListView.tsx(2 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SnapshotView/SecretItem.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- backend/e2e-test/vitest-environment-knex.ts
🚧 Files skipped from review as they are similar to previous changes (54)
- backend/src/services/secret-tag/secret-tag-dal.ts
- backend/src/lib/errors/index.ts
- frontend/src/hooks/api/dashboard/types.ts
- backend/src/server/routes/sanitizedSchemas.ts
- frontend/src/components/v2/Blur/index.tsx
- frontend/src/hooks/api/secretApprovalRequest/queries.tsx
- frontend/src/hooks/api/secretSnapshots/queries.tsx
- backend/src/ee/services/secret-replication/secret-replication-service.ts
- frontend/src/components/v2/InfisicalSecretInput/InfisicalSecretInput.tsx
- frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretNoAccessListView.tsx
- frontend/src/pages/secret-manager/SecretDashboardPage/components/SnapshotView/SecretItem.tsx
- frontend/src/hooks/api/secretImports/queries.tsx
- frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretNoAccessOverviewTableRow.tsx
- frontend/src/pages/project/AccessControlPage/components/ServiceTokenTab/components/ServiceTokenSection/AddServiceTokenModal.tsx
- backend/src/services/integration-auth/integration-delete-secret.ts
- frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretOverviewTableRow.tsx
- frontend/src/pages/secret-manager/OverviewPage/components/SecretSearchInput/components/QuickSearchSecretItem.tsx
- backend/src/services/secret/secret-queue.ts
- backend/src/services/secret-sync/secret-sync-queue.ts
- frontend/src/components/v2/Blur/Blur.tsx
- backend/src/ee/routes/v2/project-role-router.ts
- backend/src/ee/routes/v1/secret-version-router.ts
- frontend/src/components/secrets/SecretReferenceDetails/SecretReferenceDetails.tsx
- frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretRenameRow.tsx
- frontend/src/context/ProjectPermissionContext/types.ts
- frontend/src/pages/project/RoleDetailsBySlugPage/components/GeneralPermissionPolicies.tsx
- backend/src/ee/services/secret-rotation/secret-rotation-service.ts
- backend/src/ee/routes/v1/secret-approval-request-router.ts
- frontend/src/pages/secret-manager/SecretDashboardPage/SecretDashboardPage.tsx
- backend/src/lib/api-docs/constants.ts
- frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/components/MoveSecretsDialog/MoveSecretsDialog.tsx
- frontend/src/hooks/api/dashboard/queries.tsx
- frontend/src/pages/secret-manager/SecretDashboardPage/components/ActionBar/ActionBar.tsx
- backend/package.json
- backend/src/services/secret-import/secret-import-service.ts
- frontend/src/hooks/api/types.ts
- frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx
- backend/src/services/project/project-service.ts
- backend/src/services/integration/integration-service.ts
- backend/src/services/service-token/service-token-service.ts
- backend/src/services/external-migration/external-migration-fns.ts
- backend/src/services/external-migration/external-migration-queue.ts
- backend/src/ee/routes/v1/snapshot-router.ts
- frontend/src/hooks/api/secrets/types.ts
- frontend/src/pages/project/RoleDetailsBySlugPage/components/ProjectRoleModifySection.utils.tsx
- backend/src/services/secret-sync/secret-sync-service.ts
- frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/SelectionPanel.tsx
- backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts
- backend/src/services/secret/secret-types.ts
- backend/src/ee/routes/v1/secret-router.ts
- backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts
- backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts
- frontend/src/hooks/api/secrets/queries.tsx
- frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check Frontend Type and Lint check
- GitHub Check: Check TS and Lint
- GitHub Check: Check API Changes
- GitHub Check: Run integration test
🔇 Additional comments (164)
backend/src/server/routes/v3/secret-router.ts (22)
4-4: Import modifications reflect schema migrationThe import statement has been updated to remove
SecretTagsSchemawhich aligns with the schema migration changes. The PR is focused on implementing view secret value permissions.
20-20: Appropriate schema imports addedThe correct schemas are now imported from the sanitizedSchemas file, which will be used for sanitizing tag information in responses.
28-34: Good utility function addition for consistent boolean handlingThis utility function provides a standardized way to convert string-based boolean parameters from HTTP requests into actual boolean values, with support for defaults. This simplifies the code and ensures consistent behavior.
80-82: Schema update to use SanitizedTagSchemaThe response schema has been properly updated to use the new
SanitizedTagSchema.array()instead of the previousSecretTagsSchema. This change is consistent with the schema migration throughout the codebase.
136-136: Consistent schema update for tagsThis change ensures consistent use of
SanitizedTagSchemafor the detach tags endpoint, matching the schema used in other endpoints.
238-241: Refactored boolean query parameters using the new utility functionThe boolean query parameters now use the new
convertStringBooleanfunction, providing consistent handling across all parameters. TheviewSecretValueparameter is now available with a default oftrue, which is important for the new permission feature.
254-254: Added secretValueHidden flag to response schemaThis new flag indicates whether the secret value is hidden based on the user's permissions, which is a key part of the view secret value permission feature.
256-256: Updated tags schema in GET /raw responseThe tags schema in the response is now using
SanitizedTagSchema.array(), consistent with other endpoints.
267-267: Added secretValueHidden flag to imports schemaThe imports section of the response now includes the
secretValueHiddenflag, maintaining consistency with the main secrets array.
317-317: Passing viewSecretValue parameter to service layerThe controller now correctly passes the
viewSecretValueparameter from the request to the service layer, enabling the permission check for viewing secret values.
415-417: Using convertStringBoolean for query parametersThe
/raw/:secretNameendpoint now uses the standardizedconvertStringBooleanfunction for boolean parameters, providing consistency with other endpoints.
422-423: Updated response schema with secretValueHidden and SanitizedTagSchemaThe response schema now includes the
secretValueHiddenflag and usesSanitizedTagSchemafor tags, maintaining consistency with other endpoints.
455-455: Passing viewSecretValue to getSecretByNameRawThe GET endpoint for a single secret by name now correctly passes the
viewSecretValueparameter to the service layer, enabling permission checking for this endpoint.
662-664: Updated response schema for update secretThe response schema for updating a secret now includes the
secretValueHiddenflag, consistent with other endpoints. The use of.extend()instead of.merge()is the correct approach for adding new fields.
760-762: Updated response schema for delete secretThe delete secret endpoint's response schema now includes the
secretValueHiddenflag, consistent with other endpoints. This ensures API consumers can determine if the secret value is masked.
943-943: Using convertStringBoolean for include_importsThe GET
/:secretNameendpoint now uses the standardizedconvertStringBooleanfunction for theinclude_importsparameter, providing consistency with other endpoints.
1214-1214: Added secretValueHidden to PATCH response schemaThe PATCH endpoint's response schema now includes the
secretValueHiddenflag, maintaining API consistency.
1384-1389: Updated schema for DELETE /:secretName responseThe response schema for the DELETE endpoint now properly includes the
secretValueHiddenflag using.extend()instead of.merge(), which is the correct approach for adding new fields.
1701-1701: Updated schema for batch PATCH responseThe batch PATCH endpoint's response schema now includes the
secretValueHiddenflag, maintaining consistency across endpoints.
1817-1821: Updated schema for batch DELETE responseThe batch DELETE endpoint's response schema now correctly includes the
secretValueHiddenflag using the.extend()method, maintaining consistency across the API.
2082-2082: Updated schema for batch/raw PATCH responseThe batch/raw PATCH endpoint's response schema now includes the
secretValueHiddenflag, ensuring consistent API behavior.
2204-2208: Updated schema for batch/raw DELETE responseThe batch/raw DELETE endpoint's response schema now includes the
secretValueHiddenflag, maintaining API consistency. The schema is properly extended using the.extend()method.backend/src/server/routes/v1/dashboard-router.ts (12)
4-4: Updated schema to use sanitized tagsThe import changes replace
SecretTagsSchemawithSanitizedTagSchema, which aligns with the new permission model for handling secret tags with better sanitization.Also applies to: 19-19
8-9: Added ProjectPermissionSecretActions for granular permission controlThe addition of
ProjectPermissionSecretActionsenables more fine-grained permission checks specifically for secret-related actions.
120-121: Added secret visibility control to schemaThe schema now includes the
secretValueHiddenflag and usesSanitizedTagSchema, providing better control over secret visibility and proper tag sanitization in the response.Also applies to: 123-123
292-292: Added viewSecretValue parameter to getSecretsRawMultiEnvThe parameter defaults to true, enabling consistent visibility control for secret values across multiple environments.
392-392: Added viewSecretValue parameter to /secrets-details endpointThe endpoint now includes a
viewSecretValueparameter defaulting to true, allowing clients to explicitly control whether they want to see secret values based on their permissions.
410-410: Added secretValueHidden flag to secrets-details response schemaSimilar to the overview endpoint, the details endpoint now includes proper visibility control and tag sanitization in its response schema.
Also applies to: 413-413
595-613: Enhanced secrets retrieval with granular permission controlThe code now passes the user-provided
viewSecretValueflag to the secrets service and setsthrowOnMissingReadValuePermission: falseto handle permission issues gracefully instead of failing the request.
692-692: Updated schema for deep-search endpointThe deep-search endpoint now consistently includes the
secretValueHiddenflag and sanitized tags in its response schema.Also applies to: 695-695
739-739: Updated permission check to use ProjectPermissionSecretActions.DescribeSecretThe permission check now uses the more specific
DescribeSecretaction from theProjectPermissionSecretActionsenum.
860-860: Updated schema for secrets-by-keys responseThe response schema for the secrets-by-keys endpoint is now consistent with other endpoints, including the
secretValueHiddenflag and sanitized tags.Also applies to: 863-863
872-872: Added viewSecretValue parameter to getSecretsRaw callThe handler now properly passes the viewSecretValue parameter from the request to the service layer.
Also applies to: 881-881
853-854:✅ Verification successful
Added viewSecretValue parameter to secrets-by-keys endpoint
The endpoint now includes a
viewSecretValueparameter with a conservative default offalse, requiring explicit opt-in to view secret values.Verify that this default aligns with the security principle of least privilege:
🏁 Script executed:
#!/bin/bash # Check for any comments or related files that explain this design decision rg -A 3 -B 3 "viewSecretValue.*default.*false" --type tsLength of output: 693
Verified: The default value of "false" for viewSecretValue is appropriate as it enforces least privilege by requiring explicit opt-in to view secret values.
- The ripgrep output confirms that the default is correctly set in
backend/src/server/routes/v1/dashboard-router.ts.- No additional comments or related files indicate a conflicting design decision.
frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretEditRow.tsx (4)
28-28: Good addition of the Blur component import.The Blur component is correctly imported to support the conditional rendering based on secret value permissions.
43-43: Good extension of the component API with the secretValueHidden property.The secretValueHidden boolean property is appropriately added to the Props type and the component parameters, allowing the component to conditionally render based on user permissions.
Also applies to: 63-63
146-166: Well implemented conditional rendering based on permissions.The component now properly handles the case where a user doesn't have permission to view a secret value by:
- Showing a Blur component with an explanatory tooltip when permissions are missing
- Rendering the normal editable input when permissions are present
This implementation follows good UI practices by providing clear feedback to users about permission restrictions.
222-222: Correctly disabled copy functionality for hidden secret values.The copy button is now appropriately disabled when the secret value is hidden, preventing users from copying values they don't have permission to see.
backend/src/services/secret/secret-dal.ts (2)
172-212: Well-implemented data access function for retrieving secrets with tags.The newly added
findManySecretsWithTagsfunction follows the established patterns in the codebase:
- Proper error handling with specific error names
- Consistent use of transaction parameter
- Good use of joins to retrieve related data
- Appropriate use of the
sqlNestRelationshipsutility to structure the returned dataThis function will be useful for efficiently retrieving secrets with their associated tags in a single database query.
488-489: Properly exported the new function from the DAL factory.The new
findManySecretsWithTagsfunction is correctly added to the factory's return object, making it available for use by service layers.frontend/src/lib/fn/permission.ts (2)
10-36: Consider clarifying the function name to indicate its specific purpose.The
secretsPermissionCanfunction handles a mix of new and legacy permission checks for secrets. While functionally correct, the name could be more specific to indicate its role in resolving both current and legacy permission structures.Consider renaming to something more explicit like
checkSecretAccessPermissionorresolveSecretPermissionsto better indicate its purpose in handling both current and legacy permission structures.
1-9: Well-implemented permission checking with support for both new and legacy structures.The function:
- Properly handles both new granular permissions and legacy permissions
- Correctly checks permissions with and without specific subject fields
- Handles the permission context appropriately using CASL's subject functionality
This implementation ensures backward compatibility while supporting the new more granular permission model.
Also applies to: 21-36
backend/src/services/secret/secret-version-dal.ts (3)
4-6: Good update to imports for supporting the new functionality.The imports have been properly updated to include
SecretVersionsSchemaandTFindOpt, which are required for the new function's implementation.
15-57: Well-implemented pagination and sorting for secret version retrieval.The new
findBySecretIdfunction:
- Handles pagination parameters (offset, limit) correctly
- Implements proper sorting with flexible column and order specification
- Joins related tables to fetch all necessary data in a single query
- Uses appropriate error handling with specific error names
- Processes results with
sqlNestRelationshipsto structure the returned data properlyThis implementation follows established patterns in the codebase and provides a flexible way to retrieve secret versions.
196-196: Correctly exported the new function from the DAL factory.The new
findBySecretIdfunction is properly added to the factory's return object, making it available for use by service layers.backend/src/services/secret-import/secret-import-fns.ts (6)
6-6: Good addition for security masking.Adding the
INFISICAL_SECRET_VALUE_HIDDEN_MASKimport is a necessary part of implementing the new permission-based secret value visibility feature.
36-41: Well-structured type extension for security tags and visibility.The additions to the
TSecretImportSecretsV2type properly support the new security features:
secretTagsarray with properly typed propertiessecretValueHiddenboolean flag to control value visibilityThese changes align well with the permission model enhancement in this PR.
Also applies to: 49-49
161-162: Good parameter addition for permission control.Adding the
viewSecretValueparameter allows for fine-grained control over secret visibility based on user permissions.Also applies to: 168-168
181-188: Stack type properly updated for new properties.The stack type definition now correctly includes the new properties
secretValueHiddenandsecretTags, ensuring type safety throughout the function.
247-249: Secure conditional handling of secret values.The implementation properly:
- Shows the actual secret value only when
viewSecretValueis true- Uses a mask when the user doesn't have permission
- Sets the
secretValueHiddenflag based on permissions- Correctly assigns the secret tags
This is a critical security change that helps prevent unauthorized access to sensitive values.
287-288: Important optimization for hidden secrets.Skipping the expansion of secret references for hidden secrets is an important optimization that:
- Prevents unnecessary processing
- Avoids potential permission escalation issues where reference expansion might expose values
This is a thoughtful security consideration.
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretItem.tsx (7)
48-50: Good addition of permission and UI components.These imports support the new permission model and UI components for handling secret visibility:
ProjectPermissionSecretActionsfor more granular permission controlBlurcomponent for visually indicating hidden secretssecretsPermissionCanhelper for permission checksThese are necessary for implementing the UI side of the secret value visibility feature.
106-113: Properly handling hidden values in form state.The form's default values and values are now correctly set to handle hidden secrets by setting an empty value when
secretValueHiddenis true, preventing accidental exposure of sensitive data.
135-140: Updated permission checks to use new granular permissions.The code now correctly uses
ProjectPermissionSecretActions.DescribeSecretandProjectPermissionSecretActions.Editinstead of the more generic permission actions, aligning with the enhanced permission model.Also applies to: 142-142
151-152: Extracting visibility flag for better code readability.Destructuring
secretValueHiddenfrom the secret object improves code readability and makes its usage more explicit throughout the component.
284-286: Good UI implementation for hidden secrets.The conditional rendering now properly shows a blur component with a helpful tooltip when a secret value should be hidden, providing a good user experience while maintaining security.
299-299: Ensuring hidden values remain hidden in the input component.Setting an empty default value when the secret is hidden ensures that no sensitive data is accidentally exposed in the UI component.
308-308: Properly disabling sensitive operations for hidden secrets.Disabling the copy and share buttons when a secret value is hidden prevents potential security bypasses through these operations, completing the UI security implementation.
Also applies to: 516-517
backend/src/services/secret-v2-bridge/secret-version-dal.ts (5)
16-66: Well-implemented new data access function with support for tags.The new
findBySecretIdfunction is thoroughly implemented with:
- Proper table joins to retrieve all necessary data
- Support for pagination and sorting
- Structured relationship mapping with
sqlNestRelationships- Comprehensive error handling
This function enables retrieving secret versions with their associated tags, supporting the enhanced permission model.
190-200: Added tag support to existing queries.Adding joins to the tag-related tables in the existing function ensures consistent tag support across the data access layer.
211-216: Comprehensive column selection for tags.The selection of tag-related columns is thorough, ensuring all necessary tag data is available for permission checks.
233-256: Well-structured relationship mapping for version data.The
sqlNestRelationshipsimplementation properly maps parent and child relationships, ensuring a clean and consistent data structure for secret versions and their tags.
270-270: Properly exposed new function in the DAL factory.The new function is correctly added to the returned object, making it available for use by service layers.
backend/src/ee/services/permission/project-permission.ts (8)
20-27: Well-designed permission model enhancement.The new
ProjectPermissionSecretActionsenum provides more granular control over secret permissions, particularly splitting the read permission into:
DescribeSecret- ability to see metadata about a secretReadValue- ability to see the actual secret valueThis is a significant security enhancement that enables the secret value visibility feature.
127-127: Updated permission set type for new secret actions.The
ProjectPermissionSettype now correctly usesProjectPermissionSecretActionsfor theSecretssubject, ensuring type safety throughout the permission system.
473-473: Updated schema to use new secret actions.The
ProjectPermissionV2Schemanow correctly usesProjectPermissionSecretActions, aligning the schema with the enhanced permission model.
566-576: Comprehensive admin permission update.The admin permissions now include all the granular secret actions, with a thoughtful comment about not adding
DescribeAndReadValuesince it's covered by the combination ofDescribeSecretandReadValue.As noted in a previous review comment, please verify that all necessary permissions are included here for more privileged roles. This change looks comprehensive, but it's worth double-checking against all potential use cases.
637-643: Member permissions properly updated for new secret actions.The member permissions now include the appropriate granular secret actions, maintaining the correct permission level for this role.
814-816: Viewer permissions correctly limited to viewing capabilities.The viewer permissions now correctly include only
DescribeSecretandReadValueactions, maintaining the read-only nature of this role.
890-902: Service token permissions updated for granular secret actions.The service token permission builder now correctly handles the new granular secret actions, ensuring tokens can be created with the appropriate level of access.
959-966: Backward compatibility preserved with schema backfill.The
backfillPermissionV1SchemaToV2Schemafunction has been updated to handle the new secret actions, ensuring backward compatibility with existing permission configurations.backend/src/services/secret/secret-fns.ts (6)
14-14: No issues with the new import.
16-16: Enum import looks good.
54-54: Clear secret value mask definition.
A single, standardized mask helps avoid accidental value exposures.
346-346: Added secretValueHidden property.
Clearly denotes hidden vs. visible secret states.
365-373: Conditionally decrypt or mask the secret value.
Prevents unauthorized access to raw secrets while preserving functionality for authorized users.
194-197:❓ Verification inconclusive
Confirm partial path matching edge cases.
UsingCheckCanSecretsSubjectis correct, but partial matching viastartsWithmay introduce subtle folder path overlaps.
🏁 Script executed:
#!/bin/bash # Verify no unintended partial folder path matches rg 'startsWith\(' -A 3Length of output: 20295
Review Edge Cases for Folder Path Matching
The use ofstartsWithinfolder.path.startsWith(currentPath === "/" ? "" : currentPath)effectively checks for a prefix match but may inadvertently allow overlapping folder names (e.g., a
currentPathof/foowould also match/foobar). Please verify that this behavior is intentional. If these overlaps are not desired, consider refining the logic—such as checking for an exact match or ensuring that a trailing slash follows thecurrentPath(e.g., using a condition likefolder.path === currentPath || folder.path.startsWith(currentPath + "/"))—to enforce proper directory boundaries.backend/src/ee/services/permission/permission-fns.ts (5)
2-2: CASL imports are standard.
No concerns with referencing ability utilities.
3-3: Zod import for schema validations.
This is consistent with existing patterns.
6-6: Expanded error imports.
They align well with new permission checks.
9-15: Grouped project permission imports.
Organizing related items is clear.
46-72: Supports new and old permission systems.
Clean approach for backward compatibility.backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts (10)
3-3: CASL ForbiddenError import.
Essential for granular permission checks.
15-15: Imported hidden value mask.
Consistent with the new secret masking approach elsewhere.
26-26: Synchronized permission checks.
CheckForbiddenErrorSecretsSubjecthelps unify logic across the codebase.
28-32: Project permissions import aligned with new actions.
Keeps enum usage uniform throughout the code.
106-109: Verifies DescribeSecret permission for snapshot count.
Prevents unauthorized snapshot enumeration.
143-146: Ensures DescribeSecret access for snapshot listing.
Maintains consistent folder-level checks.
181-186: Full folder path resolution logic.
Facilitates accurate environment-specific permission checks.
189-215: Conditional decryption for v3 projects.
Respects read permissions before revealing secret values.
219-225: Similar folder path derivation in legacy block.
Maintains environment consistency in older project versions.
232-274: Masked or decrypted secrets for non-bridge flows.
Properly enforcesCheckCanSecretsSubjectchecks to safeguard secrets.backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (43)
1-1: Import changes reflect the new permission model.The change from using a general
PureAbilityto a specializedMongoAbility<ProjectPermissionSet>type indicates a more robust permission system that's being implemented throughout the file.
13-20: Properly importing new permission-related functions and types.The imports now include the specialized permission checking functions and the
ProjectPermissionSecretActionsenum that will be used throughout the service.
127-128: Updated parameter type for permission model.The permission parameter type is now a more specific
MongoAbility<ProjectPermissionSet>instead of a generic PureAbility, which aligns with the new permission system being implemented.
200-205: Permission checking updated to use the secret-specific actions.The code now uses the new
CheckForbiddenErrorSecretsSubjecthelper function with the specializedProjectPermissionSecretActions.ReadValuepermission. This ensures consistent permission checking throughout the service.
267-267: Updated permission action for secret creation.Changed from a generic permission action to the more specific
ProjectPermissionSecretActions.Createfor more granular permission control.
418-418: Updated permission action for secret editing.Changed from a generic permission action to the more specific
ProjectPermissionSecretActions.Editfor more granular permission control.
427-444: Improved tag validation and permission checking.The code now assigns the tags to check based on whether new tags are provided, and properly constructs the subject with conditional tag inclusion, improving both permission logic and readability.
456-463: Consistent permission checking for new secret names.When changing a secret name, the code properly checks if the user has edit permission for the new secret name location, with the same tag handling as above.
545-552: Clean permission check implementation.The code now uses the
CheckCanSecretsSubjectfunction to determine if the secret value should be hidden, creating a boolean flag that will be passed to thereshapeBridgeSecretfunction.
554-564: Consistent secret value visibility handling.The
reshapeBridgeSecretfunction now receives thesecretValueHiddenflag to determine whether to mask the secret value, ensuring consistent behavior across the service.
610-610: Updated permission action for secret deletion.Changed from a generic permission action to the more specific
ProjectPermissionSecretActions.Deletefor more granular permission control.
652-674: Added permission check for viewing secret values in deleted secrets.The code now properly checks if the user has permission to read the value of the secret being deleted, and applies the appropriate
secretValueHiddenflag when reshaping the secret for the response.
701-701: Permission check updated for secret description.The code now uses
CheckForbiddenErrorSecretsSubjectwithProjectPermissionSecretActions.DescribeSecretinstead of the generic permission approach for describing secrets.
758-764: Added support for filtering by permission action.The getSecretsByFolderMappings function now accepts a
filterByActionparameter that defaults toProjectPermissionSecretActions.ReadValue, allowing more flexible permission filtering.
782-813: Improved permission filtering for secret retrieval.The code now filters secrets based on the specified permission action and adds a
secretValueHiddenflag to each secret based on whether the user has permission to read the value.
814-829: Consistent secret reshaping with permission-based value hiding.The
reshapeBridgeSecretfunction is now consistently called with thesecretValueHiddenflag to determine whether to mask the secret value in the response.
858-858: Updated multi-environment permission check.Changed to use
CheckForbiddenErrorSecretsSubjectwith theProjectPermissionSecretActions.DescribeSecretaction for permission checking in the multi-environment context.
878-879: Added userId and permission filtering parameters.The code now properly passes the actor ID and filters by the
ProjectPermissionSecretActions.DescribeSecretaction when retrieving secrets across multiple environments.
894-899: Added viewSecretValue and throwOnMissingReadValuePermission parameters.This allows more control over whether to show secret values and whether to throw errors when the user lacks read value permission.
910-914: Updated permission check for describing secrets.The code now uses
CheckForbiddenErrorSecretsSubjectwith environment, path, and tag information when checking if a user can describe secrets.
952-996: Enhanced permission filtering for secret retrieval.This section implements a robust filtering system that:
- First checks if the user can describe the secret
- If viewSecretValue is true, checks if the user can read recursive secret values
- Optionally throws an error if the user lacks read permission and throwOnMissingReadValuePermission is true
This ensures that only secrets the user has permission to view are returned.
997-1024: Implemented secretValueHidden flag for consistency.The code now calculates a
secretValueHiddenflag based on permissions and viewSecretValue parameter, which is then passed to reshapeBridgeSecret to ensure consistent secret value visibility behavior.
1032-1037: Updated permission check for expanding secret references.The code now uses
CheckCanSecretsSubjectwith theProjectPermissionSecretActions.ReadValueaction when checking if a user can expand secret references.
1069-1092: Added viewSecretValue support to imported secrets.The code now checks both describe and read value permissions based on the viewSecretValue parameter when handling imported secrets, providing consistent behavior across the service.
1131-1136: Updated permission check for getting secret by ID.The code now uses
CheckForbiddenErrorSecretsSubjectwith theProjectPermissionSecretActions.ReadValueaction and includes all relevant context for permission checking.
1182-1182: Added viewSecretValue parameter to getSecretByName.This parameter allows the caller to control whether to include secret values in the response.
1243-1248: Updated permission check for retrieving secrets by name.The code now uses
CheckForbiddenErrorSecretsSubjectwith theProjectPermissionSecretActions.DescribeSecretaction to determine if the user can view the secret metadata.
1257-1264: Updated permission check for expanding secret references.The code now uses
CheckCanSecretsSubjectwith theProjectPermissionSecretActions.ReadValueaction when determining if a user can expand secret references.
1271-1289: Added viewSecretValue support to fnSecretsV2FromImports.The code now passes the viewSecretValue parameter and implements a proper permission check for accessing secret information from imports.
1296-1327: Added secretValueHidden handling for imported secrets.This section properly implements the
secretValueHiddenflag based on permissions and the viewSecretValue parameter, ensuring consistent secret value visibility across the service.
1349-1368: Properly handling secretValueHidden for getSecretByName.The code now calculates a
secretValueHiddenflag based on permissions and the viewSecretValue parameter, with appropriate error handling if the user requests to view a secret value they don't have permission to see.
1370-1382: Consistent reshaping of secrets by name.The
reshapeBridgeSecretfunction is now consistently called with thesecretValueHiddenflag to determine whether to mask the secret value in the response.
1525-1544: Added permission checks for bulk-created secrets.Each secret in the response from createManySecret now has its own
secretValueHiddenflag based on the user's permissions, ensuring consistent behavior when returning multiple secrets.
1587-1597: Updated return type for updateManySecret.The function now returns an array of TSecretsV2 objects with additional secretPath and tags information, providing more context for the caller.
1859-1882: Consistent secretValueHidden handling for bulk updates.Each secret in the response from updateManySecret now has its own
secretValueHiddenflag based on the user's permissions, ensuring consistent behavior when returning multiple secrets.
1983-2008: Added secretValueHidden handling for bulk deletes.Each secret in the response from deleteManySecret now has its own
secretValueHiddenflag based on the user's permissions, ensuring consistent behavior when returning multiple secrets.
2027-2031: Added null check for folderWithPath.This ensures that the code doesn't attempt to access properties of folderWithPath if it doesn't exist, preventing potential null reference errors.
2052-2071: Added secretValueHidden handling for secret versions.Each secret version now has its own
secretValueHiddenflag based on the user's permissions, ensuring consistent behavior when returning multiple secret versions.
2168-2199: Improved permission checking for moveSecrets.The code now checks multiple permissions (delete, read, describe) for source secrets and (create, edit) for destination secrets, ensuring the user has all necessary permissions for the move operation.
2520-2523: Updated permission check for getting secret reference tree.The code now uses
CheckForbiddenErrorSecretsSubjectwithProjectPermissionSecretActions.DescribeSecretto verify the user has permission to access the secret metadata.
2544-2549: Consistent permission checking for reference tree secrets.The code continues to use
CheckForbiddenErrorSecretsSubjectwithProjectPermissionSecretActions.DescribeSecretfor the specific secret, including tags information.
2561-2566: Updated permission check for reference expansion.The code now uses
CheckCanSecretsSubjectwithProjectPermissionSecretActions.ReadValueto determine if a reference can be expanded.
2569-2580: Added explicit permission check for getting reference tree.The code now verifies that the user has permission to read the secret value before attempting to get the reference tree, with a clear error message if they don't have access.
backend/src/services/secret/secret-service.ts (29)
16-16: Permission handling enhancement with new subject-based checks.The addition of
CheckCanSecretsSubjectandCheckForbiddenErrorSecretsSubjectfrom the permission module moves the codebase towards a more consistent permission checking mechanism specifically designed for secret operations.
19-21: Improved permission granularity with secret-specific actions.The change from generic
ProjectPermissionActionsto more specificProjectPermissionSecretActionsenhances security by providing fine-grained control over secret-related operations.
57-57: Utility function for consistent secret value hiding.The
conditionallyHideSecretValuefunction will ensure that secret values are consistently hidden based on user permissions across all service functions.
215-217: Permission model migration for create operations.The code now uses the more specific
ProjectPermissionSecretActions.Createinstead of generic permission actions, ensuring proper permission checks for secret creation.
333-335: Permission model migration for edit operations.The code now uses the more specific
ProjectPermissionSecretActions.Editinstead of generic permission actions, ensuring proper permission checks for secret editing.
456-467: Privacy-enhancing secret value protection.This new code block conditionally hides secret values based on the user's permissions, enhancing security by ensuring sensitive data is only exposed to authorized users.
490-492: Permission model migration for delete operations.The code now uses the more specific
ProjectPermissionSecretActions.Deleteinstead of generic permission actions, ensuring proper permission checks for secret deletion.
563-575: Consistent secret value protection in delete operations.Similar to other operations, secret deletion now properly hides sensitive values based on the user's permissions, maintaining a uniform security approach across all operations.
623-626: Improved permission checking approach using subject-based checks.The code replaces the traditional CASL permission checks with a more context-aware checking mechanism using
CheckForbiddenErrorSecretsSubject, which is better suited for secret-related operations.
648-651: Consistent permission checking for imported secrets.This update ensures that permissions are properly verified when accessing imported secrets, maintaining security integrity when accessing secrets across different environments.
702-705: Enhanced permission check for getSecretByName.Updated to use the more specific
CheckForbiddenErrorSecretsSubjectinstead of generic permission checks, aligning with the new permission model.
770-770: Explicit secret visibility for imported secrets.Setting
secretValueHiddento false for imported secrets ensures consistent behavior with the permission check at the beginning of the function.
781-787: Clear visibility indicator in secret response.Adding the
secretValueHiddenflag to the response explicitly indicates to clients whether the secret value is hidden, improving API clarity.
809-811: Consistent permission model for bulk operations.Bulk creation operations now use the same secret-specific permission actions, ensuring consistency across all secret operations.
897-899: Consistent permission checks for bulk update operations.The permission model consistency is maintained for bulk update operations with the secret-specific permission action.
939-981: Security enhancement for bulk update responses.Bulk update operations now properly check permissions and conditionally hide secret values in their responses, ensuring sensitive data is only visible to authorized users.
1015-1017: Consistent permission checks for bulk deletion.The permission model consistency is maintained for bulk delete operations with the secret-specific permission action.
1066-1074: Secret value protection for bulk delete operations.Bulk delete operations now properly hide sensitive values based on user permissions, maintaining the security model across all operations.
1250-1277: Enhanced secret access control with fine-grained permissions.This code segment implements detailed permission checking for different secret operations, providing precise control over who can access, modify, or view secret values.
1303-1311: Added view permission control for raw secret retrieval.The addition of
viewSecretValueandthrowOnMissingReadValuePermissionparameters provides more granular control over secret visibility in API responses.
1359-1359: Improved secret decryption with visibility control.Now secrets are decrypted with proper consideration of the visibility flag, enhancing security.
1427-1428: Conditional secret value expansion based on permissions.The code now skips secret value expansion if the value is hidden due to insufficient permissions, avoiding unnecessary processing and maintaining security.
Also applies to: 1444-1445
1482-1482: View permission control for individual secret retrieval.Added
viewSecretValueparameter to control visibility of secret values at the individual secret level.
1552-1555: Improved response structure for secret retrieval.Enhancing the return structure with metadata field and ensuring consistent handling of secret visibility.
1706-1715: Enhanced security for secret creation responses.The response now properly handles secret value visibility, ensuring sensitive information is only exposed to authorized users.
2111-2112: Consistent visibility control for bulk creation.Ensuring secretValueHidden flag is properly set for bulk-created secrets.
2400-2452: Enhanced version retrieval with permission-based visibility.Complete overhaul of the secret version retrieval flow to include folder path information and properly handle secret value visibility based on user permissions.
2749-2763: Project version-based processing for secret movement.Added support for V3 projects in the moveSecrets function, ensuring compatibility with the new project version.
2812-2867: Comprehensive permission checking for secret movement.Added detailed permission checks for both source and destination environments when moving secrets, ensuring security is maintained during cross-environment operations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (1)
343-352:⚠️ Potential issuePotential leak of newly created secrets to users without readValue permission.
Currently, this method hardcodes
falsefor thesecretValueHiddenflag, returning the unmasked secret value. If a user can create but not read, they still see the secret. Align with other methods by basingsecretValueHiddenonpermission.can(ProjectPermissionSecretActions.ReadValue, ...).Here is a proposed fix:
- false + !hasSecretReadValueOrDescribePermission( + permission, + ProjectPermissionSecretActions.ReadValue, + { + environment, + secretPath, + secretName + } + )
🧹 Nitpick comments (6)
backend/src/services/secret/secret-fns.ts (1)
1204-1223: Consider adding documentation for “conditionallyHideSecretValue”.
A small docstring or comment explaining typical use-cases could improve maintainability and clarity.backend/src/ee/services/permission/permission-fns.ts (2)
46-72: Naming is consistent, but consider clarifying the function’s scope.
This checks both new and legacy permissions (“DescribeAndReadValue”). A comment clarifying the fallback logic could help future maintainers.
74-74: Validation function logic is sound but the error message is wordy.
A briefer phrasing may improve clarity. Otherwise, implementation is correct.Also applies to: 76-102
backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts (2)
193-223: Refactoring might reduce repetition of read-check vs. mask logic.
Consider extracting a helper function to handle decrypt-or-mask in one place for maintainability.
239-285: Duplicate logic could be consolidated.
As above, unify the approach for decrypting or masking secrets into a shared function.backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (1)
1180-1181: Consider adding viewSecretValue support to getSecretId.The method hardcodes
falsefor thesecretValueHiddenflag, which is inconsistent with other methods that check permissions. Consider adding viewSecretValue support to this method for consistent permission handling.I noticed from past comments that this is Terraform-only where the value is needed at all times, but for consistency and future-proofing, you might still want to check permissions here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
backend/src/ee/services/permission/permission-fns.ts(1 hunks)backend/src/ee/services/permission/project-permission.ts(11 hunks)backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts(7 hunks)backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts(7 hunks)backend/src/services/integration/integration-service.ts(3 hunks)backend/src/services/project/project-service.ts(2 hunks)backend/src/services/secret-import/secret-import-service.ts(7 hunks)backend/src/services/secret-sync/secret-sync-service.ts(3 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts(50 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts(7 hunks)backend/src/services/secret/secret-fns.ts(7 hunks)backend/src/services/secret/secret-service.ts(41 hunks)frontend/src/lib/fn/permission.ts(1 hunks)frontend/src/pages/project/RoleDetailsBySlugPage/components/ProjectRoleModifySection.utils.tsx(7 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretRenameRow.tsx(2 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/SecretDashboardPage.tsx(4 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx(10 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretItem.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretRenameRow.tsx
- backend/src/services/project/project-service.ts
- backend/src/services/integration/integration-service.ts
- frontend/src/pages/secret-manager/SecretDashboardPage/SecretDashboardPage.tsx
- backend/src/services/secret-sync/secret-sync-service.ts
- backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts
- backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run integration test
🔇 Additional comments (80)
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx (12)
16-17: Added security warning icon for permission-related UI.The addition of
faTriangleExclamationicon enhances the UI by providing a visual indicator for permission warnings.
23-23: Added tailwind class merging utility.The
twMergeutility is appropriately added to handle conditional class combinations for the updated UI elements.
53-53: Imported granular secret-specific permission actions.The new
ProjectPermissionSecretActionsimport supports the enhanced permission model, enabling more fine-grained control over secret management.
60-60: Added permission helper function import.The
hasSecretReadValueOrDescribePermissionutility function centralizes permission checking logic, improving code maintainability.
135-136: Updated permission check to use secret-specific actions.Replaced generic
ProjectPermissionActions.Editwith the more specificProjectPermissionSecretActions.Edit, aligning with the enhanced permission model.
144-153: Added permission check for viewing secret values.This new permission check properly implements the "ReadValue" permission, enhancing security by ensuring only authorized users can view secret values.
155-167: Improved isReadOnly logic with comprehensive permission checks.The updated logic now properly considers both edit permissions and read value permissions, ensuring a more accurate determination of the read-only state.
347-398: Enhanced secret value display with permission-based UI feedback.The updated UI now provides clear feedback when users lack permission to view secret values, improving user experience by:
- Adding a warning message with appropriate icon
- Wrapping components in a flex container for better layout
- Including tooltips for additional context
- Properly handling the share button's disabled state
This change ensures users understand why they can't view certain secret values.
784-785: Added security check before clipboard operations.The clipboard operations now properly respect the
secretValueHiddenflag, preventing unauthorized access to secret values through clipboard actions.Also applies to: 805-806
828-840: Added descriptive tooltip for hidden secret values.The new tooltip provides clear feedback when users don't have permission to view secret values, and the conditional styling ensures appropriate visual representation.
864-864: Improved display of masked secret values.The display logic now correctly handles both cases where the secret is hidden due to permissions (
secretValueHidden) and where it's masked for security reasons.
888-902: Added version restoration functionality.This enhancement allows users to restore previous versions of secrets with a clear UI that includes tooltips and appropriate button styling.
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretItem.tsx (7)
48-50: Good addition of permission-related importsThe addition of these imports supports the refined permission model, shifting from generic actions to secret-specific actions, which aligns well with the PR's focus on secret value permission handling.
106-113: Appropriate conditional handling for hidden secret valuesThe form values are now properly conditional based on the
secretValueHiddenflag, ensuring that secret values aren't exposed in the form when a user doesn't have permission to view them.
134-144: Permission model update looks correctYou've successfully migrated from generic
ProjectPermissionActionsto the more specificProjectPermissionSecretActions.DescribeSecretandProjectPermissionSecretActions.Edit. This provides finer-grained control over secret-related permissions.Also applies to: 146-146
155-156: Good extraction of secretValueHiddenDestructuring this property from the secret object improves code readability and reduces repetition throughout the component.
288-290: Excellent UI feedback for restricted secretsUsing the
Blurcomponent with an explanatory tooltip provides clear visual feedback to users when they don't have permission to view a secret value, which is much better than simply showing an empty field.
303-303: Proper handling of defaultValueSetting the
defaultValueto an empty string whensecretValueHiddenis true ensures that secret values aren't exposed in the input when a user doesn't have permission.
312-312: Good security measure for related actionsDisabling the copy and share actions when the secret value is hidden is an important security measure. This ensures that users can't perform operations on secret values they don't have permission to view.
Also applies to: 520-520
backend/src/services/secret-import/secret-import-service.ts (5)
7-16: Good addition of granular secret permission actions.The changes introduce more granular permission control by adding the new
ProjectPermissionSecretActionsenum and permission checking functions. This allows for more fine-grained control over who can view secret metadata versus who can view the actual secret values.
100-103: Good replacement of generic permission check with specific secret permission check.Replacing the generic permission check with the more specific
throwIfMissingSecretReadValueOrDescribePermissionusingProjectPermissionSecretActions.DescribeSecretimproves security by ensuring proper permission validation for secret operations.
646-647: Added viewSecretValue parameter to control secret visibility.The
viewSecretValue: trueparameter is added to control secret visibility. This is part of the enhanced permission model that allows toggling secret value visibility based on permissions.
651-657: Improved permission check for accessing secrets.The
hasSecretAccesscallback is updated to use the more granular permission check withProjectPermissionSecretActions.ReadValue, which better restricts access to secret values based on user permissions.
683-687: Added secretValueHidden parameter to control secret value masking.The
secretValueHidden: falseparameter is added to thedecryptSecretRawfunction, controlling whether secret values should be masked based on user permissions.frontend/src/lib/fn/permission.ts (1)
1-36: Consider renaming permission function to clarify its purpose.The
hasSecretReadValueOrDescribePermissionfunction provides a good implementation for checking both new granular and legacy permissions. However, as previously suggested in comments, the function name could be clearer to indicate it's specifically for resolving legacy read permissions with the new read value/describe permissions model.Consider a more descriptive name like
hasSecretReadPermissionorcanAccessSecretWithLegacySupport.backend/src/ee/services/permission/project-permission.ts (4)
20-27: Good introduction of granular secret permission actions.The new
ProjectPermissionSecretActionsenum provides more specific control over secret-related permissions, separating the ability to view metadata (DescribeSecret) from viewing the actual values (ReadValue). The inclusion ofDescribeAndReadValuefor backward compatibility is a good approach.
566-576: Properly updated admin permission rules.The admin permission rules now correctly include all the new secret-specific actions, ensuring that admins maintain full access under the new permission model.
890-902: Enhanced service token permissions for secrets.The service token permission builder now correctly handles secret permissions, applying different permission checks for read access to secrets vs. other resources. This provides better control over what service tokens can access.
959-966:❓ Verification inconclusive
Ensure backfill logic handles all edge cases.
The backfill logic for converting permissions from V1 to V2 schema includes special handling for secrets. Verify that all combinations of permissions are correctly handled, especially for roles with custom permission sets, to avoid any permission degradation during the transition.
🏁 Script executed:
#!/bin/bash # Check for any roles with unusual permission combinations fd "backfill.*Permission" --type f -x grep -B5 -A5 "backfillPermission" {} \;Length of output: 80
Review the secret backfill logic for edge case handling in permission conversion
The search for explicit “backfillPermission” references returned no results, so we couldn’t automatically verify if any unusual permission combinations exist. However, please ensure that the conversion logic in
backend/src/ee/services/permission/project-permission.ts(lines 959-966) handles all edge cases, particularly for roles with custom permission sets. In particular, verify that:
- The mapping for
secretPoliciescorrectly augments the permission actions by appendingProjectPermissionSecretActions.ReadValuewhen necessary.- No role ends up with degraded permissions after conversion, especially if the role has a custom set of permissions.
- The absence of a dedicated backfill script (or explicit “backfillPermission” markers) is intentional and that all migration concerns are addressed within this logic.
Please manually verify these points to ensure the integrity of the permission backfill.
frontend/src/pages/project/RoleDetailsBySlugPage/components/ProjectRoleModifySection.utils.tsx (4)
30-37: Well-defined schema for secret policy actions.The new
SecretPolicyActionSchemaclearly defines the specific permissions for secret management, aligning with the backend changes and providing a structured approach for the UI form.
301-325: Good handling of new secret permissions in form conversion.The
rolePermission2Formfunction now correctly handles the new secret-specific permission actions, ensuring that roles are properly displayed and edited in the UI.
527-548: Helpful tooltip for legacy permission.The addition of a tooltip indicating that the "Read" action is legacy and will be replaced by "Describe Secret" and "Read Value" provides good guidance to users during the transition to the new permission model.
535-544: Consider removing unnecessary comments.The comments in the tooltip JSX content (lines 536-543) seem unnecessary and could be removed to improve code readability, as previously noted in review comments.
<div> - This is a legacy action and will be removed in the future. - <br /> - <br /> You should instead use the{" "} - <strong className="font-semibold">Describe Secret</strong> and{" "} - <strong className="font-semibold">Read Value</strong> actions. + This is a legacy action and will be removed in the future. + <br /> + <br /> You should instead use the <strong className="font-semibold">Describe Secret</strong> and <strong className="font-semibold">Read Value</strong> actions. </div>backend/src/services/secret/secret-fns.ts (6)
14-14: No issues importing "hasSecretReadValueOrDescribePermission".
Everything looks consistent with its usage elsewhere.
16-16: Import of "ProjectPermissionSecretActions" is valid.
No concerns; this aligns with the new permission model.
54-54: Good approach to define a masking constant.
This descriptor clearly identifies masked secrets and remains consistent across the codebase.
346-346: New boolean field seems logically correct.
Extending the secret interface withsecretValueHiddenprovides clarity on whether to expose or mask the secret value.
390-390: Exposing the hidden state is appropriate.
It’s useful to inform consumers of the API whether the value is masked or not.
365-373:✅ Verification successful
Ensure correct assignment of the
secretValueHiddenflag.
This logic relies on a properly setsecretValueHiddento mask or decrypt the value. Any mismatch could risk exposing secrets.Please run this script to confirm every assignment of
secretValueHiddenaligns with accurate permission checks:
🏁 Script executed:
#!/bin/bash # Searching for code that sets or references `secretValueHidden` rg -A 5 'secretValueHidden'Length of output: 65599
Verification Complete: The assignment of the
secretValueHiddenflag is consistent with permission checks across the codebase.After running the search script, we confirmed that:
- In backend files such as
secret-service.tsandsecret-v2-bridge-service.ts, the flag is routinely set using permission checks (e.g., viahasSecretReadValueOrDescribePermission).- The usage in
secret-fns.ts(lines 365–373) correctly follows the flag’s state to either decrypt the secret or apply the masked value.- Across both frontend and backend components, references to
secretValueHiddenuphold the intended logic, ensuring secrets are not inadvertently exposed.No discrepancies were found in the assignment or propagation of the
secretValueHiddenflag. Please continue to maintain this approach for future modifications.backend/src/ee/services/permission/permission-fns.ts (3)
1-3: ESLint configuration and imports look fine.
The disable is presumably needed; no issues with these imports.
6-6: New error imports and project-permission imports are consistent.
They align properly with the below permission checks.Also applies to: 9-15
17-45:❓ Verification inconclusive
Double-check fallback logic for partial permissions.
If the user lacks permission forReadValueorDescribeSecret, the code retries withDescribeAndReadValue. Ensure this is intended to avoid inadvertently granting broader access.
🏁 Script executed:
#!/bin/bash # Validate how this function is called to confirm the fallback is purposeful. rg -A 5 'throwIfMissingSecretReadValueOrDescribePermission'Length of output: 20992
Double-check the fallback permission logic for secret access
The function now consistently attempts the initial permission check (using eitherReadValueorDescribeSecret) and, upon failure, falls back to checking the broaderDescribeAndReadValuepermission. Based on the call sites across the codebase, this behavior appears intentional. However, please confirm that:
- Falling back to
DescribeAndReadValuein the catch block is the desired behavior and won’t inadvertently grant excessive access.- The design around partial permissions is fully aligned with the intended security model.
backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts (6)
3-3: Import of ForbiddenError is aligned with CASL usage.
No further concerns here.
15-15: Mask constant import is well-integrated.
It’s consistent with the rest of the secret masking logic.
26-29: Permission function imports fit the new approach.
No issues integrating them for snapshot checks.
31-35: Properly importing secret permission enums.
These appear consistent with usage in permission checks.
109-112: Requesting “DescribeSecret” access is crucial for snapshot reads.
Ensures the user can legitimately view metadata.
146-149: Consistent secret permission enforcement.
Same check ensures the user can describe folder’s secrets.backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (14)
1-1: Updated permission handling imports and types.The changes correctly update the imports and types to use the new permission system based on
MongoAbility<ProjectPermissionSet>and add helper functions for permission checks.Also applies to: 13-16, 18-23
203-209: Permission check replaced with specialized helper function.The code has been refactored to use the specialized
throwIfMissingSecretReadValueOrDescribePermissionhelper function, which makes the permission check more specific and consistent.
548-571: Improved permission-based secret value visibility handling.The code now properly determines whether to hide secret values based on user permissions using the
hasSecretReadValueOrDescribePermissionfunction, ensuring that sensitive information is only revealed to authorized users.
660-685: Permission-based value visibility in deleteSecret.The implementation correctly determines if the secret value should be hidden based on user permissions before returning the deleted secret.
794-813: Enhanced permission checks with secret-specific actions.The implementation properly uses the new
hasSecretReadValueOrDescribePermissionhelper function to check if users have access to read secret values or describe secrets based on specific criteria like environment, path, name, and tags.Also applies to: 1004-1028
953-1001: Comprehensive permission filtering for recursive secret access.The implementation now correctly filters secrets based on user permissions, handling both describe and read value permissions for recursive secrets. The code also determines whether to throw an error or mask the secret value based on the
throwOnMissingReadValuePermissionparameter.
1256-1261: Permission checks properly implemented for getSecretByName.The code correctly validates that users have both describe and read permissions before returning unmasked secret values, enhancing security.
Also applies to: 1364-1381
1282-1285: Proper permission handling for imported secrets.The code now correctly handles permissions for imported secrets, ensuring that users can only access values for which they have proper permissions.
Also applies to: 1309-1328
1539-1561: Permission-based value visibility in createManySecret.The code now properly hides secret values based on user permissions when returning newly created secrets.
1877-1903: Consistent permission checking in updateManySecret.The implementation applies the same permission-based visibility pattern across all returned secrets, ensuring a consistent approach to security.
2005-2029: Permission-based value visibility in deleteManySecret.The code appropriately checks permissions before revealing secret values in the returned deleted secrets.
2073-2096: Secret versions now properly respect permission-based visibility.The implementation ensures that secret version history is only visible to users with appropriate permissions.
2193-2224: Improved permission checks for moveSecrets.The code now uses a more granular approach to checking permissions with arrays of required actions for both source and destination secrets.
2594-2606: Required permission check for secret reference tree.The implementation correctly enforces that users must have read value permission to retrieve the secret reference tree, preventing unauthorized access to secret values.
backend/src/services/secret/secret-service.ts (18)
9-60: Essential security enhancement: New permission model for fine-grained secret access controlThe migration from generic actions to secret-specific actions with the newly imported
ProjectPermissionSecretActionsenum and helper functions improves security through fine-grained permission control. The inclusion ofconditionallyHideSecretValueprovides a consistent mechanism for hiding secret values based on permissions.
218-220: Updated permission checks for core secret operationsProperly replaced generic
ProjectPermissionActionswith more specificProjectPermissionSecretActionsfor create, edit, and delete operations. This enhances security by providing granular control over secret-related actions.Also applies to: 336-338, 497-499
459-474: Added secret value visibility control to updateSecretGood implementation of permission-based value hiding. The function now consistently checks if the user has permission to view secret values and conditionally hides them in the response.
570-586: Added secret value visibility control to deleteSecretProperly implemented security check to determine if the secret value should be hidden in the response based on user permissions.
634-637: Improved permission handling in getSecretsReplaced direct permission checks with the more specific
throwIfMissingSecretReadValueOrDescribePermissionandhasSecretReadValueOrDescribePermissionfunctions, enhancing both security and code readability.Also applies to: 659-662
713-716: Enhanced permission checking in getSecretByNameImplemented cleaner permission validation using the new permission functions. This provides more consistent and explicit permission handling for accessing secret values.
Also applies to: 764-767
794-798: Added secretValueHidden flag to getSecretByName responseThe method now explicitly indicates when secret values are hidden through the
secretValueHiddenproperty in the response. The flag is set to false here because permissions are checked at the beginning of the function.
820-822: Updated bulk operation permission checksCorrectly replaced generic
ProjectPermissionActionswith specificProjectPermissionSecretActionsin bulk create, update, and delete operations, ensuring consistent permission handling across the service.Also applies to: 908-910, 1030-1032
983-995: Added secret value visibility to updateManySecretWell-implemented permission check for secret value visibility in bulk update operations. The code properly applies conditional hiding to each secret in the transaction.
1081-1093: Added secret value visibility to deleteManySecretCorrectly implemented permission check for secret value visibility in bulk delete operations, ensuring sensitive data is only returned to authorized users.
1322-1340: Added viewSecretValue parameter to getSecretsRawThe method now accepts an explicit viewSecretValue parameter and properly passes it to the bridge service, enhancing control over secret visibility throughout the application.
1378-1378: Added secret value hiding in secret expansionThe code now skips expanding secret values that are hidden due to permissions, preventing unauthorized access to sensitive data during reference expansion.
Also applies to: 1446-1463
1501-1521: Added viewSecretValue parameter to getSecretByNameRawProperly updated the method signature and parameter passing to support the viewSecretValue flag, maintaining consistent permission handling across the service.
1728-1734: Enhanced response security in createSecretRawNow explicitly specifies
secretValueHidden: falsewhen decrypting secrets, ensuring proper visibility control in the response.
2130-2130: Set explicit secretValueHidden flag in createManySecretsRawAdded
secretValueHidden: falseto ensure proper handling of secret visibility in the response.
2451-2474: Enhanced security in getSecretVersionsProperly implemented permission checking for secret version history. The code now checks if the user has permission to view secret values for each version and conditionally hides sensitive data.
2500-2502: Updated permission checks for tag operationsCorrectly replaced generic
ProjectPermissionActionswith more specificProjectPermissionSecretActionsfor tag attachment and detachment, maintaining consistent permission handling across the service.Also applies to: 2606-2608
2772-2890: Enhanced security for moveSecrets operationComprehensive improvement to the moveSecrets method with proper project version check and detailed permission validation for both source and destination paths. The permission checking is thorough and ensures that users have appropriate access to both locations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/src/ee/services/permission/permission-fns.ts (1)
74-105: Well-designed validation for incompatible permission combinationsThe function successfully prevents users from selecting both the legacy DescribeAndReadValue permission and the new granular permissions simultaneously. The error message clearly explains that Read is a legacy action that has been replaced by more specific permissions.
One minor enhancement could be to make the legacy nature of the permission even more explicit in variable naming:
-const hasReadValue = permission.action.includes(ProjectPermissionSecretActions.ReadValue); -const hasDescribeSecret = permission.action.includes(ProjectPermissionSecretActions.DescribeSecret); +const hasNewReadValuePermission = permission.action.includes(ProjectPermissionSecretActions.ReadValue); +const hasNewDescribeSecretPermission = permission.action.includes(ProjectPermissionSecretActions.DescribeSecret);backend/src/ee/routes/v2/project-role-router.ts (1)
98-101: Addressed previous review comments about permission validationThis change addresses the previous review comment by adding permission validation for role updates. Note the use of
superRefinehere versusrefinein other places - both will work but it might be worth standardizing the approach across all routes for consistency.Consider standardizing to either
refineorsuperRefineacross all validation instances for better code consistency:-permissions: ProjectPermissionV2Schema.array() - .describe(PROJECT_ROLE.UPDATE.permissions) - .optional() - .superRefine(checkForInvalidPermissionCombination) +permissions: ProjectPermissionV2Schema.array() + .describe(PROJECT_ROLE.UPDATE.permissions) + .optional() + .refine(checkForInvalidPermissionCombination)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/ee/routes/v1/user-additional-privilege-router.ts(3 hunks)backend/src/ee/routes/v2/identity-project-additional-privilege-router.ts(3 hunks)backend/src/ee/routes/v2/project-role-router.ts(3 hunks)backend/src/ee/services/permission/permission-fns.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check API Changes
- GitHub Check: Check TS and Lint
- GitHub Check: Run integration test
- GitHub Check: Check Frontend Type and Lint check
🔇 Additional comments (7)
backend/src/ee/services/permission/permission-fns.ts (2)
17-44: Robust permission check with fallback to legacy permissionsThis new function implements a robust permission check that first tries the specific permission (ReadValue or DescribeSecret) and falls back to the legacy DescribeAndReadValue permission if the first check fails. This approach effectively handles backward compatibility while supporting the new more granular permission model.
46-72: Good implementation of boolean permission checkThis function cleanly checks if a user has either the new specific permission or the legacy permission, returning a simple boolean result. The explicit naming of variables and clear separation between new and old permission logic makes the code easy to follow.
backend/src/ee/routes/v2/identity-project-additional-privilege-router.ts (2)
34-36: Added validation for permission combinations during creationThe added validation using
checkForInvalidPermissionCombinationensures that users cannot select incompatible permission combinations when creating identity privileges, providing better data integrity and clearer user experience.
98-101: Added validation for permission combinations during updatesThe validation now properly prevents users from selecting incompatible permission combinations when updating identity privileges, maintaining consistency with the creation route.
backend/src/ee/routes/v2/project-role-router.ts (1)
41-43: Added validation for permission combinations in role creationThe added validation using
checkForInvalidPermissionCombinationensures that users cannot select incompatible permission combinations when creating project roles, providing consistency with the other privilege types.backend/src/ee/routes/v1/user-additional-privilege-router.ts (2)
27-29: Added validation for permission combinations in user privilegesThe added validation ensures that users cannot select incompatible permission combinations when creating user additional privileges, maintaining consistency with identity privileges and project roles.
87-88: Consistent validation for user privilege updatesThis completes the comprehensive application of permission validation across all routes that handle permissions, ensuring a consistent user experience and data integrity throughout the application.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx (1)
730-900:⚠️ Potential issueMissing “key” prop in .map iteration.
Each item rendered from an array must have a unique key to prevent rendering issues. Apply this fix:- {secretVersion?.map( - ({ createdAt, secretValue, secretValueHidden, version, id, actor }) => ( - <div className="flex flex-row"> + {secretVersion?.map( + ({ createdAt, secretValue, secretValueHidden, version, id, actor }) => ( + <div key={id} className="flex flex-row">🧰 Tools
🪛 Biome (1.9.4)
[error] 732-733: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
♻️ Duplicate comments (1)
backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (1)
344-353:⚠️ Potential issuePotential issue: Secret value visibility not correctly controlled on creation
The hardcoded
falseforsecretValueHiddenwill always show the secret value to users after creation, even if they don't have theReadValuepermission. This inconsistency could leak sensitive data.Apply this fix to align with the pattern used in other methods:
- false + !hasSecretReadValueOrDescribePermission( + permission, + ProjectPermissionSecretActions.ReadValue, + { + environment, + secretPath, + secretName, + secretTags: secret.tags?.map((i) => i.slug) + } + )
🧹 Nitpick comments (8)
backend/src/ee/services/permission/permission-fns.ts (1)
46-72: Consider simplifying this function with a helper methodThe function correctly checks both new and legacy permission types, but there's significant duplication between the conditional blocks for handling
subjectFields.export function hasSecretReadValueOrDescribePermission( permission: MongoAbility<ProjectPermissionSet>, action: Extract< ProjectPermissionSecretActions, ProjectPermissionSecretActions.DescribeSecret | ProjectPermissionSecretActions.ReadValue >, subjectFields?: SecretSubjectFields ) { - let canNewPermission = false; - let canOldPermission = false; - - if (subjectFields) { - canNewPermission = permission.can(action, subject(ProjectPermissionSub.Secrets, subjectFields)); - canOldPermission = permission.can( - ProjectPermissionSecretActions.DescribeAndReadValue, - subject(ProjectPermissionSub.Secrets, subjectFields) - ); - } else { - canNewPermission = permission.can(action, ProjectPermissionSub.Secrets); - canOldPermission = permission.can( - ProjectPermissionSecretActions.DescribeAndReadValue, - ProjectPermissionSub.Secrets - ); - } + const secretSubject = subjectFields + ? subject(ProjectPermissionSub.Secrets, subjectFields) + : ProjectPermissionSub.Secrets; + + const canNewPermission = permission.can(action, secretSubject); + const canOldPermission = permission.can( + ProjectPermissionSecretActions.DescribeAndReadValue, + secretSubject + ); return canNewPermission || canOldPermission; }frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretDropzone/CopySecretsFromBoard.tsx (3)
82-97: Permission-based data fetch logic.
UsingfilterByActionfor conditional value access is a neat approach. Just confirm that togglingshouldIncludeValuesdoes not repeatedly trigger heavy data fetches or degrade performance over time.
104-105: Potential confusion in multi-select flows.
CallingsetValue("secrets", accessibleSecrets)overrides any previously selected secrets. If partial selection is required, consider merging instead of overwriting.
243-246: Resetting secret selection on toggle.
Replacing the entiresecretsarray with[]prevents mismatched states but may surprise users by discarding previous selections seamlessly. Consider prompting the user before clearing.backend/src/server/routes/v1/dashboard-router.ts (3)
290-290: Hard-codedviewSecretValue: true.
Having a forcedtruemight be intentional for certain routes. Double-check whether permissions require a more dynamic approach to preserve privacy.
595-613: Refined secret fetch logic.
TheviewSecretValue: req.query.viewSecretValueflow plusthrowOnMissingReadValuePermission: falseindicates flexible usage. If your intention is silent fallback for unauthorized reads (instead of throwing an error), ensure logs help trace unauthorized attempts.
837-881: New route/accessible-secrets.
Great addition for retrieving secrets with the specified permission action. Ensure that any cached responses or logs do not inadvertently expose secret data.backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts (1)
177-178:secretDAL.findandsecretTagDAL.find.
Exposing.find()broadens data retrieval capabilities. Confirm that you handle indexing or search constraints to avoid performance bottlenecks on large datasets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/src/ee/services/permission/permission-fns.ts(1 hunks)backend/src/server/routes/v1/dashboard-router.ts(13 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts(53 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts(7 hunks)backend/src/services/secret/secret-service.ts(43 hunks)backend/src/services/secret/secret-types.ts(5 hunks)frontend/src/hooks/api/dashboard/index.ts(1 hunks)frontend/src/hooks/api/dashboard/queries.tsx(8 hunks)frontend/src/hooks/api/dashboard/types.ts(3 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretEditRow.tsx(8 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretDropzone/CopySecretsFromBoard.tsx(5 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/services/secret/secret-types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx
[error] 732-733: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check TS and Lint
- GitHub Check: Check API Changes
- GitHub Check: Run integration test
- GitHub Check: Check Frontend Type and Lint check
🔇 Additional comments (93)
frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretEditRow.tsx (9)
28-31: Appropriate imports for new permissions functionality.The new imports for
Blur, updated permission context, andProjectPermissionSecretActionsare correctly added to support the secret value visibility control being implemented.
34-34: Well-selected permission helper function.Good use of a dedicated permission helper function
hasSecretReadValueOrDescribePermissionthat encapsulates the permission logic, making the code more maintainable.
45-45: Property correctly added to Props interface.The
secretValueHiddenboolean property has been properly added to the Props interface, ensuring type safety when using this component.
65-65: Component parameter matches Props interface.The
secretValueHiddenparameter has been correctly added to the component destructuring, maintaining consistency with the Props interface.
85-86: Permission hook correctly implemented.The PR properly uses the project permission hook to retrieve permission data needed for the conditional rendering logic.
128-131: Permission check logic is well-implemented.The code correctly uses
hasSecretReadValueOrDescribePermissionwith the appropriate permission action (ProjectPermissionSecretActions.ReadValue). This ensures that only users with the right permissions can view secret values.
156-176: Good conditional rendering based on permissions.Excellent implementation of the permission-based UI display logic:
- When
secretValueHiddenis true, it shows aBlurcomponent with a helpful tooltip.- When
secretValueHiddenis false, it displays the normal secret input control.This effectively prevents users without proper permissions from viewing secret values while maintaining a good UX with informative feedback.
232-232: Security enhancement for copy functionality.The copy button is correctly disabled when
secretValueHiddenis true, preventing users from copying secret values they don't have permission to view.
258-258: Security enhancement for reference tree access.The reference tree button is properly disabled when the user doesn't have permission to read secret values, ensuring consistency in the permission model. This prevents users from potentially extracting secret information via the reference tree.
backend/src/ee/services/permission/permission-fns.ts (4)
6-6: LGTM - Added import for necessary error typesThe new import for
BadRequestErroris correctly added to support the error handling in the new functions.
9-16: LGTM - Comprehensive imports for permission typesAll the necessary permission-related types have been properly imported to support the new permission handling functions.
17-44: LGTM - Robust permission checking with fallback logicThis function provides a comprehensive permission check with two layers:
- First checks for the broader
DescribeAndReadValuepermission- Falls back to checking for the specific action if the broader check fails
Good use of the try-catch pattern to handle the fallback logic.
74-105: Good validation of incompatible permission combinationsThis function correctly prevents users from selecting invalid permission combinations. The error message clearly explains that the full read permission is legacy and can't be combined with the new granular permissions.
The error message construction is a bit complex with nested ternaries, but it's readable enough for this specific use case.
backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (8)
784-800: Improved permission check using action-specific permissionsThe code now correctly uses the dedicated
hasSecretReadValueOrDescribePermissionhelper instead of the generic CASL ability check, making the permission model more granular and consistent.
953-1030: Well-implemented permission filtering logicThis section correctly filters secrets based on permissions, addressing two key scenarios:
- Whether the user can see the secret at all (DescribeSecret)
- Whether the secret value should be hidden (ReadValue)
The recursive secret handling with nested path checks is particularly thorough.
2194-2200: Cleaner approach with typed constant arraysGood approach using
as constwith readonly arrays to group related permission actions. This improves code readability and type safety.
2617-2695: Well-implemented new method for filtering accessible secretsThe new
getAccessibleSecretsmethod follows best practices:
- It properly checks base permissions first
- Applies filtering based on action-specific permission
- Uses consistent patterns for reshaping secrets
- Returns a clean interface
1082-1106: Comprehensive permission check for imported secretsThis updated logic correctly handles both
DescribeSecretandReadValuepermissions for imported secrets, and uses viewSecretValue as a toggle to determine which permission to check.
2587-2593: Consistent permission checks across the serviceThe updated permission checking in
getSecretReferenceTreealigns with the new permission model, using the same helper functions used throughout the service.
2595-2607: Good user experience with clear error messagesThis block adds a clear error message when a user attempts to get a secret reference tree without having the required
ReadValuepermission, providing better feedback than a generic permission error.
1-1: Good use of granular import from @casl/abilityThe updated import specifies exactly which components are needed from the library, which is a good practice for keeping bundle size small.
frontend/src/hooks/api/dashboard/index.ts (1)
2-2: No concerns about this new export.
It seamlessly aligns with the existing pattern for exporting hooks from this module.frontend/src/hooks/api/dashboard/types.ts (3)
1-1: The new import statement is fine.
Imports forProjectPermissionSecretActionsare consistent with usage in other files.
73-73: Property addition is valid.
IntroducingviewSecretValueis a logical way to control secret value visibility. Ensure that all call sites handle it correctly.
106-113: New type definition looks good.
TGetAccessibleSecretsDTOis well-defined and helps standardize input parameters.frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx (8)
16-16: No issues with updated icon.
faTriangleExclamationusage is appropriate for warning or alert context.
23-23: ImportingtwMergeis fine.
Simplifies Tailwind class merging for cleaner code.
53-53: ImportingProjectPermissionSecretActionsis valid.
This maintains consistency with the new permission model.
60-60: Helper function import is okay.
hasSecretReadValueOrDescribePermissioncentralizes logic properly.
134-134: Perm check for editing secrets.
UsingProjectPermissionSecretActions.Editis correct for granular permission checks.
144-144: Read-value permission check.
This aligns with the new action-based permission model.
156-156: Descriptive permission usage.
ProjectPermissionSecretActions.DescribeSecretfosters clearer read logic.
347-399: UI changes for secret value input look solid.
Providing clear messaging when the user lacksread secret valuepermission enhances clarity. Consider adding test coverage for restricted view scenarios.frontend/src/hooks/api/dashboard/queries.tsx (5)
14-14: New import looks good.
BringingTGetAccessibleSecretsDTOinto scope is necessary for typed parameters.
24-24: No issues with importingSecretV3Raw.
Ensures correct typing for the newly introduced fetching logic.
64-72: Query key generator is consistent.
getAccessibleSecretsfits the established pattern indashboardKeysfor param-based memoization.
308-322: Fetching accessible secrets is straightforward.
Implementation follows standard request patterns. Consider adding minimal error handling if the endpoint can fail.
387-414: NewuseGetAccessibleSecretshook.
LeveragesuseQueryeffectively. This approach is maintainable and follows existing patterns.frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretDropzone/CopySecretsFromBoard.tsx (7)
23-23: Ensure consistent usage of permission enumerations.
ImportingProjectPermissionSecretActionsis a solid move toward more granular permission handling. Just verify that it remains in sync with server-side permission checks.
25-25: Good use of the new hook for secret retrieval.
Switching touseGetAccessibleSecretsstreamlines the logic for fetches conditioned on permissions.
36-36: Key-value schema clarity.
Renaming the properties tosecretKeyandsecretValueis more descriptive. Ensure any callers expecting the old property names are updated accordingly.
110-113: Conditional assignment of the secret value.
The approach(shouldIncludeValues && secretValue) || ""is straightforward. However, know that""might be ambiguous for some flows. If needed, clarify an empty string vs. an intentionally omitted secret value.
210-214: Dynamic placeholder text logic.
Displaying "Loading secrets...", "Select secrets...", or "No secrets found..." based on fetch results is user-friendly. This is a convenient UI approach—Nicely done.
216-217: Consistent loading state.
LinkingisLoading={isAccessibleSecretsLoading}withoptions={accessibleSecrets}ensures the UI remains responsive during fetch.
221-222: Accurate key-label mapping.
UsingsecretKeyfor both value and label provides clarity. Just ensure length or uniqueness constraints are handled ifsecretKeycan be replicated.backend/src/server/routes/v1/dashboard-router.ts (10)
4-4: Schema imports are well-organized.
IncludingSecretFoldersSchemaandSecretImportsSchemain a single statement is convenient. Be sure these remain relevant after the shift toSanitizedTagSchema.
8-8: Direct reference toProjectPermissionSecretActions.
Pulling this enum indicates a move toward more fine-grained secret-level permission checks.
19-19: ReplacingSecretTagsSchemawithSanitizedTagSchema.
Ensures that client-side or external data is properly sanitized. Keep verifying that tag-based logic still functions yet remains safe from injection or formatting issues.
120-120: New property:secretValueHidden.
This boolean signals whether the secret value is masked. Good for controlling visibility on the client side.
123-123: Updated tags schema.
Moving toSanitizedTagSchema.array()keeps the system consistent when handling potentially user-supplied tags. Good upgrade.
739-739: Filtering secrets byProjectPermissionSecretActions.DescribeSecret.
Restricting the returned set can prevent accidental overexposure. Verify all relevant calls for environment-based or folder-based permissions are also applied as needed.
898-899: Transforming keys and defaulting view-secret to false.
keys = decodeURIComponent(...)plusviewSecretValue = falseis safer for a route that fetches secrets by keys. This approach reduces accidental leaks.
905-908: Enforcing hidden secret values and sanitized tags.
AddingsecretValueHidden: z.boolean()andtags: SanitizedTagSchema.array()in the response schema is consistent with the new pattern.
917-917: Query param destructuring.
ExtractingviewSecretValuefrom the request is consistent with the route’s approach. Confirm that you handle default scenarios for backward compatibility if older callers omit this param.
392-392:❓ Verification inconclusive
Default view for secret values.
UsingbooleanSchema.default(true)means all secrets are visible by default. Ensure that only authorized users or routes rely on this default.Run a quick check to confirm sensitive data is never exposed to unauthorized roles:
Review the call sites to ensure only legitimate roles get full visibility.
🏁 Script executed:
#!/bin/bash rg --context 3 "viewSecretValue"Length of output: 34492
Security Verification: Confirm
viewSecretValueDefault UsageThe code in
backend/src/server/routes/v1/dashboard-router.tscurrently usesbooleanSchema.default(true)(line 392), which makes secret values visible by default. However, our grep results show that across the codebase the handling ofviewSecretValueis inconsistent—for example, some endpoints explicitly set it tofalse, while others derive it from the request (e.g.,req.query.viewSecretValue). This raises the following points for review:
- Endpoint Authorization: Verify that the endpoint using
default(true)is adequately protected (e.g., viaonRequest: verifyAuth([AuthMode.JWT])) so that only authorized users can trigger secret visibility.- Consistency Across Endpoints: Confirm that the intentional differences between defaulting to
trueandfalseare well documented and that no unauthorized role can inadvertently see secret values due to these inconsistencies.- Parameter Overrides: Ensure that endpoints accepting
req.query.viewSecretValuecorrectly enforce permission checks, preventing a client from overriding the default behavior in an unsafe way.Please verify that the access controls are robust and that the differing defaults align with the intended security model.
backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts (7)
4-4: Permission enum usage.
ReferencingProjectPermissionSecretActionsconsolidates permission logic. Good step toward consistent enforcement across the codebase.
40-41: Enhancing access control.
viewSecretValueplus an optionalthrowOnMissingReadValuePermissionclarifies whether to show or hide the secret text. This pattern boosts clarity for partial or read-only user roles.
54-57: New type for missing read permission scenario.
TGetSecretsMissingReadValuePermissionDTOis well-named. It helps define clear boundary conditions for calls that do not fetch secret values.
68-68: Aligning single-secret fetch with multi-secret fetch.
AddingviewSecretValuetoTGetASecretDTOmaintains feature parity when retrieving an individual secret.
204-206: Expanding data layer for updates.
IncludingbulkUpdateanddeleteTagsToSecretV2with.find()points to advanced scenario handling. Ensure usage is well-documented so attribute changes remain predictable.
352-353: OptionalfilterByActionproperty.
Filtering secrets at the data layer is efficient. Just confirm that skippingfilterByActionwon't inadvertently expose data to less-privileged requests.
355-360: IntroducingTGetAccessibleSecretsDTO.
This new type further refines permission-based secret access patterns. Great addition for cross-service alignment.backend/src/services/secret/secret-service.ts (31)
9-9: Updated permission model for granular secret access control.The code now includes
ProjectVersionand imports critical permission functions likehasSecretReadValueOrDescribePermissionandthrowIfMissingSecretReadValueOrDescribePermission. These changes support the enhanced permission model specifically for secret operations throughProjectPermissionSecretActions.Also applies to: 16-25
60-61: Added utility function for conditional secret value hiding.The
conditionallyHideSecretValuefunction has been imported from secret-fns, enabling conditional hiding of sensitive secret values based on user permissions throughout the service.
84-84: New DTO type for accessible secrets.Addition of
TGetAccessibleSecretsDTOtype supports the new API endpoint for retrieving secrets with specific access permissions.
218-221: Updated permission check for secret creation.Permission check has been updated to use the more specific
ProjectPermissionSecretActions.Createinstead of a generic permission action, providing more granular control over secret management operations.
336-339: Refined permission model for editing secrets.Updated permission check to use
ProjectPermissionSecretActions.Editenhances the security model by providing specific action permissions for secret operations.
460-475: Enhanced security through conditional secret value visibility.The implementation now correctly checks if the user has permission to read the secret value and conditionally hides sensitive data based on this permission check. This is an important security enhancement that prevents unauthorized access to secret values.
497-500: Updated permission check for secret deletion.Permission check now uses the more specific
ProjectPermissionSecretActions.Deleteaction, consistent with the overall permission model enhancement.
571-587: Consistent security model for deleted secrets.Even when returning deleted secrets, the code properly handles permission checks and conditionally hides secret values. This consistent approach ensures sensitive data isn't accidentally leaked during deletion operations.
635-638: Improved permission validation for reading secrets.Replaced old permission check with more explicit
throwIfMissingSecretReadValueOrDescribePermissionfunction that provides better error handling and more granular permission control.
660-664: Enhanced import security for service tokens.Updated permission check for imported secrets uses the specific
ProjectPermissionSecretActions.ReadValueaction and ensures proper access control even for service token scenarios.
714-717: Consistent permission check for secret retrieval by name.Using
throwIfMissingSecretReadValueOrDescribePermissioninstead of the previous check provides consistent permission verification across all secret retrieval methods.
765-769: Secured access to imported secrets.Permission check for imported secrets has been updated to use the specific
ProjectPermissionSecretActions.ReadValueaction, ensuring consistent permission control across different secret retrieval paths.
782-786: Added secretValueHidden flag for consistent visibility control.The code now sets an explicit
secretValueHiddenflag when returning secrets, ensuring consistent visibility control across different parts of the application. In getSecretByName, it's correctly set to false since permissions are checked at the beginning of the function.Also applies to: 793-799
821-823: Updated permission check for bulk secret creation.Permission check now uses the specific
ProjectPermissionSecretActions.Createaction, maintaining consistency with the new permission model.
908-911: Consistent permission model for bulk secret updates.Permission check now uses the specific
ProjectPermissionSecretActions.Editaction for bulk operations, ensuring consistency across the permission model.
951-997: Enhanced security for bulk secret updates.The code now properly checks for read value permissions and conditionally hides secret values in the response for bulk update operations, ensuring consistent security handling across all secret methods.
1031-1033: Updated permission check for bulk secret deletion.Permission check now uses the specific
ProjectPermissionSecretActions.Deleteaction for consistent permission handling across all deletion operations.
1082-1094: Consistent security handling for bulk deletion responses.The code properly checks permissions and conditionally hides secret values in the response for bulk delete operations, ensuring security across all operations.
1316-1347: Added new method for retrieving accessible secrets.This new method provides an efficient way to get secrets that a user has access to, delegating to the bridge service and providing proper error handling for unsupported project versions.
1356-1357: Enhanced security with viewSecretValue parameter.The
viewSecretValueandthrowOnMissingReadValuePermissionparameters provide explicit control over secret visibility during secret retrieval operations.Also applies to: 1373-1374
1412-1412: Improved security for decrypting secrets.The code adds the
secretValueHiddenproperty when decrypting secrets, ensuring consistent security model across all secret-related operations.
1419-1425: Added secretValueHidden property for imported secrets.The code now explicitly sets the
secretValueHiddenflag for imported secrets, maintaining a consistent security model.
1480-1480: Prevented secret value expansion when hidden.The code now correctly skips expanding secret values when they're hidden, ensuring sensitive data remains protected even when processing references.
Also applies to: 1497-1497
1535-1535: Added consistent viewSecretValue parameter.The
viewSecretValueparameter has been added to the method signature and is properly passed to the bridge service, ensuring consistent permission handling.Also applies to: 1555-1555
1759-1768: Enhanced security when creating secrets.The code now properly handles the
secretValueHiddenflag when returning created secrets, ensuring a consistent security model.
2164-2164: Proper secret visibility control for bulk creation.The code explicitly sets
secretValueHiddento false for bulk created secrets, ensuring proper security control.
2453-2457: Fixed potential error in folder path retrieval.Added proper error handling when folder path cannot be found, preventing potential runtime errors.
2472-2508: Enhanced security for secret version retrieval.Added proper permission checking for viewing secret values in versions, ensuring consistent security enforcement across all secret operations.
2806-2820: Added Project Version compatibility check.The code now properly checks if the project is using version 3 before delegating to the appropriate service for moving secrets, ensuring compatibility.
2858-2861: Improved secret movement security.Enhanced security for the moveSecrets function by introducing proper permission checks for both source and destination actions, and correctly implementing secret decryption with appropriate permission validation.
Also applies to: 2869-2924
3298-3299: Added new service methods to the exported interface.Methods
getSecretByIdRawandgetAccessibleSecretshave been added to the exported interface, making them available to consumers of this service.
...c/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx
Outdated
Show resolved
Hide resolved
...nd/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretEditRow.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/lib/fn/string.ts (1)
11-13: Good refactoring - consider handling null/undefined inputs.Moving the
camelCaseToSpacesutility function to this central string manipulation file improves code organization and reusability. The implementation is correct for converting camelCase to spaced text (e.g., "viewSecretValue" → "view Secret Value").For increased robustness, consider handling null or undefined inputs:
export const camelCaseToSpaces = (input: string) => { + if (!input) return ''; return input.replace(/([a-z])([A-Z])/g, "$1 $2"); };frontend/src/hooks/api/dashboard/queries.tsx (2)
24-24: Consider using absolute imports for consistency.You're importing
SecretV3Rawusing a relative path../typeswhile other imports use absolute paths with the@appalias. For consistency, consider using absolute imports throughout the file.-import { SecretV3Raw } from "../types"; +import { SecretV3Raw } from "@app/hooks/api/types";
391-418: Consider adding a select callback for data transformation.Other hooks in this file (like
useGetProjectSecretsQuickSearch) include aselectcallback for data transformation. For consistency, you might want to add one here as well, especially if the raw secrets data might need any processing before being used by components.export const useGetAccessibleSecrets = ({ projectId, secretPath, environment, filterByAction, options }: TGetAccessibleSecretsDTO & { options?: Omit< UseQueryOptions< SecretV3Raw[], unknown, SecretV3Raw[], ReturnType<typeof dashboardKeys.getAccessibleSecrets> >, "queryKey" | "queryFn" >; }) => { return useQuery({ ...options, queryKey: dashboardKeys.getAccessibleSecrets({ projectId, secretPath, environment, filterByAction }), queryFn: () => fetchAccessibleSecrets({ projectId, secretPath, environment, filterByAction }), + select: useCallback((data: SecretV3Raw[]) => { + // Transform data if needed + return data; + }, []), + placeholderData: (previousData) => previousData }); };frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx (1)
804-827: Consider extracting duplicate clipboard functionality.The clipboard functionality is duplicated between the click and keydown event handlers. Consider extracting this into a shared function to improve maintainability.
+ const copySecretValueToClipboard = (target) => { + if (secretValueHidden) return; + + navigator.clipboard.writeText(secretValue || ""); + target.style.borderBottom = "1px dashed"; + target.style.paddingBottom = "-1px"; + + // Create and insert popup + const popup = document.createElement("div"); + popup.className = + "w-16 flex justify-center absolute top-6 left-0 text-xs text-primary-100 bg-mineshaft-800 px-1 py-0.5 rounded-md border border-primary-500/50"; + popup.textContent = "Copied!"; + target.parentElement?.appendChild(popup); + + // Remove popup and border after delay + setTimeout(() => { + popup.remove(); + target.style.borderBottom = "none"; + }, 3000); + }; onClick={(e) => { - if (secretValueHidden) return; - - navigator.clipboard.writeText(secretValue || ""); - const target = e.currentTarget; - target.style.borderBottom = "1px dashed"; - target.style.paddingBottom = "-1px"; - - // Create and insert popup - const popup = document.createElement("div"); - popup.className = - "w-16 flex justify-center absolute top-6 left-0 text-xs text-primary-100 bg-mineshaft-800 px-1 py-0.5 rounded-md border border-primary-500/50"; - popup.textContent = "Copied!"; - target.parentElement?.appendChild(popup); - - // Remove popup and border after delay - setTimeout(() => { - popup.remove(); - target.style.borderBottom = "none"; - }, 3000); + copySecretValueToClipboard(e.currentTarget); }} onKeyDown={(e) => { - if (secretValueHidden) return; - if (e.key === "Enter" || e.key === " ") { - navigator.clipboard.writeText(secretValue || ""); - const target = e.currentTarget; - target.style.borderBottom = "1px dashed"; - target.style.paddingBottom = "-1px"; - - // Create and insert popup - const popup = document.createElement("div"); - popup.className = - "w-16 flex justify-center absolute top-6 left-0 text-xs text-primary-100 bg-mineshaft-800 px-1 py-0.5 rounded-md border border-primary-500/50"; - popup.textContent = "Copied!"; - target.parentElement?.appendChild(popup); - - // Remove popup and border after delay - setTimeout(() => { - popup.remove(); - target.style.borderBottom = "none"; - }, 3000); + copySecretValueToClipboard(e.currentTarget); } }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/hooks/api/dashboard/queries.tsx(8 hunks)frontend/src/hooks/api/reactQuery.tsx(1 hunks)frontend/src/lib/fn/string.ts(1 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/hooks/api/reactQuery.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check Frontend Type and Lint check
- GitHub Check: Check TS and Lint
- GitHub Check: Run integration test
- GitHub Check: Check API Changes
🔇 Additional comments (10)
frontend/src/hooks/api/dashboard/queries.tsx (3)
65-75: LGTM on the accessible-secrets query key implementation.The implementation correctly adds a specific "accessible-secrets" key as the 2nd element of the array, following the suggested pattern from previous review comments.
224-224: LGTM on the viewSecretValue parameter additions.The parameter has been properly added to the function signature, query key generation, and query function call, which ensures the parameter is correctly passed through the entire flow.
Also applies to: 249-249, 266-266
312-326: LGTM on the fetchAccessibleSecrets implementation.The function correctly follows the established pattern for API requests in this file and returns the secrets data from the response.
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx (7)
53-53: Good enhancement: Fine-grained permission control.The addition of
ProjectPermissionSecretActionsimports provides more specific action types for secret management, which is a better security practice than using generic permission actions.
136-143: Appropriate permission check update.Updated to use the more specific
ProjectPermissionSecretActions.Editinstead of the genericProjectPermissionActions.Edit, which aligns with the enhanced permission model.
145-154: Good security enhancement: Secret value visibility control.The added permission check for reading secret values improves security by ensuring users have explicit permission to view sensitive data.
156-168: Logical refinement of read-only determination.The
isReadOnlylogic now correctly considers both edit permissions and read value permissions, creating a more comprehensive permission check.
348-399: Excellent UX improvement for permission restrictions.The UI now provides clear feedback when users lack permission to view secret values, with appropriate warning messages and visual indicators. The flex container helps maintain a consistent layout while accommodating the additional UI elements.
731-901: Consistent secret value protection in version history.The version history now properly respects the
secretValueHiddenstate, preventing unauthorized access to historical secret values while maintaining a good user experience with appropriate placeholder text.
940-943: Improved readability in permission tooltips.Using
camelCaseToSpacesto format action names in tooltips improves readability for users, making the permission information more accessible.
Reverts #3189
Summary by CodeRabbit
New Features
secretValueHiddenandviewSecretValueto improve visibility handling of secret values.Blurintroduced for improved UI representation of restricted secret values.Bug Fixes
Refactor
Chores