Skip to content

Commit 304ae1a

Browse files
committed
fix: address PR review comments
- Use relative path in .sisyphus/boulder.json - Redact username in NTLM debug logs - Fix syntax error (extra brace) in xhrApi.ts - Fix find command precedence in apply-patches.sh - Add log viewer preload build to rollup config - Fix undefined logLoggingFailure in renderer context - Don't emit raw data on privacy redaction failure - Add allowlist for log file paths (security) - Prevent double-start in preload.ts - Fix markdown table formatting in docs
1 parent e20e95f commit 304ae1a

File tree

11 files changed

+238
-17
lines changed

11 files changed

+238
-17
lines changed

.sisyphus/boulder.json

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
{
2-
"active_plan": "/Users/jean/Github/Rocket.Chat.Electron/.sisyphus/plans/log-viewer-professional-review.md",
2+
"active_plan": ".sisyphus/plans/log-viewer-professional-review.md",
33
"started_at": "2026-01-29T14:31:04.420Z",
4-
"session_ids": [
5-
"ses_3f5e67dc9ffeRCoFXMk5DAQmSe"
6-
],
4+
"session_ids": ["ses_3f5e67dc9ffeRCoFXMk5DAQmSe"],
75
"plan_name": "log-viewer-professional-review"
8-
}
6+
}
Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
# Fix Context Filter Mismatches
2+
3+
## TL;DR
4+
5+
> **Quick Summary**: Fix 3 mismatches between log viewer filters and automatic context detection so filters work correctly.
6+
>
7+
> **Deliverables**:
8+
>
9+
> - Fixed `context.ts` to return matching context names
10+
>
11+
> **Estimated Effort**: Quick (5 minutes)
12+
> **Parallel Execution**: NO - single file change
13+
> **Critical Path**: Edit → Lint → Commit
14+
15+
---
16+
17+
## Context
18+
19+
### Original Request
20+
21+
Verify all context filters follow the same pattern as the outlook filter (automatic detection via stack trace).
22+
23+
### Analysis Findings
24+
25+
**Mismatches Found:**
26+
27+
| Filter Option | Auto-Detection Returns | Should Return | Issue |
28+
| --------------- | --------------------------- | ----------------- | ------------------- |
29+
| `updates` | `'update'` (singular) | `'updates'` | Singular vs plural |
30+
| `notifications` | `'notification'` (singular) | `'notifications'` | Singular vs plural |
31+
| `servers` | None | `'servers'` | No detection exists |
32+
33+
**Working Correctly:**
34+
35+
- `main`, `renderer`, `webview` - Match processType
36+
- `videocall`, `outlook`, `auth`, `ipc` - Match component context
37+
38+
---
39+
40+
## Work Objectives
41+
42+
### Core Objective
43+
44+
Align auto-detected context names with filter options and scoped logger names.
45+
46+
### Concrete Deliverables
47+
48+
- `src/logging/context.ts` updated with 3 fixes
49+
50+
### Definition of Done
51+
52+
- [x] `yarn lint` passes ✅
53+
- [x] Filters match auto-detection and scoped loggers ✅
54+
55+
### Must NOT Have
56+
57+
- No changes to filter logic in logViewerWindow.tsx (detection should match filters, not vice versa)
58+
- No breaking changes to existing working filters
59+
60+
---
61+
62+
## Verification Strategy
63+
64+
### Automated Verification
65+
66+
```bash
67+
yarn lint
68+
```
69+
70+
---
71+
72+
## TODOs
73+
74+
- [x] 1. Fix context detection mismatches in context.ts ✅ (commit: 8923ee14)
75+
76+
**What to do**:
77+
In `src/logging/context.ts`, function `getComponentContext()`:
78+
79+
1. Change `return 'notification';``return 'notifications';` (around line 131)
80+
2. Change `return 'update';``return 'updates';` (around line 134)
81+
3. Add new detection block for servers (before the videocall check):
82+
83+
```typescript
84+
if (
85+
stack.includes('server') ||
86+
stack.includes('Server') ||
87+
stack.includes('servers')
88+
) {
89+
return 'servers';
90+
}
91+
```
92+
93+
**Must NOT do**:
94+
95+
- Don't change any other return values
96+
- Don't modify the detection logic patterns (stack.includes checks)
97+
98+
**Recommended Agent Profile**:
99+
100+
- **Category**: `quick`
101+
- **Skills**: `[]` (no special skills needed)
102+
103+
**Parallelization**:
104+
105+
- **Can Run In Parallel**: NO
106+
- **Parallel Group**: Sequential
107+
- **Blocks**: Task 2
108+
- **Blocked By**: None
109+
110+
**References**:
111+
112+
- `src/logging/context.ts:129-135` - Current notification/update detection to fix
113+
- `src/logging/context.ts:147-152` - videocall detection pattern to follow for servers
114+
- `src/logViewerWindow/logViewerWindow.tsx:127-142` - Filter options that must match
115+
- `src/logging/scopes.ts:34-37` - Scoped logger names to align with
116+
117+
**Acceptance Criteria**:
118+
119+
- [x] `return 'notification'` changed to `return 'notifications'`
120+
- [x] `return 'update'` changed to `return 'updates'`
121+
- [x] New servers detection block added ✅
122+
- [x] `yarn lint` → 0 errors ✅
123+
124+
**Commit**: YES
125+
126+
- Message: `fix(logging): align context detection with filter options`
127+
- Files: `src/logging/context.ts`
128+
- Pre-commit: `yarn lint`
129+
130+
---
131+
132+
## Commit Strategy
133+
134+
| After Task | Message | Files | Verification |
135+
| ---------- | ----------------------------------------------------------- | ---------- | ------------ |
136+
| 1 | `fix(logging): align context detection with filter options` | context.ts | yarn lint |
137+
138+
---
139+
140+
## Success Criteria
141+
142+
### Verification Commands
143+
144+
```bash
145+
yarn lint # Expected: 0 errors
146+
```
147+
148+
### Final Checklist
149+
150+
- [x] `notifications` filter catches notification-related console.logs ✅
151+
- [x] `updates` filter catches update-related console.logs ✅
152+
- [x] `servers` filter catches server-related console.logs ✅
153+
- [x] All existing filters still work ✅
154+
155+
## Completion
156+
157+
**Status**: ✅ COMPLETE
158+
**Commit**: `8923ee14202857a37ac865edd5cc98ea91ec9398`
159+
**Pushed**: Yes (debug-outlook branch)

docs/alpha-release-process.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ The Rocket.Chat Desktop app supports three release channels:
1212
## How Channels Work
1313

1414
| Channel | Version Format | Who Gets It | Update File |
15-
|---------|---------------|-------------|-------------|
15+
| ------- | ------------- | ----------- | ----------- |
1616
| Stable | `4.12.0` | All users (default) | `latest.yml` |
1717
| Beta | `4.12.0-beta.1` | Beta opt-in users | `beta.yml` |
1818
| Alpha | `4.12.0-alpha.1` | Alpha opt-in users | `alpha.yml` |
@@ -85,7 +85,7 @@ The setting persists - users don't need to select it again.
8585
Create `update.json` in the user data directory:
8686

8787
| Platform | Location |
88-
|----------|----------|
88+
| -------- | -------- |
8989
| Windows | `%APPDATA%\Rocket.Chat\update.json` |
9090
| macOS | `~/Library/Application Support/Rocket.Chat/update.json` |
9191
| Linux | `~/.config/Rocket.Chat/update.json` |

patches-src/@ewsjs/xhr/src/ntlmProvider.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ export class NtlmProvider implements IProvider {
4343
console.log('[DEBUG] NTLM Provider - Starting NTLM authentication');
4444
console.log('[DEBUG] NTLM Provider - Target URL:', options.url);
4545
console.log('[DEBUG] NTLM Provider - Domain:', this.domain);
46-
console.log('[DEBUG] NTLM Provider - Username:', this.username);
46+
console.log('[DEBUG] NTLM Provider - Username:', '[REDACTED]');
4747
console.log('[DEBUG] NTLM Provider - Workstation:', ntlmOptions.workstation);
4848
console.log('[DEBUG] NTLM Provider - Reject Unauthorized:', options.rejectUnauthorized);
4949

patches-src/@ewsjs/xhr/src/xhrApi.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,6 @@ export class XhrApi implements IXHRApi {
165165
} else {
166166
console.log('🚫 [INFO] XhrApi - No proxy configuration - direct connection will be attempted');
167167
}
168-
}
169168
options = this.getOptions(options)
170169

171170
console.log('[DEBUG] XhrApi - Final options before auth:', JSON.stringify({

patches-src/apply-patches.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ apply_patch() {
1717
echo "📦 Processing $package_name..."
1818

1919
# Copy all files from patches-src to node_modules
20-
find "$src_dir" -type f -name "*.ts" -o -name "*.js" -o -name "*.json" | while read -r file; do
20+
find "$src_dir" -type f \( -name "*.ts" -o -name "*.js" -o -name "*.json" \) | while read -r file; do
2121
# Get relative path from src_dir
2222
rel_path="${file#$src_dir/}"
2323
dest_file="$dest_dir/$rel_path"

rollup.config.mjs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,41 @@ export default [
182182
},
183183
],
184184
},
185+
// Log viewer preload
186+
{
187+
external: [
188+
...builtinModules,
189+
...Object.keys(appManifest.dependencies),
190+
...Object.keys(appManifest.devDependencies),
191+
].filter((moduleName) => moduleName !== '@bugsnag/js'),
192+
input: 'src/logViewerWindow/preload.ts',
193+
preserveEntrySignatures: 'strict',
194+
plugins: [
195+
json(),
196+
replace({
197+
'process.env.NODE_ENV': JSON.stringify(NODE_ENV),
198+
'preventAssignment': true,
199+
}),
200+
babel({
201+
babelHelpers: 'bundled',
202+
extensions,
203+
}),
204+
nodeResolve({
205+
browser: true,
206+
extensions,
207+
}),
208+
commonjs(),
209+
],
210+
output: [
211+
{
212+
dir: 'app/preload',
213+
entryFileNames: 'log-viewer-preload.js',
214+
format: 'cjs',
215+
sourcemap: 'inline',
216+
interop: 'auto',
217+
},
218+
],
219+
},
185220
{
186221
external: [
187222
...builtinModules,

src/logViewerWindow/ipc.ts

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ const readFile = promisify(fs.readFile);
1515
const writeFile = promisify(fs.writeFile);
1616

1717
let logViewerWindow: BrowserWindow | null = null;
18+
const allowedLogPaths = new Set<string>();
1819

1920
const getLogFilePath = (): string => {
2021
// Use electron-log's default path: ~/Library/Logs/{app name}/main.log
@@ -150,10 +151,17 @@ export const startLogViewerWindowHandler = (): void => {
150151
return { success: false, canceled: true };
151152
}
152153

154+
const selectedPath = result.filePaths[0];
155+
const validation = validateLogFilePath(selectedPath);
156+
if (!validation.valid) {
157+
return { success: false, error: validation.error };
158+
}
159+
const normalizedPath = path.normalize(selectedPath);
160+
allowedLogPaths.add(normalizedPath);
153161
return {
154162
success: true,
155-
filePath: result.filePaths[0],
156-
fileName: path.basename(result.filePaths[0]),
163+
filePath: normalizedPath,
164+
fileName: path.basename(normalizedPath),
157165
};
158166
} catch (error) {
159167
console.error('Failed to select log file:', error);
@@ -172,7 +180,20 @@ export const startLogViewerWindowHandler = (): void => {
172180
if (!validation.valid) {
173181
return { success: false, error: validation.error };
174182
}
175-
logPath = options.filePath;
183+
const normalizedPath = path.normalize(options.filePath);
184+
const defaultLogPath = path.normalize(getLogFilePath());
185+
// Only allow default log path or paths explicitly selected via dialog
186+
if (
187+
normalizedPath !== defaultLogPath &&
188+
!allowedLogPaths.has(normalizedPath)
189+
) {
190+
return {
191+
success: false,
192+
error:
193+
'Log file not authorized. Please select it via the file dialog first.',
194+
};
195+
}
196+
logPath = normalizedPath;
176197
} else {
177198
logPath = getLogFilePath();
178199
}

src/logging/index.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ export const setupWebContentsLogging = () => {
219219
220220
// Get webContents ID and server URL for context
221221
const webContentsId = ${webContents.id};
222-
const serverUrl = '${serverUrl}';
222+
const serverUrl = '${serverUrl.replace(/'/g, "\\'")}';
223223
224224
// Override console methods to send to main process with context
225225
console.log = (...args) => {
@@ -250,7 +250,7 @@ export const setupWebContentsLogging = () => {
250250
// Add marker to know console override is active
251251
console.original = originalConsole;
252252
} catch (error) {
253-
logLoggingFailure(error, 'setupWebContentsLogging - console override injection');
253+
console.error('[logging] Failed to override console in webContents:', error);
254254
}
255255
})();
256256
`;

src/logging/privacy.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,17 +57,20 @@ export const createPrivacyHook = () => {
5757
return (message: any, _transport: any, transportName?: string) => {
5858
if (transportName !== 'file') return message;
5959
try {
60-
const sanitizedData = message.data.map((item: any) => {
60+
// Guard against non-array data
61+
const data = Array.isArray(message.data) ? message.data : [message.data];
62+
const sanitizedData = data.map((item: any) => {
6163
if (typeof item === 'string') return redactSensitiveData(item);
6264
if (typeof item === 'object' && item !== null)
6365
return redactObject(item);
6466
return item;
6567
});
6668
return { ...message, data: sanitizedData };
6769
} catch {
70+
// Don't emit raw data on failure - only safe placeholder
6871
return {
6972
...message,
70-
data: ['[Privacy redaction failed]', ...message.data],
73+
data: ['[Privacy redaction failed]'],
7174
};
7275
}
7376
};

0 commit comments

Comments
 (0)