fix(api): propagate HTTP errors as typed envelopes + token safety + path escapes#22
Merged
Merged
Conversation
…ath escapes
Closes the Critical issue surfaced in the post-merge review: every print*
helper checked transport errors but not response status, so 4xx/5xx
response bodies flowed through as Envelope.Data with no Envelope.Error
set. Agents parsing the envelope couldn't distinguish a 404 body from
the resource they asked for. The internal/errors.ClassifyHTTPError
machinery existed for exactly this but was wired into nothing.
Adds a central respond() helper in cmd/sure-cli/root/respond.go that:
- Classifies transport failures via errs.ClassifyNetworkError.
- Classifies any r.StatusCode() >= 400 via errs.ClassifyHTTPError.
- Always attaches {status, body} in error details (with the body
capped at 1 KiB to keep large 422 payloads from spamming output and
to bound the security review's concern about echoing user-submitted
data verbatim in 4xx bodies).
- Renders the success payload via output.Print on the success path.
Also adds checkResponse() for call sites that do typed processing on
the body (status, transactions windowing) and need only the
fail-on-error half.
Migrates every print* helper across reference_cmds, investment_helpers,
financial_cmds, family_exports, imports, users to use respond. Inline
GET/POST sites in accounts, transactions list/show/create/update/delete,
sync, whoami, plan, status, propose, export, and imports preflight all
migrate too — meaning every API response in the CLI now lands in the
correct envelope half.
Other High-priority fixes from the review:
- **Token rotation safety** (internal/api/client.go:74-106,
login_cmd.go:45-73, refresh_cmd.go:22-39): guard every refresh-token
/ access-token persistence behind an explicit empty-string check.
Previously a partial server response (rotation disabled, future API
change, 4xx body decoded into TokenResponse) would silently overwrite
the saved refresh token with "" and log the user out. ensureFreshToken
also now treats >=400 from /api/v1/auth/refresh as a hard failure
rather than persisting the bogus empty TokenResponse.
- **3 missing url.PathEscape sites** (transactions_update_cmd.go:39,
transactions_delete_cmd.go:25, propose_cmd.go:77): every sibling
endpoint URL-escapes the id; these three did not. Brings them in
line so an id with /, ?, #, +, : or whitespace can't break the
request shape.
- **tools/smoke-oauth.sh:37 broken/unsafe**: invoked a --password flag
that does not exist on login_cmd, so the script was either silently
broken or, if anyone added the flag, would have leaked the password
via argv/ps/shell history. Pipes the password to stdin instead.
login_cmd now falls back to a plain stdin read when stdin is not a
TTY (so the piped path works), keeping term.ReadPassword for the
interactive case.
- **export_cmd --format flag collision** (export_cmd.go:25,70): local
--format (csv|json) shadowed the persistent --format (json|table)
from root. Renamed local flag to --out-format. Breaking change for
pre-0.2 users; documented in README. This is a 0.x release.
Cleanup:
- **--quiet and --trace flags removed** (root.go:11-15,38-42): both
were declared on the persistent flag set but read by nothing. Dead
flags lie to users. Implementing --trace correctly requires a header
redactor for Authorization / X-Api-Key (called out in the security
review) — left as a future task with the proper plumbing.
- **Missing return after output.Fail** at every site touched (login,
refresh, plan, transactions, etc.). Currently safe because
output.Fail os.Exits, but a defense-in-depth fix for the day anyone
refactors Fail to be testable / non-exiting.
Tests:
- respond_test.go covers (a) the classifier contract for every
HTTP status the CLI handles (401/403/404/422/429/5xx), (b)
mergeErrorDetails always attaches the status + truncated body, and
(c) classifier-provided details are preserved. Uses httptest +
the established viper-wiring pattern from internal/api/client_test.go.
Avoids exercising output.Fail's os.Exit path; the success path is
already covered by dispatch_write_test.go's stdout-capture tests.
go test ./... -race is green across the suite.
Refs the post-merge code review.
superagent-security
Bot
added
the
contributor:verified
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (29)
📝 WalkthroughWalkthroughThis PR centralizes HTTP error handling across the CLI via a new ChangesCLI Error Handling and Authentication Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Closes the Critical bug surfaced in the post-merge review: every `print*` helper checked transport errors but not response status, so 4xx/5xx response bodies flowed through as `Envelope.Data` with no `Envelope.Error` set. Agents parsing the envelope couldn't distinguish a 404 from a real resource. The `internal/errors.ClassifyHTTPError` machinery existed for exactly this but was wired into nothing.
Critical
1. Status-code blindness — fixed across every print helper.* New `respond()` helper in `cmd/sure-cli/root/respond.go`:
Companion `checkResponse()` for sites that do typed processing on the body (status, transactions windowing).
Migrated every print* helper (reference, investment, financial, family_exports, imports, users) and every inline GET/POST site (accounts, transactions list/show/create/update/delete, sync, whoami, plan, status, propose, export, imports preflight) — meaning every API response in the CLI now lands in the correct envelope half.
High
2/3. Token rotation safety. `client.go:74-106`, `login_cmd.go:45-73`, `refresh_cmd.go:22-39`: guard every refresh-token / access-token persistence behind an empty-string check. A partial server response would previously overwrite the saved refresh token with `""` and silently log the user out. `ensureFreshToken` also treats `>=400` from `/api/v1/auth/refresh` as a hard failure instead of persisting an empty `TokenResponse`.
4. Three missing `url.PathEscape` at `transactions_update_cmd.go:39`, `transactions_delete_cmd.go:25`, `propose_cmd.go:77` — every sibling uses it; brought these in line.
5. `tools/smoke-oauth.sh` broken/unsafe. Invoked a `--password` flag that didn't exist on `login_cmd`. Pipes the password to stdin now; `login_cmd` falls back to a plain read when stdin isn't a TTY (interactive case still uses `term.ReadPassword`).
6. `export_cmd` `--format` collision. Local `--format (csv|json)` shadowed the persistent `--format (json|table)`. Renamed local to `--out-format`. Breaking change for pre-0.2 users; README updated. This is a 0.x release.
Cleanup
Tests
`respond_test.go` covers:
Uses httptest + the viper-wiring pattern already established in `internal/api/client_test.go`. The success path was already covered end-to-end by `dispatch_write_test.go`'s stdout-capture tests.
`go test ./... -race` green across the full suite.
Diff scope
29 files changed, +345/-250. Most of the delta is replacing 8-line `if err != nil { Fail }; if err := Print; Fail(...)` repetitions with a single `respond(r, err, res)`.
Refs the post-merge code review.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
--formatto--out-formatto prevent flag collisions.Chores
--quietand--traceflags.