Repository navigation
New signals - #92
Open
trueroguan wants to merge 2 commits into
Open
New signals#92trueroguan wants to merge 2 commits into
trueroguan wants to merge 2 commits into
Conversation
PR Summary by QodoAdd threshold-gated camera alerts and external actions
AI Description
Diagram
High-Level Assessment
Files changed (43)
|
Code Review by Qodo
1. Active events miss later deliveries
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+167
to
+170
| if (!ep.NotifyDone) | ||
| { | ||
| ep.NotifyDone = true; | ||
| if (ShouldNotifyLocked(e, nowLocal)) |
There was a problem hiding this comment.
1. Active events miss later deliveries 🐞 Bug ≡ Correctness
NotificationCoordinator.Evaluate sets each channel's done flag before checking whether cooldown or quiet hours permit delivery. If an episode first passes its thresholds while a channel is suppressed, later progress updates cannot deliver through that channel even after the suppression ends.
Agent Prompt
## Issue description
An episode permanently marks a channel complete when its delivery gate rejects the first threshold-passing update.
## Fix Focus Areas
- src/OpenIPC.Viewer.Core/Notifications/NotificationCoordinator.cs[159-205]
## Recommended Fix
Set each done flag only when its channel is selected to deliver. Keep suppressed channels eligible on subsequent open-episode updates, while preserving the once-per-channel guarantee after selection.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+221
to
+224
| if (notify) | ||
| { | ||
| var hasName = !string.IsNullOrWhiteSpace(name); | ||
| _sink.Show(new NotificationRequest(title, Describe(e, hasName ? name : null), e.Kind, e.CameraId)); |
There was a problem hiding this comment.
2. A failed toast also drops external actions 🐞 Bug ☼ Reliability
NotificationCoordinator.DeliverAsync calls _sink.Show before dispatching actions inside one try block that silently catches exceptions. If the notification sink throws, the selected actions are skipped and the failure leaves no log entry.
Agent Prompt
## Issue description
A throwing notification sink prevents external actions from running for the same event, and the exception is discarded.
## Fix Focus Areas
- src/OpenIPC.Viewer.Core/Notifications/NotificationCoordinator.cs[208-237]
## Recommended Fix
Handle sink and dispatcher failures independently so action dispatch still runs after a sink exception. Log delivery exceptions with their stack traces.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+120
to
+121
| using var stream = File.OpenRead(sourcePath); | ||
| clip = WavCodec.Read(stream); |
There was a problem hiding this comment.
3. Large sound imports can exhaust memory 🐞 Bug ➹ Performance
NotificationSoundService.ImportAsync reads an entire WAV file through WavCodec.Read before applying its 15-second truncation limit. A long uncompressed file causes a full data-chunk allocation and a second PCM allocation during conversion, despite only a short clip being needed.
Agent Prompt
## Issue description
WAV import loads and converts the complete file before enforcing the custom-sound duration limit.
## Fix Focus Areas
- src/OpenIPC.Viewer.App/Services/NotificationSoundService.cs[112-133]
- src/OpenIPC.Viewer.Core/Notifications/PcmClip.cs[80-118]
## Recommended Fix
Pass the duration limit into WAV reading and read or convert no more frames than that limit permits. Keep playback loading subject to the same bound.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| CooldownSeconds: ActionsCooldown?.Value ?? 60, | ||
| SignalEnabled: SignalEnabled, | ||
| SignalProtocol: SignalProtocol?.Value ?? EventActionSettings.ProtocolTcp, | ||
| SignalHost: SignalHost.Trim(), |
There was a problem hiding this comment.
4. Null action settings break notification saves 🐞 Bug ☼ Reliability
NotificationSettingsViewModel.BuildActions calls Trim() on action fields copied directly from deserialized settings without checking for null. If usersettings.json contains JSON null for an action string, a subsequent notification edit or test invokes SaveNowAsync and raises a NullReferenceException.
Agent Prompt
## Issue description
JSON null action fields reach unconditional Trim calls when notification settings are saved.
## Fix Focus Areas
- src/OpenIPC.Viewer.App/ViewModels/NotificationSettingsViewModel.cs[276-303]
- src/OpenIPC.Viewer.App/ViewModels/NotificationSettingsViewModel.cs[406-430]
## Recommended Fix
Normalize nullable action strings to empty strings on load, and make construction of saved action settings null-safe.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+188
to
+201
| var args = EventActionTemplate.Render(s.ProgramArguments ?? "", context); | ||
| var ext = Path.GetExtension(path).ToLowerInvariant(); | ||
|
|
||
| ProcessStartInfo psi; | ||
| if (ext == ".ps1") | ||
| { | ||
| // A script the user picked themselves; run it without the profile and | ||
| // without tripping over the machine's default execution policy. | ||
| psi = new ProcessStartInfo("powershell.exe", | ||
| $"-NoProfile -NonInteractive -ExecutionPolicy Bypass -File \"{path}\" {args}"); | ||
| } | ||
| else if (ext is ".exe" or ".bat" or ".cmd" or ".com" || !OperatingSystem.IsWindows()) | ||
| { | ||
| psi = new ProcessStartInfo(path, args); |
There was a problem hiding this comment.
5. Camera or event text can inject script commands 🐞 Bug ⛨ Security
StartProgram renders ProgramArguments with EventActionTemplate.Render and no encoder, so
{camera} and {summary} go into the raw command-line string unchanged. The default template is
"{camera}" {kind} "{summary}", so a " in a camera name or detection summary ends the quoted
argument and adds extra arguments. For .bat/.cmd targets, cmd.exe parses that line, so &, |
or > in those values run extra commands.
Agent Prompt
## Issue description
Program action arguments are built by putting unescaped event values (camera name, summary) into a command-line string. Quotes split the arguments, and for .bat/.cmd files cmd.exe metacharacters run extra commands.
## Fix Focus Areas
- src/OpenIPC.Viewer.Infrastructure/Notifications/EventActionDispatcher.cs[182-207]
- src/OpenIPC.Viewer.Core/Notifications/EventActionSettings.cs[48-51]
## Recommended Fix
Pass an encoder to `EventActionTemplate.Render` for the program channel. It should strip or escape `"` and backslashes using Windows argv rules, and for .bat/.cmd it should also neutralise `& | < > ^ %`. A safer alternative is to tokenise the template first, render each token separately and add the results to `ProcessStartInfo.ArgumentList`. Scripts can also read the values from the OPENIPC_* environment variables that are already set.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+22
to
+28
| var options = (s.SmtpSecurity ?? "auto").Trim().ToLowerInvariant() switch | ||
| { | ||
| "ssl" => SecureSocketOptions.SslOnConnect, | ||
| "starttls" => SecureSocketOptions.StartTls, | ||
| "none" => SecureSocketOptions.None, | ||
| _ => port == 465 ? SecureSocketOptions.SslOnConnect : SecureSocketOptions.StartTlsWhenAvailable, | ||
| }; |
There was a problem hiding this comment.
6. Smtp password can be sent unencrypted 🐞 Bug ⛨ Security
SmtpMailSender.SendAsync maps the default SmtpSecurity value of "auto" to StartTlsWhenAvailable on ports other than 465, then calls AuthenticateAsync without checking whether TLS was negotiated. With the default port 587, a server that does not offer STARTTLS—or a network attacker that strips it—can cause the login and password retrieved from credential storage to travel in cleartext.
Agent Prompt
## Issue description
The default SMTP `auto` mode permits authentication with stored credentials over plaintext when STARTTLS is unavailable or stripped.
## Fix Focus Areas
- src/OpenIPC.Viewer.Infrastructure/Notifications/SmtpMailSender.cs[21-46]
- src/OpenIPC.Viewer.Core/Notifications/EventActionSettings.cs[44-46]
## Recommended Fix
Require TLS before authenticating in the default mode: map `auto` on ports other than 465 to `SecureSocketOptions.StartTls`. If opportunistic STARTTLS must remain, check `client.IsSecure` before `AuthenticateAsync` and throw a clear error when it is false, unless the security mode was explicitly set to `none`. Keep any explicitly selected insecure mode clearly distinct from the default.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+101
to
+105
| if (!isNew && state.Published && (grew || tick.At - state.LastPublishedTickAt >= ProgressInterval)) | ||
| { | ||
| update = Snapshot(state, endedAt: null); | ||
| state.LastPublishedTickAt = tick.At; | ||
| } |
There was a problem hiding this comment.
7. Open events rebuild the events page every second 🐞 Bug ➹ Performance
OnTick now publishes the open event again (same Id) at least once per second while ticks keep arriving, and on every peak rise. EventsPageViewModel.OnLiveEvent answers each publication with a full ApplyFilter on the UI thread, which regroups, refolds and repages up to 10,000 cached events and clears/rebuilds PageItems. With several active cameras this means several full list rebuilds per second while the Events page is open, which costs CPU and causes visible list churn.
Agent Prompt
## Issue description
The new once-a-second progress updates of open events trigger a full Events page re-filter and rebuild each time.
## Fix Focus Areas
- src/OpenIPC.Viewer.Core/Events/EventIngestionService.cs[86-114]
- src/OpenIPC.Viewer.App/ViewModels/EventsPageViewModel.cs[380-394]
## Recommended Fix
One option is to publish progress updates on a separate observable that only NotificationCoordinator subscribes to. The other is to make EventsPageViewModel ignore updates for an event it already has (same Id, EndedAt still null) unless a field it displays changed, or to coalesce rebuilds with a debounce.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Collaborator
|
Resolve comments and we will run workflow to doublecheck |
keyldev
requested changes
Oct 17, 2026
keyldev
left a comment
keyldev
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for this, it's a solid piece of work. The threshold and episode logic in NotificationCoordinator holds up: thresholds are checked on every publication, each channel fires at most once per episode, and Core still has no package dependencies. The P/Invoke and COM layouts (WinRT toasts, CREDENTIALW, MAPI) check out, and Windows-only code is properly guarded. Locally the PR builds with 0 warnings.
CI is red for an unrelated reason: a new advisory on System.Security.Cryptography.Pkcs 10.0.0 (GHSA-2fmv-rv2c-59x7, published 2026-10-13) fails restore on main as well. That will be fixed on main; rebase once it lands.
A few things need changing before merge.
Must fix
-
Command injection in the "run program" action (
EventActionDispatcher.cs,StartProgram).ProgramArgumentsis rendered and then passed as one raw command-line string..bat/.cmdgo throughcmd /c, so& | > ^ %in the values are interpreted by the shell.- The default template quotes
"{camera}", but the camera name isn't always typed by the user. Auto-detect fills it from the name the device reports (CameraEditorViewModel). A device on the LAN that calls itselfx" & calc & "runscalcon every event. - The
.ps1and ShellExecute branches have the same quoting break. - Fix: split the template into tokens before rendering, render each token separately, and pass them through
psi.ArgumentList. For.bat/.cmd, also reject or strip"%&|<>^and line breaks in substituted values, or pass event data only through theOPENIPC_*environment variables. Those variables are currently not set on the ShellExecute branch, although the comment says scripts can rely on them.
-
Event data can inject bytes into TCP/UDP payloads (
SendSignalAsync).- The template is rendered first and the escapes are decoded after, so a camera named
Gate\r\nDISARMor containing\x00puts raw bytes into an alarm panel or PLC stream. - In
hex:mode, a placeholder value that isn't hex throws. - Fix: decode escapes in the template before substituting values (or escape
\in the values), and reject placeholders in hex mode.
- The template is rendered first and the escapes are decoded after, so a camera named
-
SMTP "auto" can send the password in plain text (
SmtpMailSender.cs).- On any port other than 465, "auto" maps to
StartTlsWhenAvailable. Someone in the network path can strip the STARTTLS offer, and AUTH then goes out unencrypted. - Fix: when a login is configured, "auto" should use
StartTls, which fails if STARTTLS is missing.
- On any port other than 465, "auto" maps to
Should fix
- The Events page does a full refilter every second per active camera.
EventIngestionServicenow publishes progress about once a second and on every peak rise.EventsPageViewModel.OnLiveEventrunsApplyFilterover up to 10,000 events on the UI thread for each update. With several AI cameras active, that is several full rebuilds per second. Mark progress publications so the Events page can skip them, or throttleOnLiveEvent. - An open event can arrive after the close.
OpenAsyncpublishes only afterawait _repo.AddAsync, while the quiet-close timer runs independently. If the insert takes more than 5 s, the coordinator sees the close first and removes the episode. The late open event then creates a new episode that is never removed, and it notifies or acts a second time. Mark the state as closed and skip the late publish, or keep a short set of recently closed IDs in the coordinator. - An unsaved SMTP password is silently lost.
LoadAsyncruns each time Settings is shown and overwritesSmtpUser/SmtpPasswordfrom the store.FlushAsyncdoesn't save dirty credentials. A user who types a password and navigates away without pressing Save/Test gets empty fields back, and every e-mail action then fails authentication. Save the credentials inFlushAsyncwhen they're dirty, or don't overwrite dirty fields on load. - "Test notification" can crash the app.
TestNotificationAsyncawaitsSaveNowAsync()without a try/catch. An IOException (antivirus or OneDrive lockingusersettings.json.tmp, a full disk) escapes theAsyncRelayCommandand takes the process down. The other Test commands catch their errors. - Importing a big WAV freezes the UI.
WavCodec.Readruns synchronously on the UI thread and reads the entiredatachunk before trimming to 15 s. A 1-hour WAV allocates hundreds of MB, and a file over 2 GB overflows the(int)cast into anArgumentOutOfRangeException, which doesn't fall back to FFmpeg. Cap the read atMaxCustomLengthand run it inTask.Run. Also reject absurd sample rates: a rate of 2^30 overflowsBytesPerSecondto 0 and leaves the player loop spinning. - MAPI and Outlook sends are unbounded. Each send gets a new STA thread that can't be cancelled; the 20 s timeout only stops waiting for it. A compose window or an Outlook security prompt keeps that thread alive, and a burst of events stacks up threads and dialogs. Allow one send at a time.
- Phone widths. The Test-button rows put a status
TextBlock MaxWidth="420"inside a horizontalStackPanel, so long errors are clipped at about 360 dp. Fixed-width sliders also crowd the labels. Use aGrid(Auto,*) or put the status on its own line, and useb:Adaptive.CompactBelowlike the other sections.
Nits
- Cooldowns compare local wall-clock time. On the DST fall-back night
now - atgoes negative and blocks both channels for about an hour. Use UTC for cooldowns and local time only for quiet hours. - Actions are gated by the toast toggles (
NotifyOnMotion/NotifyOnDetection) but ignore theNotificationsEnabledmaster switch. Turning off motion toasts silently turns off motion actions too. Either give actions their own filter or document this. WindowsToastNotifier.ShowafterDisposethrowsInvalidOperationException.RunAndWaitcatches this;Showdoesn't.- The "Not an http(s) URL" warning logs the full webhook URL, which often carries a token. Log only the scheme and host.
- The
.ps1branch always runspowershell.exe. Usepwshoff Windows, or take this branch only on Windows. NotificationSoundPlayernever disposes supersededCancellationTokenSources, andScheduleSavehas the same leak.- The program-path hint and the README describe MAPI, Outlook and Credential Manager without saying they're Windows-only. Off Windows, "system secure storage" is actually the app's
secrets.bin. SettingsPageViewModel.Persiststill uses the snapshotUpdateAsync(...), which can overwrite a notification save that is still in flight. Switch it to the functional overload.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends camera-event notifications and adds external actions:
Related
Phase 19 — Community & app-level (19.3 Notifications).
Type
Checklist
TreatWarningsAsErrors=true).dotnet test); new Core logic has unit tests.AppreferencesCoreonly (Infrastructure / Video / Devices wired via DI in a head).Platforms tested
Screenshots / notes
SshSessionIntegrationTestsconstructor call.🤖 Generated with Claude Code