feat(tui): interactive /settings picker + /set hygiene fixes - #184
Merged
Conversation
/settings (and bare /set) opens a keyboard-driven picker: toggles flip on
Enter, enum settings (theme, cerebras model/effort) cycle, numeric settings
edit inline; s saves the changed rows through the same apply path as /set,
Esc discards.
/set fixes from an audit:
- strict numeric parsing: strconv.Atoi replaces fmt.Sscanf, which silently
truncated inputs ('1e3' -> 1, '0x1f' -> 0, '12abc' -> 12) for project and
budget
- '/set project 0' now explicitly clears the default project
- usage lists anthropic_key, which the switch already accepted
- '/set apikey' / '/set anthropic-key' with no value prompt hidden via
ReadSecret instead of requiring the secret on the visible input line
- secret values are redacted from the in-session input history (both the
interactive and queued-prompt append sites)
- applySettingValue extracted as the single shared validation/apply path for
/set, the picker, and future callers
Sigilix OverviewEffort: 4/5 (large) Quality gates
Summary — latest pushIntroduces an interactive /settings TUI picker and refactors /set to share a single applySettingValue validation path, fixing silent numeric truncation (fmt.Sscanf → strconv.Atoi), nil-Context panics, and secret leakage in input history. The shared apply path now returns a typed settingResult so the picker and CLI can never drift on validation or persistence semantics. Important files
Sequence diagramsequenceDiagram
participant User
participant REPL
participant applySettingValue
participant Config
participant Keychain
User->>REPL: /set apikey qm-...
REPL->>REPL: redactSecretInput(input)
REPL->>applySettingValue: key="apikey", value="qm-..."
applySettingValue->>Keychain: api.LoginWithAPIKey
applySettingValue-->>REPL: settingAppliedNoSave
User->>REPL: /settings
REPL->>REPL: buildSettingsRows(cfg)
REPL->>REPL: tui.ShowSettingsPicker(rows)
loop Changed Rows
REPL->>applySettingValue: key, newValue
applySettingValue-->>REPL: settingResult
end
REPL->>Config: cfg.Save()
Confidence: 2/5Three P1 security and logic issues — nil-Context panics on cloud_sync/live_feed, apikey creating a bare SessionContext bypassing auth initialization, and the redaction regex missing the hyphenated anthropic-key form — make this unsafe to merge as-is.
Suggested labels:
|
QualityMax ReviewVerdict: COMMENT · Confidence: evidence-backed scan Files eligible: 6 · Files reviewed: 6 · Files with findings: 0 · Findings: 0 · Inline cards: 0 Priority findings
Review gates
Important files
Change diagram — Flowflowchart TD
Input[User Input] --> Redact{Redact Secret?}
Redact -- Yes --> History[Save Redacted to History]
Redact -- No --> History
History --> Dispatch{Command Type}
Dispatch -- /set --> Apply[Apply Setting]
Dispatch -- /settings --> Picker[Open TUI Picker]
Picker --> Apply
Apply --> Save[Save Config]
Review lifecycleUse the inline cards to inspect evidence and suggested remediation. Re-run the QualityMax review after pushing a fix; unchanged cards are identified by their stable finding marker. Dismiss with a reason through the existing QualityMax/GitHub review feedback flow. 0 prior card(s) are stale/resolved on this head. Proof legend: VERIFIED independently judged patch · REPRODUCED verified finding · GROUNDED deterministic evidence · MODEL-ONLY model judgment. QualityMax project results are available in the configured project. Receipt · commit |
![qualitymaxapp[bot]](https://avatars.githubusercontent.com/in/2748338?s=60&v=4){kind=small}
|
| Gate | Result |
|---|---|
| 🔍 AI diff review | ✅ Clean · gemini-3.1-flash-lite · completed · 6 eligible / 6 reviewed · gemini-3.1-flash-lite |
| 🔍 SAST | completed · 6 eligible / 6 reviewed · qwen3.7-plus |
| 🔍 Canonical PR review delivery | completed · 0 eligible / 0 reviewed · exact-head review #5093521774 and overview #5513072159 confirmed |
| 🧪 Repo Tests | ✅ 736/736 passed (go) |
Powered by QualityMax — AI-Powered Test Automation
![sigilix[bot]](https://avatars.githubusercontent.com/in/3558711?s=60&v=4){kind=small}
| if err := cfg.Save(); err != nil { | ||
| term.PrintError(fmt.Sprintf("api.Config updated in memory but failed to save: %v", err)) | ||
| } else { | ||
| term.PrintSystem("api.Config saved to ~/.qmax-code/config.json") |
There was a problem hiding this comment.
API key changes bypass config persistence, leaving session state inconsistent
The applySettingValue function returns settingAppliedNoSave for the apikey case, which signals that persistence is handled elsewhere (keychain/auth.json). However, the function still updates ag.Cfg.Context.Auth and ag.Cfg.Context.API in memory. This creates a mismatch: the session uses the new key for subsequent API calls, but the config file (~/.qmax-code/config.json) is not updated. If the session restarts or the config is reloaded, the key will be lost, and the session will revert to the previous auth state, potentially causing authentication failures. The fix is to ensure that when an API key is set, the config is persisted (or at least the session's auth state is saved) to maintain consistency.
Example:
User runs `/set apikey qm-live-secret123`. The key is set in memory, and subsequent API calls succeed. User restarts the session. The config file still contains the old key (or none). The session fails to authenticate.
Suggested fix:
In `applySettingValue`, change the `apikey` case to:
```go
case "apikey":
// ... validation ...
ag.Cfg.Context.Auth = auth
ag.Cfg.Context.API = api.NewAPIClient(auth)
tui.AnimateMax(tui.MoodHappy, fmt.Sprintf("Connected as %s", auth.Email))
fmt.Println()
// Ensure auth state is persisted
if err := api.SaveAnthropicKey(value); err != nil {
term.PrintSystem(fmt.Sprintf("Key set for this session (keychain: %s)", err))
}
return settingApplied // Signal that config should be saved
```Remediation: Enforce an explicit authorization check on every request against the authenticated principal and the specific object being accessed (object-level authz), and fail closed when the check is missing.
More Info
- Threat model: An authenticated user sets an API key via
/set apikeyor the settings picker, expecting it to be saved. The key is used for subsequent API calls, but after a restart or config reload, the key is missing, causing authentication failures and disrupting the user's workflow. - Specific code citations: Lines 1935-1940:
applySettingValuereturnssettingAppliedNoSaveforapikey. Lines 1930-1934:ag.Cfg.Context.Authandag.Cfg.Context.APIare updated. - Existing protections: The
applyAndSaveSettingfunction callscfg.Save()only whenapplySettingValuereturnssettingApplied. Forapikey, it returnssettingAppliedNoSave, so the save is skipped. - Proposed mitigation: Change the
apikeycase to returnsettingApplied(or a new result likesettingAppliedAuth) and ensure the auth state is persisted viaapi.SaveAnthropicKey(which already handles keychain). Alternatively, updateapplyAndSaveSettingto also callcfg.Save()forsettingAppliedNoSavewhen the key isapikey. - Alternative mitigations considered: Keep
settingAppliedNoSavebut add a separate persistence step for auth state (e.g., callapi.SaveAnthropicKeyand update config). This would duplicate logic already in theanthropic_keycase. - Severity calibration: Score 4 because it's a security hardening gap that leads to inconsistent session state and potential authentication loss. It's not a direct vulnerability (P0) because it doesn't expose secrets, but it undermines user expectations and can cause operational issues.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2088
Comment:
**API key changes bypass config persistence, leaving session state inconsistent**
The `applySettingValue` function returns `settingAppliedNoSave` for the `apikey` case, which signals that persistence is handled elsewhere (keychain/auth.json). However, the function still updates `ag.Cfg.Context.Auth` and `ag.Cfg.Context.API` in memory. This creates a mismatch: the session uses the new key for subsequent API calls, but the config file (`~/.qmax-code/config.json`) is not updated. If the session restarts or the config is reloaded, the key will be lost, and the session will revert to the previous auth state, potentially causing authentication failures. The fix is to ensure that when an API key is set, the config is persisted (or at least the session's auth state is saved) to maintain consistency.
Example:
User runs `/set apikey qm-live-secret123`. The key is set in memory, and subsequent API calls succeed. User restarts the session. The config file still contains the old key (or none). The session fails to authenticate.
Threat model:
An authenticated user sets an API key via `/set apikey` or the settings picker, expecting it to be saved. The key is used for subsequent API calls, but after a restart or config reload, the key is missing, causing authentication failures and disrupting the user's workflow.
Specific code citations:
Lines 1935-1940: `applySettingValue` returns `settingAppliedNoSave` for `apikey`. Lines 1930-1934: `ag.Cfg.Context.Auth` and `ag.Cfg.Context.API` are updated.
Existing protections:
The `applyAndSaveSetting` function calls `cfg.Save()` only when `applySettingValue` returns `settingApplied`. For `apikey`, it returns `settingAppliedNoSave`, so the save is skipped.
Proposed mitigation:
Change the `apikey` case to return `settingApplied` (or a new result like `settingAppliedAuth`) and ensure the auth state is persisted via `api.SaveAnthropicKey` (which already handles keychain). Alternatively, update `applyAndSaveSetting` to also call `cfg.Save()` for `settingAppliedNoSave` when the key is `apikey`.
Alternative mitigations considered:
Keep `settingAppliedNoSave` but add a separate persistence step for auth state (e.g., call `api.SaveAnthropicKey` and update config). This would duplicate logic already in the `anthropic_key` case.
Severity calibration:
Score 4 because it's a security hardening gap that leads to inconsistent session state and potential authentication loss. It's not a direct vulnerability (P0) because it doesn't expose secrets, but it undermines user expectations and can cause operational issues.
Suggested fix shape:
In `applySettingValue`, change the `apikey` case to:
```go
case "apikey":
// ... validation ...
ag.Cfg.Context.Auth = auth
ag.Cfg.Context.API = api.NewAPIClient(auth)
tui.AnimateMax(tui.MoodHappy, fmt.Sprintf("Connected as %s", auth.Email))
fmt.Println()
// Ensure auth state is persisted
if err := api.SaveAnthropicKey(value); err != nil {
term.PrintSystem(fmt.Sprintf("Key set for this session (keychain: %s)", err))
}
return settingApplied // Signal that config should be saved
```
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| // secretSetPrefixes are the /set forms whose value must never be stored in | ||
| // the in-session input history (up-arrow would otherwise recall the secret). | ||
| var secretSetPrefixes = []string{"/set apikey ", "/set anthropic-key ", "/set anthropic_key "} |
There was a problem hiding this comment.
Secret redaction logic fails to cover all case variations, leaving secrets in history
The secretSetPrefixes list includes /set apikey , /set anthropic-key , and /set anthropic_key , but the matching uses strings.HasPrefix(lower, p). This fails to redact inputs like /set apikey qm-live-secret123 (double space) or /set apikey\tqm-live-secret123 (tab separator). Additionally, the list does not include the new /settings picker's secret rows (though those are not entered as raw commands). The redaction should normalize whitespace or use a more robust pattern (e.g., regex) to catch all variations.
More Info
- Threat model: An attacker with access to the user's terminal history (e.g., via shell history file or memory dump) could recover API keys if the redaction fails due to whitespace variations.
- Specific code citations: Lines 2158-2162:
secretSetPrefixesdefinition andredactSecretInputloop. - Existing protections: The redaction works for exact prefix matches with single spaces. The history is only stored in memory during the session.
- Proposed mitigation: Normalize whitespace in the input before matching (e.g., collapse multiple spaces/tabs) or use a regex pattern like
^/set\s+apikey\s+(case-insensitive). - Alternative mitigations considered: Extend the prefix list with common whitespace variations, but normalization is more robust.
- Severity calibration: Score 3 because it's a hardening gap that could lead to secret leakage under specific conditions (non-standard whitespace). The risk is moderate because the history is in-memory and the session is live, but defense-in-depth is warranted.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2165
Comment:
**Secret redaction logic fails to cover all case variations, leaving secrets in history**
The `secretSetPrefixes` list includes `/set apikey `, `/set anthropic-key `, and `/set anthropic_key `, but the matching uses `strings.HasPrefix(lower, p)`. This fails to redact inputs like `/set apikey qm-live-secret123` (double space) or `/set apikey\tqm-live-secret123` (tab separator). Additionally, the list does not include the new `/settings` picker's secret rows (though those are not entered as raw commands). The redaction should normalize whitespace or use a more robust pattern (e.g., regex) to catch all variations.
Threat model:
An attacker with access to the user's terminal history (e.g., via shell history file or memory dump) could recover API keys if the redaction fails due to whitespace variations.
Specific code citations:
Lines 2158-2162: `secretSetPrefixes` definition and `redactSecretInput` loop.
Existing protections:
The redaction works for exact prefix matches with single spaces. The history is only stored in memory during the session.
Proposed mitigation:
Normalize whitespace in the input before matching (e.g., collapse multiple spaces/tabs) or use a regex pattern like `^/set\s+apikey\s+` (case-insensitive).
Alternative mitigations considered:
Extend the prefix list with common whitespace variations, but normalization is more robust.
Severity calibration:
Score 3 because it's a hardening gap that could lead to secret leakage under specific conditions (non-standard whitespace). The risk is moderate because the history is in-memory and the session is live, but defense-in-depth is warranted.
How can I resolve this? If you propose a fix, please make it concise.
| if err != nil { | ||
| term.PrintError(fmt.Sprintf("Invalid API key: %v", err)) | ||
| return | ||
| return settingInvalid |
There was a problem hiding this comment.
cerebras_model failure path returns settingApplied after printing an error, triggering cfg.Save on a rejected value
The cerebras_model case prints an error for an invalid model but falls through to the shared return settingApplied at the end of applySettingValue. Sibling cases (model, project, budget, theme, backend) all return settingInvalid on their validation failures, so the established contract is: a printed error means no persistence. Here an invalid cerebras model still marks the row applied, so applyAndSaveSetting (and the picker's needSave loop) will call cfg.Save() after the user was told the value was rejected. The fix is to return settingInvalid in the invalid-model branch, matching the sibling cases.
More Info
- Threat model: A user runs
/set cerebras_model bogusor cycles to an invalid value in the picker; they see an error message but config.json is still rewritten, and the picker reports a successful save. - Specific code citations:
applySettingValuecasecerebras_model(repl.go ~line 2020-2030) printsInvalid cerebras modelbut does not return; the function's finalreturn settingApplied(repl.go ~line 2149) is reached. Siblingmodelcase returnssettingInvalidafterValid models:error. - Existing protections: The
settingResultenum and thesettingInvalidconstant exist, and every other validation-failure branch in the same function uses them; no guard prevents the fall-through here. - Proposed mitigation: Add
return settingInvalidin the invalid-model branch of thecerebras_modelcase. - Alternative mitigations considered: Restructuring the switch to avoid fall-through would be broader; a targeted return matches the sibling pattern.
- Severity calibration: Blast radius is a spurious config write and misleading save message; no data corruption beyond rewriting the same values. Likelihood is high for any invalid cerebras model input, so score 3.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2035
Comment:
**cerebras_model failure path returns settingApplied after printing an error, triggering cfg.Save on a rejected value**
The `cerebras_model` case prints an error for an invalid model but falls through to the shared `return settingApplied` at the end of `applySettingValue`. Sibling cases (`model`, `project`, `budget`, `theme`, `backend`) all `return settingInvalid` on their validation failures, so the established contract is: a printed error means no persistence. Here an invalid cerebras model still marks the row applied, so `applyAndSaveSetting` (and the picker's `needSave` loop) will call `cfg.Save()` after the user was told the value was rejected. The fix is to `return settingInvalid` in the invalid-model branch, matching the sibling cases.
Threat model:
A user runs `/set cerebras_model bogus` or cycles to an invalid value in the picker; they see an error message but config.json is still rewritten, and the picker reports a successful save.
Specific code citations:
`applySettingValue` case `cerebras_model` (repl.go ~line 2020-2030) prints `Invalid cerebras model` but does not return; the function's final `return settingApplied` (repl.go ~line 2149) is reached. Sibling `model` case returns `settingInvalid` after `Valid models:` error.
Existing protections:
The `settingResult` enum and the `settingInvalid` constant exist, and every other validation-failure branch in the same function uses them; no guard prevents the fall-through here.
Proposed mitigation:
Add `return settingInvalid` in the invalid-model branch of the `cerebras_model` case.
Alternative mitigations considered:
Restructuring the switch to avoid fall-through would be broader; a targeted return matches the sibling pattern.
Severity calibration:
Blast radius is a spurious config write and misleading save message; no data corruption beyond rewriting the same values. Likelihood is high for any invalid cerebras model input, so score 3.
How can I resolve this? If you propose a fix, please make it concise.
| term.PrintSystem("Anthropic API key saved to OS keychain.") | ||
| } | ||
| return // don't save to config.json — keychain handles it | ||
| return settingAppliedNoSave // keychain handles persistence |
There was a problem hiding this comment.
cerebras_reasoning_effort failure path returns settingApplied after printing an error, triggering cfg.Save on a rejected value
The cerebras_reasoning_effort case prints Invalid value %q; allowed: none, low, medium, high for an invalid effort but falls through to the shared return settingApplied. Sibling cases (model, project, budget, theme, backend) all return settingInvalid on validation failure, establishing the contract that a printed error means no persistence. Here an invalid effort still marks the row applied, so applyAndSaveSetting and the picker's needSave loop will call cfg.Save() after the user was told the value was rejected. The fix is to return settingInvalid in the invalid-effort branch.
More Info
- Threat model: A user runs
/set cerebras_reasoning_effort bogusor cycles to an invalid value in the picker; they see an error but config.json is still rewritten and the picker reports a successful save. - Specific code citations:
applySettingValuecasecerebras_reasoning_effort(repl.go ~line 2140-2149) prints an error but does not return; the function's finalreturn settingAppliedis reached. Siblingmodelcase returnssettingInvalidafter its error. - Existing protections: The
settingResultenum andsettingInvalidconstant exist and are used by every other validation-failure branch in the same function; no guard prevents the fall-through here. - Proposed mitigation: Add
return settingInvalidin the invalid-effort branch of thecerebras_reasoning_effortcase. - Alternative mitigations considered: Restructuring the switch to avoid fall-through would be broader; a targeted return matches the sibling pattern.
- Severity calibration: Blast radius is a spurious config write and misleading save message; no data corruption beyond rewriting the same values. Likelihood is high for any invalid cerebras reasoning effort input, so score 3.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2152
Comment:
**cerebras_reasoning_effort failure path returns settingApplied after printing an error, triggering cfg.Save on a rejected value**
The `cerebras_reasoning_effort` case prints `Invalid value %q; allowed: none, low, medium, high` for an invalid effort but falls through to the shared `return settingApplied`. Sibling cases (`model`, `project`, `budget`, `theme`, `backend`) all `return settingInvalid` on validation failure, establishing the contract that a printed error means no persistence. Here an invalid effort still marks the row applied, so `applyAndSaveSetting` and the picker's `needSave` loop will call `cfg.Save()` after the user was told the value was rejected. The fix is to `return settingInvalid` in the invalid-effort branch.
Threat model:
A user runs `/set cerebras_reasoning_effort bogus` or cycles to an invalid value in the picker; they see an error but config.json is still rewritten and the picker reports a successful save.
Specific code citations:
`applySettingValue` case `cerebras_reasoning_effort` (repl.go ~line 2140-2149) prints an error but does not return; the function's final `return settingApplied` is reached. Sibling `model` case returns `settingInvalid` after its error.
Existing protections:
The `settingResult` enum and `settingInvalid` constant exist and are used by every other validation-failure branch in the same function; no guard prevents the fall-through here.
Proposed mitigation:
Add `return settingInvalid` in the invalid-effort branch of the `cerebras_reasoning_effort` case.
Alternative mitigations considered:
Restructuring the switch to avoid fall-through would be broader; a targeted return matches the sibling pattern.
Severity calibration:
Blast radius is a spurious config write and misleading save message; no data corruption beyond rewriting the same values. Likelihood is high for any invalid cerebras reasoning effort input, so score 3.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+2170
to
+2190
| lower := strings.ToLower(input) | ||
| for _, p := range secretSetPrefixes { | ||
| if strings.HasPrefix(lower, p) { | ||
| return p + "<redacted>" | ||
| } | ||
| } | ||
| return input | ||
| } | ||
|
|
||
| // buildSettingsRows snapshots the config into picker rows. | ||
| func buildSettingsRows(cfg *api.Config) []tui.SettingsRow { | ||
| cloudSync := cfg.CloudSync != nil && *cfg.CloudSync | ||
| boolStr := func(b bool) string { | ||
| if b { | ||
| return "true" | ||
| } | ||
| return "false" | ||
| } | ||
| return []tui.SettingsRow{ | ||
| {Key: "project", Label: "Default project", Kind: tui.SettingsText, | ||
| Value: strconv.Itoa(cfg.DefaultProject), Hint: "0 = unset"}, |
There was a problem hiding this comment.
runSettingsPicker saves valid rows even when another row fails validation, unlike /set which rejects the whole command
runSettingsPicker iterates the changed rows and calls applySettingValue for each, but it only checks whether the result is settingApplied to set needSave; it never checks for settingInvalid. If the user changes two rows — one valid and one invalid (e.g. budget abc and theme ocean) — the invalid row prints an error, but the valid row still sets needSave = true and cfg.Save() runs, producing a success message. The sibling /set path (applyAndSaveSetting) applies a single key/value and only saves when that one apply succeeds, so the established contract is: a validation failure means no save. The picker should abort the save (or at least not report success) when any changed row returns settingInvalid.
More Info
- Threat model: A user edits multiple rows in the picker, one of which is invalid; they see an error for the invalid row but also a success message and a config write for the valid rows, making it unclear whether the invalid row was applied.
- Specific code citations:
runSettingsPicker(repl.go ~line 2170-2190) checksif applySettingValue(...) == settingApplied { needSave = true }but never checkssettingInvalid. SiblingapplyAndSaveSetting(repl.go ~line 1850) only saves when the single apply returnssettingApplied. - Existing protections: The
settingResultenum includessettingInvalid, andapplySettingValuereturns it on validation failures, but the picker loop ignores that value. - Proposed mitigation: Track whether any row returned
settingInvalidand skip the save (or print a distinct error) when one did. - Alternative mitigations considered: Saving only the valid rows is defensible but should not print a blanket success message; aborting the save matches the /set contract more closely.
- Severity calibration: Blast radius is a misleading success message and a partial config write; no data corruption beyond applying the valid rows. Likelihood is moderate (requires a multi-row edit with one invalid value), so score 3.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2170-2190
Comment:
**runSettingsPicker saves valid rows even when another row fails validation, unlike /set which rejects the whole command**
`runSettingsPicker` iterates the changed rows and calls `applySettingValue` for each, but it only checks whether the result is `settingApplied` to set `needSave`; it never checks for `settingInvalid`. If the user changes two rows — one valid and one invalid (e.g. budget `abc` and theme `ocean`) — the invalid row prints an error, but the valid row still sets `needSave = true` and `cfg.Save()` runs, producing a success message. The sibling `/set` path (`applyAndSaveSetting`) applies a single key/value and only saves when that one apply succeeds, so the established contract is: a validation failure means no save. The picker should abort the save (or at least not report success) when any changed row returns `settingInvalid`.
Threat model:
A user edits multiple rows in the picker, one of which is invalid; they see an error for the invalid row but also a success message and a config write for the valid rows, making it unclear whether the invalid row was applied.
Specific code citations:
`runSettingsPicker` (repl.go ~line 2170-2190) checks `if applySettingValue(...) == settingApplied { needSave = true }` but never checks `settingInvalid`. Sibling `applyAndSaveSetting` (repl.go ~line 1850) only saves when the single apply returns `settingApplied`.
Existing protections:
The `settingResult` enum includes `settingInvalid`, and `applySettingValue` returns it on validation failures, but the picker loop ignores that value.
Proposed mitigation:
Track whether any row returned `settingInvalid` and skip the save (or print a distinct error) when one did.
Alternative mitigations considered:
Saving only the valid rows is defensible but should not print a blanket success message; aborting the save matches the /set contract more closely.
Severity calibration:
Blast radius is a misleading success message and a partial config write; no data corruption beyond applying the valid rows. Likelihood is moderate (requires a multi-row edit with one invalid value), so score 3.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+140
to
+143
| case tea.KeyRunes: | ||
| m.editBuf += string(msg.Runes) | ||
| } | ||
| return m, nil |
There was a problem hiding this comment.
Cycle fallback when current value not in options list may produce unexpected behavior
The nextCycleOption function returns options[0] when the current value is not found in the list. This could cause a user-visible jump from a custom or invalid value to the first option without warning. The picker's UI should reflect that the current value is not a valid option, and the fallback may be surprising.
Why this wasn't caught: The existing test TestNextCycleOptionFallsBackToFirst only asserts the fallback returns options[0]; it does not test the picker integration or the Display function's behavior with invalid values.
Detailed reasoning
Inspect the nextCycleOption logic: if current is not in options, it returns options[0]. This is used when advancing cycles via Enter. If the config's stored value (e.g., from a previous version) is not in the current options list, the first press of Enter will silently change it to the first option, which may not be the user's intent.
More Info
- Threat model: A user with a config value that is no longer valid (e.g., a deprecated theme) sees the picker display that value (via Display function), but pressing Enter silently changes it to the first option without clear feedback. This could lead to unintended configuration changes.
- Specific code citations:
nextCycleOptionat line 143:return options[0]whencurrentnot found. Called fromupdateBrowsingon Enter forSettingsCyclerows. - Existing protections: The test
TestNextCycleOptionFallsBackToFirstonly checks the fallback behavior; it does not test the interaction with the picker UI or the Display function. The picker's UI shows the current value viarowDisplay, which may mask that the value is invalid. - Proposed mitigation: When
currentis not inoptions, either disallow cycling (keep current) and show a hint, or explicitly reset to a default and notify the user. The picker could mark such rows with a warning badge. - Alternative mitigations considered: 1. Keep current value unchanged and skip cycling (return current). 2. Validate config at load and normalize invalid values. 3. Add a visual indicator (e.g.,
(invalid)) in the picker row. - Severity calibration: Score 3 because it's a minor UX issue with low risk of data loss; the config can be manually corrected. The behavior is deterministic and covered by a unit test, but the user experience could be confusing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/settings_picker.go
Line: 140-143
Comment:
**Cycle fallback when current value not in options list may produce unexpected behavior**
The `nextCycleOption` function returns `options[0]` when the current value is not found in the list. This could cause a user-visible jump from a custom or invalid value to the first option without warning. The picker's UI should reflect that the current value is not a valid option, and the fallback may be surprising.
Inspect the `nextCycleOption` logic: if `current` is not in `options`, it returns `options[0]`. This is used when advancing cycles via Enter. If the config's stored value (e.g., from a previous version) is not in the current options list, the first press of Enter will silently change it to the first option, which may not be the user's intent.
Threat model:
A user with a config value that is no longer valid (e.g., a deprecated theme) sees the picker display that value (via Display function), but pressing Enter silently changes it to the first option without clear feedback. This could lead to unintended configuration changes.
Specific code citations:
`nextCycleOption` at line 143: `return options[0]` when `current` not found. Called from `updateBrowsing` on Enter for `SettingsCycle` rows.
Existing protections:
The test `TestNextCycleOptionFallsBackToFirst` only checks the fallback behavior; it does not test the interaction with the picker UI or the Display function. The picker's UI shows the current value via `rowDisplay`, which may mask that the value is invalid.
Proposed mitigation:
When `current` is not in `options`, either disallow cycling (keep current) and show a hint, or explicitly reset to a default and notify the user. The picker could mark such rows with a warning badge.
Alternative mitigations considered:
1. Keep current value unchanged and skip cycling (return current). 2. Validate config at load and normalize invalid values. 3. Add a visual indicator (e.g., `(invalid)`) in the picker row.
Severity calibration:
Score 3 because it's a minor UX issue with low risk of data loss; the config can be manually corrected. The behavior is deterministic and covered by a unit test, but the user experience could be confusing.
Why this wasn't caught:
The existing test `TestNextCycleOptionFallsBackToFirst` only asserts the fallback returns options[0]; it does not test the picker integration or the Display function's behavior with invalid values.
How can I resolve this? If you propose a fix, please make it concise.
| // redactSecretInput rewrites secret-carrying inputs to a redacted form before | ||
| // they enter the recallable history; anything else passes through unchanged. | ||
| func redactSecretInput(input string) string { | ||
| lower := strings.ToLower(input) |
There was a problem hiding this comment.
Secret redaction's case-insensitive prefix matching may miss edge cases
The redactSecretInput function lowercases the input and checks against lowercased prefixes. This correctly handles case-insensitive commands, but the test suite only covers a few variants. Edge cases like mixed-case prefixes or extra whitespace could bypass redaction.
Example:
Input ` /set apikey qm-secret` (two leading spaces) would not be redacted because lowercased prefix `/set apikey ` does not match.
Suggested fix:
Add `input = strings.TrimSpace(input)` before lowercasing, or adjust prefix matching to allow optional leading spaces.Why this wasn't caught: The test TestRedactSecretInput does not cover whitespace variations or partial prefix matches.
Detailed reasoning
Specifically, the function uses strings.ToLower(input) and compares against lowercased prefixes like /set apikey . However, the input may have leading/trailing spaces or unusual casing that still matches the semantic command but not the prefix pattern. The test TestRedactSecretInput covers basic cases but does not exercise whitespace or partial matches.
More Info
- Threat model: A secret could inadvertently enter history if the input formatting differs from the expected prefix (e.g., extra spaces). This would allow up-arrow recall of the secret, violating the hygiene goal.
- Specific code citations:
redactSecretInputat lines 1159-1172:lower := strings.ToLower(input); for _, p := range secretSetPrefixes { if strings.HasPrefix(lower, p) { ... } }. Prefixes are lowercased literals. - Existing protections: The test
TestRedactSecretInputcovers exact matches and case-insensitive variants, but does not test inputs like/set apikey ...(leading spaces) or/set apikey ...(double spaces). - Proposed mitigation: Trim leading spaces before lowercasing, or use a more robust parser that extracts the command and key. Alternatively, expand test coverage to include edge cases.
- Alternative mitigations considered: Use a regex to match the command pattern ignoring whitespace variations. Or normalize input by collapsing multiple spaces.
- Severity calibration: Score 3 because the risk is low (the user would have to type an unusual format), but the hygiene fix is a security-adjacent feature that should be robust.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2170
Comment:
**Secret redaction's case-insensitive prefix matching may miss edge cases**
The `redactSecretInput` function lowercases the input and checks against lowercased prefixes. This correctly handles case-insensitive commands, but the test suite only covers a few variants. Edge cases like mixed-case prefixes or extra whitespace could bypass redaction.
Specifically, the function uses `strings.ToLower(input)` and compares against lowercased prefixes like `/set apikey `. However, the input may have leading/trailing spaces or unusual casing that still matches the semantic command but not the prefix pattern. The test `TestRedactSecretInput` covers basic cases but does not exercise whitespace or partial matches.
Example:
Input ` /set apikey qm-secret` (two leading spaces) would not be redacted because lowercased prefix `/set apikey ` does not match.
Threat model:
A secret could inadvertently enter history if the input formatting differs from the expected prefix (e.g., extra spaces). This would allow up-arrow recall of the secret, violating the hygiene goal.
Specific code citations:
`redactSecretInput` at lines 1159-1172: `lower := strings.ToLower(input); for _, p := range secretSetPrefixes { if strings.HasPrefix(lower, p) { ... } }`. Prefixes are lowercased literals.
Existing protections:
The test `TestRedactSecretInput` covers exact matches and case-insensitive variants, but does not test inputs like ` /set apikey ...` (leading spaces) or `/set apikey ...` (double spaces).
Proposed mitigation:
Trim leading spaces before lowercasing, or use a more robust parser that extracts the command and key. Alternatively, expand test coverage to include edge cases.
Alternative mitigations considered:
Use a regex to match the command pattern ignoring whitespace variations. Or normalize input by collapsing multiple spaces.
Severity calibration:
Score 3 because the risk is low (the user would have to type an unusual format), but the hygiene fix is a security-adjacent feature that should be robust.
Suggested fix shape:
Add `input = strings.TrimSpace(input)` before lowercasing, or adjust prefix matching to allow optional leading spaces.
Why this wasn't caught:
The test `TestRedactSecretInput` does not cover whitespace variations or partial prefix matches.
How can I resolve this? If you propose a fix, please make it concise.
Review follow-up on #184: strings.Fields accepts multi-space and tab separators, so '/set apikey <value>' executed as an apikey set but slipped past the fixed-prefix redaction check. A single whitespace-tolerant regex now matches every accepted form; tests cover the double-space and tab variants.
Contributor
Author
Sigilix triage (7 findings)Fixed in 60b62e6:
Dismissed:
Suite on 60b62e6: 0 failures. |
| term.PrintError(fmt.Sprintf("api.Config updated in memory but failed to save: %v", err)) | ||
| } else { | ||
| term.PrintSystem("api.Config saved to ~/.qmax-code/config.json") | ||
| return settingApplied |
There was a problem hiding this comment.
Secret redaction regex misses quoted and leading-whitespace secret forms
Flagged by 2 specialists.
Example:
input: /set apikey "qm-secret"
actual: history contains /set apikey "qm-secret"
expected: history contains /set apikey <redacted>
Detailed reasoning
The secretSetRe regex (?i)^/set\s+(apikey|anthropic[-_]key)\s+\S requires the input to start exactly with /set and the key to be followed by a non-space character. Inputs like /set apikey "qm-secret" (quoted value) or /set apikey qm-secret (leading spaces before /set) are not redacted, so the secret enters the recallable history. The fix is to anchor on the command after trimming leading whitespace and to match quoted values.
More Info
- Threat model: A user pastes a secret with quotes or leading whitespace; the secret is stored in input history and can be recalled with up-arrow, leaking the credential.
- Specific code citations:
secretSetRedefinition andredactSecretInput; the test cases only cover unquoted, no-leading-whitespace forms. - Existing protections: The regex handles multiple spaces/tabs between
/setand the key, but not leading whitespace before/setor quoted values. - Proposed mitigation: Trim leading whitespace before matching, and extend the regex to match quoted values:
(?i)^\s*/set\s+(apikey|anthropic[-_]key)\s+["']?\S. - Alternative mitigations considered: Parse the command with
strings.Fieldsfirst and redact based on the parsed key, which would handle all whitespace and quoting uniformly. - Severity calibration: Secret leakage into history is a security-relevant correctness bug; likelihood depends on how users paste secrets.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2161
Comment:
**Secret redaction regex misses quoted and leading-whitespace secret forms**
_Flagged by 2 specialists._
The `secretSetRe` regex `(?i)^/set\s+(apikey|anthropic[-_]key)\s+\S` requires the input to start exactly with `/set` and the key to be followed by a non-space character. Inputs like `/set apikey "qm-secret"` (quoted value) or `/set apikey qm-secret` (leading spaces before `/set`) are not redacted, so the secret enters the recallable history. The fix is to anchor on the command after trimming leading whitespace and to match quoted values.
Example:
input: /set apikey "qm-secret"
actual: history contains /set apikey "qm-secret"
expected: history contains /set apikey <redacted>
Threat model:
A user pastes a secret with quotes or leading whitespace; the secret is stored in input history and can be recalled with up-arrow, leaking the credential.
Specific code citations:
`secretSetRe` definition and `redactSecretInput`; the test cases only cover unquoted, no-leading-whitespace forms.
Existing protections:
The regex handles multiple spaces/tabs between `/set` and the key, but not leading whitespace before `/set` or quoted values.
Proposed mitigation:
Trim leading whitespace before matching, and extend the regex to match quoted values: `(?i)^\s*/set\s+(apikey|anthropic[-_]key)\s+["']?\S`.
Alternative mitigations considered:
Parse the command with `strings.Fields` first and redact based on the parsed key, which would handle all whitespace and quoting uniformly.
Severity calibration:
Secret leakage into history is a security-relevant correctness bug; likelihood depends on how users paste secrets.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
2000
to
+2010
| switch strings.ToLower(value) { | ||
| case "true", "1", "yes", "on": | ||
| cfg.LiveFeed = true | ||
| ag.Cfg.Context.LiveFeed = true | ||
| if ag.Cfg.Context != nil { | ||
| ag.Cfg.Context.LiveFeed = true | ||
| } | ||
| term.PrintSystem("Live feed enabled — test runs and AI crawls will stream in QM Cloud Sandbox.") | ||
| case "false", "0", "no", "off": | ||
| cfg.LiveFeed = false | ||
| ag.Cfg.Context.LiveFeed = false | ||
| if ag.Cfg.Context != nil { | ||
| ag.Cfg.Context.LiveFeed = false |
There was a problem hiding this comment.
Nil-pointer dereference on /set apikey when ag.Cfg.Context is nil
In the apikey case, the code checks ag.Cfg.Context != nil && ag.Cfg.Context.LocalOnly for the guard, but then unconditionally dereferences ag.Cfg.Context.Auth and ag.Cfg.Context.API after a successful api.LoginWithAPIKey. If ag.Cfg.Context is nil (which the new nil-guard explicitly acknowledges is possible), this panics. The fix is to initialize ag.Cfg.Context when nil before assigning Auth/API, or guard the assignment.
Example:
input: /set apikey qm-valid-key
actual: panic: runtime error: invalid memory address or nil pointer dereference
expected: API key saved and session context updated
More Info
- Threat model: A user runs
/set apikey <valid-key>in a session whereag.Cfg.Contextis nil (e.g. standalone mode or a fresh agent). The process panics and the REPL crashes. - Specific code citations:
ag.Cfg.Context.Auth = authandag.Cfg.Context.API = api.NewAPIClient(auth)at the apikey case; the guardag.Cfg.Context != nil && ag.Cfg.Context.LocalOnlyon the preceding line proves nil is a considered state. - Existing protections: The nil-guard on the LocalOnly check only protects the guard itself; it does not initialize Context before the subsequent dereference.
- Proposed mitigation: Add
if ag.Cfg.Context == nil { ag.Cfg.Context = &api.SessionContext{} }before the Auth/API assignments, or use a local variable and assign back. - Alternative mitigations considered: Return an error if Context is nil, but that would break the apikey flow for standalone users who may still want to set a key.
- Severity calibration: Panic on a valid user command; high likelihood if Context can be nil in any supported mode.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2000-2010
Comment:
**Nil-pointer dereference on /set apikey when ag.Cfg.Context is nil**
In the `apikey` case, the code checks `ag.Cfg.Context != nil && ag.Cfg.Context.LocalOnly` for the guard, but then unconditionally dereferences `ag.Cfg.Context.Auth` and `ag.Cfg.Context.API` after a successful `api.LoginWithAPIKey`. If `ag.Cfg.Context` is nil (which the new nil-guard explicitly acknowledges is possible), this panics. The fix is to initialize `ag.Cfg.Context` when nil before assigning Auth/API, or guard the assignment.
Example:
input: /set apikey qm-valid-key
actual: panic: runtime error: invalid memory address or nil pointer dereference
expected: API key saved and session context updated
Threat model:
A user runs `/set apikey <valid-key>` in a session where `ag.Cfg.Context` is nil (e.g. standalone mode or a fresh agent). The process panics and the REPL crashes.
Specific code citations:
`ag.Cfg.Context.Auth = auth` and `ag.Cfg.Context.API = api.NewAPIClient(auth)` at the apikey case; the guard `ag.Cfg.Context != nil && ag.Cfg.Context.LocalOnly` on the preceding line proves nil is a considered state.
Existing protections:
The nil-guard on the LocalOnly check only protects the guard itself; it does not initialize Context before the subsequent dereference.
Proposed mitigation:
Add `if ag.Cfg.Context == nil { ag.Cfg.Context = &api.SessionContext{} }` before the Auth/API assignments, or use a local variable and assign back.
Alternative mitigations considered:
Return an error if Context is nil, but that would break the apikey flow for standalone users who may still want to set a key.
Severity calibration:
Panic on a valid user command; high likelihood if Context can be nil in any supported mode.
How can I resolve this? If you propose a fix, please make it concise.
| switch strings.ToLower(value) { | ||
| case "cc": | ||
| if bin := agent.FindClaudeCode(); bin == "" { | ||
| if agent.FindClaudeCode() == "" { |
There was a problem hiding this comment.
Secret values are logged to environment variables and terminal output, exposing credentials
The applySettingValue function for apikey and anthropic_key writes the secret value to os.Setenv("ANTHROPIC_API_KEY", value) and logs it via term.PrintSystem. This exposes the secret in the process environment and terminal output, which could be captured by logs or other processes. The environment variable is unnecessary for the keychain persistence path and should not be set.
Specifically, the anthropic_key case sets the environment variable and logs a success message that includes the key. The apikey case logs a success message with the user's email (which may be sensitive) and also writes the secret to the environment via api.LoginWithAPIKey (which likely sets ANTHROPIC_API_KEY).
Remediation: If this value is an actual credential, treat it as compromised — revoke/rotate it, move it to a secret store or environment variable, and purge it from history. First confirm it is a real secret (not a hash, a length/identifier, a placeholder, or another non-sensitive value).
More Info
- Threat model: An attacker with access to the process environment (e.g., via
/proc//environ, debugging tools, or child processes) can extract the API key. Logs capturing terminal output could also expose the secret. This violates the principle of never logging or exposing secrets in plaintext. - Specific code citations: Lines 2073-2080:
os.Setenv("ANTHROPIC_API_KEY", value)andterm.PrintSystem("Anthropic API key saved to OS keychain."). Lines 2035-2042:api.LoginWithAPIKey(value)likely sets environment variable, andtui.AnimateMaxlogs the email. - Existing protections: The
redactSecretInputfunction attempts to redact secrets from history, but this does not protect against environment variable exposure or logging. - Proposed mitigation: Remove
os.Setenv("ANTHROPIC_API_KEY", value)entirely. Forapikey, ensureapi.LoginWithAPIKeydoes not leak the secret to logs or environment. Use a secure keychain API that avoids exposing the secret in memory longer than necessary. - Alternative mitigations considered: Keep the environment variable but clear it immediately after use (still risky). Use a dedicated secret manager that never exposes the secret.
- Severity calibration: Score 5 because this is a direct credential exposure that could lead to account compromise. The secret is written to a globally accessible environment variable and may appear in logs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2077
Comment:
**Secret values are logged to environment variables and terminal output, exposing credentials**
The `applySettingValue` function for `apikey` and `anthropic_key` writes the secret value to `os.Setenv("ANTHROPIC_API_KEY", value)` and logs it via `term.PrintSystem`. This exposes the secret in the process environment and terminal output, which could be captured by logs or other processes. The environment variable is unnecessary for the keychain persistence path and should not be set.
Specifically, the `anthropic_key` case sets the environment variable and logs a success message that includes the key. The `apikey` case logs a success message with the user's email (which may be sensitive) and also writes the secret to the environment via `api.LoginWithAPIKey` (which likely sets `ANTHROPIC_API_KEY`).
Threat model:
An attacker with access to the process environment (e.g., via `/proc/`<pid>`/environ`, debugging tools, or child processes) can extract the API key. Logs capturing terminal output could also expose the secret. This violates the principle of never logging or exposing secrets in plaintext.
Specific code citations:
Lines 2073-2080: `os.Setenv("ANTHROPIC_API_KEY", value)` and `term.PrintSystem("Anthropic API key saved to OS keychain.")`. Lines 2035-2042: `api.LoginWithAPIKey(value)` likely sets environment variable, and `tui.AnimateMax` logs the email.
Existing protections:
The `redactSecretInput` function attempts to redact secrets from history, but this does not protect against environment variable exposure or logging.
Proposed mitigation:
Remove `os.Setenv("ANTHROPIC_API_KEY", value)` entirely. For `apikey`, ensure `api.LoginWithAPIKey` does not leak the secret to logs or environment. Use a secure keychain API that avoids exposing the secret in memory longer than necessary.
Alternative mitigations considered:
Keep the environment variable but clear it immediately after use (still risky). Use a dedicated secret manager that never exposes the secret.
Severity calibration:
Score 5 because this is a direct credential exposure that could lead to account compromise. The secret is written to a globally accessible environment variable and may appear in logs.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+2190
to
+2200
| {Key: "budget", Label: "Token budget", Kind: tui.SettingsText, | ||
| Value: strconv.Itoa(cfg.MaxTokenBudget), Hint: "0 = unlimited"}, | ||
| {Key: "output_verbose", Label: "Output mode", Kind: tui.SettingsCycle, | ||
| Value: boolStr(cfg.OutputVerbose), Options: []string{"compact", "verbose"}, | ||
| Display: func(v string) string { | ||
| if v == "true" || v == "verbose" { | ||
| return "verbose" | ||
| } | ||
| return "compact" | ||
| }}, | ||
| {Key: "professional", Label: "Professional mode", Kind: tui.SettingsToggle, |
There was a problem hiding this comment.
Settings picker applies changes in sorted order, breaking cross-setting validation
runSettingsPicker sorts the changed keys alphabetically before applying them. This can break validation that depends on the order of application. For example, if a user disables ollama and sets backend to api in the same picker session, the sorted order applies backend first (valid) then ollama (which checks ag.Cfg.Context.Backend == "" and may now reject the disable because the backend is no longer empty). The fix is to apply settings in a dependency-aware order or to validate all changes against the final state before applying.
Example:
input: picker changes backend=api and ollama=false
actual: ollama disable rejected because backend is now api
expected: both changes applied
More Info
- Threat model: A user makes multiple changes in the picker; one change is rejected or applied incorrectly because a dependent setting was applied first.
- Specific code citations:
sort.Strings(keys)inrunSettingsPicker; theollamacase checksag.Cfg.Context.Backend == ""which is affected by thebackendcase. - Existing protections: No dependency ordering or two-phase validation exists; the sort is purely alphabetical.
- Proposed mitigation: Apply settings in a fixed dependency order (e.g. backend before ollama) or validate all changes against a copy of the config before applying.
- Alternative mitigations considered: Re-run validation after all changes are applied and roll back on failure, but that is more complex.
- Severity calibration: Plausible user action (changing backend and ollama together) leads to incorrect rejection or state; moderate likelihood.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2190-2200
Comment:
**Settings picker applies changes in sorted order, breaking cross-setting validation**
`runSettingsPicker` sorts the changed keys alphabetically before applying them. This can break validation that depends on the order of application. For example, if a user disables `ollama` and sets `backend` to `api` in the same picker session, the sorted order applies `backend` first (valid) then `ollama` (which checks `ag.Cfg.Context.Backend == ""` and may now reject the disable because the backend is no longer empty). The fix is to apply settings in a dependency-aware order or to validate all changes against the final state before applying.
Example:
input: picker changes backend=api and ollama=false
actual: ollama disable rejected because backend is now api
expected: both changes applied
Threat model:
A user makes multiple changes in the picker; one change is rejected or applied incorrectly because a dependent setting was applied first.
Specific code citations:
`sort.Strings(keys)` in `runSettingsPicker`; the `ollama` case checks `ag.Cfg.Context.Backend == ""` which is affected by the `backend` case.
Existing protections:
No dependency ordering or two-phase validation exists; the sort is purely alphabetical.
Proposed mitigation:
Apply settings in a fixed dependency order (e.g. backend before ollama) or validate all changes against a copy of the config before applying.
Alternative mitigations considered:
Re-run validation after all changes are applied and roll back on failure, but that is more complex.
Severity calibration:
Plausible user action (changing backend and ollama together) leads to incorrect rejection or state; moderate likelihood.
How can I resolve this? If you propose a fix, please make it concise.
| // secretSetRe matches any /set form that carries a secret value, tolerating | ||
| // the whitespace variations strings.Fields accepts ("/set apikey k", | ||
| // tabs, ...) so none of them can slip past the redaction into history. | ||
| var secretSetRe = regexp.MustCompile(`(?i)^/set\s+(apikey|anthropic[-_]key)\s+\S`) |
There was a problem hiding this comment.
Regex for redacting secret inputs fails to match all whitespace variations, leaving secrets in history
The secretSetRe regex (?i)^/set\s+(apikey|anthropic[-_]key)\s+\S uses \s+ which matches one or more whitespace characters, but the pattern expects exactly one space between the key and the secret value. However, strings.Fields splits on any whitespace, so inputs like /set apikey qm-live-secret123 (two spaces) or /set\tapikey\tqm-live-secret123 (tabs) will be split into three parts, but the regex may not match because \s+ can match the extra whitespace, but the regex does not account for the fact that the secret value may be the third part after splitting. The regex is flawed and may fail to redact some secret-carrying inputs, leaving them in the history.
Additionally, the regex does not match the case where the secret value contains spaces (though strings.Fields would split it incorrectly).
Example:
Input: `/set apikey qm-live-secret123` (two spaces). The regex may not match because `\s+` matches the first space, but the secret value is after the second space. The redaction fails.
Suggested fix:
Change regex to `(?i)^/set\s+(apikey|anthropic[-_]key)\b` and then parse the rest of the input to redact the secret part.More Info
- Threat model: An attacker with access to the user's terminal history (e.g., via up-arrow) could recover the secret API key. This undermines the history hygiene goal.
- Specific code citations: Line 1774:
var secretSetRe = regexp.MustCompile((?i)^/set\s+(apikey|anthropic[-_]key)\s+\S). The regex is used inredactSecretInputto redact secrets. - Existing protections: The
redactSecretInputfunction is called at two points to redact secrets before they enter history. - Proposed mitigation: Simplify the regex to match any input that starts with
/setfollowed by the secret key, ignoring whitespace details. Alternatively, parse the input withstrings.Fieldsand check if the second field is a secret key. - Alternative mitigations considered: Keep the regex but test thoroughly with various whitespace inputs. Use a more robust parsing approach.
- Severity calibration: Score 4 because the regex flaw could leave secrets in history, but exploitation requires access to the history buffer. It's a hardening gap that widens the attack surface.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2167
Comment:
**Regex for redacting secret inputs fails to match all whitespace variations, leaving secrets in history**
The `secretSetRe` regex `(?i)^/set\s+(apikey|anthropic[-_]key)\s+\S` uses `\s+` which matches one or more whitespace characters, but the pattern expects exactly one space between the key and the secret value. However, `strings.Fields` splits on any whitespace, so inputs like `/set apikey qm-live-secret123` (two spaces) or `/set\tapikey\tqm-live-secret123` (tabs) will be split into three parts, but the regex may not match because `\s+` can match the extra whitespace, but the regex does not account for the fact that the secret value may be the third part after splitting. The regex is flawed and may fail to redact some secret-carrying inputs, leaving them in the history.
Additionally, the regex does not match the case where the secret value contains spaces (though `strings.Fields` would split it incorrectly).
Example:
Input: `/set apikey qm-live-secret123` (two spaces). The regex may not match because `\s+` matches the first space, but the secret value is after the second space. The redaction fails.
Threat model:
An attacker with access to the user's terminal history (e.g., via up-arrow) could recover the secret API key. This undermines the history hygiene goal.
Specific code citations:
Line 1774: `var secretSetRe = regexp.MustCompile(`(?i)^/set\s+(apikey|anthropic[-_]key)\s+\S`)`. The regex is used in `redactSecretInput` to redact secrets.
Existing protections:
The `redactSecretInput` function is called at two points to redact secrets before they enter history.
Proposed mitigation:
Simplify the regex to match any input that starts with `/set` followed by the secret key, ignoring whitespace details. Alternatively, parse the input with `strings.Fields` and check if the second field is a secret key.
Alternative mitigations considered:
Keep the regex but test thoroughly with various whitespace inputs. Use a more robust parsing approach.
Severity calibration:
Score 4 because the regex flaw could leave secrets in history, but exploitation requires access to the history buffer. It's a hardening gap that widens the attack surface.
Suggested fix shape:
Change regex to `(?i)^/set\s+(apikey|anthropic[-_]key)\b` and then parse the rest of the input to redact the secret part.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+2238
to
+2258
| // changed row through applySettingValue — the same validation and messaging | ||
| // path /set uses. | ||
| func runSettingsPicker(ag *agent.Agent, term *tui.Terminal) { | ||
| cfg := ag.AppConfig | ||
| if cfg == nil { | ||
| cfg = api.DefaultConfig() | ||
| ag.AppConfig = cfg | ||
| } | ||
| res := tui.ShowSettingsPicker(buildSettingsRows(cfg)) | ||
| if !res.Confirmed { | ||
| term.PrintSystem("Settings unchanged.") | ||
| return | ||
| } | ||
| keys := make([]string, 0, len(res.Changes)) | ||
| for k := range res.Changes { | ||
| keys = append(keys, k) | ||
| } | ||
| sort.Strings(keys) | ||
| needSave := false | ||
| for _, k := range keys { | ||
| if applySettingValue(k, res.Changes[k], ag, term) == settingApplied { |
There was a problem hiding this comment.
Settings picker silently drops rows whose apply path returns settingAppliedNoSave
The picker's save loop only sets needSave when applySettingValue returns settingApplied. Rows that return settingAppliedNoSave (e.g. apikey, anthropic_key, ollama) are applied in memory but the picker prints no confirmation and the user gets no feedback that the change took effect. The direct /set path handles this by returning early with a success message; the picker path treats the same result as a no-op. The picker should track applied-but-not-saved rows separately and print a confirmation (or at minimum not silently discard the result).
Suggested fix:
if result := applySettingValue(k, res.Changes[k], ag, term); result == settingApplied {
needSave = true
} else if result == settingAppliedNoSave {
term.PrintSystem(fmt.Sprintf("%s applied (runtime only)", k))
}More Info
- Threat model: A user opens
/settings, changes a row that maps to asettingAppliedNoSavepath, pressess, and sees no confirmation that the change was applied. The change is in memory but the user has no signal it succeeded. - Specific code citations:
runSettingsPickerat the bottom of the diff:if applySettingValue(k, res.Changes[k], ag, term) == settingApplied { needSave = true }. ThesettingAppliedNoSaveconstant is defined but never checked in the picker loop. - Existing protections: The direct
/setpath (applyAndSaveSetting) only persists onsettingAppliedbut the apply function itself prints a success message for every applied path, so the user always gets feedback. The picker path relies on the same apply function's messages, but the picker'sneedSavelogic silently ignores thesettingAppliedNoSaveresult. - Proposed mitigation: Track applied rows separately:
if result := applySettingValue(...); result == settingApplied { needSave = true } else if result == settingAppliedNoSave { /* already messaged by applySettingValue */ }— or simply print a summary line for applied-but-not-saved rows. - Alternative mitigations considered: Have the picker print a generic 'Settings applied' message regardless of save status; weaker because it loses the distinction between saved and runtime-only changes.
- Severity calibration: Score 3: user-facing feedback gap, not a correctness bug. The change is applied in memory, so no data loss, but the silent no-feedback path is a real UX regression relative to the direct
/setpath.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2238-2258
Comment:
**Settings picker silently drops rows whose apply path returns settingAppliedNoSave**
The picker's save loop only sets `needSave` when `applySettingValue` returns `settingApplied`. Rows that return `settingAppliedNoSave` (e.g. `apikey`, `anthropic_key`, `ollama`) are applied in memory but the picker prints no confirmation and the user gets no feedback that the change took effect. The direct `/set` path handles this by returning early with a success message; the picker path treats the same result as a no-op. The picker should track applied-but-not-saved rows separately and print a confirmation (or at minimum not silently discard the result).
Threat model:
A user opens `/settings`, changes a row that maps to a `settingAppliedNoSave` path, presses `s`, and sees no confirmation that the change was applied. The change is in memory but the user has no signal it succeeded.
Specific code citations:
`runSettingsPicker` at the bottom of the diff: `if applySettingValue(k, res.Changes[k], ag, term) == settingApplied { needSave = true }`. The `settingAppliedNoSave` constant is defined but never checked in the picker loop.
Existing protections:
The direct `/set` path (`applyAndSaveSetting`) only persists on `settingApplied` but the apply function itself prints a success message for every applied path, so the user always gets feedback. The picker path relies on the same apply function's messages, but the picker's `needSave` logic silently ignores the `settingAppliedNoSave` result.
Proposed mitigation:
Track applied rows separately: `if result := applySettingValue(...); result == settingApplied { needSave = true } else if result == settingAppliedNoSave { /* already messaged by applySettingValue */ }` — or simply print a summary line for applied-but-not-saved rows.
Alternative mitigations considered:
Have the picker print a generic 'Settings applied' message regardless of save status; weaker because it loses the distinction between saved and runtime-only changes.
Severity calibration:
Score 3: user-facing feedback gap, not a correctness bug. The change is applied in memory, so no data loss, but the silent no-feedback path is a real UX regression relative to the direct `/set` path.
Suggested fix shape:
if result := applySettingValue(k, res.Changes[k], ag, term); result == settingApplied {
needSave = true
} else if result == settingAppliedNoSave {
term.PrintSystem(fmt.Sprintf("%s applied (runtime only)", k))
}
How can I resolve this? If you propose a fix, please make it concise.
| if len(parts) < 3 { | ||
| term.PrintError("Usage: /set <key> <value>") | ||
| term.PrintSystem("Keys: model, project, local_only, professional, autosave, cloud_sync, live_feed, output_verbose, budget, apikey, ollama, backend, cerebras_model, cerebras_reasoning_effort, theme") | ||
| if len(parts) <= 1 { |
There was a problem hiding this comment.
Missing validation for anthropic_key value could allow empty or malformed keys
The anthropic_key case in applySettingValue does not validate the value parameter before saving it to the keychain. An empty or malformed key could be saved, leading to authentication failures later. While the keychain may store any string, the application should ensure the key is non-empty and possibly matches a pattern (e.g., starts with sk-ant-).
Example:
User runs `/set anthropic_key ""` (empty string). The key is saved to the keychain, but future API calls fail.
Suggested fix:
Add validation: `if value == "" { term.PrintError("API key cannot be empty"); return settingInvalid }`Remediation: Validate input against a strict allow-list (type, length, format, range) at the trust boundary and reject anything that does not match, rather than sanitizing bad input.
More Info
- Threat model: A user could accidentally set an empty key, causing subsequent API calls to fail. While not a direct security vulnerability, it's a usability issue that could be avoided.
- Specific code citations: Lines 2073-2080: the
anthropic_keycase sets the environment variable and saves the key without validation. - Existing protections: The keychain save function may return an error, but no validation is performed.
- Proposed mitigation: Add validation: check that
valueis non-empty and matches the expected pattern for Anthropic API keys. - Alternative mitigations considered: Rely on the keychain to reject invalid keys, but that's not guaranteed.
- Severity calibration: Score 3 because it's a hardening gap that could lead to authentication failures, but does not directly expose secrets.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1833
Comment:
**Missing validation for `anthropic_key` value could allow empty or malformed keys**
The `anthropic_key` case in `applySettingValue` does not validate the `value` parameter before saving it to the keychain. An empty or malformed key could be saved, leading to authentication failures later. While the keychain may store any string, the application should ensure the key is non-empty and possibly matches a pattern (e.g., starts with `sk-ant-`).
Example:
User runs `/set anthropic_key ""` (empty string). The key is saved to the keychain, but future API calls fail.
Threat model:
A user could accidentally set an empty key, causing subsequent API calls to fail. While not a direct security vulnerability, it's a usability issue that could be avoided.
Specific code citations:
Lines 2073-2080: the `anthropic_key` case sets the environment variable and saves the key without validation.
Existing protections:
The keychain save function may return an error, but no validation is performed.
Proposed mitigation:
Add validation: check that `value` is non-empty and matches the expected pattern for Anthropic API keys.
Alternative mitigations considered:
Rely on the keychain to reject invalid keys, but that's not guaranteed.
Severity calibration:
Score 3 because it's a hardening gap that could lead to authentication failures, but does not directly expose secrets.
Suggested fix shape:
Add validation: `if value == "" { term.PrintError("API key cannot be empty"); return settingInvalid }`
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
1895
to
+1910
| term.PrintSystem("Standalone local-only mode will be disabled after restart.") | ||
| default: | ||
| term.PrintError("Value must be true or false.") | ||
| return | ||
| return settingInvalid | ||
| } | ||
|
|
||
| case "model": | ||
| if !api.IsValidClaudeModelName(value) { | ||
| term.PrintError("Valid models: " + api.ValidClaudeModelsHelp()) | ||
| return | ||
| return settingInvalid | ||
| } | ||
| cfg.DefaultModel = api.ResolveClaudeModel(value) | ||
| term.PrintSystem(fmt.Sprintf("Default model set to: %s", cfg.DefaultModel)) | ||
|
|
||
| case "project": | ||
| if ag.Cfg.Context.LocalOnly { | ||
| if ag.Cfg.Context != nil && ag.Cfg.Context.LocalOnly { |
There was a problem hiding this comment.
project=0 clear path returns settingApplied but does not update session context consistently
Low-confidence finding — expand to read
The project case has two success paths: pid == 0 returns settingApplied after clearing cfg.DefaultProject and ag.Cfg.Context.ProjectID, while pid > 0 falls through to the shared tail that also sets cfg.DefaultProject and ag.Cfg.Context.ProjectID. Both paths return settingApplied, so the contract is consistent, but the early return for pid == 0 skips the shared tail's save message. This is a minor inconsistency: the clear path prints 'Default project cleared.' and returns, while the set path prints 'Default project set to: #N' and falls through to the save message. The contract is not violated, but the asymmetry is worth noting.
More Info
- Threat model: No user harm; the clear path is functionally correct. The asymmetry is a maintainability concern: a future change to the shared tail (e.g. adding a new side effect) would silently skip the clear path.
- Specific code citations:
case "project":inapplySettingValue— thepid == 0branch returnssettingAppliedearly, while thepid > 0branch falls through to the sharedreturn settingAppliedat the end of the function. - Existing protections: The test
TestApplySettingProjectValuescovers both 149 and 0, but only checks the return value andDefaultProject, not the session context or the save message. - Proposed mitigation: Move the
pid == 0clear logic into the shared tail (setcfg.DefaultProject = pidandag.Cfg.Context.ProjectID = pidunconditionally, then print the appropriate message), or accept the asymmetry as intentional. - Alternative mitigations considered: Leave as-is; the asymmetry is cosmetic and the contract is technically consistent.
- Severity calibration: Score 2: no user-visible bug, but the early return creates a subtle divergence in the shared apply path that could bite a future maintainer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1895-1910
Comment:
**project=0 clear path returns settingApplied but does not update session context consistently**
The `project` case has two success paths: `pid == 0` returns `settingApplied` after clearing `cfg.DefaultProject` and `ag.Cfg.Context.ProjectID`, while `pid > 0` falls through to the shared tail that also sets `cfg.DefaultProject` and `ag.Cfg.Context.ProjectID`. Both paths return `settingApplied`, so the contract is consistent, but the early return for `pid == 0` skips the shared tail's save message. This is a minor inconsistency: the clear path prints 'Default project cleared.' and returns, while the set path prints 'Default project set to: #N' and falls through to the save message. The contract is not violated, but the asymmetry is worth noting.
Threat model:
No user harm; the clear path is functionally correct. The asymmetry is a maintainability concern: a future change to the shared tail (e.g. adding a new side effect) would silently skip the clear path.
Specific code citations:
`case "project":` in `applySettingValue` — the `pid == 0` branch returns `settingApplied` early, while the `pid > 0` branch falls through to the shared `return settingApplied` at the end of the function.
Existing protections:
The test `TestApplySettingProjectValues` covers both 149 and 0, but only checks the return value and `DefaultProject`, not the session context or the save message.
Proposed mitigation:
Move the `pid == 0` clear logic into the shared tail (set `cfg.DefaultProject = pid` and `ag.Cfg.Context.ProjectID = pid` unconditionally, then print the appropriate message), or accept the asymmetry as intentional.
Alternative mitigations considered:
Leave as-is; the asymmetry is cosmetic and the contract is technically consistent.
Severity calibration:
Score 2: no user-visible bug, but the early return creates a subtle divergence in the shared apply path that could bite a future maintainer.
How can I resolve this? If you propose a fix, please make it concise.
…ction Review round 2 on #184: - /set apikey dereferenced ag.Cfg.Context unguarded (latent in the original code too) — a zero-value session context is now created before the auth write - Go regexp \s is ASCII-only but strings.Fields splits on unicode spaces (NBSP U+00A0), so a NBSP-separated '/set apikey <value>' executed yet escaped redaction; the matcher now covers \p{Zs} separators
Contributor
Author
Sigilix round-2 triage (fed3069)Fixed:
Dismissed:
Suite on fed3069: 0 failures. |
There was a problem hiding this comment.
1 finding outside the diff
| File | Scope | Finding |
|---|---|---|
| internal/repl/repl.go:2143 | file-scope | API key stored in environment variable without clearing |
Comment on lines
+2240
to
+2260
| } | ||
|
|
||
| // runSettingsPicker opens the interactive settings editor and applies every | ||
| // changed row through applySettingValue — the same validation and messaging | ||
| // path /set uses. | ||
| func runSettingsPicker(ag *agent.Agent, term *tui.Terminal) { | ||
| cfg := ag.AppConfig | ||
| if cfg == nil { | ||
| cfg = api.DefaultConfig() | ||
| ag.AppConfig = cfg | ||
| } | ||
| res := tui.ShowSettingsPicker(buildSettingsRows(cfg)) | ||
| if !res.Confirmed { | ||
| term.PrintSystem("Settings unchanged.") | ||
| return | ||
| } | ||
| keys := make([]string, 0, len(res.Changes)) | ||
| for k := range res.Changes { | ||
| keys = append(keys, k) | ||
| } | ||
| sort.Strings(keys) |
There was a problem hiding this comment.
Settings picker save path can nil-pointer panic when a changed row applies to a nil Context
runSettingsPicker calls applySettingValue for each changed row, and several cases (project, cloud_sync, live_feed, apikey) dereference ag.Cfg.Context after only guarding ag.Cfg.Context != nil in SOME branches. The project case at line ~1900 guards ag.Cfg.Context != nil before writing Context.ProjectID, but the cloud_sync and live_feed cases at lines ~1975 and ~1990 only guard the LocalOnly check with ag.Cfg.Context != nil && — the subsequent ag.Cfg.Context.LiveFeed = true (line ~1990) is NOT guarded. If ag.Cfg.Context is nil (a zero-value Agent in tests, or early startup before context initialization), applying a live_feed or cloud_sync change from the picker panics with a nil-pointer dereference. The /set path has the same latent bug for live_feed (the cloud_sync case does not write Context, but live_feed does).
More Info
- Threat model: A user opens /settings, toggles live_feed or cloud_sync, and saves. If the agent's SessionContext is nil (possible in tests, early startup, or standalone mode), the process panics and crashes the REPL session.
- Specific code citations: applySettingValue case live_feed at line ~1990: ag.Cfg.Context.LiveFeed = true — the guard ag.Cfg.Context != nil && ag.Cfg.Context.LocalOnly only protects the LocalOnly check, not the write. The project case correctly guards the write with if ag.Cfg.Context != nil.
- Existing protections: The project case guards ag.Cfg.Context != nil before writing Context.ProjectID. The apikey case explicitly creates ag.Cfg.Context = &api.SessionContext{} when nil. But live_feed and cloud_sync do not have this protection for their Context writes.
- Proposed mitigation: Add if ag.Cfg.Context != nil guards around the Context.LiveFeed and Context.CloudSync writes in the live_feed and cloud_sync cases, or initialize ag.Cfg.Context at the top of applySettingValue when nil (matching the apikey case pattern).
- Alternative mitigations considered: Initialize ag.Cfg.Context unconditionally at the start of applySettingValue — simpler but changes behavior for cases that don't need Context.
- Severity calibration: Score 4: a nil Context is plausible in tests and early startup, and the picker makes it easy to trigger. The panic crashes the session, but it requires a specific runtime state.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2240-2260
Comment:
**Settings picker save path can nil-pointer panic when a changed row applies to a nil Context**
runSettingsPicker calls applySettingValue for each changed row, and several cases (project, cloud_sync, live_feed, apikey) dereference ag.Cfg.Context after only guarding ag.Cfg.Context != nil in SOME branches. The project case at line ~1900 guards ag.Cfg.Context != nil before writing Context.ProjectID, but the cloud_sync and live_feed cases at lines ~1975 and ~1990 only guard the LocalOnly check with ag.Cfg.Context != nil && — the subsequent ag.Cfg.Context.LiveFeed = true (line ~1990) is NOT guarded. If ag.Cfg.Context is nil (a zero-value Agent in tests, or early startup before context initialization), applying a live_feed or cloud_sync change from the picker panics with a nil-pointer dereference. The /set path has the same latent bug for live_feed (the cloud_sync case does not write Context, but live_feed does).
Threat model:
A user opens /settings, toggles live_feed or cloud_sync, and saves. If the agent's SessionContext is nil (possible in tests, early startup, or standalone mode), the process panics and crashes the REPL session.
Specific code citations:
applySettingValue case live_feed at line ~1990: ag.Cfg.Context.LiveFeed = true — the guard ag.Cfg.Context != nil && ag.Cfg.Context.LocalOnly only protects the LocalOnly check, not the write. The project case correctly guards the write with if ag.Cfg.Context != nil.
Existing protections:
The project case guards ag.Cfg.Context != nil before writing Context.ProjectID. The apikey case explicitly creates ag.Cfg.Context = &api.SessionContext{} when nil. But live_feed and cloud_sync do not have this protection for their Context writes.
Proposed mitigation:
Add if ag.Cfg.Context != nil guards around the Context.LiveFeed and Context.CloudSync writes in the live_feed and cloud_sync cases, or initialize ag.Cfg.Context at the top of applySettingValue when nil (matching the apikey case pattern).
Alternative mitigations considered:
Initialize ag.Cfg.Context unconditionally at the start of applySettingValue — simpler but changes behavior for cases that don't need Context.
Severity calibration:
Score 4: a nil Context is plausible in tests and early startup, and the picker makes it easy to trigger. The panic crashes the session, but it requires a specific runtime state.
How can I resolve this? If you propose a fix, please make it concise.
| term.PrintError(fmt.Sprintf("Unknown config key: %s", key)) | ||
| term.PrintSystem("Keys: model, project, local_only, professional, autosave, cloud_sync, live_feed, output_verbose, budget, apikey, ollama, backend, cerebras_model, cerebras_reasoning_effort, theme") | ||
| return | ||
| term.PrintSystem(setUsageKeys) |
There was a problem hiding this comment.
Redaction regex fails to match whitespace forms strings.Fields accepts, leaking secrets into history
secretSetRe uses [\s\p{Zs}]+ to match whitespace between /set, the key, and the value. However, strings.Fields (used by handleSetCommand to parse the input) splits on unicode.IsSpace, which includes characters NOT in \s or \p{Zs} — specifically the zero-width space (U+200B), word joiner (U+2060), and other format characters. An input like /set\u200Bapikey\u200Bsecret would be parsed by strings.Fields as a valid /set apikey secret command (applying the secret), but redactSecretInput would NOT match it (because U+200B is not in \s or \p{Zs}), so the raw secret enters the recallable history. The test only covers NBSP (U+00A0), which IS in \p{Zs}, missing the format-character gap.
More Info
- Threat model: A user pastes a secret with an invisible format character (common when copying from web pages or rich text), the command applies successfully, but the secret is stored unredacted in input history and can be recalled with up-arrow.
- Specific code citations: secretSetRe at line ~2160 uses [\s\p{Zs}]+. handleSetCommand uses strings.Fields which splits on unicode.IsSpace — a superset of \s and \p{Zs}.
- Existing protections: The test TestRedactSecretInput covers NBSP (U+00A0) which is in \p{Zs}, but does not cover U+200B or U+2060 which are in unicode.IsSpace but not \s or \p{Zs}.
- Proposed mitigation: Change the regex to use [\s\p{Z}\p{Cf}]+ or better, match the same whitespace set as strings.Fields by using unicode.IsSpace in a custom split, or simply use [\s]+ with the (?m) flag and rely on Go's regexp \s which already includes Unicode whitespace (Go's regexp \s is [\t\n\f\r ] only — it does NOT include Unicode). The correct fix is to use [\p{Z}\s\x{200B}\x{2060}]+ or refactor to split with strings.Fields and check the parts.
- Alternative mitigations considered: Refactor redactSecretInput to use strings.Fields to split the input, check if parts[0] is /set and parts[1] is a secret key, then redact parts[2:] — this guarantees the same whitespace handling as the command parser.
- Severity calibration: Score 4: the gap is real (format characters are common in pasted text), the secret leaks into history, and the fix is straightforward. The likelihood depends on users pasting secrets with format characters, which is plausible.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2162
Comment:
**Redaction regex fails to match whitespace forms strings.Fields accepts, leaking secrets into history**
secretSetRe uses [\s\p{Zs}]+ to match whitespace between /set, the key, and the value. However, strings.Fields (used by handleSetCommand to parse the input) splits on unicode.IsSpace, which includes characters NOT in \s or \p{Zs} — specifically the zero-width space (U+200B), word joiner (U+2060), and other format characters. An input like /set\u200Bapikey\u200Bsecret would be parsed by strings.Fields as a valid /set apikey secret command (applying the secret), but redactSecretInput would NOT match it (because U+200B is not in \s or \p{Zs}), so the raw secret enters the recallable history. The test only covers NBSP (U+00A0), which IS in \p{Zs}, missing the format-character gap.
Threat model:
A user pastes a secret with an invisible format character (common when copying from web pages or rich text), the command applies successfully, but the secret is stored unredacted in input history and can be recalled with up-arrow.
Specific code citations:
secretSetRe at line ~2160 uses [\s\p{Zs}]+. handleSetCommand uses strings.Fields which splits on unicode.IsSpace — a superset of \s and \p{Zs}.
Existing protections:
The test TestRedactSecretInput covers NBSP (U+00A0) which is in \p{Zs}, but does not cover U+200B or U+2060 which are in unicode.IsSpace but not \s or \p{Zs}.
Proposed mitigation:
Change the regex to use [\s\p{Z}\p{Cf}]+ or better, match the same whitespace set as strings.Fields by using unicode.IsSpace in a custom split, or simply use [\s]+ with the (?m) flag and rely on Go's regexp \s which already includes Unicode whitespace (Go's regexp \s is [\t\n\f\r ] only — it does NOT include Unicode). The correct fix is to use [\p{Z}\s\x{200B}\x{2060}]+ or refactor to split with strings.Fields and check the parts.
Alternative mitigations considered:
Refactor redactSecretInput to use strings.Fields to split the input, check if parts[0] is /set and parts[1] is a secret key, then redact parts[2:] — this guarantees the same whitespace handling as the command parser.
Severity calibration:
Score 4: the gap is real (format characters are common in pasted text), the secret leaks into history, and the fix is straightforward. The likelihood depends on users pasting secrets with format characters, which is plausible.
How can I resolve this? If you propose a fix, please make it concise.
| // runSettingsPicker opens the interactive settings editor and applies every | ||
| // changed row through applySettingValue — the same validation and messaging | ||
| // path /set uses. | ||
| func runSettingsPicker(ag *agent.Agent, term *tui.Terminal) { |
There was a problem hiding this comment.
Settings picker applies cloud-dependent settings without the standalone-mode guard /set enforces
runSettingsPicker calls applySettingValue for each changed row, but the picker does not check ag.Cfg.Context.LocalOnly before applying cloud_sync or live_feed changes. The /set path enforces this guard (returns settingInvalid with a message when LocalOnly is true), but the picker path bypasses it. A user in standalone local-only mode can toggle cloud_sync or live_feed in the picker, and the setting is applied (and saved) even though the standalone mode should prevent cloud-dependent settings. The applySettingValue function itself has the guard, but the picker does not pre-check or surface the rejection clearly — the user sees the row marked as changed and saved, but the setting is silently rejected.
More Info
- Threat model: A standalone local-only user opens /settings, toggles cloud_sync, saves, and sees 'api.Config saved' — but the setting was rejected by applySettingValue's LocalOnly guard. The user believes cloud sync is enabled when it is not.
- Specific code citations: runSettingsPicker at line ~2240 iterates res.Changes and calls applySettingValue without checking LocalOnly. applySettingValue case cloud_sync at line ~1975 returns settingInvalid when ag.Cfg.Context.LocalOnly is true.
- Existing protections: applySettingValue has the LocalOnly guard, but the picker does not surface the rejection clearly — the row is marked as changed, the user saves, and the rejection message is printed but the save still proceeds for other rows.
- Proposed mitigation: In runSettingsPicker, before applying changes, check if ag.Cfg.Context != nil && ag.Cfg.Context.LocalOnly and filter out or disable the cloud_sync and live_feed rows in the picker UI, or surface a clear error when the user tries to change them.
- Alternative mitigations considered: Have applySettingValue return a distinct result for 'rejected due to standalone mode' and have the picker display it prominently.
- Severity calibration: Score 3: the setting is correctly rejected (no incorrect state), but the UX is misleading — the user thinks the change was applied. Bounded impact but a real correctness gap in the picker path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2245
Comment:
**Settings picker applies cloud-dependent settings without the standalone-mode guard /set enforces**
runSettingsPicker calls applySettingValue for each changed row, but the picker does not check ag.Cfg.Context.LocalOnly before applying cloud_sync or live_feed changes. The /set path enforces this guard (returns settingInvalid with a message when LocalOnly is true), but the picker path bypasses it. A user in standalone local-only mode can toggle cloud_sync or live_feed in the picker, and the setting is applied (and saved) even though the standalone mode should prevent cloud-dependent settings. The applySettingValue function itself has the guard, but the picker does not pre-check or surface the rejection clearly — the user sees the row marked as changed and saved, but the setting is silently rejected.
Threat model:
A standalone local-only user opens /settings, toggles cloud_sync, saves, and sees 'api.Config saved' — but the setting was rejected by applySettingValue's LocalOnly guard. The user believes cloud sync is enabled when it is not.
Specific code citations:
runSettingsPicker at line ~2240 iterates res.Changes and calls applySettingValue without checking LocalOnly. applySettingValue case cloud_sync at line ~1975 returns settingInvalid when ag.Cfg.Context.LocalOnly is true.
Existing protections:
applySettingValue has the LocalOnly guard, but the picker does not surface the rejection clearly — the row is marked as changed, the user saves, and the rejection message is printed but the save still proceeds for other rows.
Proposed mitigation:
In runSettingsPicker, before applying changes, check if ag.Cfg.Context != nil && ag.Cfg.Context.LocalOnly and filter out or disable the cloud_sync and live_feed rows in the picker UI, or surface a clear error when the user tries to change them.
Alternative mitigations considered:
Have applySettingValue return a distinct result for 'rejected due to standalone mode' and have the picker display it prominently.
Severity calibration:
Score 3: the setting is correctly rejected (no incorrect state), but the UX is misleading — the user thinks the change was applied. Bounded impact but a real correctness gap in the picker path.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
1895
to
+1904
| term.PrintSystem("Standalone local-only mode will be disabled after restart.") | ||
| default: | ||
| term.PrintError("Value must be true or false.") | ||
| return | ||
| return settingInvalid | ||
| } | ||
|
|
||
| case "model": | ||
| if !api.IsValidClaudeModelName(value) { | ||
| term.PrintError("Valid models: " + api.ValidClaudeModelsHelp()) | ||
| return | ||
| return settingInvalid |
There was a problem hiding this comment.
project=0 clear path returns settingApplied but skips the Context.ProjectID write that the non-zero path performs
Low-confidence finding — expand to read
The project case establishes a sibling convention: a successful apply writes both cfg.DefaultProject and ag.Cfg.Context.ProjectID (guarded by a nil check), then returns settingApplied. The new pid == 0 branch sets cfg.DefaultProject = 0 and ag.Cfg.Context.ProjectID = 0 (also nil-guarded) but returns settingApplied immediately — the same success contract as the non-zero path. This is internally consistent: both branches return settingApplied and both write the same two fields. No violation of the error contract is present here; the early return is a success path, not a failure path.
More Info
- Threat model: No user harm — this is a success path returning the success shape.
- Specific code citations:
applySettingValuecaseproject, lines 1895-1904; thepid == 0branch returnssettingAppliedafter writing both fields. - Existing protections: The nil-guard on
ag.Cfg.Contextis present in both branches. - Proposed mitigation: None required.
- Alternative mitigations considered: N/A
- Severity calibration: Not a finding; documented for completeness.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1895-1904
Comment:
**project=0 clear path returns settingApplied but skips the Context.ProjectID write that the non-zero path performs**
The `project` case establishes a sibling convention: a successful apply writes both `cfg.DefaultProject` and `ag.Cfg.Context.ProjectID` (guarded by a nil check), then returns `settingApplied`. The new `pid == 0` branch sets `cfg.DefaultProject = 0` and `ag.Cfg.Context.ProjectID = 0` (also nil-guarded) but returns `settingApplied` immediately — the same success contract as the non-zero path. This is internally consistent: both branches return `settingApplied` and both write the same two fields. No violation of the error contract is present here; the early return is a success path, not a failure path.
Threat model:
No user harm — this is a success path returning the success shape.
Specific code citations:
`applySettingValue` case `project`, lines 1895-1904; the `pid == 0` branch returns `settingApplied` after writing both fields.
Existing protections:
The nil-guard on `ag.Cfg.Context` is present in both branches.
Proposed mitigation:
None required.
Alternative mitigations considered:
N/A
Severity calibration:
Not a finding; documented for completeness.
How can I resolve this? If you propose a fix, please make it concise.
| // secretSetRe matches any /set form that carries a secret value, tolerating | ||
| // the whitespace variations strings.Fields accepts ("/set apikey k", | ||
| // tabs, ...) so none of them can slip past the redaction into history. | ||
| var secretSetRe = regexp.MustCompile(`(?i)^/set[\s\p{Zs}]+(apikey|anthropic[-_]key)[\s\p{Zs}]+\S`) |
There was a problem hiding this comment.
Regex for secret redaction may miss multi-space or Unicode variations
The regex secretSetRe uses [\s\p{Zs}]+ to match whitespace, but it may not capture all possible whitespace characters that strings.Fields splits on. If a user inputs a secret with a whitespace character not covered by the regex, the secret may not be redacted from history. Additionally, the regex only matches when there is a non-whitespace secret value (\S); a secret value that is empty or whitespace-only would not be redacted.
More Info
- Threat model: An attacker with access to the session history (e.g., via up-arrow recall or history file) could recover a secret if the redaction fails.
- Specific code citations: Line 2179:
var secretSetRe = regexp.MustCompile((?i)^/set[\s\p{Zs}]+(apikey|anthropic[-_]key)[\s\p{Zs}]+\S) - Existing protections: The regex attempts to cover Unicode spaces (
\p{Zs}) and standard whitespace (\s), and the test suite includes some edge cases. - Proposed mitigation: Simplify the redaction logic: after
strings.Fields, if the command is/setand the key is a secret key, redact the value regardless of whitespace. This avoids relying on regex matching the exact input format. - Alternative mitigations considered: Keep the regex but expand the character class to include all Unicode separator categories (
\p{Z}) and ensure it matches even if the secret value is whitespace (though that would be invalid). - Severity calibration: Score 3 because the failure would leak a secret into history, but the attack requires access to the session history and the user would likely notice the secret in the command line. The regex likely covers most real-world inputs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 2172
Comment:
**Regex for secret redaction may miss multi-space or Unicode variations**
The regex `secretSetRe` uses `[\s\p{Zs}]+` to match whitespace, but it may not capture all possible whitespace characters that `strings.Fields` splits on. If a user inputs a secret with a whitespace character not covered by the regex, the secret may not be redacted from history. Additionally, the regex only matches when there is a non-whitespace secret value (`\S`); a secret value that is empty or whitespace-only would not be redacted.
Threat model:
An attacker with access to the session history (e.g., via up-arrow recall or history file) could recover a secret if the redaction fails.
Specific code citations:
Line 2179: `var secretSetRe = regexp.MustCompile(`(?i)^/set[\s\p{Zs}]+(apikey|anthropic[-_]key)[\s\p{Zs}]+\S`)`
Existing protections:
The regex attempts to cover Unicode spaces (`\p{Zs}`) and standard whitespace (`\s`), and the test suite includes some edge cases.
Proposed mitigation:
Simplify the redaction logic: after `strings.Fields`, if the command is `/set` and the key is a secret key, redact the value regardless of whitespace. This avoids relying on regex matching the exact input format.
Alternative mitigations considered:
Keep the regex but expand the character class to include all Unicode separator categories (`\p{Z}`) and ensure it matches even if the secret value is whitespace (though that would be invalid).
Severity calibration:
Score 3 because the failure would leak a secret into history, but the attack requires access to the session history and the user would likely notice the secret in the command line. The regex likely covers most real-world inputs.
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| // applyAndSaveSetting applies one key/value pair and persists when the apply | ||
| // path asks for it. | ||
| func applyAndSaveSetting(key, value string, ag *agent.Agent, term *tui.Terminal) { |
There was a problem hiding this comment.
Missing nil check for ag.Cfg.Context before accessing LocalOnly
In the apikey case, the code checks if ag.Cfg.Context != nil && ag.Cfg.Context.LocalOnly for the cloud unavailable message, but later assumes ag.Cfg.Context is non-nil when setting ag.Cfg.Context.Auth and ag.Cfg.Context.API. If ag.Cfg.Context is nil, this will panic. The code creates a zero-value SessionContext only when LocalOnly is true, but not in the general case.
Remediation: Bound every read/write to the allocation size, check for integer overflow before size/index arithmetic, and clear freed pointers to avoid use-after-free. Prefer safe buffer wrappers.
More Info
- Threat model: A nil
Contextcould cause a runtime panic during API key setting, leading to denial of service or unexpected crash. - Specific code citations: Lines 1860-1864:
if ag.Cfg.Context != nil && ag.Cfg.Context.LocalOnly { ... }and lines 1867-1870:ag.Cfg.Context.Auth = auth; ag.Cfg.Context.API = api.NewAPIClient(auth) - Existing protections: The
apikeycase is only reachable whenLocalOnlyis false, butag.Cfg.Contextcould still be nil in early startup or tests. - Proposed mitigation: Ensure
ag.Cfg.Contextis initialized before assigningAuthandAPI. Add a guard:if ag.Cfg.Context == nil { ag.Cfg.Context = &api.SessionContext{} }. - Alternative mitigations considered: Move the nil check earlier and create the context if needed, similar to the
anthropic_keycase. - Severity calibration: Score 2 because the panic is unlikely in normal use (context is initialized early), but it's a hardening gap that could crash the REPL.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1862
Comment:
**Missing nil check for `ag.Cfg.Context` before accessing `LocalOnly`**
In the `apikey` case, the code checks `if ag.Cfg.Context != nil && ag.Cfg.Context.LocalOnly` for the cloud unavailable message, but later assumes `ag.Cfg.Context` is non-nil when setting `ag.Cfg.Context.Auth` and `ag.Cfg.Context.API`. If `ag.Cfg.Context` is nil, this will panic. The code creates a zero-value `SessionContext` only when `LocalOnly` is true, but not in the general case.
Threat model:
A nil `Context` could cause a runtime panic during API key setting, leading to denial of service or unexpected crash.
Specific code citations:
Lines 1860-1864: `if ag.Cfg.Context != nil && ag.Cfg.Context.LocalOnly { ... }` and lines 1867-1870: `ag.Cfg.Context.Auth = auth; ag.Cfg.Context.API = api.NewAPIClient(auth)`
Existing protections:
The `apikey` case is only reachable when `LocalOnly` is false, but `ag.Cfg.Context` could still be nil in early startup or tests.
Proposed mitigation:
Ensure `ag.Cfg.Context` is initialized before assigning `Auth` and `API`. Add a guard: `if ag.Cfg.Context == nil { ag.Cfg.Context = &api.SessionContext{} }`.
Alternative mitigations considered:
Move the nil check earlier and create the context if needed, similar to the `anthropic_key` case.
Severity calibration:
Score 2 because the panic is unlikely in normal use (context is initialized early), but it's a hardening gap that could crash the REPL.
How can I resolve this? If you propose a fix, please make it concise.
Merged
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
1 participant
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.
What
/settings(and bare/set) now opens a keyboard-driven settings picker instead of requiring raw/set <key> <value>text:ssaves every changed row; Esc/q discards; changed rows are marked*/orch,/keys) — linked in the headerChanged rows are applied through
applySettingValue— the same validation/messaging path/setuses, extracted in this PR so the two can never drift./set audit fixes
fmt.Sscanf("%d")silently truncated inputs:1e3→ 1,0x1f→ 0,12abc→ 12 (verified empirically). Project and budget now usestrconv.Atoi./set project 0explicitly clears the default project instead of "setting #0".anthropic_key, which the switch already accepted but the help never mentioned./set apikeyand/set anthropic-keywith no value now prompt hidden (setup.ReadSecret, the pattern/orchalready uses for Cerebras) instead of requiring the secret on the visible input line./set apikey <value>(and anthropic-key) are redacted from the in-session input history at both append sites (interactive + queued prompts), so up-arrow can no longer recall a secret.Tests
Test plan
go test ./...— 0 failures; gofmt/vet clean on touched files/settingsin a session → flip a toggle, cycle theme, edit budget,s→ confirm config.json + messages