fix(egress): gate the forward proxy host behind a dedicated [egress].allowed_proxy list (BACKLOG #1659)

[wshallwshall]

BACKLOG #1659 — the forward proxy host is a credential-bearing egress target the allow-list never checks. P1.

What was wrong

The ADR 0126 forward/egress web proxy is a credential-bearing egress host that no allow-list checked. check_egress_allowed's HTTP arm reads dest.settings["url"] and then the credential token URLs in _CREDENTIAL_EGRESS_URL_KEYS, which holds only smart_token_url and oauth2_token_url.

Under the default proxy_auth_type = basic the connector mints a pre-emptive Proxy-Authorization: Basic ... (transports/rest.py, proxy_auth_handler_from_settings), and ADR 0126 records that urllib moves it into the CONNECT tunnel for an https destination, so it reaches the proxy on both destination schemes.

Severity is conditional, per CLAUDE.md section 0: nothing is deployed. An un-listed proxy would receive that credential on a site's first delivery, and for a permitted cleartext http destination the PHI request would follow it. proxy_url is operator configuration, never attacker input.

What was built, and what was deliberately not

Not built: adding proxy_url to _CREDENTIAL_EGRESS_URL_KEYS. The ledger row's own closing step 1 asks for exactly that, and it is the mechanism ADR 0126 forbids in terms:

[egress].allowed_http gate scope — the proxy host is OUT of scope. The fail-closed allowlist gates the destination host (where PHI is sent), which is unchanged. The forward proxy is an operator-chosen transport intermediary, not a PHI destination; gating it against allowed_http would be wrong (one corporate proxy fronts many hosts, and it would have to be co-listed with every destination).

ADR 0126 is Accepted, dated 2026-07-17, with no amendment section and nothing superseding it; docs/adr/README.md restates the scope line. Verified at origin/main before building. A note now sits at _CREDENTIAL_EGRESS_URL_KEYS naming ADR 0126 as the reason the key stays out, and a test pins it, so the next pass does not re-file this.

Built instead: a dedicated [egress].allowed_proxy list. It answers the ADR's stated objection rather than evading it — listing the proxy once permits it for every destination, so there is no co-listing — and it leaves the ADR's scope sentence literally true. It is also the house pattern: ADR 0135 gave the AI broker [ai].allowed_endpoints for the same reason, that allowed_http is permissive-when-empty and cannot be the gate for a credential-bearing egress surface.

• EgressSettings.allowed_proxy, host / host:port matching like every other [egress] list, env MEFOR_EGRESS_ALLOWED_PROXY.
• Checked on both arms — check_egress_allowed (outbound REST/SOAP/FHIR/DICOMweb) and check_fhir_lookup_allowed (the fhir_lookup read) — outside each one's allowed_http guard, because an empty allowed_http says nothing about whether the proxy is permitted. That lockstep is the DELTA-04 lesson.
• The "default" sentinel is skipped.

Deny semantics, and why the two lists differ

Deny-by-default, following [ai].allowed_endpoints rather than allowed_http's permissive-when-empty: an explicit proxy_url with an empty allowed_proxy is refused at config load.

The asymmetry is deliberate and is documented at the field, at the helper, in docs/CONFIGURATION.md and in the ADR amendment. The reasoning:

1. An empty allowed_proxy refuses nothing until a proxy is actually configured, so an operator who uses no proxy is unaffected. That is unlike allowed_http, where deny-when-empty would refuse every HTTP destination on a stock config.
2. Permissive-when-empty would leave the credential-bearing host ungated on exactly the default posture — which is the hole this key exists to close. ADR 0135 made the same call for the same reason.
3. Section 0: zero deployments, so a breaking default costs nothing today.

Why the "default" sentinel is exempt

proxy_config_from_settings already refuses to combine proxy_url = "default" with explicit proxy credentials, so that path mints no Proxy-Authorization — the credential-bearing case is exactly the explicit-URL case. It also names no address at config time; urllib resolves the OS proxy per request, so there is no host for a config-load gate to match. A residual gap is recorded in the ADR amendment: a system proxy chosen by the OS environment still sees the CONNECT target or a cleartext body, and no config-time list can name it.

Corrections to two prose claims this change would otherwise falsify

• The false universal was in a code comment, not docs/SECURITY.md. The row's closing step 2 named SECURITY.md; its two allowed_http mentions (around :1343 and :2253) are both specific and correct, so neither was touched. The universal actually lived at wiring_runner.py's _CREDENTIAL_EGRESS_URL_KEYS header, which said every such key "must ride the same [egress].allowed_http allowlist or it is a fail-open credential-exfiltration hole". That is now scoped to destination-class hosts, with the proxy exception and its ADR-0126 reason beside it.
• "EIGHT allowed_* lists" in __main__.py's open-egress gate and in test_security_config.py's docstring now read "EIGHT allowed_* DESTINATION lists", naming allowed_proxy as a ninth allowed_* key that is not one of them, is not in _allowlist_for, and must never satisfy that gate.

The recorded risk was already retired

The row warned the default sentinel must be skipped without breaking the runner's copy-in of [egress].proxy_url. Confirmed rather than assumed: _apply_egress_proxy_default runs inside _dest_config ahead of the gate, and at wiring_runner.py:7170 ahead of check_fhir_lookup_allowed, so the merged value is already in dest.settings when the gate reads it. test_egress_default_proxy_is_gated_by_allowed_proxy asserts that through the real _dest_config. No reordering needed.

Tests

Five new tests fail without the fix, confirmed by neutralizing the two _check_forward_proxy_egress call sites and re-running (5 failed, 36 passed; restored byte-identical afterwards):

• test_outbound_denies_unlisted_proxy_host
• test_outbound_permits_listed_proxy_host
• test_proxy_gate_is_deny_by_default_with_an_empty_list
• test_proxy_gate_covers_the_fhir_lookup_read_arm
• test_egress_default_proxy_is_gated_by_allowed_proxy

Three deliberately do NOT fail without the change, and each says so in its own docstring rather than being presented as red-first: the pin that proxy_url stays out of _CREDENTIAL_EGRESS_URL_KEYS (a guard against a future edit — the one the ledger row asks for in writing), the "default" carve-out (it asserts a non-raise), and the pin that a listed proxy does not satisfy the destination gate.

Checks run

• ruff format --check and ruff check over the tree: clean.
• mypy messagefoundry (strict): 13 errors in 3 files, all pre-existing and all missing-optional-dependency noise in parsing/dicom/_deps.py, parsing/fhir/_deps.py and transports/dicom.py. Zero in any file this change touches.
• pytest, targeted census: 2456 passed, 94 skipped, plus a follow-up run of 167 more. Enumeration method: every test module matching any symbol this change touches (EgressSettings, check_egress_allowed, check_fhir_lookup_allowed, _allowlist_for, _apply_egress_proxy_default, _dest_config, _CREDENTIAL_EGRESS_URL_KEYS, PROXY_DEFAULT, proxy_url, allowed_http) or referencing the docs edited, unioned — 58 files — then a second pass over the modules mentioning EgressSettings or egress that the first missed, plus the doc-drift and ADR-index gates.
• Extras installed to match CI (dev,harness,fhir,dicom,x12,xml,vault), so the collection-gated modules actually ran rather than removing themselves.

Skipped: the full suite. It does not finish inside one turn on this box.

Hosted legs to read: windows-service-smoke (NSSM, never visible locally), the ide leg, webconsole, and the Linux matrix legs. Nothing here touches the service, the IDE or the console, so no failure is expected from them.

Collisions

messagefoundry/pipeline/wiring_runner.py and messagefoundry/config/settings.py are also edited by PR 1212, in the [inbound] streaming region. This change stays in the [egress] region — _allowlist_for, _CREDENTIAL_EGRESS_URL_KEYS, check_fhir_lookup_allowed, check_egress_allowed — and did not need to touch 1212's region. The doc edits are in their own last commit so they can be resolved separately.

One unrelated working-tree edit to scripts/hooks/context-budget.ps1 was present in this worktree and is not part of this change; it was left unstaged rather than discarded, since an uncommitted edit is not recoverable.

Open questions

1. proxy_no_proxy does not exempt the gate. A destination on the bypass list gets no proxy handler and no credential, so in principle a fully-bypassed connection needs no listed proxy. The gate ignores that: it asks whether the configured proxy address is permitted, not whether a particular target happens to bypass it. Simpler, fail-closed, and the bypass is evaluated per target host at construction — including token-endpoint hosts, which can differ from the destination — so a per-connection "is everything bypassed?" test would be fragile. Flagging it in case the reviewer wants the carve-out.
2. The "default" sentinel's residual gap is recorded in the ADR amendment rather than closed. Closing it would mean resolving getproxies() at config load and gating what it returns, which makes a config-load refusal depend on process environment at that moment. Not built; say if it should be filed.

[wshallwshall]

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 best thing in this PR is that it found the invariant its own change would have falsified

__main__.py carried "[egress] declares EIGHT allowed_ lists and every one is enforced downstream
by _allowlist_for"*. That was true on main -- I counted EgressSettings and _allowlist_for
and got eight on both sides. allowed_proxy is genuinely the ninth key that silently breaks it, and
the builder caught that and amended the gate and the test docstring to say EIGHT DESTINATION
lists.

A counted invariant in a comment is exactly what a new key breaks with nothing reporting it.

Deny-by-default is real in code, not only in prose

if not proxy_url or proxy_url.lower() == PROXY_DEFAULT:
    return
if not allowed_proxy:
    raise WiringError(... "[egress].allowed_proxy is empty -- list the proxy host to permit it" ...)

Empty list plus a configured proxy refuses. Contrast allowed_http on main, where an empty list makes
the branch fall through entirely -- permissive-when-empty, as claimed. Following ADR 0135's
allowed_endpoints rather than allowed_http is the right precedent and the ADR says so in the same
words.

The ordering that the whole gate depends on also holds: every check_egress_allowed call site runs
_dest_config(...) immediately before, and _dest_config applies the egress proxy default before
returning. So a site-wide [egress].proxy_url cannot slip the gate. That was the risk most worth
checking.

The one thing I would ask for, and it is the PR's own lesson applied one screen away

docs/CONFIGURATION.md:972 reads:

Only SIX of the eight lists satisfy the open-egress gate.

This PR edits that file to add an allowed_proxy row to the table directly beneath that warning,
and does not touch the warning. A reader now counts nine allowed_* rows under a sentence that
says eight. That is the same ambiguity the PR just fixed in two code sites, left unfixed in the file
it was already editing.

Three further spots in docs/DEPLOYMENT.md enumerate "all eight", including a "Setting | Confines"
table that is the operator's egress checklist and now omits the only deny-by-default key. An
operator following that runbook and configuring a proxy meets a startup refusal the runbook never
mentions. CHANGELOG.md is historical and correct as written -- leave it.

No CI gate catches any of these. No test pins the egress table or the eight-count, and none
enumerates EgressSettings.model_fields. So this drift is invisible to the suite and only review
will catch it.

What I am doing about it

Not blocking. The code is correct, it is a security improvement, and holding a sound deny-by-default
gate on a documentation counter would be the wrong trade. But the CONFIGURATION.md line is a
one-line fix in a file already in this diff, and I would rather it land here than become the next
stale count someone measures.

If you push it, I will re-read and land. If you would rather keep the diff tight, say so and I will
land as-is and file the doc drift separately.

Verdict

Verdict: merge.

[github-actions]

CI failed while this pull request was in the merge queue, so the queue ejected it.

Its own head can still be green: the queue revalidates the merge, and the path gates that skip on a pull request run there. Read the run before retrying.

https://github.com/MEFORORG/MessageFoundry/actions/runs/35166188124