Conversation
📝 WalkthroughWalkthroughThis PR enhances URL validation and security practices across multiple modules while introducing comprehensive tray application infrastructure. Input modules now validate URL schemes and hosts before creating clients, with hardcoded endpoints replaced by proper URL construction. Security lint suppressions are added to HTTP request operations. The tray package receives significant new functionality: configuration generation for adder via YAML, status tracking with callbacks, health polling via HTTP endpoints, event parsing from JSON streams, process auto-restart with exponential backoff, and platform-specific service registration for macOS (LaunchAgent), Linux (systemd), and Windows (scheduled tasks). Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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.
5 issues found across 13 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tray/health.go">
<violation number="1" location="tray/health.go:110">
P2: Handle non-200 HTTP responses as failures; otherwise a server error that still returns JSON can be treated as healthy and reset the failure count.</violation>
</file>
<file name="tray/service_windows.go">
<violation number="1" location="tray/service_windows.go:72">
P2: `serviceStatusCheck` suppresses all query errors and reports `ServiceNotRegistered`, which can hide real failures (e.g., permissions or command errors). Return the error (or at least surface it) so callers can distinguish “not registered” from a failed query.</violation>
</file>
<file name="tray/service_linux.go">
<violation number="1" location="tray/service_linux.go:38">
P2: Quote the binary and config paths in the systemd unit template so paths containing spaces/special characters are passed correctly to systemd.</violation>
</file>
<file name="tray/events.go">
<violation number="1" location="tray/events.go:57">
P2: Stop() doesn’t actually stop the parser while Scan is blocked, so the goroutine can hang indefinitely if the reader stays open. Consider storing an io.ReadCloser and closing it in Stop (or adding a context-aware reader) so Stop reliably unblocks Scan.</violation>
</file>
<file name="tray/service_darwin.go">
<violation number="1" location="tray/service_darwin.go:40">
P2: Escape path values when rendering the plist. Using raw `text/template` inserts `BinaryPath`, `ConfigPath`, and `LogDir` without XML escaping, so paths containing `&` or `<` will break the plist or allow injection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| return | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
P2: Handle non-200 HTTP responses as failures; otherwise a server error that still returns JSON can be treated as healthy and reset the failure count.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/health.go, line 110:
<comment>Handle non-200 HTTP responses as failures; otherwise a server error that still returns JSON can be treated as healthy and reset the failure count.</comment>
<file context>
@@ -0,0 +1,147 @@
+ }
+ return
+ }
+ defer resp.Body.Close()
+
+ var hr HealthResponse
</file context>
| "/NH", | ||
| ).CombinedOutput() | ||
| if err != nil { | ||
| return ServiceNotRegistered, nil |
There was a problem hiding this comment.
P2: serviceStatusCheck suppresses all query errors and reports ServiceNotRegistered, which can hide real failures (e.g., permissions or command errors). Return the error (or at least surface it) so callers can distinguish “not registered” from a failed query.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/service_windows.go, line 72:
<comment>`serviceStatusCheck` suppresses all query errors and reports `ServiceNotRegistered`, which can hide real failures (e.g., permissions or command errors). Return the error (or at least surface it) so callers can distinguish “not registered” from a failed query.</comment>
<file context>
@@ -0,0 +1,80 @@
+ "/NH",
+ ).CombinedOutput()
+ if err != nil {
+ return ServiceNotRegistered, nil
+ }
+
</file context>
|
|
||
| [Service] | ||
| Type=simple | ||
| ExecStart={{.BinaryPath}}{{if .ConfigPath}} --config {{.ConfigPath}}{{end}} |
There was a problem hiding this comment.
P2: Quote the binary and config paths in the systemd unit template so paths containing spaces/special characters are passed correctly to systemd.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/service_linux.go, line 38:
<comment>Quote the binary and config paths in the systemd unit template so paths containing spaces/special characters are passed correctly to systemd.</comment>
<file context>
@@ -0,0 +1,128 @@
+
+[Service]
+Type=simple
+ExecStart={{.BinaryPath}}{{if .ConfigPath}} --config {{.ConfigPath}}{{end}}
+Restart=on-failure
+RestartSec=5
</file context>
|
|
||
| // Stop signals the event parser to stop. Note that the parser will | ||
| // also stop naturally when the underlying reader is closed. | ||
| func (ep *EventParser) Stop() { |
There was a problem hiding this comment.
P2: Stop() doesn’t actually stop the parser while Scan is blocked, so the goroutine can hang indefinitely if the reader stays open. Consider storing an io.ReadCloser and closing it in Stop (or adding a context-aware reader) so Stop reliably unblocks Scan.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/events.go, line 57:
<comment>Stop() doesn’t actually stop the parser while Scan is blocked, so the goroutine can hang indefinitely if the reader stays open. Consider storing an io.ReadCloser and closing it in Stop (or adding a context-aware reader) so Stop reliably unblocks Scan.</comment>
<file context>
@@ -0,0 +1,104 @@
+
+// Stop signals the event parser to stop. Note that the parser will
+// also stop naturally when the underlying reader is closed.
+func (ep *EventParser) Stop() {
+ select {
+ case <-ep.done:
</file context>
| <string>io.blinklabs.adder</string> | ||
| <key>ProgramArguments</key> | ||
| <array> | ||
| <string>{{.BinaryPath}}</string> |
There was a problem hiding this comment.
P2: Escape path values when rendering the plist. Using raw text/template inserts BinaryPath, ConfigPath, and LogDir without XML escaping, so paths containing & or < will break the plist or allow injection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/service_darwin.go, line 40:
<comment>Escape path values when rendering the plist. Using raw `text/template` inserts `BinaryPath`, `ConfigPath`, and `LogDir` without XML escaping, so paths containing `&` or `<` will break the plist or allow injection.</comment>
<file context>
@@ -0,0 +1,137 @@
+ <string>io.blinklabs.adder</string>
+ <key>ProgramArguments</key>
+ <array>
+ <string>{{.BinaryPath}}</string>
+ {{- if .ConfigPath}}
+ <string>--config</string>
</file context>
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
feature/tray-enhanced-menu
branch
from
5dfb069 to
c648701
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
output/webhook/webhook.go (1)
343-347:⚠️ Potential issue | 🟡 MinorPre-existing: response body leak when
io.ReadAllfails.
defer resp.Body.Close()is registered at line 347, after the early return at line 345. Ifio.ReadAllreturns an error, the body is never closed. Move the defer immediately after theresp == nilguard (after line 342) so it fires on every return path.🐛 Proposed fix
if resp == nil { return fmt.Errorf("failed to send payload: %s", data) } + defer resp.Body.Close() respBody, err := io.ReadAll(resp.Body) if err != nil { return fmt.Errorf("%w", err) } - defer resp.Body.Close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@output/webhook/webhook.go` around lines 343 - 347, The response body can leak if io.ReadAll(resp.Body) returns an error because defer resp.Body.Close() is registered after that call; move the defer immediately after the nil-check for resp (i.e., right after the resp == nil guard) so resp.Body.Close() is guaranteed on every return path, keeping the existing io.ReadAll usage and error handling intact; update the function that performs the HTTP call where resp, resp.Body and io.ReadAll are used to register defer resp.Body.Close() before attempting to read the body.
🧹 Nitpick comments (4)
tray/health.go (1)
82-94: First health poll won't occur until 10 seconds afterStart().The ticker fires after the first interval elapses, so the status will remain in its previous state (likely
StatusStarting) for up to 10 seconds before the first check runs. Consider doing an immediatehp.poll()call before entering the ticker loop if faster initial feedback is desired.♻️ Optional: add an immediate first poll
func (hp *HealthPoller) run() { ticker := time.NewTicker(healthPollInterval) defer ticker.Stop() + // Immediate first check + hp.poll() + for { select { case <-hp.stopCh: return case <-ticker.C: hp.poll() } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tray/health.go` around lines 82 - 94, The initial health check is delayed because HealthPoller.run only starts the ticker; call hp.poll() once immediately before entering the for/select loop so the first health poll occurs right away; specifically, add an immediate hp.poll() invocation at the start of HealthPoller.run (before creating or before the for loop) so subsequent ticker-driven polls remain unchanged and hp.stopCh handling via the select stays the same.tray/service_linux.go (1)
75-99:registerServiceenables but does not start the service.After
daemon-reloadandenable, the unit is set to start at login but is not started immediately. The Darwin implementation usesRunAtLoad+launchctl loadwhich starts the agent. Consider addingsystemctl --user startfor parity, or document that the service only starts on next login.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tray/service_linux.go` around lines 75 - 99, The registerService function currently enables the unit but doesn't start it; after the existing systemctl --user enable step, invoke systemctl --user start <serviceName> (using exec.Command and CombinedOutput like the daemon-reload and enable calls) and propagate a formatted error including the trimmed output on failure so the service is started immediately (or alternatively add a comment/doc noting it only starts at next login); locate and update registerService and use the existing serviceName symbol for the start command.tray/app.go (1)
144-145: "About" action only logs — not visible to the user.Clicking "About Adder" calls
slog.Info(...), which writes to the application log but shows nothing in the UI. Users would expect a dialog or notification. This appears to be a placeholder implementation.Would you like me to open an issue to track adding a proper About dialog (e.g., using a notification or a small window)?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tray/app.go` around lines 144 - 145, The "About" menu handler currently only calls slog.Info inside the mAbout.ClickedCh case which logs but shows nothing to users; replace that logging call with a UI-visible action by invoking a function like showAboutDialog or showNotification from the mAbout.ClickedCh branch (create it if missing) that displays a small modal or system notification containing the app name, version and brief description; ensure the new function is called instead of slog.Info in the case block and reuse existing UI/notification utilities in the codebase if available.tray/service_darwin.go (1)
104-108: Replace deprecatedlaunchctl load/unloadwithbootstrap/bootoutcommands.
launchctl loadandunloadare legacy subcommands; macOS recommendsbootstrap,bootout,enable, anddisableinstead. Proper migration requires:
load -w(line 104): Replace withenable+bootstrap gui/$(id -u) <plist-path>to restore the persistence behavior of-wunload(line 112): Replace withbootout gui/$(id -u) <plist-path>+disableSince this is a per-user LaunchAgent (in
~/Library/LaunchAgents/), the domain target isgui/$(id -u).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tray/service_darwin.go` around lines 104 - 108, Replace the legacy "launchctl load -w" invocation in the install/start code (the exec.Command call that uses servicePlistPath()) with two invocations that (1) run "launchctl enable <domain>/<label>" and (2) run "launchctl bootstrap <domain> <plistPath>" using the per-user domain "gui/$(id -u)" — obtain the uid programmatically (os.Getuid()) to form "gui/<uid>" and execute both commands via exec.Command/CombinedOutput and preserve the existing error wrapping/logging; likewise, replace the corresponding "launchctl unload" in the uninstall/stop path with "launchctl bootout <domain> <plistPath>" followed by "launchctl disable <domain>/<label>" (or disable the plist path) using the same servicePlistPath() and uid-derived domain, ensuring you keep CombinedOutput and the same error formatting (fmt.Errorf) on failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@input/chainsync/chainsync.go`:
- Around line 768-769: Replace the incorrect gosec suppression tag above the
HTTP call: change the comment "// `#nosec` G704 -- Kupo endpoint is
user-configured and validated before use." to use the correct rule ID "// `#nosec`
G107 -- Kupo endpoint is user-configured and validated before use." so the
suppression applies to the tainted URL rule around the httpClient.Do(req) call
(the line where resp, err := httpClient.Do(req) is invoked).
In `@output/push/fcm/message.go`:
- Around line 77-81: The URL path is built by interpolating projectId into
url.URL.Path which leaves it unescaped and the current `#nosec G704` is
incorrect; fix by percent-escaping or using safe join: replace the manual
url.URL construction for fcmURL with a call that escapes projectId (e.g., use
url.PathEscape(projectId) when building the Path or better use
url.JoinPath("https://fcm.googleapis.com", "v1/projects", projectId,
"messages:send") and handle the returned error), and update/remove the `#nosec`
comment so it accurately reflects the mitigation applied; locate and change the
code that constructs fcmURL and the `projectId` usage in message.go.
In `@output/webhook/webhook.go`:
- Around line 335-336: Move the response body close so it cannot leak: after
calling client.Do(req) and before calling io.ReadAll, immediately check err and
then defer resp.Body.Close() (so resp.Body is always closed even if ReadAll
fails); also add URL scheme validation for w.url (the value used to build the
request) similar to chainsync/mempool modules by parsing w.url and rejecting any
non-http/https scheme prior to making client.Do(req) to ensure consistent
defense-in-depth.
In `@tray/app.go`:
- Around line 55-61: The process manager is constructed with
WithAutoRestart(true) via NewProcessManager but Stop() in tray/process.go sets
pm.autoRestart = false permanently, and Start() never restores it; change
Start() (the Start method on the ProcessManager) to reset pm.autoRestart to the
configured default when starting (e.g., reapply the configured autoRestart flag
stored on the manager or re-run the constructor-provided option value) so that a
Stop→Start only suppresses auto-restart for that stop cycle rather than
disabling it for the session; reference NewProcessManager, WithAutoRestart,
Stop(), Start(), and pm.autoRestart to locate the relevant code.
In `@tray/events.go`:
- Around line 51-53: Start() currently launches goroutine unguarded which lets
multiple calls race on the same bufio.Scanner and cause close(ep.events) to run
twice; add a guard (preferably a sync.Once field like startOnce on EventParser)
and change Start() to call startOnce.Do(func(){ go ep.run() }) so run() is only
started once (this avoids double-closing ep.events and races on the scanner);
ensure the new startOnce field is initialized with zero value and reference
EventParser.Start(), EventParser.run(), and ep.events in the change.
In `@tray/health.go`:
- Around line 96-110: In HealthPoller.poll(), when hp.client.Get returns a
non-nil resp and a non-nil err the code currently returns without closing
resp.Body; update the error path to close the response body before returning
(e.g., if resp != nil call resp.Body.Close() or defer it immediately after
verifying resp != nil) so that any open response is always closed when leaving
poll; reference the hp.client.Get call and the resp variable in your change.
In `@tray/process_test.go`:
- Around line 162-174: TestEventParser_EOF starts an EventParser with ep.Start()
but never calls ep.Stop(), risking a goroutine leak; update the test
(TestEventParser_EOF) to call ep.Stop() (preferably via defer ep.Stop()
immediately after ep.Start()) so the EventParser goroutine is always terminated,
mirroring other tests that call ep.Stop() and ensuring clean shutdown on EOF.
In `@tray/process.go`:
- Around line 252-279: The restart goroutine can race with Stop(): after
sleeping it should re-check pm.autoRestart under pm.mu before calling pm.Start
to respect a user Stop; modify the anonymous goroutine started after
backoffDelay(restartCount) to acquire pm.mu (or use a helper) after the sleep,
verify pm.autoRestart is still true (and optionally that pm.process == nil if
needed), only then call pm.Start(), and ensure any shared counters
(pm.restartCount) are consistent while holding pm.mu; reference pm.mu,
pm.autoRestart, pm.restartCount, backoffDelay and pm.Start when applying the
change.
In `@tray/service_darwin.go`:
- Around line 32-56: The plistTemplate uses text/template which does not escape
XML special chars; update the code that renders plistTemplate to use
html/template (or explicitly XML-escape values) so BinaryPath, ConfigPath and
LogDir are safely encoded for XML; locate the template variable/string named
plistTemplate and the rendering call that executes it (where BinaryPath,
ConfigPath, LogDir are supplied) and switch to html/template.New(...).Parse(...)
or run values through an XML-escaping helper before executing the template so
the generated plist is well-formed.
- Around line 127-137: serviceStatusCheck currently treats any successful exec
of "launchctl list <label>" as ServiceRunning; change it to run the command with
Output(), parse the returned stdout for the PID column, and only return
ServiceRunning when that PID field is a numeric PID; if the PID field is "-" (or
non-numeric) return ServiceRegistered. Specifically, in serviceStatusCheck use
exec.Command("launchctl","list", launchAgentLabel).Output(), trim/split the
output (the single-line listing has PID as the first field), check strconv.Atoi
(or regex) on the PID token to determine running vs registered, and fall back to
returning ServiceRegistered on parse errors. Keep the initial
os.Stat(servicePlistPath()) check unchanged.
In `@tray/service_linux.go`:
- Line 38: The ExecStart template must quote paths and escape systemd '%'
specifiers to avoid splitting on spaces or misinterpreting specifiers: change
the template to include quoted values (e.g.,
ExecStart="{{.BinaryPathEscaped}}"{{if .ConfigPath}} --config
"{{.ConfigPathEscaped}}"{{end}}) and add escaped versions of BinaryPath and
ConfigPath (e.g., BinaryPathEscaped, ConfigPathEscaped) that replace "%" with
"%%" (use strings.ReplaceAll) before rendering; update the code that builds the
template data so ExecStart uses these quoted/escaped fields.
In `@tray/service_windows.go`:
- Around line 28-31: The built command string uses cfg.BinaryPath and
cfg.ConfigPath directly (in the local variable command) which allows path
injection if either contains a double quote or other shell metacharacters; fix
by adding strict validation in ServiceConfig.Validate() to reject any paths
containing double quotes and other dangerous characters (e.g., '"', '&', '|',
'<', '>', ';') and return an error, and then rely on cfg.BinaryPath /
cfg.ConfigPath being safe before building the command in service_windows.go
(alternatively you can escape embedded quotes when constructing command, but
implement validation in ServiceConfig.Validate() and reference BinaryPath and
ConfigPath there).
- Around line 75-77: The code currently detects running tasks by matching the
localized substring "Running" (see the strings.Contains(..., "Running") check
that returns ServiceRunning), which fails on non-English Windows; change the
implementation that invokes schtasks to call `schtasks /query /tn <taskname>
/xml` (or use PowerShell `Get-ScheduledTask -TaskName <name> | Select-Object
-ExpandProperty State`) and parse the returned XML/enum instead of grepping
localized text: run schtasks with `/xml`, decode the XML using Go's
encoding/xml, extract the task state element (or use the PowerShell State value)
and map that canonical state to ServiceRunning/other statuses, replacing the
current strings.Contains-based check.
In `@tray/service.go`:
- Around line 56-62: The Validate() method on ServiceConfig currently only
checks BinaryPath emptiness; update ServiceConfig.Validate() to perform basic
path sanitization for both BinaryPath and ConfigPath by rejecting empty
BinaryPath and returning an error if either path contains any unsafe characters
(at minimum: '"', '<', '>', '%' and backticks or other shell/meta characters you
consider risky), and include clear error messages naming the offending field
(e.g., "BinaryPath contains invalid character: %q" or "ConfigPath contains
invalid character: %q"); keep the check centralized in ServiceConfig.Validate()
so platform implementations (macOS plist, Windows /TR, systemd) get this
defense-in-depth automatically.
---
Outside diff comments:
In `@output/webhook/webhook.go`:
- Around line 343-347: The response body can leak if io.ReadAll(resp.Body)
returns an error because defer resp.Body.Close() is registered after that call;
move the defer immediately after the nil-check for resp (i.e., right after the
resp == nil guard) so resp.Body.Close() is guaranteed on every return path,
keeping the existing io.ReadAll usage and error handling intact; update the
function that performs the HTTP call where resp, resp.Body and io.ReadAll are
used to register defer resp.Body.Close() before attempting to read the body.
---
Nitpick comments:
In `@tray/app.go`:
- Around line 144-145: The "About" menu handler currently only calls slog.Info
inside the mAbout.ClickedCh case which logs but shows nothing to users; replace
that logging call with a UI-visible action by invoking a function like
showAboutDialog or showNotification from the mAbout.ClickedCh branch (create it
if missing) that displays a small modal or system notification containing the
app name, version and brief description; ensure the new function is called
instead of slog.Info in the case block and reuse existing UI/notification
utilities in the codebase if available.
In `@tray/health.go`:
- Around line 82-94: The initial health check is delayed because
HealthPoller.run only starts the ticker; call hp.poll() once immediately before
entering the for/select loop so the first health poll occurs right away;
specifically, add an immediate hp.poll() invocation at the start of
HealthPoller.run (before creating or before the for loop) so subsequent
ticker-driven polls remain unchanged and hp.stopCh handling via the select stays
the same.
In `@tray/service_darwin.go`:
- Around line 104-108: Replace the legacy "launchctl load -w" invocation in the
install/start code (the exec.Command call that uses servicePlistPath()) with two
invocations that (1) run "launchctl enable <domain>/<label>" and (2) run
"launchctl bootstrap <domain> <plistPath>" using the per-user domain "gui/$(id
-u)" — obtain the uid programmatically (os.Getuid()) to form "gui/<uid>" and
execute both commands via exec.Command/CombinedOutput and preserve the existing
error wrapping/logging; likewise, replace the corresponding "launchctl unload"
in the uninstall/stop path with "launchctl bootout <domain> <plistPath>"
followed by "launchctl disable <domain>/<label>" (or disable the plist path)
using the same servicePlistPath() and uid-derived domain, ensuring you keep
CombinedOutput and the same error formatting (fmt.Errorf) on failures.
In `@tray/service_linux.go`:
- Around line 75-99: The registerService function currently enables the unit but
doesn't start it; after the existing systemctl --user enable step, invoke
systemctl --user start <serviceName> (using exec.Command and CombinedOutput like
the daemon-reload and enable calls) and propagate a formatted error including
the trimmed output on failure so the service is started immediately (or
alternatively add a comment/doc noting it only starts at next login); locate and
update registerService and use the existing serviceName symbol for the start
command.
| // #nosec G704 -- Kupo endpoint is user-configured and validated before use. | ||
| resp, err := httpClient.Do(req) |
There was a problem hiding this comment.
Same incorrect gosec rule ID G704 — should be G107.
Same issue as noted in input/mempool/mempool.go. The rule for HTTP request URL taint input is G107.
🔧 Proposed fix
- // `#nosec` G704 -- Kupo endpoint is user-configured and validated before use.
+ // `#nosec` G107 -- Kupo endpoint is user-configured and validated before use.📝 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.
| // #nosec G704 -- Kupo endpoint is user-configured and validated before use. | |
| resp, err := httpClient.Do(req) | |
| // `#nosec` G107 -- Kupo endpoint is user-configured and validated before use. | |
| resp, err := httpClient.Do(req) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@input/chainsync/chainsync.go` around lines 768 - 769, Replace the incorrect
gosec suppression tag above the HTTP call: change the comment "// `#nosec` G704 --
Kupo endpoint is user-configured and validated before use." to use the correct
rule ID "// `#nosec` G107 -- Kupo endpoint is user-configured and validated before
use." so the suppression applies to the tainted URL rule around the
httpClient.Do(req) call (the line where resp, err := httpClient.Do(req) is
invoked).
| fcmURL := &url.URL{ | ||
| Scheme: "https", | ||
| Host: "fcm.googleapis.com", | ||
| Path: fmt.Sprintf("/v1/projects/%s/messages:send", projectId), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -g 'go.mod' --exec cat {}Repository: blinklabs-io/adder
Length of output: 4886
🏁 Script executed:
cat -n output/push/fcm/message.go | sed -n '70,115p'Repository: blinklabs-io/adder
Length of output: 1411
projectId interpolated into URL path without escaping — #nosec justification is inaccurate.
url.URL.Path stores the unescaped path; String() does not percent-encode /, so a projectId containing slashes or .. segments is treated as literal path components. For example, projectId = "foo/../../v2" would result in /v1/projects/foo/../../v2/messages:send on the wire.
The #nosec G704 comment claims "validated path," but no validation or escaping is applied — this misrepresents the code's security posture.
Fix with url.PathEscape:
-Path: fmt.Sprintf("/v1/projects/%s/messages:send", projectId),
+Path: fmt.Sprintf("/v1/projects/%s/messages:send", url.PathEscape(projectId)),Or use url.JoinPath (Go 1.19+, which your project supports):
fcmURL, err := url.JoinPath("https://fcm.googleapis.com", "v1/projects", projectId, "messages:send")
if err != nil {
return err
}Update the #nosec comment to reflect the actual mitigation:
-// `#nosec` G704 -- Request targets the fixed FCM host with a validated path.
+// `#nosec` G704 -- Request targets a fixed external host; projectId is path-escaped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@output/push/fcm/message.go` around lines 77 - 81, The URL path is built by
interpolating projectId into url.URL.Path which leaves it unescaped and the
current `#nosec G704` is incorrect; fix by percent-escaping or using safe join:
replace the manual url.URL construction for fcmURL with a call that escapes
projectId (e.g., use url.PathEscape(projectId) when building the Path or better
use url.JoinPath("https://fcm.googleapis.com", "v1/projects", projectId,
"messages:send") and handle the returned error), and update/remove the `#nosec`
comment so it accurately reflects the mitigation applied; locate and change the
code that constructs fcmURL and the `projectId` usage in message.go.
| // #nosec G704 -- Webhook URL is user-configured and intentionally allowed. | ||
| resp, err := client.Do(req) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the webhook.go file to see the context around lines 320-350
fd -n "webhook.go" | head -20Repository: blinklabs-io/adder
Length of output: 292
🏁 Script executed:
# Get the actual file paths to locate webhook.go
git ls-files | grep -i webhookRepository: blinklabs-io/adder
Length of output: 138
🏁 Script executed:
# Once we have the path, examine the code context
cat -n output/webhook/webhook.go | sed -n '320,350p'Repository: blinklabs-io/adder
Length of output: 1378
🏁 Script executed:
# Also check line 323 to verify the existing nosec pattern
cat -n output/webhook/webhook.go | sed -n '320,330p'Repository: blinklabs-io/adder
Length of output: 654
🌐 Web query:
G704 gosec rule SSRF taint analysis
💡 Result:
gosec rule G704 is the SSRF (Server-Side Request Forgery) detector implemented as a taint-analysis rule (G7xx = taint rules). [3]
What it flags (conceptually)
G704 reports an issue when tainted (externally influenced) data can reach a network/HTTP “sink” that initiates an outbound request / connection. [1][3]
Taint sources used by G704
G704’s SSRF taint config treats data as tainted when it originates from, or flows from: [1]
*net/http.Request(as a parameter type source)os.Argsandos.Getenv(function sources)*bufio.Reader,*bufio.Scanner(I/O sources)
Sinks checked by G704 (where tainted data is dangerous)
The rule checks specific arguments (“CheckArgs”) for taint at these sinks, including: [1]
net/http:http.Get,Post,Head,PostForm(URL argument)net/http:NewRequest,NewRequestWithContext(URL argument position)(*http.Client):Do,Get,Post,Headnet:Dial,DialTimeout(address argument),LookupHost(host argument)net/http/httputil:NewSingleHostReverseProxy(target URL argument)
Sanitizers
The SSRF rule config effectively ships with no standard-library sanitizers (it explicitly notes that parsing like url.Parse does not make something SSRF-safe because it doesn’t enforce an allowlist). [1]
Examples (from gosec tests)
- Flagged: user-controlled query param used as
http.Get(url)[2] - Flagged:
os.Getenv("TARGET_URL")used ashttp.Post(target, ...)[2] - Not flagged: hardcoded URL in
http.Get("https://api.example.com/data")[2]
🏁 Script executed:
# Search for URL validation patterns in chainsync and mempool modules mentioned in the review
fd -n "\.go$" | xargs rg -l "url\.Parse|scheme.*http|urlutil" | head -20Repository: blinklabs-io/adder
Length of output: 402
🏁 Script executed:
# Check for URL scheme validation in the codebase
rg "Scheme.*http|scheme.*http" -A 2 -B 2 | head -50Repository: blinklabs-io/adder
Length of output: 1100
🏁 Script executed:
# Find where w.url is set in webhook.go
cat -n output/webhook/webhook.go | grep -E "w\.url|\.url\s*=" | head -20Repository: blinklabs-io/adder
Length of output: 258
🏁 Script executed:
# Look at the WebhookOutput struct definition to see w.url
rg "type WebhookOutput" -A 10 output/webhook/webhook.goRepository: blinklabs-io/adder
Length of output: 353
🏁 Script executed:
# Check mempool.go for URL validation
cat -n input/mempool/mempool.go | sed -n '1,100p' | grep -E "Scheme|http|https"Repository: blinklabs-io/adder
Length of output: 137
🏁 Script executed:
# Get a broader view of mempool URL handling
rg "http\.|url\.|Parse" -B 2 -A 2 input/mempool/mempool.go | head -40Repository: blinklabs-io/adder
Length of output: 1056
🏁 Script executed:
# Check the SendWebhook function signature and full context
cat -n output/webhook/webhook.go | sed -n '300,360p'Repository: blinklabs-io/adder
Length of output: 2203
🏁 Script executed:
# Find where WebhookOutput is instantiated or w.url is set
rg "WebhookOutput\{|\.url\s*=" output/webhook/ -B 2 -A 2Repository: blinklabs-io/adder
Length of output: 613
🏁 Script executed:
# Check the options or initialization code
cat -n output/webhook/options.go | head -80Repository: blinklabs-io/adder
Length of output: 2379
🏁 Script executed:
# Look at where webhook URL comes from in configuration
rg "url" output/webhook/options.go -B 2 -A 2Repository: blinklabs-io/adder
Length of output: 239
Address the response body leak and consider adding URL validation for consistency with other modules.
The G704 suppression is justified for a config-only URL, but two issues remain:
-
Response body leak (lines 343–347): The
defer resp.Body.Close()is placed afterio.ReadAll(). Ifio.ReadAll()fails, the early return at line 345 exits without ever closing the body, causing a resource leak. Move the defer before the ReadAll call. -
Missing URL validation: Other modules in this PR (chainsync, mempool) added explicit URL scheme validation before making HTTP calls (e.g., rejecting non-
http/httpsschemes). The webhook output applies no such check tow.url. While the config-only justification for the nosec is sound, adding validation would improve consistency and defense-in-depth, especially since mempool's nosec comment explicitly notes "validated before use."
Example fix
respBody, err := io.ReadAll(resp.Body)
+ defer resp.Body.Close()
if err != nil {
return fmt.Errorf("%w", err)
}
- defer resp.Body.Close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@output/webhook/webhook.go` around lines 335 - 336, Move the response body
close so it cannot leak: after calling client.Do(req) and before calling
io.ReadAll, immediately check err and then defer resp.Body.Close() (so resp.Body
is always closed even if ReadAll fails); also add URL scheme validation for
w.url (the value used to build the request) similar to chainsync/mempool modules
by parsing w.url and rejecting any non-http/https scheme prior to making
client.Do(req) to ensure consistent defense-in-depth.
| process: NewProcessManager( | ||
| WithBinary(cfg.AdderBinary), | ||
| WithConfigFile(cfg.AdderConfig), | ||
| WithStatusTracker(status), | ||
| WithAPIEndpoint(defaultAPIAddress, defaultAPIPort), | ||
| WithAutoRestart(true), | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the tray/process.go file
fd process.go tray/Repository: blinklabs-io/adder
Length of output: 77
🏁 Script executed:
# Check the file size before reading
wc -l tray/process.goRepository: blinklabs-io/adder
Length of output: 81
🏁 Script executed:
# Now let's examine the Stop() method and surrounding context
sed -n '300,320p' tray/process.goRepository: blinklabs-io/adder
Length of output: 533
🏁 Script executed:
# Let's also check the Start() method implementation
ast-grep --pattern $'func (pm *ProcessManager) Start() error {
$$$
}'Repository: blinklabs-io/adder
Length of output: 3354
🏁 Script executed:
# Let's search for all mentions of autoRestart to understand its usage
rg 'autoRestart' tray/process.go -n -B 2 -A 2Repository: blinklabs-io/adder
Length of output: 614
Auto-restart is permanently disabled after an explicit Stop.
WithAutoRestart(true) is set at construction (line 60), but Stop() in tray/process.go (line 306) permanently sets pm.autoRestart = false. After the user clicks Stop → Start, auto-restart remains disabled for the rest of the session. Start() (lines 133-212) never re-enables it.
If the intent is that auto-restart should only be suppressed for the current stop cycle, Start() should restore autoRestart to its configured value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tray/app.go` around lines 55 - 61, The process manager is constructed with
WithAutoRestart(true) via NewProcessManager but Stop() in tray/process.go sets
pm.autoRestart = false permanently, and Start() never restores it; change
Start() (the Start method on the ProcessManager) to reset pm.autoRestart to the
configured default when starting (e.g., reapply the configured autoRestart flag
stored on the manager or re-run the constructor-provided option value) so that a
Stop→Start only suppresses auto-restart for that stop cycle rather than
disabling it for the session; reference NewProcessManager, WithAutoRestart,
Stop(), Start(), and pm.autoRestart to locate the relevant code.
| func (ep *EventParser) Start() { | ||
| go ep.run() | ||
| } |
There was a problem hiding this comment.
No guard against double Start() — calling it twice will panic.
If Start() is called twice, two goroutines will race on the same bufio.Scanner, and close(ep.events) in run() will execute twice, causing a panic. Consider adding a sync.Once or a started flag.
🛡️ Proposed fix using sync.Once
type EventParser struct {
scanner *bufio.Scanner
events chan event.Event
done chan struct{}
+ once sync.Once
} func (ep *EventParser) Start() {
- go ep.run()
+ ep.once.Do(func() {
+ go ep.run()
+ })
}📝 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.
| func (ep *EventParser) Start() { | |
| go ep.run() | |
| } | |
| func (ep *EventParser) Start() { | |
| ep.once.Do(func() { | |
| go ep.run() | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tray/events.go` around lines 51 - 53, Start() currently launches goroutine
unguarded which lets multiple calls race on the same bufio.Scanner and cause
close(ep.events) to run twice; add a guard (preferably a sync.Once field like
startOnce on EventParser) and change Start() to call startOnce.Do(func(){ go
ep.run() }) so run() is only started once (this avoids double-closing ep.events
and races on the scanner); ensure the new startOnce field is initialized with
zero value and reference EventParser.Start(), EventParser.run(), and ep.events
in the change.
| func serviceStatusCheck() (ServiceStatus, error) { | ||
| if _, err := os.Stat(servicePlistPath()); os.IsNotExist(err) { | ||
| return ServiceNotRegistered, nil | ||
| } | ||
|
|
||
| if err := exec.Command("launchctl", "list", launchAgentLabel).Run(); err == nil { | ||
| return ServiceRunning, nil | ||
| } | ||
|
|
||
| return ServiceRegistered, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the actual file and surrounding context
find . -name "service_darwin.go" -o -name "service_linux.go" | head -20Repository: blinklabs-io/adder
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Get the full context of the darwin service implementation
cat -n tray/service_darwin.go | head -150Repository: blinklabs-io/adder
Length of output: 4779
🏁 Script executed:
#!/bin/bash
# Look for the Linux implementation to compare
cat -n tray/service_linux.go 2>/dev/null | head -150Repository: blinklabs-io/adder
Length of output: 4696
🏁 Script executed:
#!/bin/bash
# Search for any other launchctl usages in the codebase
rg -n 'launchctl' --type=goRepository: blinklabs-io/adder
Length of output: 319
🌐 Web query:
launchctl list output format PID status crashed job
💡 Result:
launchctl list prints three columns:
PID Status Label
- PID: the process ID if the job is currently running; otherwise
-. [1] - Status: the job’s last exit status.
0usually means it last exited successfully (or may not have run yet). [1][5]- If negative, it means the job was terminated by a signal; the value is the negative of the signal number (e.g.,
-15= SIGTERM). [1][3]
- If negative, it means the job was terminated by a signal; the value is the negative of the signal number (e.g.,
- Label: the job identifier (what you use with other
launchctlcommands). [1]
So “crashed” is typically inferred from Status ≠ 0 (or a negative signal-based status), not shown as a separate column. [1][3]
Sources: [1] [3] [5]
🌐 Web query:
macOS launchctl list exit code running vs registered job
💡 Result:
launchctl list prints three columns:
- PID: the job’s process ID if it’s currently running; otherwise it shows
-(loaded/registered but not running). [1][4][5] - Status: the job’s most recent exit status (aka “last exit code”). If it’s negative, it usually means the job was terminated by a signal (e.g.,
-15= SIGTERM). [1] - Label: the job’s label from its plist. [5]
So, interpreting “running vs registered/loaded”:
- Running job:
PIDis a number. [1][2] - Loaded/registered but not running:
PIDis-. [4][5] - Status = 0 with PID = -: often means either it exited successfully last time or it hasn’t run yet (so “0” doesn’t prove it ran). [2]
If you need a clearer state than list provides, use the modern detailed view:
launchctl print <domain>/<label>(e.g.,launchctl print gui/501/com.example.job), which can show fields likestate = running,pid = …, andlast exit code = …. [6]
Sources: [1] [2] [4] [5] [6]
Parse launchctl list output to distinguish running from registered services.
launchctl list <label> returns exit code 0 for any known job, whether it's running or stopped. To distinguish ServiceRunning from ServiceRegistered, parse the output's PID column: a numeric PID indicates the service is running, while - indicates it's registered but not running. A crashed or stopped service incorrectly returns ServiceRunning with the current code.
Align with the Linux implementation pattern, which parses systemctl is-active output for precise status reporting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tray/service_darwin.go` around lines 127 - 137, serviceStatusCheck currently
treats any successful exec of "launchctl list <label>" as ServiceRunning; change
it to run the command with Output(), parse the returned stdout for the PID
column, and only return ServiceRunning when that PID field is a numeric PID; if
the PID field is "-" (or non-numeric) return ServiceRegistered. Specifically, in
serviceStatusCheck use exec.Command("launchctl","list",
launchAgentLabel).Output(), trim/split the output (the single-line listing has
PID as the first field), check strconv.Atoi (or regex) on the PID token to
determine running vs registered, and fall back to returning ServiceRegistered on
parse errors. Keep the initial os.Stat(servicePlistPath()) check unchanged.
|
|
||
| [Service] | ||
| Type=simple | ||
| ExecStart={{.BinaryPath}}{{if .ConfigPath}} --config {{.ConfigPath}}{{end}} |
There was a problem hiding this comment.
ExecStart will break if BinaryPath or ConfigPath contain spaces or systemd specifiers.
Systemd interprets % as a specifier prefix (e.g., %h = home dir) and splits on unquoted whitespace in ExecStart. If either path contains spaces or %, the unit will fail to start or behave unexpectedly. Paths should be quoted in the template, and % should be escaped as %%.
Proposed fix for space handling
-ExecStart={{.BinaryPath}}{{if .ConfigPath}} --config {{.ConfigPath}}{{end}}
+ExecStart="{{.BinaryPath}}"{{if .ConfigPath}} --config "{{.ConfigPath}}"{{end}}Note: This doesn't address % specifier escaping. If paths may contain %, you'll need a template function to replace % with %%.
📝 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.
| ExecStart={{.BinaryPath}}{{if .ConfigPath}} --config {{.ConfigPath}}{{end}} | |
| ExecStart="{{.BinaryPath}}"{{if .ConfigPath}} --config "{{.ConfigPath}}"{{end}} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tray/service_linux.go` at line 38, The ExecStart template must quote paths
and escape systemd '%' specifiers to avoid splitting on spaces or
misinterpreting specifiers: change the template to include quoted values (e.g.,
ExecStart="{{.BinaryPathEscaped}}"{{if .ConfigPath}} --config
"{{.ConfigPathEscaped}}"{{end}}) and add escaped versions of BinaryPath and
ConfigPath (e.g., BinaryPathEscaped, ConfigPathEscaped) that replace "%" with
"%%" (use strings.ReplaceAll) before rendering; update the code that builds the
template data so ExecStart uses these quoted/escaped fields.
| command := fmt.Sprintf(`"%s"`, cfg.BinaryPath) | ||
| if cfg.ConfigPath != "" { | ||
| command = fmt.Sprintf(`"%s" --config "%s"`, cfg.BinaryPath, cfg.ConfigPath) | ||
| } |
There was a problem hiding this comment.
Command string in /TR is vulnerable to path injection.
BinaryPath and ConfigPath are embedded into a single command string using fmt.Sprintf with only quote wrapping. If either path contains a double quote ("), the command string breaks and could allow arbitrary command execution. The //nolint:gosec comment claims "validated config," but ServiceConfig.Validate() in tray/service.go (line 57–62) only checks that BinaryPath is non-empty — it does not sanitize path characters.
Consider either validating that paths don't contain special characters in Validate(), or escaping embedded quotes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tray/service_windows.go` around lines 28 - 31, The built command string uses
cfg.BinaryPath and cfg.ConfigPath directly (in the local variable command) which
allows path injection if either contains a double quote or other shell
metacharacters; fix by adding strict validation in ServiceConfig.Validate() to
reject any paths containing double quotes and other dangerous characters (e.g.,
'"', '&', '|', '<', '>', ';') and return an error, and then rely on
cfg.BinaryPath / cfg.ConfigPath being safe before building the command in
service_windows.go (alternatively you can escape embedded quotes when
constructing command, but implement validation in ServiceConfig.Validate() and
reference BinaryPath and ConfigPath there).
| if strings.Contains(string(out), "Running") { | ||
| return ServiceRunning, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
schtasks.exe output locale dependent Windows non-English
💡 Result:
schtasks.exe output is locale-dependent on non‑English Windows (column headers, some field values, and even some prompts), so parsing its human-readable output is brittle.
Practical ways to make it locale-robust
-
Prefer XML output (best for locale-independence)
schtasks /query /xmlemits task definitions as XML, which you can parse reliably regardless of UI language. (learn.microsoft.com)
-
If you must parse text, avoid localized headers
- Use CSV without headers and provide your own fixed headers in your script:
schtasks /query /fo csv /nh
- This avoids localized column names (which break
ConvertFrom-Csvif you rely on the first line). (learn.microsoft.com) - Caveat: some values inside the CSV can still be localized (for example, status/state strings), so avoid string-matching on those when possible.
- Use CSV without headers and provide your own fixed headers in your script:
-
Use APIs / PowerShell cmdlets instead of schtasks (often the most robust)
- For scripting, Task Scheduler APIs (or PowerShell scheduled-task cmdlets where available) return structured properties that don’t depend on localized display text (unlike
schtasks).
- For scripting, Task Scheduler APIs (or PowerShell scheduled-task cmdlets where available) return structured properties that don’t depend on localized display text (unlike
Related “locale gotcha”
- Historically,
schtasksconfirmation prompts could display localized “Yes/No” keys but only acceptY/Non some systems (old Windows XP issue). (betaarchive.com)
Citations:
- 1: https://learn.microsoft.com/windows-server/administration/windows-commands/schtasks-query?utm_source=openai
- 2: https://learn.microsoft.com/windows-server/administration/windows-commands/schtasks-query?utm_source=openai
- 3: https://www.betaarchive.com/wiki/index.php/Microsoft_KB_Archive/321121?utm_source=openai
🌐 Web query:
schtasks.exe Running status localization German Japanese
💡 Result:
schtasks.exe /query output is localized (both the column headers and the status values) according to the language/UI culture of the machine/session you run it in (examples show “Statut : Prêt” in French, and Japanese headers when parsing /FO CSV). [1], [2]
“Running” status string
If you’re parsing the Status column/value, “Running” appears as:
- German (de-DE):
Wird ausgeführt[3] - Japanese (ja-JP):
実行中[4]
Practical implication (recommended)
Because these strings vary by locale, avoid string-matching schtasks /query text. Prefer one of:
schtasks /query /xmland parse the XML instead of localized table/list text. [5]- Or use PowerShell scheduled-task APIs (e.g.,
Get-ScheduledTask) which expose a non-localizedStatevalue (enum) rather than localized strings (not fromschtasksoutput).
Sources:
[1] Stack Overflow discussion showing localized schtasks fields (e.g., “Statut : Prêt”) https://stackoverflow.com/questions/13530473/schtasks-output-is-in-different-encoding-whether-it-runs-locally-or-in-remote-se
[2] Blog example parsing schtasks.exe /query /V /FO CSV with Japanese column names https://www.jluvl.com/2018/02/windows.html
[3] IBM Docs (German UI) showing status changes to “Wird ausgeführt” while running https://www.ibm.com/docs/de/SSEKCU_1.1.2.1/com.ibm.psc.doc/admin/as_t_admin_view_task_status_gui.html
[4] Microsoft Learn (Japanese) using 状態 = “実行中” https://learn.microsoft.com/ja-jp/entra/identity/hybrid/connect/how-to-connect-sync-feature-scheduler
[5] Microsoft Learn: schtasks query supports /xml output https://learn.microsoft.com/windows-server/administration/windows-commands/schtasks-query
🌐 Web query:
schtasks.exe /V parameter verbose output locale
💡 Result:
schtasks.exe /Query /V(/vis case-insensitive) enables verbose task output (adds advanced properties). Microsoft notes/vis only valid with/FO LISTor/FO CSV. [1]schtasksdoes not provide a switch to force the output language/locale. The human-readable field names (and often date/time formatting) come out localized based on the environment whereschtasks.exeruns, which makes/Voutput inherently locale-dependent for parsing. [2]- If you need locale-stable / machine-parseable output, prefer:
schtasks /Query /TN "<task>" /XML(XML task definition output). [1]- Or avoid
schtaskstext output entirely and query via the Task Scheduler API (e.g., theSchedule.ServiceCOM object) to get language-independent properties. [2]
Sources
[1] Microsoft Learn — schtasks query (/v, /fo, /xml) https://learn.microsoft.com/windows-server/administration/windows-commands/schtasks-query
[2] Stack Overflow — discussion showing locale/encoding-dependent schtasks output and recommending Schedule.Service COM API https://stackoverflow.com/questions/13530473/schtasks-output-is-in-different-encoding-whether-it-runs-locally-or-in-remote-se
"Running" status string is locale-dependent on non-English Windows and must not be matched as a string.
schtasks.exe localizes the status output based on the OS locale. In German Windows it returns "Wird ausgeführt" and in Japanese "実行中". The /V flag does not solve this—it remains localized. Instead, use schtasks /query /xml to get locale-independent XML output, or switch to PowerShell's Get-ScheduledTask cmdlet (returns non-localized State enum), or query the Task Scheduler API via COM (Schedule.Service COM object).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tray/service_windows.go` around lines 75 - 77, The code currently detects
running tasks by matching the localized substring "Running" (see the
strings.Contains(..., "Running") check that returns ServiceRunning), which fails
on non-English Windows; change the implementation that invokes schtasks to call
`schtasks /query /tn <taskname> /xml` (or use PowerShell `Get-ScheduledTask
-TaskName <name> | Select-Object -ExpandProperty State`) and parse the returned
XML/enum instead of grepping localized text: run schtasks with `/xml`, decode
the XML using Go's encoding/xml, extract the task state element (or use the
PowerShell State value) and map that canonical state to ServiceRunning/other
statuses, replacing the current strings.Contains-based check.
| // Validate checks that the ServiceConfig contains the minimum required fields. | ||
| func (c ServiceConfig) Validate() error { | ||
| if c.BinaryPath == "" { | ||
| return errors.New("binary path must not be empty") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate() should sanitize paths to prevent platform-specific injection issues.
As flagged in the platform implementations, unsanitized BinaryPath and ConfigPath values can cause XML injection (macOS plist), command injection (Windows /TR), or systemd specifier issues (Linux %). Adding basic path validation here (e.g., rejecting characters like ", <, >, %) would provide defense-in-depth at the shared layer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tray/service.go` around lines 56 - 62, The Validate() method on ServiceConfig
currently only checks BinaryPath emptiness; update ServiceConfig.Validate() to
perform basic path sanitization for both BinaryPath and ConfigPath by rejecting
empty BinaryPath and returning an error if either path contains any unsafe
characters (at minimum: '"', '<', '>', '%' and backticks or other shell/meta
characters you consider risky), and include clear error messages naming the
offending field (e.g., "BinaryPath contains invalid character: %q" or
"ConfigPath contains invalid character: %q"); keep the check centralized in
ServiceConfig.Validate() so platform implementations (macOS plist, Windows /TR,
systemd) get this defense-in-depth automatically.
There was a problem hiding this comment.
12 issues found across 17 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="output/push/fcm/message.go">
<violation number="1" location="output/push/fcm/message.go:80">
P2: `projectId` is interpolated into the URL path without escaping. A `projectId` containing `/` or `..` segments (e.g., `"foo/../../v2"`) would result in path traversal. Use `url.PathEscape(projectId)` to prevent this.</violation>
</file>
<file name="tray/events.go">
<violation number="1" location="tray/events.go:51">
P2: No guard against double `Start()` — calling it twice will launch two goroutines racing on the same `bufio.Scanner`, and `close(ep.events)` in `run()` will execute twice, causing a panic. Add a `sync.Once` or a started flag to prevent this.</violation>
<violation number="2" location="tray/events.go:57">
P2: Stop doesn’t actually stop the parser if the reader is blocked; the goroutine can hang indefinitely because `scanner.Scan()` isn’t interruptible. Consider closing the underlying reader or adding a cancelable read mechanism so Stop reliably terminates the loop.</violation>
</file>
<file name="tray/health.go">
<violation number="1" location="tray/health.go:112">
P2: Handle non-200 HTTP status codes as failures before decoding the body; otherwise a 500/404 response can be treated as healthy and reset the failure counter.</violation>
</file>
<file name="tray/service_windows.go">
<violation number="1" location="tray/service_windows.go:30">
P2: `BinaryPath` and `ConfigPath` are embedded into the `/TR` command string with only quote wrapping. If either path contains a double quote (`"`), the command string breaks and could allow arbitrary command execution. `ServiceConfig.Validate()` only checks that `BinaryPath` is non-empty — it does not sanitize path characters.</violation>
<violation number="2" location="tray/service_windows.go:42">
P2: Don’t swallow schtasks query errors. A failed query (permissions, command failure) should return an error instead of reporting “not registered,” otherwise the tray can misreport service state.</violation>
<violation number="3" location="tray/service_windows.go:75">
P2: The `"Running"` status string is locale-dependent on non-English Windows. For example, German Windows returns `"Wird ausgeführt"` and Japanese returns `"実行中"`. This check will always fail on non-English systems. Use `schtasks /query /xml` for locale-independent XML output, or switch to PowerShell's `Get-ScheduledTask` cmdlet.</violation>
</file>
<file name="tray/process.go">
<violation number="1" location="tray/process.go:270">
P1: Race condition: scheduled auto-restart will still fire after an explicit `Stop()` call. The restart goroutine captures the decision to restart before sleeping, but `Stop()` sets `autoRestart = false` during the sleep window. Since the goroutine never re-checks `autoRestart` after waking, the process restarts despite being explicitly stopped.</violation>
<violation number="2" location="tray/process.go:306">
P1: Auto-restart is permanently disabled after an explicit `Stop()`. `Stop()` sets `pm.autoRestart = false`, but `Start()` never re-enables it. After a user clicks Stop → Start, the process will no longer auto-restart on crash for the rest of the session. `Start()` should restore `autoRestart` to its configured value.</violation>
</file>
<file name="tray/service_linux.go">
<violation number="1" location="tray/service_linux.go:38">
P2: Quote/escape BinaryPath and ConfigPath in the systemd unit so paths with spaces don't break ExecStart parsing.</violation>
</file>
<file name="tray/service_darwin.go">
<violation number="1" location="tray/service_darwin.go:40">
P2: Values in the LaunchAgent plist are injected without XML escaping. If BinaryPath/ConfigPath/LogDir contains characters like `&` or `<`, the generated plist becomes invalid (and can be abused to inject extra plist keys). Escape these values or render the plist via an XML/plist encoder before writing.</violation>
<violation number="2" location="tray/service_darwin.go:132">
P2: `launchctl list <label>` returns exit code 0 for any known (loaded) job, whether it's currently running or stopped. This means a crashed or stopped service incorrectly returns `ServiceRunning`. Parse the output's PID column instead: a numeric PID indicates running, while `-` indicates registered but not running.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| go func() { | ||
| time.Sleep(delay) |
There was a problem hiding this comment.
P1: Race condition: scheduled auto-restart will still fire after an explicit Stop() call. The restart goroutine captures the decision to restart before sleeping, but Stop() sets autoRestart = false during the sleep window. Since the goroutine never re-checks autoRestart after waking, the process restarts despite being explicitly stopped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/process.go, line 270:
<comment>Race condition: scheduled auto-restart will still fire after an explicit `Stop()` call. The restart goroutine captures the decision to restart before sleeping, but `Stop()` sets `autoRestart = false` during the sleep window. Since the goroutine never re-checks `autoRestart` after waking, the process restarts despite being explicitly stopped.</comment>
<file context>
@@ -95,24 +173,126 @@ func (pm *ProcessManager) Start() error {
+ "restart_count", restartCount+1,
+ )
+
+ go func() {
+ time.Sleep(delay)
+ if err := pm.Start(); err != nil {
</file context>
| go func() { | |
| time.Sleep(delay) | |
| go func() { | |
| time.Sleep(delay) | |
| pm.mu.Lock() | |
| if !pm.autoRestart { | |
| pm.mu.Unlock() | |
| slog.Info("automatic restart cancelled") | |
| return | |
| } | |
| pm.mu.Unlock() |
| } | ||
|
|
||
| // Disable auto-restart when explicitly stopped | ||
| pm.autoRestart = false |
There was a problem hiding this comment.
P1: Auto-restart is permanently disabled after an explicit Stop(). Stop() sets pm.autoRestart = false, but Start() never re-enables it. After a user clicks Stop → Start, the process will no longer auto-restart on crash for the rest of the session. Start() should restore autoRestart to its configured value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/process.go, line 306:
<comment>Auto-restart is permanently disabled after an explicit `Stop()`. `Stop()` sets `pm.autoRestart = false`, but `Start()` never re-enables it. After a user clicks Stop → Start, the process will no longer auto-restart on crash for the rest of the session. `Start()` should restore `autoRestart` to its configured value.</comment>
<file context>
@@ -122,6 +302,9 @@ func (pm *ProcessManager) Stop() error {
}
+ // Disable auto-restart when explicitly stopped
+ pm.autoRestart = false
+
slog.Info("stopping adder process", "pid", pm.cmd.Process.Pid)
</file context>
| fcmURL := &url.URL{ | ||
| Scheme: "https", | ||
| Host: "fcm.googleapis.com", | ||
| Path: fmt.Sprintf("/v1/projects/%s/messages:send", projectId), |
There was a problem hiding this comment.
P2: projectId is interpolated into the URL path without escaping. A projectId containing / or .. segments (e.g., "foo/../../v2") would result in path traversal. Use url.PathEscape(projectId) to prevent this.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At output/push/fcm/message.go, line 80:
<comment>`projectId` is interpolated into the URL path without escaping. A `projectId` containing `/` or `..` segments (e.g., `"foo/../../v2"`) would result in path traversal. Use `url.PathEscape(projectId)` to prevent this.</comment>
<file context>
@@ -73,25 +74,24 @@ func NewMessage(token string, opts ...MessageOption) (*Message, error) {
+ fcmURL := &url.URL{
+ Scheme: "https",
+ Host: "fcm.googleapis.com",
+ Path: fmt.Sprintf("/v1/projects/%s/messages:send", projectId),
+ }
</file context>
| Path: fmt.Sprintf("/v1/projects/%s/messages:send", projectId), | |
| Path: fmt.Sprintf("/v1/projects/%s/messages:send", url.PathEscape(projectId)), |
| // Start begins parsing events in a background goroutine. The events | ||
| // channel is closed when the reader returns EOF or an error, or when | ||
| // Stop is called. | ||
| func (ep *EventParser) Start() { |
There was a problem hiding this comment.
P2: No guard against double Start() — calling it twice will launch two goroutines racing on the same bufio.Scanner, and close(ep.events) in run() will execute twice, causing a panic. Add a sync.Once or a started flag to prevent this.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/events.go, line 51:
<comment>No guard against double `Start()` — calling it twice will launch two goroutines racing on the same `bufio.Scanner`, and `close(ep.events)` in `run()` will execute twice, causing a panic. Add a `sync.Once` or a started flag to prevent this.</comment>
<file context>
@@ -0,0 +1,104 @@
+// Start begins parsing events in a background goroutine. The events
+// channel is closed when the reader returns EOF or an error, or when
+// Stop is called.
+func (ep *EventParser) Start() {
+ go ep.run()
+}
</file context>
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| var hr HealthResponse |
There was a problem hiding this comment.
P2: Handle non-200 HTTP status codes as failures before decoding the body; otherwise a 500/404 response can be treated as healthy and reset the failure counter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/health.go, line 112:
<comment>Handle non-200 HTTP status codes as failures before decoding the body; otherwise a 500/404 response can be treated as healthy and reset the failure counter.</comment>
<file context>
@@ -0,0 +1,147 @@
+ }
+ defer resp.Body.Close()
+
+ var hr HealthResponse
+ if err := json.NewDecoder(resp.Body).Decode(&hr); err != nil {
+ hp.consecutiveFailures++
</file context>
|
|
||
| // Stop signals the event parser to stop. Note that the parser will | ||
| // also stop naturally when the underlying reader is closed. | ||
| func (ep *EventParser) Stop() { |
There was a problem hiding this comment.
P2: Stop doesn’t actually stop the parser if the reader is blocked; the goroutine can hang indefinitely because scanner.Scan() isn’t interruptible. Consider closing the underlying reader or adding a cancelable read mechanism so Stop reliably terminates the loop.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/events.go, line 57:
<comment>Stop doesn’t actually stop the parser if the reader is blocked; the goroutine can hang indefinitely because `scanner.Scan()` isn’t interruptible. Consider closing the underlying reader or adding a cancelable read mechanism so Stop reliably terminates the loop.</comment>
<file context>
@@ -0,0 +1,104 @@
+
+// Stop signals the event parser to stop. Note that the parser will
+// also stop naturally when the underlying reader is closed.
+func (ep *EventParser) Stop() {
+ select {
+ case <-ep.done:
</file context>
| return ServiceNotRegistered, nil | ||
| } | ||
|
|
||
| if err := exec.Command("launchctl", "list", launchAgentLabel).Run(); err == nil { |
There was a problem hiding this comment.
P2: launchctl list <label> returns exit code 0 for any known (loaded) job, whether it's currently running or stopped. This means a crashed or stopped service incorrectly returns ServiceRunning. Parse the output's PID column instead: a numeric PID indicates running, while - indicates registered but not running.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/service_darwin.go, line 132:
<comment>`launchctl list <label>` returns exit code 0 for any known (loaded) job, whether it's currently running or stopped. This means a crashed or stopped service incorrectly returns `ServiceRunning`. Parse the output's PID column instead: a numeric PID indicates running, while `-` indicates registered but not running.</comment>
<file context>
@@ -0,0 +1,137 @@
+ return ServiceNotRegistered, nil
+ }
+
+ if err := exec.Command("launchctl", "list", launchAgentLabel).Run(); err == nil {
+ return ServiceRunning, nil
+ }
</file context>
| return ServiceNotRegistered, nil | ||
| } | ||
|
|
||
| if strings.Contains(string(out), "Running") { |
There was a problem hiding this comment.
P2: The "Running" status string is locale-dependent on non-English Windows. For example, German Windows returns "Wird ausgeführt" and Japanese returns "実行中". This check will always fail on non-English systems. Use schtasks /query /xml for locale-independent XML output, or switch to PowerShell's Get-ScheduledTask cmdlet.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/service_windows.go, line 75:
<comment>The `"Running"` status string is locale-dependent on non-English Windows. For example, German Windows returns `"Wird ausgeführt"` and Japanese returns `"実行中"`. This check will always fail on non-English systems. Use `schtasks /query /xml` for locale-independent XML output, or switch to PowerShell's `Get-ScheduledTask` cmdlet.</comment>
<file context>
@@ -0,0 +1,80 @@
+ return ServiceNotRegistered, nil
+ }
+
+ if strings.Contains(string(out), "Running") {
+ return ServiceRunning, nil
+ }
</file context>
| func registerService(cfg ServiceConfig) error { | ||
| command := fmt.Sprintf(`"%s"`, cfg.BinaryPath) | ||
| if cfg.ConfigPath != "" { | ||
| command = fmt.Sprintf(`"%s" --config "%s"`, cfg.BinaryPath, cfg.ConfigPath) |
There was a problem hiding this comment.
P2: BinaryPath and ConfigPath are embedded into the /TR command string with only quote wrapping. If either path contains a double quote ("), the command string breaks and could allow arbitrary command execution. ServiceConfig.Validate() only checks that BinaryPath is non-empty — it does not sanitize path characters.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/service_windows.go, line 30:
<comment>`BinaryPath` and `ConfigPath` are embedded into the `/TR` command string with only quote wrapping. If either path contains a double quote (`"`), the command string breaks and could allow arbitrary command execution. `ServiceConfig.Validate()` only checks that `BinaryPath` is non-empty — it does not sanitize path characters.</comment>
<file context>
@@ -0,0 +1,80 @@
+func registerService(cfg ServiceConfig) error {
+ command := fmt.Sprintf(`"%s"`, cfg.BinaryPath)
+ if cfg.ConfigPath != "" {
+ command = fmt.Sprintf(`"%s" --config "%s"`, cfg.BinaryPath, cfg.ConfigPath)
+ }
+
</file context>
| <string>io.blinklabs.adder</string> | ||
| <key>ProgramArguments</key> | ||
| <array> | ||
| <string>{{.BinaryPath}}</string> |
There was a problem hiding this comment.
P2: Values in the LaunchAgent plist are injected without XML escaping. If BinaryPath/ConfigPath/LogDir contains characters like & or <, the generated plist becomes invalid (and can be abused to inject extra plist keys). Escape these values or render the plist via an XML/plist encoder before writing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/service_darwin.go, line 40:
<comment>Values in the LaunchAgent plist are injected without XML escaping. If BinaryPath/ConfigPath/LogDir contains characters like `&` or `<`, the generated plist becomes invalid (and can be abused to inject extra plist keys). Escape these values or render the plist via an XML/plist encoder before writing.</comment>
<file context>
@@ -0,0 +1,137 @@
+ <string>io.blinklabs.adder</string>
+ <key>ProgramArguments</key>
+ <array>
+ <string>{{.BinaryPath}}</string>
+ {{- if .ConfigPath}}
+ <string>--config</string>
</file context>
|
Closing: this PR is stale (merge conflicts with main) and its enhancements (enhanced menu, status display, config/logs folders, about dialog) are captured in the TRAY_WS_REWRITE_PLAN.md Tasks C-H. The menu enhancements will be re-implemented as part of Task E (App Lifecycle Refactoring) using the new ConnectionManager architecture. |
1 participant
Summary by cubic
Enhances the tray with a live status display and new menu actions to manage Adder. Adds health checks, event streaming, and auto-restart to make the app more resilient.
New Features
Bug Fixes
Written for commit c648701. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests