Skip to content

Commit 17c1075

Browse files
fix: stop camera credentials leaking through logs and the monitor settings UI (#308)
* fix(logs): find credentials under any URL scheme, keep ERROR details structured A camera's password lives in the monitor's source path as URL userinfo (rtsp://admin:secret@cam/live). Redaction never found it: sanitizeUrl fires only on strings starting http:// or https://, and sanitizeLogMessage matches only /https?:\/\//, so rtsp, rtmp, and anything mid-string passed through verbatim. The HTTP client logs response bodies, and its failure branch logs at ERROR, which clears the default level with no opt-in, so a failed monitors.json request wrote every camera credential to the log file. lib/security/url-credentials.ts owns the one regex that knows where a password sits in a URL, whatever the scheme, wherever in the string. The two copies of the string rules in log-sanitizer collapse into one sanitizeString that runs it first. URL handling now precedes form-data handling, so a query string with a password in it stays a readable URL instead of being percent-encoded into one blob. The ERROR path serialized its details with JSON.stringify before sanitizing. The sanitizer redacts by key, and a flattened object has no keys, so every secret in an ERROR detail survived across ~130 call sites. Details are passed as objects at every level now; both sinks already pretty-print them. Also: cookie and credential join the sensitive keys (ZMSESSID rode out in logged headers), a sensitive key holding an object is recursed into rather than stringified to '[obje...', a single-field form body no longer needs an '&' to be recognized, and console.error(error.stack) is sanitized, since the stack embeds the message where the URL usually is. Refs #307 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(logs): redact ZoneMinder's own log lines before display and export The Server tab maps zmLog.Message straight into the display entry, and the Save and Share buttons export whatever is on screen. zmc and zma log the ffmpeg command line they run, which carries the camera credential, so the log file users attach to a bug report handed out every camera password on the server. Same sanitizeLogMessage the app's own entries go through, applied where the server's entries enter the view. Refs #307 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(monitors): mask camera credentials in the settings dialog ZoneMinder returns Path, User, and Pass to any account that can view a monitor, and pre-1.38 servers have nowhere but the source path to keep a camera password. The dialog rendered both verbatim, with a reveal toggle on the password field, so the credentials for every camera were one tap and one screenshot away. While log redaction is on, the source path shows with its password segment masked and the Pass field loses its reveal button. Host, port, and stream path stay readable: that is what makes the field worth showing. Both fields stay editable. maskUrlCredentials produces what is on screen and restoreUrlCredentials puts the real password back on save, so changing a camera's hostname does not silently wipe a password the user cannot see. Typing over the mask means the new value is the new password. The same helpers back the log sanitizer, so what the UI hides and what the logs hide cannot drift. Refs #307 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(android): keep the access token out of logcat in the PiP activity PipActivity logged the URL it was handed and the raw media3 playback error. The URL is an event video URL, which carries the ZoneMinder access token as a query parameter, and a media3 HTTP failure quotes the URL it failed on with the cause chain repeating it. Both went to logcat in cleartext, where any adb session or bug report picks them up. The onCreate line logs whether a URL arrived rather than the URL. The error line logs the error code name plus a message with query strings stripped, and drops the throwable, whose cause chain reprinted the URL. Not covered by the JS sanitizer: this is native logging, on the other side of the bridge from lib/logger.ts. No gate reaches it, so it is prose in agents/project/native.md rather than a lint rule. Refs #307 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 679bb84 commit 17c1075

18 files changed

Lines changed: 608 additions & 78 deletions

File tree

agents/project/domain-context.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,17 @@ matching reality, fixing it is a protocol change like any rule edit.
1313
`Tags.Id` accepts a single value only and cannot combine with `Id IN`.
1414
Repeating `MonitorId` params ORs them. Filter URLs cap out near 8KB;
1515
batch long id lists.
16+
- `monitors.json` returns the full monitor row to any account that can view
17+
the monitor: `Path`, `User`, `Pass`, `ONVIF_Password`, `Options`. There is
18+
no per-field ACL, so an unprivileged account reads every camera's
19+
credentials straight from the API. Nothing app-side can prevent that; the
20+
app's job is to not widen it by logging or displaying those values
21+
(refs #307).
22+
- A camera password lives inside `Path` as URL userinfo
23+
(`rtsp://user:pass@host/stream`), and pre-1.38 servers have no other field
24+
for it. `lib/security/url-credentials.ts` is the only place that knows how
25+
to find it; both the log sanitizer and the monitor settings UI go through
26+
it.
1627
- Event Server v7.0.22 and later always sends a real `eid` in pushes. The
1728
historical fake-eid bug (a `Date.now()` value where an event id belongs)
1829
was app-side tray handling, not the ES.

agents/project/native.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ Read before Capacitor, TLS, Electron, downloads, or native-path work.
66
- Prefer least-breaking hardening. If a fix necessarily breaks an existing native behavior, document accepted risk.
77
- Plugin import style, TLS trust, and download paths: Native contract in `AGENTS.project.md`. Additionally, plugins match the `@capacitor/core` major version and get mocks in `tests/setup.ts`.
88
- Use `hooks/useCapacitorListener` for plugin listeners.
9+
- Native logging (`Log.*`, `NSLog`, `CAPLog.print`) never passes through the Logging contract's sanitizer, which lives on the JS side. Nothing gates it, so it is on you: never log a URL handed across the bridge, and never log a media or HTTP error object raw. Stream and event URLs carry the access token in the query, media3 quotes the failing URL in its message and again in the cause chain, and logcat is readable by anyone with adb (refs #307). Log an error code and a scrubbed message instead.
910
- `npm run android:sync`, `npm run ios:sync`, and commands that invoke them bump native versions. Revert incidental bumps before commit. Intended bumps are standalone `chore:` commits.
1011
- Never resolve a promise with a `registerPlugin` proxy: the proxy intercepts every property access as a native method call, so `.then` on it is probed as an unimplemented method and the awaiter hangs forever. An async helper resolves with the plugin module's namespace and callers destructure the plugin from it after the await.
1112

app/android/app/src/main/java/com/zoneminder/zmNinjaNG/PipActivity.java

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,12 @@ public class PipActivity extends Activity {
2121

2222
private static final String TAG = "PipActivity";
2323

24+
/** Drops the query string from any URL in the text. ZoneMinder puts the
25+
* access token in the query, and logcat is readable by anyone with adb. */
26+
private static String stripQuery(String text) {
27+
return text == null ? "null" : text.replaceAll("\\?\\S*", "");
28+
}
29+
2430
private ExoPlayer player;
2531
private PlayerView playerView;
2632
private MediaSession mediaSession;
@@ -43,7 +49,10 @@ protected void onCreate(Bundle savedInstanceState) {
4349
long position = getIntent().getLongExtra("position", 0);
4450
String aspectRatioStr = getIntent().getStringExtra("aspectRatio");
4551

46-
Log.d(TAG, "onCreate url=" + url + " position=" + position);
52+
// The URL is deliberately not logged. ZoneMinder stream URLs carry the
53+
// access token as a query parameter, and logcat is readable by anyone
54+
// with adb, so this line handed out a live session token (refs #307).
55+
Log.d(TAG, "onCreate hasUrl=" + (url != null) + " position=" + position);
4756

4857
if (url == null) {
4958
Log.e(TAG, "No URL provided");
@@ -86,7 +95,11 @@ public void onRenderedFirstFrame() {
8695

8796
@Override
8897
public void onPlayerError(@NonNull androidx.media3.common.PlaybackException error) {
89-
Log.e(TAG, "Player error: " + error.getMessage(), error);
98+
// Neither the message nor the throwable is logged raw: a media3
99+
// HTTP failure quotes the URL it failed on, token and all, and
100+
// the cause chain repeats it (refs #307). The error code name
101+
// is the part that identifies the failure anyway.
102+
Log.e(TAG, "Player error " + error.getErrorCodeName() + ": " + stripQuery(error.getMessage()));
90103
finishWithPosition();
91104
}
92105
});

app/src/components/monitor-detail/MonitorSettingsDialog.tsx

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '.
2020
import type { Monitor } from '../../api/types';
2121
import type { MonitorFunction } from '../../pages/hooks/useModeControl';
2222
import { isZmVersionAtLeast } from '../../lib/zm/zm-version';
23+
import { maskUrlCredentials, restoreUrlCredentials } from '../../lib/security/url-credentials';
2324
import { useSettingsStore } from '../../stores/settings';
2425
import { useCurrentProfile } from '../../hooks/useCurrentProfile';
2526
import { toast } from 'sonner';
@@ -88,6 +89,17 @@ export function MonitorSettingsDialog({
8889
const updateProfileSettings = useSettingsStore((state) => state.updateProfileSettings);
8990
const profileSettings = currentProfile ? getProfileSettings(currentProfile.id) : null;
9091

92+
// A camera's password lives in the source URL as userinfo, and on pre-1.38
93+
// servers that is the only place it can live. While log redaction is on, the
94+
// password segment is masked here and the reveal toggle is dropped from the
95+
// Pass field, so a screenshot or a shoulder-surfer gets nothing (refs #307).
96+
// Both fields stay editable: the real password is restored on save whenever
97+
// the mask survives the edit.
98+
const maskCredentials = !profileSettings?.disableLogRedaction;
99+
const displayedPath = maskCredentials
100+
? maskUrlCredentials(monitor.Path ?? '')
101+
: monitor.Path ?? '';
102+
91103
// Per-monitor Go2RTC override
92104
const globalStreamingMethod = profileSettings?.streamingMethod ?? 'auto';
93105
const monitorOverride = profileSettings?.monitorStreamingOverrides?.[monitor.Id];
@@ -123,7 +135,7 @@ export function MonitorSettingsDialog({
123135
const [localVideoWriter, setLocalVideoWriter] = useState(monitor.VideoWriter ?? '0');
124136

125137
// --- Video tab local state ---
126-
const [localPath, setLocalPath] = useState(monitor.Path ?? '');
138+
const [localPath, setLocalPath] = useState(displayedPath);
127139
const [localUser, setLocalUser] = useState(monitor.User ?? '');
128140
const [localPass, setLocalPass] = useState(monitor.Pass ?? '');
129141
const [localMethod, setLocalMethod] = useState(monitor.Method ?? 'rtpRtsp');
@@ -145,7 +157,7 @@ export function MonitorSettingsDialog({
145157
Enabled: (monitor.Enabled === '1' || monitor.Enabled === 'true') ? '1' : '0',
146158
SaveJPEGs: monitor.SaveJPEGs ?? '0',
147159
VideoWriter: monitor.VideoWriter ?? '0',
148-
Path: monitor.Path ?? '',
160+
Path: displayedPath,
149161
User: monitor.User ?? '',
150162
Pass: monitor.Pass ?? '',
151163
Method: monitor.Method ?? 'rtpRtsp',
@@ -154,7 +166,7 @@ export function MonitorSettingsDialog({
154166
Orientation: monitor.Orientation ?? 'ROTATE_0',
155167
EventStartCommand: monitor.EventStartCommand ?? '',
156168
EventEndCommand: monitor.EventEndCommand ?? '',
157-
}), [monitor]);
169+
}), [monitor, displayedPath]);
158170

159171
// One descriptor per editable field. `key` is the ZM API field name sent in
160172
// the save payload, `applies` gates version-specific fields, `value` is the
@@ -195,6 +207,11 @@ export function MonitorSettingsDialog({
195207
fields.forEach((f) => {
196208
if (f.applies && f.value !== serverValues[f.key]) changes[f.key] = f.value;
197209
});
210+
// The Path on screen may carry a mask where the password is. Put the real
211+
// one back, unless the user typed over it (refs #307).
212+
if (changes.Path !== undefined) {
213+
changes.Path = restoreUrlCredentials(changes.Path, monitor.Path ?? '');
214+
}
198215
await onSave(changes);
199216
};
200217

@@ -427,6 +444,7 @@ export function MonitorSettingsDialog({
427444
value={localPass}
428445
onChange={(e) => setLocalPass(e.target.value)}
429446
disabled={!editable || isSaving}
447+
showToggle={!maskCredentials}
430448
className="w-40"
431449
inputClassName="h-8 text-xs"
432450
data-testid="settings-password-input"

app/src/components/monitor-detail/__tests__/MonitorSettingsDialog.test.tsx

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,14 @@ vi.mock('react-i18next', () => ({
1616
useTranslation: () => ({ t: (key: string) => key }),
1717
}));
1818

19+
let disableLogRedaction = false;
20+
1921
const settingsState = {
20-
getProfileSettings: () => ({ streamingMethod: 'auto', monitorStreamingOverrides: {} }),
22+
getProfileSettings: () => ({
23+
streamingMethod: 'auto',
24+
monitorStreamingOverrides: {},
25+
disableLogRedaction,
26+
}),
2127
updateProfileSettings: vi.fn(),
2228
};
2329

@@ -61,6 +67,7 @@ const baseMonitor = {
6167
describe('MonitorSettingsDialog', () => {
6268
beforeEach(() => {
6369
vi.clearAllMocks();
70+
disableLogRedaction = false;
6471
});
6572

6673
it('keeps Save disabled until a field changes', () => {
@@ -100,3 +107,88 @@ describe('MonitorSettingsDialog', () => {
100107
expect(onSave).toHaveBeenCalledWith({ Path: 'rtsp://cam/new-stream' });
101108
});
102109
});
110+
111+
/**
112+
* Camera credentials on screen (refs #307).
113+
*
114+
* Pre-1.38 ZoneMinder has nowhere to put a camera password but the source URL,
115+
* so `Path` is where the secret usually is. While log redaction is on, the
116+
* password segment is masked and the reveal toggle is gone, but both fields
117+
* stay editable: a user changing a camera's hostname must not be forced to
118+
* retype a password they cannot see.
119+
*/
120+
describe('MonitorSettingsDialog credential masking', () => {
121+
const credentialMonitor = {
122+
...baseMonitor,
123+
Path: 'rtsp://admin:S3cret@cam.lan:554/h264',
124+
Pass: 'S3cret',
125+
} as unknown as Monitor;
126+
127+
const renderDialog = (onSave = vi.fn().mockResolvedValue(undefined)) => {
128+
render(
129+
<MonitorSettingsDialog
130+
open
131+
onOpenChange={vi.fn()}
132+
monitor={credentialMonitor}
133+
zmVersion="1.38.0"
134+
onSave={onSave}
135+
/>
136+
);
137+
return onSave;
138+
};
139+
140+
beforeEach(() => {
141+
vi.clearAllMocks();
142+
disableLogRedaction = false;
143+
});
144+
145+
it('masks the password in the source path but keeps the host readable', () => {
146+
renderDialog();
147+
const input = screen.getByTestId('settings-source-input') as HTMLInputElement;
148+
expect(input.value).not.toContain('S3cret');
149+
expect(input.value).toContain('cam.lan:554/h264');
150+
expect(input.value).toContain('admin');
151+
});
152+
153+
it('hides the password reveal toggle while redaction is on', () => {
154+
renderDialog();
155+
expect(screen.queryByLabelText('common.show_password')).toBeNull();
156+
});
157+
158+
it('restores the real password when the user edits the host around the mask', async () => {
159+
const onSave = renderDialog();
160+
const input = screen.getByTestId('settings-source-input') as HTMLInputElement;
161+
162+
fireEvent.change(input, { target: { value: input.value.replace('cam.lan', 'newcam.lan') } });
163+
fireEvent.click(screen.getByTestId('settings-video-save-button'));
164+
165+
await waitFor(() => expect(onSave).toHaveBeenCalledTimes(1));
166+
expect(onSave).toHaveBeenCalledWith({ Path: 'rtsp://admin:S3cret@newcam.lan:554/h264' });
167+
});
168+
169+
it('saves a password the user typed over the mask', async () => {
170+
const onSave = renderDialog();
171+
172+
fireEvent.change(screen.getByTestId('settings-source-input'), {
173+
target: { value: 'rtsp://admin:brandNew@cam.lan:554/h264' },
174+
});
175+
fireEvent.click(screen.getByTestId('settings-video-save-button'));
176+
177+
await waitFor(() => expect(onSave).toHaveBeenCalledTimes(1));
178+
expect(onSave).toHaveBeenCalledWith({ Path: 'rtsp://admin:brandNew@cam.lan:554/h264' });
179+
});
180+
181+
it('keeps Save disabled when only the mask is on screen', () => {
182+
renderDialog();
183+
expect(screen.getByTestId('settings-video-save-button')).toBeDisabled();
184+
});
185+
186+
it('shows the real path and the reveal toggle once redaction is turned off', () => {
187+
disableLogRedaction = true;
188+
renderDialog();
189+
190+
const input = screen.getByTestId('settings-source-input') as HTMLInputElement;
191+
expect(input.value).toBe('rtsp://admin:S3cret@cam.lan:554/h264');
192+
expect(screen.getByLabelText('common.show_password')).toBeTruthy();
193+
});
194+
});

app/src/lib/__tests__/global-error-handlers.test.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,10 @@ describe('installGlobalErrorHandlers', () => {
4545
expect(logs[0].level).toBe('ERROR');
4646
expect(logs[0].context).toMatchObject({ component: 'App' });
4747
expect(logs[0].message).toContain('Uncaught window error');
48-
const details = String(logs[0].args?.[0]);
48+
// Details reach the sink as the object they were logged as, not
49+
// pre-serialized: a flattened string has no keys left for the redaction
50+
// rules to match (refs #307).
51+
const details = JSON.stringify(logs[0].args?.[0]);
4952
expect(details).toContain('Uncaught Error: boom');
5053
expect(details).toContain('app.js:12:5');
5154
expect(details).toContain('boom');
@@ -61,7 +64,7 @@ describe('installGlobalErrorHandlers', () => {
6164
expect(logs).toHaveLength(1);
6265
expect(logs[0].level).toBe('ERROR');
6366
expect(logs[0].message).toContain('Unhandled promise rejection');
64-
const details = String(logs[0].args?.[0]);
67+
const details = JSON.stringify(logs[0].args?.[0]);
6568
expect(details).toContain('async failure');
6669
expect(event.defaultPrevented).toBe(false);
6770
});
@@ -75,9 +78,9 @@ describe('installGlobalErrorHandlers', () => {
7578
// The log store prepends, so logs[0] is the most recent entry.
7679
const logs = useLogStore.getState().logs;
7780
expect(logs).toHaveLength(3);
78-
expect(String(logs[2].args?.[0])).toContain('42');
79-
expect(String(logs[1].args?.[0])).toContain('plain string reason');
80-
expect(String(logs[0].args?.[0])).toContain('undefined');
81+
expect(JSON.stringify(logs[2].args?.[0])).toContain('42');
82+
expect(JSON.stringify(logs[1].args?.[0])).toContain('plain string reason');
83+
expect(JSON.stringify(logs[0].args?.[0])).toContain('undefined');
8184
});
8285

8386
it('truncates long stacks', () => {
@@ -88,7 +91,7 @@ describe('installGlobalErrorHandlers', () => {
8891

8992
const logs = useLogStore.getState().logs;
9093
expect(logs).toHaveLength(1);
91-
const details = String(logs[0].args?.[0]);
94+
const details = JSON.stringify(logs[0].args?.[0]);
9295
expect(details).toContain('truncated');
9396
// The full untruncated stack must not be present.
9497
expect(details).not.toContain('x'.repeat(LOGGING.maxStackLength + 500));

app/src/lib/__tests__/log-sanitizer.test.ts

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -463,3 +463,87 @@ describe('sanitizeLogArgs', () => {
463463
expect(result[3]).toEqual(['item1', 'item2']);
464464
});
465465
});
466+
467+
/**
468+
* Camera credentials reaching the log pipeline (refs #307).
469+
*
470+
* A ZoneMinder API response carries every monitor's connection settings, and
471+
* the HTTP client logs response bodies. These lock the paths that used to
472+
* carry a camera password through redaction untouched.
473+
*/
474+
describe('camera credentials (refs #307)', () => {
475+
beforeEach(() => {
476+
setLogRedactionGate({ isRedactionDisabled: () => false });
477+
});
478+
479+
it('redacts the password inside a monitor Path', () => {
480+
const body = {
481+
monitors: [
482+
{
483+
Monitor: {
484+
Id: '1',
485+
Path: 'rtsp://admin:S3cret@192.168.1.9:554/h264',
486+
SecondPath: 'rtsp://admin:S3cret@192.168.1.9:554/low',
487+
},
488+
},
489+
],
490+
};
491+
expect(JSON.stringify(sanitizeObject(body))).not.toContain('S3cret');
492+
});
493+
494+
it('keeps the host of a monitor Path readable after redacting the password', () => {
495+
const result = sanitizeObject({ Path: 'rtsp://admin:S3cret@cam.lan:554/h264' }) as Record<string, string>;
496+
expect(result.Path).toBe('rtsp://admin:[REDACTED]@cam.lan:554/h264');
497+
});
498+
499+
it('redacts a credential embedded in an ffmpeg option list', () => {
500+
const result = sanitizeObject({
501+
Options: '-rtsp_transport tcp -i rtsp://admin:S3cret@cam/live',
502+
}) as Record<string, string>;
503+
expect(result.Options).not.toContain('S3cret');
504+
});
505+
506+
it('redacts ONVIF credential fields returned by monitors.json', () => {
507+
const result = sanitizeObject({
508+
ONVIF_Username: 'admin',
509+
ONVIF_Password: 'S3cret',
510+
onvif_password: 'S3cret',
511+
}) as Record<string, string>;
512+
expect(result.ONVIF_Password).toBe('[REDACTED]');
513+
expect(result.onvif_password).toBe('[REDACTED]');
514+
expect(result.ONVIF_Username).toBe('admin');
515+
});
516+
517+
it('redacts a credential URL in a log message, whatever the scheme', () => {
518+
const result = sanitizeLogMessage('capture failed for rtsp://admin:S3cret@10.0.0.5/live');
519+
expect(result).not.toContain('S3cret');
520+
});
521+
522+
it('redacts the session cookie in logged headers', () => {
523+
const result = sanitizeObject({
524+
headers: { Cookie: 'ZMSESSID=abcdef123456', 'set-cookie': 'ZMSESSID=abcdef123456' },
525+
}) as { headers: Record<string, string> };
526+
expect(result.headers.Cookie).not.toContain('abcdef123456');
527+
expect(result.headers['set-cookie']).not.toContain('abcdef123456');
528+
});
529+
530+
it('redacts a key named credential', () => {
531+
const result = sanitizeObject({ credential: 'S3cret' }) as Record<string, string>;
532+
expect(result.credential).toBe('[REDACTED]');
533+
});
534+
535+
it('redacts a single-field form body, which has no & to key off', () => {
536+
expect(sanitizeObject('pass=S3cret')).not.toContain('S3cret');
537+
const result = sanitizeObject({ body: 'pass=S3cret' }) as Record<string, string>;
538+
expect(result.body).not.toContain('S3cret');
539+
});
540+
541+
it('redacts a streaming URL query without mangling it into form data', () => {
542+
const result = sanitizeObject({
543+
url: 'https://zm.lan/zm/cgi-bin/nph-zms?user=admin&pass=S3cret&connkey=1',
544+
}) as Record<string, string>;
545+
expect(result.url).not.toContain('S3cret');
546+
expect(result.url).toContain('/zm/cgi-bin/nph-zms');
547+
expect(result.url).toContain('connkey=1');
548+
});
549+
});

0 commit comments

Comments
 (0)