Skip to content

fix(credentials): a malformed credential archive is a 400 and an unstorable secret value is a named 422 (#3325) - #3417

Merged
vybe merged 2 commits into
devfrom
feature/3325-credential-error-types
Oct 9, 2026
Merged

vybe merged 2 commits into
devfrom
feature/3325-credential-error-types

Conversation

@vybe

@vybe vybe commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #3325 — independent fix, one of ten small bug fixes in the trinity-pm chain-easy-1008 run. Base is dev. Draft until the operator merges.

What

  • src/backend/services/credential_encryption.py::decrypt: every malformed envelope shape raises ValueError, as the function promises — JSON that is not an object ([], null), and a non-string nonce or ciphertext. Importing such a .credentials.enc now returns 400 instead of a 500 with a stack trace in the log. The envelope format and every successful decrypt are unchanged.
  • src/backend/services/secret_settings.py::encrypt_secret_setting: the key is checked first, in its own try, and only that check can raise MissingEncryptionKeyError. A value that cannot be encoded (for example a lone surrogate) raises the new SecretSettingValueError(ValueError). Its message names the setting, never the value.
  • src/backend/error_handlers.py + src/backend/main.py: one app-level handler maps SecretSettingValueError to 422. src/backend/routers/settings/credentials.py: three pass-through lines in the routes whose catch-all would have turned it into a 500 (Anthropic, GitHub PAT, PUT /slack).
  • Tests: the three strict-xfail markers in test_ec_credential_crypto_edges.py are removed; new tests/unit/test_3325_credential_error_types.py (registered in tests/registry.json) asserts no value in the message or the exception chain, a missing key reported before a bad value, PUT /api/settings/api-keys/anthropic → 422, and that main.py registers the handler.

Rulings carried (orchestrator, on the operator's behalf — plan file)

  • TD-1: one app-level handler plus the three pass-through lines — as recommended.
  • TD-2: a .credentials.enc that decrypts correctly but whose contents are not an object is deferred — as recommended; forging one needs the platform key.
  • TD-3: 422, matching the existing 422 for refused cleartext writes — as recommended.

Review + security

/review reading pass (claude-fable-5-1, report-only): MERGEABLE, no critical findings. The secret cannot leave through the new error: it is raised after the except block, so __context__ is cleared, not merely hidden; the handler returns only the message; nothing logs it; audit rows are written only after a successful write. The only residual carrier is traceback frame locals, which nothing in the backend renders. The handler is keyed on the subclass, so no other ValueError is swallowed or relabelled. /cso --diff: no findings.

Evidence sweep (claude-opus-5-5): GREEN. 23 unit files import the touched modules; the 16 most direct ran shuffled (seed 12345), one file per process, all passed. All 7 tests in the new file ran, 0 skipped. With both service files taken from dev, exactly the three formerly-xfail tests fail (8 cases). Seven less direct files are left to CI and named in the review file. git merge-tree against dev f6bedcd is clean.

Tests

Targeted counts above. Not run here: the full unit island (CI).

Before merge

Handoffs (not in this diff)

  • TD-2 above: the decrypted-but-not-an-object case can still 500 on import.
  • PUT /api/settings/slack saves the client ID before a later secret write fails (partial update, pre-existing).
  • The GitHub PAT, Resend and Gemini routes are covered by the app-level handler but not exercised by the new route test.

🤖 Generated with Claude Code

Trinity Agent (trinity) and others added 2 commits October 8, 2026 19:35
…ope shape (#3325)

CredentialEncryptionService.decrypt documents ValueError for a bad format,
but a non-object JSON document ("[]", "null", "1", '"x"') hit data.get() and
raised AttributeError, and a non-string nonce/ciphertext made b64decode raise
TypeError outside the (KeyError, ValueError) arm. .credentials.enc is
agent-writable, so POST /api/agents/{n}/credentials/import answered 500 with a
stack trace where its ValueError arm promises 400.

Reject a non-object envelope up front and widen the base64 arm to TypeError.
Any input that decrypts today decrypts identically.

Removes the two strict xfails (M50a-f, M50g); they went red with the
markers removed and the fix absent (7 failed), green with it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… "add the key" (#3325)

encrypt_secret_setting wrapped the whole encrypt() call in `except
ValueError` -> MissingEncryptionKeyError. A lone UTF-16 surrogate in the value
makes the UTF-8 encode raise UnicodeEncodeError (a ValueError), so a configured
install was told to add CREDENTIAL_ENCRYPTION_KEY and the settings routes
answered 500.

- secret_settings: probe the key on its own (the only key-error source), then
  encrypt; an UnicodeEncodeError becomes the new SecretSettingValueError
  (ValueError subclass, .key). It is raised outside the except block and
  `from None`, so neither __cause__ nor __context__ holds the
  UnicodeEncodeError, whose .object is the whole secret. The message names
  the key, never the value.
- error_handlers + main.py: one app-level handler maps it to 422 {"detail"}.
- routers/settings/credentials.py: pass-through arms in the three routes whose
  `except Exception` would turn it into a 500 (Anthropic key, GitHub PAT,
  PUT /slack). The routes without a try reach the handler directly.

Missing/invalid key is unchanged: still MissingEncryptionKeyError, and now
reported before any value check.

Tests: removes the M58 strict xfail; new tests/unit/test_3325_credential_error_types.py
(value absent from message, args, chain and rendered traceback; key-first
order; anthropic / slack / slack-connect -> value-free 422 with no log line
carrying it; main.py registers the handler). Mutation: with the anthropic
pass-through arm and the main.py registration reverted,
test_route_answers_a_value_free_422[anthropic] and
test_main_registers_the_handler went red; restored byte-identical.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@trinity-ability trinity-ability left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge-train: batch validated on train/20261009-0735 (#3424)

@vybe
vybe merged commit fd4bd50 into dev Oct 9, 2026
29 of 30 checks passed
vybe added a commit that referenced this pull request Oct 9, 2026
…L is refused, not stored (#3323) (#3420)

Fixes #3323 — independent fix, one of ten small bug fixes in the trinity-pm `chain-easy-1008` run. Base is `dev`. **Draft until the operator merges.**

## What
- `src/backend/utils/url_validation.py::reject_embedded_credentials`: strips its input and treats a leading `//` as already having a host, the same rule `strip_url_credentials` uses. `//<token>@github.com/o/r` is now refused like the `https://` form, including with leading whitespace or mixed case. All three write paths that store a skills-library URL go through this one function (`routers/skills.py` create and update, legacy adoption in `services/skill_service.py`), so the token is no longer saved in plaintext to `skill_sources.url` or the audit details.
- If `urlparse` refuses an input that starts with `//`, the function falls back to the module's existing userinfo regex instead of raising, so this fix adds no new 500.
- Tests: the D3 strict-xfail marker is removed; four more refused shapes and a four-case "must not raise / must not over-reach" test are added.

## Rulings carried (orchestrator, on the operator's behalf — plan file)
- TD-1: the fallback applies only to inputs starting with `//`; the bare `ValueError` for every other malformed input is #3322's and is unchanged (D4 stays a strict xfail) — as recommended.
- TD-2: keep the username/password check; `//@host` carries no secret and is not refused — as recommended.
- TD-3, TD-4: deferred, see Handoffs — as recommended.

## Review + security
`/review` reading pass (claude-fable-5-1, report-only): **MERGEABLE**, zero critical, five informational. The pre-fix and post-fix modules were executed side by side over forty inputs: nothing that was refused before now passes, credential-free inputs are unchanged, the fallback regex is anchored and linear. `/cso --diff`: no findings.

Fixed after review:
- `546f329a` — the `tests/registry.json` descriptions no longer call #3323 a strict xfail.
- `a75a5e04` — the reject stub in `test_ent183_skill_packages.py` mirrors the fixed `//` rule. It is deliberately stricter than production on a malformed bracket (`//[oops/x` raises).

Evidence sweep after the fix (claude-opus-5-5): **GREEN**. 28 unit files import the module; the 14 most direct ran shuffled (seed 12345), one file per process. Three did not come back clean, none because of this branch:
- `test_ent236_skills_lifecycle.py::TestNonRepoDirectoryRecovers::test_real_repo_still_pulls` and `test_skill_service_user_agent.py` (collection error) fail the same way on `dev` f6bedcd in the engineer's environment.
- `test_ent237_skill_sources.py` hit the 240 s per-file limit at about 72 % with no failure printed — left to CI.

`git merge-tree` against `dev` f6bedcd is clean.

## Tests
895 passed across the 11 files that completed cleanly. Not run here: the 14 less direct importer files, `test_ent237_skill_sources.py` to completion, and the full unit island (CI).

## Before merge
- Ten open PRs share `tests/registry.json` with this branch (#3417, #3412, #3411, #3407, #3304, #3271, #3256, #2984, #2956, #2709). A test-merge of this head against each adds no conflict beyond what that PR already has with `dev`.
- One stale docstring remains at `tests/unit/test_ec_url_validation_properties.py:36` (still calls D3 a strict xfail).

## Handoffs (not in this diff)
- TD-3 (P3): shapes that still pass both reject and strip — `/\t/tok@`, `/\n/tok@`, `///tok@`, `https:/tok@` (`urlparse` removes tab and newline after the check). Fixing it needs the same change in `strip_url_credentials` and cases in both test families.
- TD-4: a property test that reject and strip always agree over the shared `test_2052` corpus; it needs exceptions for empty userinfo.
- #3322 (bare `ValueError` / `UnicodeError` → 500) is the sibling issue in the same file and is untouched.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
vybe added a commit that referenced this pull request Oct 9, 2026
…t the database (#3385) (#3421)

Fixes #3385 — one of ten small bug fixes in the trinity-pm `chain-easy-1008` run. **Stacked on #3414** (the fix for #3314): base is `feature/3314-queue-fingerprint-falsy`, because both issues edit `_clamp_ingested_item` and the fingerprint helpers. Merge #3414 first, then retarget this to `dev`. **Draft until the operator merges.**

## What
- `src/backend/services/operator_queue_service.py`: new `OPERATOR_QUEUE_TYPE_MAX = 64` and `_bounded_type`. An agent-written `type` longer than 64 characters is shortened with the existing `…[truncated]` marker at ingest (`_clamp_ingested_item`); the ask is never refused or lost. `_comparable_type` applies the same bound, so a shortened row does not read as a rewrite (#2915) and a row stored before this fix at full length still compares equal.
- `src/backend/db/operator_queue.py::_insert_values`: `_DB_BELT_TYPE_MAX_BYTES = 1024`; above it the insert raises `ValueError`, like the id / title / question / context belts. All three create paths go through it.
- Docs: five lines in `architecture/api-endpoints.md`, `feature-flows/operating-room.md` and `requirements/security.md` that list the clamp and belt fields now name `type`.
- Tests in `tests/unit/test_1632_operator_queue_caps.py`: an oversized type is shortened with the marker; every platform-emitted type and every type `ask_operator` accepts is unchanged; the belt rejects an oversized type and passes 64 four-byte characters; `test_3385_clamped_type_is_not_a_rewrite` (clamped row vs raw entry, legacy full-length row, a real rewrite inside the first 52 characters).

## Rulings carried (orchestrator, on the operator's behalf — plan file)
- T1: a fixed constant, no environment variable — as recommended.
- T2: 64 characters — as recommended. The longest type the platform emits is `workspace_problem_report`, 24.
- T3: the DB belt raises, like its siblings — as recommended.
- Orchestrator: stacked on #3314 instead of branching from `dev`, so the two fixes do not hand the operator a conflict in the same two functions.

## Review + security
`/review` reading pass (claude-fable-5-1, report-only, against the stacked base): **MERGEABLE**, zero critical. Census of every literal `type` the platform and the enterprise submodule pass to the insert paths: longest is 24 characters, none is altered. Every truncated value ends in the marker, which no set member can match, so truncation cannot move a value into or out of a set that decides control flow (`_BUDGETED_ALERT_TYPES` and neighbours). The new `ValueError` is reachable only from the file sync loop, which quarantines it under its existing handler. `/cso --diff`: no findings; the error text carries the constant, never the value.

Fixed after review: `eab91c10` — I1, the five doc lines. Noted, not changed: I2, a non-string `type` over 1 KiB now raises the named `ValueError` where the base failed at the bind — same quarantine outcome.

Evidence sweep after the fix (claude-opus-5-5): **GREEN**. 13 operator-queue files ran shuffled (seed 12345), one file per process: 836 passed, 7 xfailed. With both production files taken from the #3314 branch, the two `TestClampType` cases, the belt test and the clamped-row fingerprint case go red. `git merge-tree` against `dev` f6bedcd is clean; the base branch has not moved (`923b1edc`).

## Tests
Counts above. Not run here: `test_ent815_queue_walk`, `test_ent815_broad_list_agent_scope`, `test_ent751_gate_entries` (2–4 minutes each; they ran green on the #3314 base in that PR's sweep) and the full unit island.

## Before merge
- **This PR's checks are weaker than a dev-based PR's:** a PR whose base is a feature branch runs a reduced CI set and `backend-unit-test` does not run. The first full run happens when it retargets to `dev` after #3414 merges — read that run before merging.
- Accepted trade-off: an agent that rewrites an oversized type only after character 52 is not detected as a change. Titles and questions already behave this way.
- Thirteen open PRs share a file with this branch (mostly `tests/registry.json` and `docs/memory/architecture/api-endpoints.md`; #3256 also `db/operator_queue.py` and the three docs). A test-merge of this head against each of #3420, #3417, #3412, #3411, #3407, #3304, #3271, #3256, #2984, #2956, #2713 and #2709 adds no conflict beyond what that PR already has with `dev`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants