fix: mask creds on get #10361
fix: mask creds on get #10361
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 4
🧹 Nitpick comments (1)
packages/nocodb/src/helpers/responseHelpers.ts (1)
2-4: Improve documentation for security-critical functionThe current comment is too basic for a security-focused function.
- /* - remove password from knex config - */ + /** + * Masks sensitive credentials in Knex configuration objects + * @param payload - The configuration object containing potential credentials + * @returns A new object with masked credentials + * @security This function helps prevent credential exposure in API responses + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nocodb/src/controllers/integrations.controller.ts(2 hunks)packages/nocodb/src/controllers/sources.controller.ts(2 hunks)packages/nocodb/src/helpers/responseHelpers.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/nocodb/src/helpers/responseHelpers.ts
[error] 6-8: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 10-10: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests
- GitHub Check: unit-tests-pg
| export function maskKnexConfig(payload: Partial<{ config: any }>) { | ||
| /* | ||
| remove password from knex config | ||
| */ | ||
| if ( | ||
| payload.config && | ||
| payload.config.connection && | ||
| payload.config.connection.password | ||
| ) { | ||
| delete payload.config.connection.password; | ||
| } | ||
|
|
||
| return payload; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and security of credential masking
The current implementation has several areas for improvement:
- Uses
anytype which reduces type safety - Only masks the top-level password field
- Mutates the input object directly
Consider this more robust implementation:
-export function maskKnexConfig(payload: Partial<{ config: any }>) {
+interface KnexConnection {
+ password?: string;
+ [key: string]: unknown;
+}
+
+interface KnexConfig {
+ connection?: KnexConnection;
+ [key: string]: unknown;
+}
+
+export function maskKnexConfig(payload: Partial<{ config: KnexConfig }>) {
+ if (!payload?.config?.connection) return payload;
+
+ // Create a deep clone to avoid mutations
+ const maskedPayload = JSON.parse(JSON.stringify(payload));
+
+ // Mask the password
+ if (maskedPayload.config.connection.password) {
+ maskedPayload.config.connection.password = undefined;
+ }
+
+ return maskedPayload;
+}This implementation:
- Adds proper TypeScript interfaces
- Avoids direct mutation of input
- Uses optional chaining
- Replaces delete with undefined assignment
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function maskKnexConfig(payload: Partial<{ config: any }>) { | |
| /* | |
| remove password from knex config | |
| */ | |
| if ( | |
| payload.config && | |
| payload.config.connection && | |
| payload.config.connection.password | |
| ) { | |
| delete payload.config.connection.password; | |
| } | |
| return payload; | |
| } | |
| interface KnexConnection { | |
| password?: string; | |
| [key: string]: unknown; | |
| } | |
| interface KnexConfig { | |
| connection?: KnexConnection; | |
| [key: string]: unknown; | |
| } | |
| export function maskKnexConfig(payload: Partial<{ config: KnexConfig }>) { | |
| if (!payload?.config?.connection) return payload; | |
| // Create a deep clone to avoid mutations | |
| const maskedPayload = JSON.parse(JSON.stringify(payload)); | |
| // Mask the password | |
| if (maskedPayload.config.connection.password) { | |
| maskedPayload.config.connection.password = undefined; | |
| } | |
| return maskedPayload; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-8: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 10-10: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| maskKnexConfig(source); | ||
|
|
There was a problem hiding this comment.
Ensure consistent credential masking
The masking operation should happen before any conditional nullification to ensure credentials are always masked, even in error cases.
- if (source.isMeta()) {
- source.config = undefined;
- }
- source.integration_config = undefined;
-
- maskKnexConfig(source);
+ // Mask credentials first
+ maskKnexConfig(source);
+
+ // Then apply nullification rules
+ if (source.isMeta()) {
+ source.config = undefined;
+ }
+ source.integration_config = undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| maskKnexConfig(source); | |
| // Mask credentials first | |
| maskKnexConfig(source); | |
| // Then apply nullification rules | |
| if (source.isMeta()) { | |
| source.config = undefined; | |
| } | |
| source.integration_config = undefined; |
| if (integration.type === IntegrationsType.Database) { | ||
| maskKnexConfig(integration); | ||
| } | ||
|
|
There was a problem hiding this comment.
Review credential masking strategy
Current implementation has several security considerations:
- Only database integrations are masked
- Masking happens after config visibility check
- // hide config if not the owner or if not requested
- if (
- includeConfig !== 'true' ||
- (integration.is_private && req.user.id !== integration.created_by)
- )
- integration.config = undefined;
-
- if (integration.type === IntegrationsType.Database) {
- maskKnexConfig(integration);
- }
+ // Always mask credentials first
+ if (integration.config) {
+ // Database credentials
+ if (integration.type === IntegrationsType.Database) {
+ maskKnexConfig(integration);
+ }
+ // Consider masking credentials for other integration types
+ // TODO: Implement credential masking for other integration types
+ }
+
+ // Then apply visibility rules
+ if (
+ includeConfig !== 'true' ||
+ (integration.is_private && req.user.id !== integration.created_by)
+ ) {
+ integration.config = undefined;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (integration.type === IntegrationsType.Database) { | |
| maskKnexConfig(integration); | |
| } | |
| // Always mask credentials first | |
| if (integration.config) { | |
| // Database credentials | |
| if (integration.type === IntegrationsType.Database) { | |
| maskKnexConfig(integration); | |
| } | |
| // Consider masking credentials for other integration types | |
| // TODO: Implement credential masking for other integration types | |
| } | |
| // Then apply visibility rules | |
| if ( | |
| includeConfig !== 'true' || | |
| (integration.is_private && req.user.id !== integration.created_by) | |
| ) { | |
| integration.config = undefined; | |
| } |
💡 Codebase verification
Consider extending credential masking to all integration types
While database credentials are currently masked, other integration types may also contain sensitive information like API keys, OAuth tokens, or access credentials. Since the configurations are already encrypted at storage, consider implementing similar masking for all integration types when returning their configurations in API responses.
Specific locations to update:
packages/nocodb/src/controllers/integrations.controller.ts: Apply masking regardless of integration typepackages/nocodb/src/helpers/responseHelpers.ts: Consider creating a generic maskIntegrationConfig helper
🔗 Analysis chain
Consider credential masking for non-database integrations
The current implementation only masks database credentials, but other integration types might also contain sensitive information.
Let's analyze other integration types for potential credential exposure:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for credential-related fields in integration configurations
echo "Searching for potential credential fields in integration configurations..."
# Look for common credential-related terms
rg -i "password|secret|key|token|credential" --type ts
# Look for specific integration type definitions
ast-grep --pattern 'export enum IntegrationsType {
$$$
}'
Length of output: 80932
Script:
#!/bin/bash
# Search for IntegrationsType enum and related integration configurations
echo "Searching for integration types and configurations..."
# Look for IntegrationsType enum with a more relaxed pattern
rg -A 10 "enum IntegrationsType" --type ts
# Look for credential-related fields specifically in integration files
rg -i "password|secret|key|token|credential" --type ts -g "*integration*.ts"
# Look for masking functions
rg -i "mask.*config|mask.*credential|mask.*secret" --type ts
Length of output: 3316
Script:
#!/bin/bash
# Search for integration types and their configurations
echo "Searching for integration types and configurations..."
# Look for IntegrationsType enum with a different pattern
rg "type.*=.*'Database'" --type ts -B 5 -A 10
# Look for integration configuration interfaces/types
rg "interface.*Integration|type.*Integration" --type ts -A 10
# Look for integration config fields
rg "config.*=|config.*:|config.*{" --type ts -g "*integration*.ts" -A 5
Length of output: 66145
Script:
#!/bin/bash
# Search for integration configurations and sensitive fields
echo "Searching for integration configurations and sensitive fields..."
# Look for integration config structures
rg "interface.*Config|type.*Config" --type ts -g "*integration*.ts" -A 10
# Look for sensitive fields in integration-related files
rg -i "password|secret|key|token|credential|auth" --type ts -g "*integration*.ts" -B 2 -A 2
# Look for config usage in integration files
rg "\.config\." --type ts -g "*integration*.ts" -B 2 -A 2
Length of output: 11496
Script:
#!/bin/bash
# Search for OAuth and API configurations
echo "Searching for OAuth and API configurations..."
# Look for OAuth-related code
rg -i "oauth|api.?key|client.?id|client.?secret" --type ts -g "*integration*.ts" -B 2 -A 2
# Look for form definitions with credential fields
rg "form.*=.*{" --type ts -g "*integration*.ts" -A 20
# Look for credential validation or encryption
rg "encrypt|validate.*credential|validate.*config" --type ts -g "*integration*.ts" -B 2 -A 2
Length of output: 911
|
Uffizzi Preview |
o1lab
force-pushed
the
nc-chore/hide-creds
branch
from
514cf92 to
be8d309
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/nc-gui/components/workspace/integrations/forms/EditOrAdd/Database/index.vue (4)
97-101: Extract password masking logic into a composableThe password masking functionality could be extracted into a reusable composable for better maintainability and reusability.
Consider creating a composable:
// usePasswordMasking.ts export function usePasswordMasking() { const isMasked = ref(false) const maskHelpText = computed(() => isMasked.value ? 'Re-enter your password to test or update connection' : undefined ) const handleFocus = (formState: any) => { if (isMasked.value) { formState.dataSource.connection.password = '' isMasked.value = false } } return { isMasked, maskHelpText, handleFocus } }
501-506: Improve password field focus handlerThe password field focus handler could be more descriptive and handle edge cases better.
Consider this improved implementation:
-const onFocusPassword = () => { - if (maskedPassword.value) { - formState.value.dataSource.connection.password = '' - maskedPassword.value = false - } -} +const onFocusPassword = () => { + // Only clear if currently masked to preserve user input + if (maskedPassword.value) { + // Clear the masked value + formState.value.dataSource.connection.password = '' + // Allow editing + maskedPassword.value = false + // Optional: Set focus explicitly + nextTick(() => { + const passwordInput = document.querySelector('.nc-extdb-host-password input') + passwordInput?.focus() + }) + } +}
547-550: Improve password masking initializationThe password masking initialization could be more explicit and handle edge cases better.
Consider this improved implementation:
-if (formState.value.dataSource?.connection?.password === null) { - maskedPassword.value = true - formState.value.dataSource.connection.password = '*'.repeat(8) -} +// Check for null/undefined password to indicate masked state +const hasStoredPassword = formState.value.dataSource?.connection?.password === null +if (hasStoredPassword) { + maskedPassword.value = true + // Use a consistent mask length to avoid leaking password length + formState.value.dataSource.connection.password = '*'.repeat(12) +}
612-612: Improve test connection button disabled stateThe test connection button's disabled state could be more descriptive.
-:disabled="isLoading || maskedPassword" +:disabled="isLoading || (maskedPassword && !formState.value.dataSource.connection.password)"This ensures the button is only disabled when actually needed - during loading or when the password is masked but not entered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/nc-gui/components/workspace/integrations/forms/EditOrAdd/Database/index.vue(6 hunks)packages/nocodb/src/controllers/integrations.controller.ts(2 hunks)packages/nocodb/src/controllers/sources.controller.ts(2 hunks)packages/nocodb/src/helpers/responseHelpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nocodb/src/controllers/integrations.controller.ts
- packages/nocodb/src/controllers/sources.controller.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/nocodb/src/helpers/responseHelpers.ts
[error] 6-8: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: release-docker / buildx
- GitHub Check: test (docker-compose/1_Auto_Upstall/tests/install/watchtower.bats)
- GitHub Check: test (docker-compose/1_Auto_Upstall/tests/install/scale.bats)
- GitHub Check: test (docker-compose/1_Auto_Upstall/tests/install/redis.bats)
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests
- GitHub Check: unit-tests-pg
🔇 Additional comments (1)
packages/nocodb/src/helpers/responseHelpers.ts (1)
1-14:⚠️ Potential issueEnhance type safety and security of credential masking
The current implementation has several areas for improvement:
- Uses
anytype which reduces type safety- Directly mutates the input object
- Only masks the top-level password field
- Uses null instead of undefined for masking
Consider this more robust implementation:
-export function maskKnexConfig(payload: Partial<{ config: any }>) { +interface KnexConnection { + connection?: { + password?: string; + [key: string]: unknown; + }; + [key: string]: unknown; +} + +export function maskKnexConfig(payload: Partial<{ config: KnexConnection }>) { + if (!payload?.config?.connection?.password) return payload; + + // Create a deep clone to avoid mutations + const maskedPayload = JSON.parse(JSON.stringify(payload)); + + // Mask the password + maskedPayload.config.connection.password = undefined; + + return maskedPayload; +}This implementation:
- Adds proper TypeScript interfaces
- Avoids direct mutation of input
- Uses optional chaining
- Uses undefined instead of null for better semantics
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-8: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Change type