Fix length-1 quote trap in shared settings-reader cleaned() helper#1106
Conversation
A near-identical `cleaned(_:)` helper is copy-pasted across 32 cross-platform-core
files (29 provider settings readers, CodexBarConfig, CLIConfigCommand, and
ProviderTokenResolver). Each one strips a single layer of wrapping quotes:
if (value.hasPrefix("\"") && value.hasSuffix("\"")) ||
(value.hasPrefix("'") && value.hasSuffix("'"))
{
value.removeFirst()
value.removeLast()
}
When `value` is exactly one character — a lone `"` or `'` — both `hasPrefix` and
`hasSuffix` return true, `removeFirst()` empties the string, and `removeLast()` then
traps the process with "Can't remove last element from empty collection." This is
reachable from user-controlled inputs:
* env vars (e.g. `ALIBABA_TOKEN_PLAN_COOKIE='"'`, `OLLAMA_API_KEY="'"`),
* the `apiKey`/`cookieHeader` JSON values in `~/.codexbar/config.json`,
* `codexbar config set-api-key --api-key "'"` (or `\"`) on the CLI.
Any of those crashes the menu-bar app (or the CLI) outright the next time the
affected provider is probed — there is no graceful degradation, no error message,
just an illegal-instruction trap.
Reproduced on Ubuntu 24.04 x86_64 with Swift 6.2.4:
swift -e 'var v="\""; v.removeFirst(); v.removeLast()'
# Swift/StringStorage.swift:88: Fatal error: Can't remove last element from empty collection
# Illegal instruction (core dumped)
The fix replaces the two-line removeFirst/removeLast pair with
`value = String(value.dropFirst().dropLast())` in all 32 locations.
`dropFirst()`/`dropLast()` are empty-safe (both return the empty
substring on empty input), so length-0 and length-1 inputs no longer
trap. For length ≥ 2 inputs the new code produces the same result as the
old code; for length-1 lone-quote inputs the new code returns "" instead
of crashing, which the surrounding `.isEmpty` guards then convert to nil.
Regression coverage: new TestsLinux suite `SettingsReaderQuoteUnwrapTrapTests`
exercises two representative public readers (Alibaba Token Plan and Ollama
API key) with lone `"`, lone `'`, and properly-wrapped values, and asserts
the readers return nil for the trap-inducing inputs and unwrap correctly
for the valid ones. 7/7 tests pass on the fixed code; the lone-quote tests
would have trapped (and killed the test process) on master.
swift build -> Build complete!
swift test -> 21/21 tests in 4 suites passed (up from 14)
make check -> 0 new lint violations introduced by this PR
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Codex review: needs real behavior proof before merge. Latest ClawSweeper review: 2026-05-22 12:01 UTC / May 22, 2026, 8:01 AM ET. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. Current source shows the deterministic PR rating Rank-up moves:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. Real behavior proof Risk before merge
Maintainer options:
Next step before merge Security Review detailsBest possible solution: Land the narrow mechanical crash fix with focused regression coverage after the contributor adds real CLI or app behavior proof using a bad env/config value, while keeping any later shared-helper refactor separate. Do we have a high-confidence way to reproduce the issue? Yes. Current source shows the deterministic Is this the best way to solve the issue? Yes. The replacement preserves the old behavior for properly wrapped values and turns the length-1 trap case into an empty value that existing callers already treat as absent; extracting a shared helper would be a follow-up cleanup, not required for this fix. Label changes:
Label justifications:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against fd4878ba669c. |
|
The two Linux CI failures here (build-linux-cli on linux-x64 and They are the pre-existing Linux build break introduced by #1098 (Alibaba I have a separate one-line PR open at #1105 that fixes exactly Verified by reproducing on upstream main 123fe84 with no patches |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
@clawsweeper re-review Maintainer proof for the real CLI path on the patched branch: That exercises the user-facing Additional local proof: The earlier Linux failures are superseded by #1104 / fd4878b. The current rerun for this PR has Linux x64 and Linux arm64 green; macOS |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
2 participants
PROBLEM
The provider settings readers in CodexBarCore (29 of them, plus
CodexBarConfig, CLIConfigCommand, and ProviderTokenResolver -- 32 files
total) share a copy-pasted cleaned() helper that strips one layer of
wrapping quotes:
When value is exactly one character (a lone " or '), both hasPrefix and
hasSuffix return true, removeFirst() empties the string, and removeLast()
then traps the process with:
The trap fires deep inside libswiftCore.so's
RangeReplaceableCollection.removeLast() and is not catchable. The whole
process dies (menu bar app or CLI command). Restarting with the same bad
env var or config value crashes again on next probe.
REACHABILITY
Triggered by these user-controllable inputs:
misquoted shell script, launchd plist, systemd unit, or paste error)
VERIFIED REPRODUCTION
Ubuntu 24.04 x86_64 with Swift 6.2.4, against upstream HEAD 123fe84
(no patches applied). The PR 1098 follow-up fix for FoundationNetworking
was applied first so CodexBarCore could compile, but the cleaned()
helpers were left unpatched. Then a single test:
Output:
After applying this patch, the same test passes in 0.001s.
FIX
Replace the two-line removeFirst() + removeLast() pair in all 32
locations with:
dropFirst() and dropLast() are empty-safe (they return an empty
Substring on empty input). For value.count >= 2 the result is
byte-identical to the old code. For the single trap case
(value.count == 1 with matching quote prefix/suffix), the new code
returns "" which the surrounding .isEmpty guards convert to nil --
strictly better than a process crash.
REGRESSION COVERAGE
New TestsLinux/SettingsReaderQuoteUnwrapTrapTests.swift exercises two
representative public readers (Alibaba Token Plan, Ollama API key) with
lone-quote and properly-quoted inputs across 7 test cases. 7/7 pass on
the fixed code; the lone-quote tests would have killed the test process
before this patch.
swift build -> Build complete!
swift test -> 21 tests in 4 suites passed (was 14 before this PR)
make check -> no new lint violations introduced by this PR
NOTES
The trigger (lone " or ' as a config value) is unusual but well
within "user provides unexpected input" territory. The
disproportion of the consequence (full process crash, repeated on
restart while the bad value persists) justifies the fix even
though the trigger is rare.
I chose dropFirst/dropLast over a length guard or a shared helper
because the diff per file is the smallest possible (2 lines
removed, 1 line added) and the idiom is natively empty-safe.
Happy to rework to a different shape (value.count >= 2 guard, or
extracted CodexBarCore helper) if you prefer either approach.
Scope is 32 files because the pattern is copy-pasted that many
times. Patching only one copy would leave the other 31 still
crashable. A future refactor to a shared CodexBarCore helper
would prevent future copy-paste of the pattern.