Skip to content

fix(security): raise content-free refusals outside the except block, so __context__ cannot carry the payload - #1209

Open
wshallwshall wants to merge 1 commit into
mainfrom
claude/encode-wire-body-context-residual
Open

wshallwshall wants to merge 1 commit into
mainfrom
claude/encode-wire-body-context-residual

Conversation

@wshallwshall

Copy link
Copy Markdown
Collaborator

from None is not a redaction tool, and five refusals were relying on it

raise ... from None clears __cause__ and sets __suppress_context__. It leaves
__context__ populated.
The flag only tells the default traceback printer to stop walking
the chain. It does not detach the exception.

So anything that reads the chain by attribute rather than formatting it the default way still
reaches the suppressed exception: a structured-logging serializer, a crash reporter, a debugger,
a custom formatter, or a bare exc.__context__.object.

That sentence is the most useful thing in this PR. The next person reaching for from None to
keep a payload out of an error needs to find it.

The measurement

Run against encode_wire_body at origin/main, Python 3.14, synthetic HL7 only:

__cause__ is None                : True
__suppress_context__             : True
__context__ type                 : UnicodeEncodeError
__context__.object IS the payload: True

UnicodeEncodeError.object is the string the encode failed on, which here is the entire wire
body
. The function's docstring already named the hazard and raised from None precisely to
avoid it. The intent was right; the mechanism did not deliver it.

One nuance worth recording, because it shapes how the test is written: the payload does not
appear in traceback.format_exception, even with compact=False. TracebackException honours
__suppress_context__ when it walks. A test that asserted only on the default rendering would
have passed against this bug.
The reachable surface is attribute access, not the default
printer.

The fix

CPython populates __context__ only when the raise happens while an exception is being
handled
. Hoisting the raise out of the handler -- keeping only the index, or the status code,
in a local -- leaves both chains empty.

try:
    return payload.encode(encoding)
except UnicodeEncodeError as exc:
    offending_position = exc.start   # the INDEX only; `exc` dies with the handler
raise NegativeAckError(...)          # outside the handler: __context__ stays empty

The sweep, characterised

I grepped the package for from None and for __suppress_context__, then read every hit. They
sort into two groups, and the discriminator is what the suppressed exception carries, not how
the raise is written.

Group one -- the suppressed exception carries a payload or a secret. All fixed. Five sites,
each with a comment or docstring already stating that withholding is the point:

Site What __context__ was holding
transports/base.py encode_wire_body UnicodeEncodeError.object -- the entire wire body
transports/soap.py body-secret encode check UnicodeEncodeError.object -- here the string IS the credential
transports/signing.py _load_private_key the cryptography deserialization error the docstring says must not echo key material
parsing/fhir/resource.py FhirResource.parse a pydantic ValidationError, which carries the offending input values (PHI)
auth/oidc/flow.py exchange_code an HTTPError -- a readable response object whose body may echo this POST's request params, client secret among them

Group two -- the suppressed exception carries nothing sensitive. Left alone, deliberately.
These use from None for message tidiness: the raised error already says everything useful, and
the chained one would just add noise. The shape is a lookup or coercion miss over a closed,
operator-authored vocabulary -- an unknown enum name, a KeyError on a code-set or reference-set
lookup, an unknown framing preset, a queue.Empty from a sandbox timeout, a malformed CLI
anchor, a connector-registry miss. The api/ HTTPException translations are a third variant of
the same thing: their detail is str(exc) of an engine error that is deliberately being
surfaced, so there is nothing being withheld for the flag to fail to withhold.

I did not touch group two. Changing it would be the general redaction refactor this was scoped
away from, and it would add churn with no security content.

Left for others, not taken

  • messagefoundry/logging_setup.py -- its RedactionFilter docstring says it scrubs the
    formatted traceback "chained __cause__/__context__ included". It renders via
    logging.Formatter().formatException, i.e. the default printer, which honours
    __suppress_context__ -- so it never reached these residuals and is not itself defective.
    Another builder owns this file; I did not open it beyond reading. Flagging it only because
    the docstring's phrasing invites a reader to assume the filter covers an attribute walk. It
    does not, and after this PR there is nothing on those chains for it to cover.

Tests

The existing guard in tests/test_encode_wire_body.py read:

assert exc.__context__ is None or exc.__suppress_context__

A disjunction the flag alone satisfies. It passed against this bug for its entire life. It now
asserts __context__ is None outright, plus a _walk_chain helper that follows
__cause__/__context__ ignoring __suppress_context__ on purpose -- because that
indifference is what makes it an instrument for this defect rather than a restatement of the
default printer's behaviour.

Confirmed failing without the source change. I stashed only messagefoundry/, leaving the
new tests in place, and ran them against pristine origin/main source:

11 failed, 127 passed, 1 skipped

The failures span all five modules: test_encode_wire_body.py (8),
test_soap_body_secrets.py (1), test_outbound_signing.py (1), test_auth_oidc.py (1).
test_fhir_resource.py was the 1 skipped in that run because the [fhir] extra was not yet
installed locally; I installed it and re-ran, and it passes with the fix (see below). Its
assertion is therefore verified locally now, and it runs in CI because the test leg installs
.[dev,console,fhir].

Checks run

Check Result
ruff format --check . pass (1306 files)
ruff check . pass
mypy messagefoundry (strict) zero errors in all five changed files
targeted census, 37 modules 1306 passed, 104 skipped, 2 failed
pre-commit (12 hooks incl. leak guard, ledger gate, bandit) all pass

On mypy: the run reports 16 errors total, all in auth/webauthn.py, transports/dicom.py and
parsing/dicom/_deps.py, all import-not-found for optional extras absent on this box
(webauthn, pynetdicom). None are in files this PR touches. The 16 also serve as the positive
control that the instrument fires, so "none in my files" is a real negative rather than a broken
query.

The 2 failures are pre-existing and not mine. Both are
test_tls_trust_anchor.py::test_hvac_clients_take_a_ca_only_when_one_is_configured, from the
[vault] extra being absent locally. I ran the control: stashing all my changes and running
that module against pristine origin/main reproduces the same two failures identically.

Census method, stated so it can be checked: I enumerated the modules by grepping tests/ for
every changed symbol and its neighbours -- encode_wire_body, SoapDestination, MessageSigner,
signer_from_destination, FhirResource, exchange_code, ConnectorType.SOAP,
ConnectorType.FHIR, transports.signing, auth.oidc -- which returned 37 test modules, and ran
all of them. The full local suite was not run; it does not finish on this box.

Hosted legs a reviewer must read

I cannot see these; my process exits when this PR opens.

  1. The test leg on Linux and Windows -- it installs .[dev,console,fhir] and [vault], so it
    is the first place the FHIR assertion and the two Vault failures above are exercised under
    their real extras.
  2. mypy under CI's full extra set -- the 16 local import-not-found errors should not appear
    there, which also confirms none of mine were masked behind them.
  3. Any leg that exercises auth/oidc/flow.py end to end against a real IdP stub.

Open questions

  1. No ledger item, by instruction. The dispatching Manager confirmed the ledger lives in the
    maintainer-internal repository and could not allocate a number, so this PR deliberately cites
    no BACKLOG #N rather than inventing one that would resolve to unrelated work later. It needs
    a number allocated and this subject filed against it.
  2. Is parsing/fhir/resource.py clear of PR 1205? That PR owns transports/fhir.py. I read
    these as different files in different packages and proceeded. If 1205 also moved the parsing
    one, this will conflict and I did not see it.
  3. Is auth/oidc/flow.py clear of the BACKLOG fix(ci): an executable file is CODE wherever it lives, including under docs/ (#1200) #299 limb A work? That limb owns
    auth/oidc_http.py, a sibling file. Same reasoning, same caveat.
  4. Should a lint forbid from None where the handler binds a payload-bearing exception? This
    defect family is invisible to review because the code reads as if it already handles the
    hazard -- the comment says so. A check that flags from None inside an except UnicodeEncodeError/ValidationError handler would catch the next one. Out of scope here;
    naming it rather than building it.

Deployment posture

MessageFoundry has zero running instances, so nothing is exposed today and no present-tense
impact claim would be true. Stated conditionally: on a first deployment, a chain-walking log
handler or crash reporter would reach the full message body
from a refusal specifically written
to withhold it. Every fixture, commit message and sentence here uses synthetic values.

…so __context__ cannot carry the payload

`raise ... from None` is NOT a redaction tool. It clears `__cause__` and sets
`__suppress_context__`, but it LEAVES `__context__` populated. The flag only tells
the default traceback printer to stop walking the chain; it does not detach the
exception. Anything that reads the chain by attribute rather than formatting it
the default way -- a structured-logging serializer, a crash reporter, a debugger,
a custom formatter, or a bare `exc.__context__.object` -- still reaches it.

Measured on the pre-fix `encode_wire_body`, Python 3.14, synthetic HL7: `__cause__`
is None and `__suppress_context__` is True, yet `exc.__context__` is the
`UnicodeEncodeError` and `exc.__context__.object` is byte-identical to the whole
payload. The docstring's intent was right; the mechanism did not deliver it.

CPython populates `__context__` only when the raise happens while an exception is
being handled, so hoisting the raise out of the handler -- keeping only the index
or the status code in a local -- leaves BOTH chains empty.

Five sites, all of them `from None` used deliberately to keep a payload or a
secret out of an error:

  transports/base.py  encode_wire_body -- UnicodeEncodeError.object IS the wire body
  transports/soap.py  body-secret encode check -- .object IS the credential
  transports/signing.py  private-key load -- withheld deserialization detail
  parsing/fhir/resource.py  pydantic ValidationError carries the PHI input values
  auth/oidc/flow.py  token-endpoint HTTPError, a readable response object

`from None` used merely for tidiness is left alone -- an unknown enum name, a
KeyError on a code-set lookup, a queue.Empty. Those suppress nothing sensitive.

The existing guard read `assert exc.__context__ is None or exc.__suppress_context__`,
a disjunction the flag alone satisfies, so it passed against this bug for its whole
life. It now asserts `__context__ is None` outright and walks the chain by attribute,
ignoring the flag on purpose. Confirmed failing at origin/main: 11 assertions across
five test modules go red without the source change.

No deployment is affected -- MessageFoundry has zero running instances. On a first
deployment, a chain-walking log handler would reach the full body.
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

LANDER INSPECTION -- labelled self-review, not a peer review

Posted under the korus LANDER.md line 364 obligation. I did not author this change.

The claim is true, and it was verified by execution rather than by reading

--- raise ... from None ---
__cause__ is None            : True
__suppress_context__         : True
__context__ is None          : False     <-- the claim
__context__.object IS secret : True
secret in default traceback  : False     <-- the printer is blind to it
attribute walk reaches       : ['NegativeAckError', 'UnicodeEncodeError']

from None clears __cause__ and sets a flag the default printer honours. The payload stays one
attribute access away. Five shipped sites used that as a redaction tool.

The 2x2 cross-run is what settles it, and it is the right experiment

OLD code x OLD guard : PASS      <-- why it shipped
OLD code x NEW guard : FAIL      <-- the instrument fires
NEW code x OLD guard : PASS
NEW code x NEW guard : PASS

The old assertion was assert exc.__context__ is None or exc.__suppress_context__a disjunction
the flag alone satisfies
, so the right operand was unconditionally true on the defective code and
the left was never evaluated against reality. The replacement fails on the old code, which is the
only question that matters about a replacement guard.

This is the cleanest demonstration I have read tonight that a new test is a real instrument rather
than a restatement.
Four cells, not one.

All five sites fixed, and the obvious trap avoided

Every sentinel uses is not None rather than truthiness, so an offending character at position 0
still raises. That was the trap and it is not present. The fix removes reachability__context__ is None on the new code — rather than suppressing rendering.

No control-flow change: no site has an enclosing try in the same function, so the set of enclosing
handlers is identical before and after.

One correction to the PR's framing

The discovery is not new, and it is already written down on main. transports/rest.py's
_latin1_refusal_position documents this exact mechanism, ending:

"Measured: the shipped body-path guard encode_wire_body uses from None inside its except
and its __context__.object is still the whole payload."

and tests/test_rest_transport.py already asserts __context__ is None with a mutation note. So
rest.py is a sixth site that was already correct, and it is the precedent this PR generalises.
That corroborates the mechanism independently — but the body reads as a fresh finding and should
cite the prior art.

Residual worth filing separately

transports/http_auth.py:307 and transports/smart.py:301 are the closest siblings to the fixed
OIDC site and use from exc — chaining the response object on __cause__ deliberately, where
the printer will render it. Same hazard class, opposite policy, and invisible to a from None
grep
. Out of scope here.

Verdict

Verdict: merge.

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.

1 participant