fix: add masking to log values #38211
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds masking for sensitive setting values in audit events, applies masking across user/system/app update and reset audit paths, adds unit and end-to-end tests, and includes the new spec path in Jest config. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin/User
participant Settings as SettingsService
participant DB as SettingsStore
participant Audit as AuditLogger
Admin->>Settings: updateSetting(id, newValue)
Settings->>DB: fetchPreviousValue(id)
DB-->>Settings: previousValue
Settings->>Settings: shouldMaskSettingInAudit(id)
Settings->>Settings: maskIfNeeded(id, previousValue)
Settings->>Settings: maskIfNeeded(id, newValue)
Settings->>DB: persist(newValue)
Settings->>Audit: emitAuditEvent({ id, prev: maskedPrev, curr: maskedCurr, actor })
Audit-->>Settings: ack
Settings-->>Admin: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/server/settings/lib/auditedSettingUpdates.ts">
<violation number="1" location="apps/meteor/server/settings/lib/auditedSettingUpdates.ts:28">
P1: Revealing the first 3 characters of a secret/password value in audit logs is a security anti-pattern. This partial exposure can aid attackers through brute force narrowing or pattern recognition (e.g., API keys often start with known prefixes like `sk_`). Best practice is to fully mask sensitive values with a fixed placeholder like `[REDACTED]` or `*****` regardless of length, which also avoids leaking the secret's length.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38211 +/- ##
=========================================
Coverage 70.71% 70.71%
=========================================
Files 3142 3142
Lines 108819 108925 +106
Branches 19609 19627 +18
=========================================
+ Hits 76949 77027 +78
- Misses 29869 29894 +25
- Partials 2001 2004 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/20 12:30 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Tue, 20 Jan 2026 12:30:23 GMT |
jonasflorencio
changed the title
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts">
<violation number="1" location="apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts:92">
P2: The “exactly 8 character values” test expects eight asterisks even though the mocked setting value and update are still three-character strings, so the test both fails (actual mask is `'***'`) and no longer exercises the 8-character edge case it claims to cover.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts`:
- Around line 66-99: The test expectations for masked values are inconsistent
with the intended "≤8 chars should be fully masked" policy; update the
assertions in auditedSettingUpdates.spec.ts (including the block around
updateAuditedByUser and the other occurrences at lines ~347-373) so that values
of length ≤8 are replaced by asterisks matching the original value length (e.g.,
for original 'abc' expect '***' and for new 'xy' expect '**'), and adjust the
mockCreateAuditServerEvent call arguments to use those correct masked strings.
- Around line 493-520: The test named "should handle exactly 8 character values"
is using 3-char inputs ('abc' and 'xyz') for settingId 'Three_Char_Password';
update the test data to match the intent by either renaming the test/setting to
reflect 3 characters or changing the mocked values in mockSettings.set and the
auditedFn call to 8-character strings (e.g., replace 'abc' and 'xyz' with 8-char
values) so updateAuditedByUser, mockSettings.set, and the expected masked audit
payload use consistent 8-char inputs.
- Around line 527-558: The test indicates numeric values should not be coerced
or masked by string-only masking logic; update the masking step inside
updateAuditedByUser (or the helper it calls) to apply masking only when typeof
value === 'string' (and leave non-strings like numbers untouched), ensuring the
previous/current audit values use the original numeric values for settingId
'Numeric_Password' and that mockCreateAuditServerEvent receives 12345 and 67890
(or their unmasked non-string representations) instead of '123**'/'678**'.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.tsapps/meteor/server/settings/lib/auditedSettingUpdates.tsapps/meteor/tests/end-to-end/api/settings.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/server/settings/lib/auditedSettingUpdates.ts
- apps/meteor/tests/end-to-end/api/settings.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
🧠 Learnings (11)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
🧬 Code graph analysis (1)
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts (2)
packages/core-typings/src/ISetting.ts (1)
SettingValue(11-20)apps/meteor/server/settings/lib/auditedSettingUpdates.ts (4)
updateAuditedByUser(62-86)updateAuditedBySystem(88-112)updateAuditedByApp(114-136)resetAuditedSettingByUser(42-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts (7)
1-27: Test scaffolding looks solid.
Mock setup and per-test reset keep isolation clean.
29-64: Long-value masking coverage is clear.
The prefix+mask assertions validate the main path well.
102-208: Good coverage for secret/non‑secret and empty values.
These cases exercise the key branches without over‑coupling to internals.
211-305: Wrapper behavior assertions look solid.
Return values and audit emission checks are consistent and focused.
308-345: App‑actor wrapper test is clean and focused.
Good verification of event payload and passthrough return.
382-455: Reset path tests are well‑scoped.
Covers both standard and masked reset flows succinctly.
458-491: Single‑character edge case is well covered.
This guards the smallest input path nicely.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/tests/end-to-end/api/settings.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/settings.ts:486">
P2: The new assertion compares an 8-character substring against the 3-character string `'***'`, so it always passes and no longer verifies that the first eight characters of the previous value contain unmasked data.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/tests/end-to-end/api/settings.ts`:
- Around line 458-542: The audit.settings queries are returning unrelated older
events causing flaky assertions for testPassword/testUsername; fix by recording
a timestamp window around when the SMTP settings are updated (e.g. capture
startTime before performing the update and endTime after) and include those
timestamps in the request.get(api('audit.settings')) query so the returned
events are filtered to that update window; apply this change to both the
SMTP_Password and SMTP_Username queries and then find the
passwordEvent/usernameEvent as before to assert masked previous/current values.
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/settings.ts (1)
433-455: Remove inline comments in the test block.
These comments aren’t needed and conflict with the “avoid code comments in implementation” guideline. As per coding guidelines, please remove them.♻️ Proposed cleanup
- // Get original values to restore later originalSmtpPassword = (await getSettingValueById('SMTP_Password')) as string | undefined; originalSmtpUsername = (await getSettingValueById('SMTP_Username')) as string | undefined; - // Update SMTP settings with test values await updateSetting('SMTP_Password', testPassword); await updateSetting('SMTP_Username', testUsername); @@ - // Restore original values if (originalSmtpPassword !== undefined) { await updateSetting('SMTP_Password', originalSmtpPassword); } else { await updateSetting('SMTP_Password', '');
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/server/settings/lib/auditedSettingUpdates.spec.tsapps/meteor/tests/end-to-end/api/settings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/end-to-end/api/settings.ts
🧠 Learnings (4)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/tests/end-to-end/api/settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/settings.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/tests/end-to-end/api/settings.ts (1)
6-6: Import addition looks fine.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
jonasflorencio
force-pushed
the
fix/security-logs-plain-text
branch
from
104857e to
8380a0d
Compare
Co-authored-by: Kevin Aleman <kaleman960@gmail.com>
Co-authored-by: Kevin Aleman <kaleman960@gmail.com>
julio-rocketchat
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
julio-rocketchat
added
do not merge
jonasflorencio
dismissed stale reviews from julio-rocketchat and KevLehman
via
7017607
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/tests/end-to-end/api/settings.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/settings.ts:427">
P2: Remove `.only` from the describe block so the full test suite runs in CI.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/tests/end-to-end/api/settings.ts`:
- Around line 427-428: The test suite is using describe.only which limits
execution to only that suite; locate the test block with the title 'Masking
sensitive settings' (where the variable originalSmtpPassword is declared) and
remove the .only so it becomes a normal describe call; ensure no other
describe.only or it.only remains in the file so the full e2e suite runs.
♻️ Duplicate comments (2)
apps/meteor/server/settings/lib/auditedSettingUpdates.ts (1)
31-35: Avoid leaking secret prefixes/lengths in audit logs.Current masking reveals the first 3 characters (and length via
*count), which still exposes sensitive material. For audit logs, fully redact to a fixed placeholder to eliminate leakage.🔒 Suggested change (fixed placeholder)
- if (valueLength <= 8) { - maskedValue = '*'.repeat(valueLength); - } else { - maskedValue = valueString.substring(0, 3) + '*'.repeat(valueLength - 3); - } + maskedValue = '[REDACTED]';apps/meteor/server/settings/lib/auditedSettingUpdates.spec.ts (1)
238-266: Test name/data mismatch for the 8‑character edge case.This test claims to validate exactly 8 characters but uses 3‑character values, so it doesn’t exercise the boundary it describes.
🧪 Suggested fix (use 8‑char values)
- it('should handle exactly 8 character values', () => { - const settingId = 'Three_Char_Password'; + it('should handle exactly 8 character values', () => { + const settingId = 'Eight_Char_Password'; @@ - value: 'abc', + value: 'abcdefgh', @@ - const auditedFn = updateAuditedByUser(actor)(mockFn, settingId, 'xyz'); + const auditedFn = updateAuditedByUser(actor)(mockFn, settingId, 'ijklmnop'); @@ - previous: '***', - current: '***', + previous: '********', + current: '********',
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/tests/end-to-end/api/settings.ts`:
- Around line 477-483: Guard against originalSmtpPassword being undefined before
doing substring comparisons: in the test around the variables previous and
originalSmtpPassword, only perform the two expect(...) assertions that call
originalSmtpPassword.substring when originalSmtpPassword is defined (e.g. if
(originalSmtpPassword) {
expect(previous.substring(0,3)).to.equal(originalSmtpPassword.substring(0,3));
expect(previous.substring(3)).to.match(/^\*+$/); } ), and if
originalSmtpPassword is undefined assert an appropriate masked-only behavior for
previous (e.g. expect(previous).to.match(/^\*+$/)); this prevents comparing a
string to undefined and fixes the failing assertion.
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/settings.ts (1)
522-525: Consider adding consistent substring validation for username's previous value.The password test validates that
previous.substring(0, 3)matches the original and the rest are asterisks (lines 477-482), but the username test only verifiespreviouscontains an asterisk. For consistency in verifying the masking implementation, consider adding similar substring checks here.♻️ Suggested enhancement
if (previous && typeof previous === 'string' && previous.length > 0) { expect(previous).to.include('*'); expect(previous).to.not.equal(originalSmtpUsername); + if (originalSmtpUsername && originalSmtpUsername.length > 8) { + expect(previous.substring(0, 3)).to.equal( + originalSmtpUsername.substring(0, 3), + ); + expect(previous.substring(3)).to.match(/^\*+$/); + } }
|
QQ, instead of a brand new label |
julio-rocketchat
added
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
The purpose of this PR is to mask sensitive settings data that was logged and exposed through the security-logs functionality, such as the
secretandpasswordtypes.Before:
After:
Issue(s)
VLN-155
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Tests
Chores
Public API
✏️ Tip: You can customize this high-level summary in your review settings.