security: the network check could not see httpx2 - #8565
Conversation
httpx2 is the pydantic-maintained successor to httpx and a separate import name, so the httpx-only alternative in RE_NETWORK did not match it. openai 3.0.0 requires httpx2 and routes every call through it, which made the SDK's own HTTP invisible to every combined check that needs a network half: secrets plus network, IMDS plus network, archive plus network. The visible symptom was openai/auth/_workload.py reporting as a standalone HIGH "accesses cloud metadata" rather than the combined CRITICAL, because the scanner could not correlate the IMDS URLs with the httpx2.Client() calls beside them. Widening it surfaces four findings, all in openai, all reviewed and benign, all baselined here. Two tests pin the behaviour so the widening cannot be reverted silently.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31f1e5dfe4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "evidence": "Env: L210: api_key = os.environ.get(\"OPENAI_API_KEY\") | L220: admin_api_key = os.environ.get(\"OPENAI_ADMIN_KEY\") | L244: webhook_secret = os.environ.get(\"OPENAI_WEBHOOK_SECRET\") | L816: api_key = os.environ.get(\"OPENAI_API_KEY\") | L826: admin_api_key = os.environ.get(\"OPENAI_ADMIN_KEY\") | L850: webhook_secret = os.environ.get(\"OPENAI_WEBHOOK_SECRET\")\nNetwork: L147: http_client: httpx2.Client | None = None, | L599: http_client: httpx2.Client | None = None, | L753: http_client: httpx2.AsyncClient | None = None, | L1216: http_client: httpx2.AsyncClient | None = None,", | ||
| "evidence_hash": "ee8e7ed79d2bfe99f85f53b1bff3d0f59cfc0de3393ab31a2955ef4365005e3c" |
There was a problem hiding this comment.
If a later or compromised openai artifact preserves these environment reads and annotations but adds an instance request such as client.post(..., data=api_key), the CRITICAL finding retains the same _finding_key and is suppressed by this entry: RE_NETWORK matches httpx2.Client in the annotations but does not match calls through client, so the malicious request contributes no evidence. The Azure and Bedrock entries have the same problem. Baseline only findings backed by executable request evidence, extend the evidence to bind instance requests, or pin these annotation-only suppressions to the reviewed file digest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it was the right thing to stop me on. I reproduced it before fixing it.
Taking the exact shape described, a file that reads a credential and carries httpx2.Client annotations, then appending client.post(..., data=api_key):
benign evidence_hash=8c81a79123c7d3bf
with client.post exfil evidence_hash=8c81a79123c7d3bf
Byte-identical. RE_NETWORK does not match a call through an instance, so the payload contributes no evidence, the key is unchanged, and my entry would have gone on suppressing it. That is the opposite of what _finding_key promises in its own docstring.
Fixed by pinning the reviewed file digest, which is the mechanism _load_baseline already documents for this shape ("for files whose danger sits outside the matched lines"). All four entries are pinned, not just the three annotation-only ones: _workload.py has real httpx2.Client() calls, but an added client.post would leave its evidence unchanged too, so it has the same hole.
Verified both directions against the reviewed wheel:
reviewed artifact: tampered (client.post appended):
suppressed openai/_client.py REOPENS openai/_client.py
suppressed openai/auth/_workload.py REOPENS openai/auth/_workload.py
suppressed openai/lib/azure.py REOPENS openai/lib/azure.py
suppressed openai/lib/bedrock.py REOPENS openai/lib/bedrock.py
Chasing this turned up the same defect already in the file, which I would not have looked for otherwise. There were older unpinned entries for these four paths from when openai used bare httpx., carrying the identical annotation-only evidence. They are inert against openai 3.0.0, but they would suppress on any resolve that goes back to an httpx-based version, payload included. I cannot pin them, because they describe an artifact I have not reviewed, so I dropped them: five entries, four superseded CRITICALs plus the _workload.py HIGH that the combined CRITICAL now supersedes. If that evidence ever recurs the finding reopens and gets a fresh review, which is the behaviour I want.
Net: 224 -> 223 entries, 4 added and digest-pinned, 5 stale removed. A test pins the property so the digest pins cannot be dropped silently; it fails against the unpinned baseline rather than passing vacuously.
RE_NETWORK matches httpx2.Client where it appears in a signature, but not a call through an instance. So a client.post(..., data=api_key) appended to one of these files contributes no evidence: the evidence hash is unchanged and the entry keeps suppressing it. Reproduced, the two hashes are byte-identical. All four entries now pin the reviewed digest, the mechanism _load_baseline already documents for files whose danger sits outside the matched lines. _workload.py is pinned too: it has real httpx2.Client() calls, but an added client.post would leave its evidence unchanged just the same. Five older entries for these paths are dropped. They were baselined when openai used bare httpx and carry the identical annotation-only evidence, inert against 3.0.0 but suppressing on any resolve back to an httpx-based version. They cannot be pinned to an artifact that was never reviewed, so they go and reopen if that evidence recurs.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
unsloth/scripts/scan_packages_baseline.json
Lines 1723 to 1724 in 466d85d
Pin the httpx2 baseline entry to its reviewed digest
When a compromised httpx2/_models.py preserves the existing history ... read and urllib.request evidence but appends an instance request such as client.post(..., data=secret), this unpinned entry still suppresses the CRITICAL finding because RE_NETWORK does not match calls through client. I reproduced this against the cached httpx2/_models.py: the appended request changed the file digest but left _finding_key unchanged. Add the reviewed file_sha256 here, as done for the new OpenAI entries, so edits outside the matched evidence reopen the finding.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
for more information, see https://pre-commit.ci
|
@codex review |
1 similar comment
|
@codex review |
|
Following the P1 above further than it was raised, because the same shape is already in the file beyond the entries this PR touches. I audited every baseline entry whose evidence carries a
The three An import. A I have deliberately not fixed these here. Pinning 13 entries across 10 packages means every bump of botocore, torch, urllib3 or numpy reopens a finding and needs re-triage, which is a policy call about maintenance cost rather than a bug fix, and it does not belong in a PR whose subject is one regex alternative. This PR stays scoped to the four entries it introduces. Recording it so the audit is not lost. Happy to do it as a follow-up. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ies (#9148) * Security audit: pin openai, and re-review the four digest-pinned entries The extras shard of the security audit has been red on main since 2026-08-17 19:14Z with four non-baselined CRITICALs, all in `openai`: Harvests environment variables/secrets AND makes network calls openai/_client.py, openai/lib/azure.py, openai/lib/bedrock.py Accesses cloud metadata/IMDS AND makes network calls openai/auth/_workload.py It is not a code change here. The last green run was 18:23Z and every run after 18:40Z failed on the same four findings, on main and on unrelated PR branches alike, which is upstream drift rather than anything in the tree. `openai>=2.7.2` is the only floating spec in extras.txt that carries digest-pinned baseline entries. openai 3.2.0 published at 19:14Z and changed all four files. Confirmed by digest: the four `file_sha256` values in the baseline match openai 3.0.0 and 3.1.0 exactly and none of 3.2.0's. The pin is deliberate and correct (#8104, #8565): the evidence for these entries records that a network call exists but not where it goes, so `client.post(..., data=api_key)` appended to one of these files would leave the evidence hash untouched. Only the file digest can reopen that, so any edit upstream makes must red the gate until someone re-reads it. Which is the review, done here. Every destination in the four files at 3.2.0 is first-party or a documented cloud metadata endpoint: api.openai.com, auth.openai.com, 169.254.169.254 (Azure IMDS), metadata.google.internal, management.azure.com, and bedrock-mantle.{region}.api.aws. The one instance-level POST is the workload-identity token exchange, which defaults to https://auth.openai.com/oauth/token and sets follow_redirects=False. The env reads are OPENAI_API_KEY, OPENAI_ADMIN_KEY, OPENAI_WEBHOOK_SECRET, AZURE_OPENAI_API_KEY, AZURE_OPENAI_AD_TOKEN and AWS_BEARER_TOKEN_BEDROCK, each used to authenticate to its own service. Same benign patterns as the reviewed 3.0.0, so the four entries are re-pinned to 3.2.0's digests and nothing else in the 214-entry baseline is touched. Re-baselining alone would only buy time until the next release, so the spec is pinned too. Every other requirement in extras.txt is already an exact pin (#8408 pinned the bare ones and skipped this one because it had a specifier). Pinning makes the bump deliberate: whoever raises the version is the one who re-runs --write-baseline and re-reads the diff, instead of the gate going red on release day for a change nobody here made. The new test asserts that invariant directly, since it is the part that will be got wrong again: a package with digest-pinned baseline entries must not float in studio/backend/requirements. Reverting the spec to `openai>=2.7.2` fails it and names extras.txt:62. Verified: - `scan_packages.py 'openai==3.2.0'` exits 0; on 3.1.0 it exits 1, so the pin still bites and the entries were not widened. - The whole extras shard (`--with-deps -r extras.txt`, 128 archives) exits 0 with 0 active CRITICAL/HIGH, against 4 CRITICAL and exit 1 before. - tests/security/test_scan_packages.py: 121 passed. Unrelated, and not a defect: the `openai-whisper==20250625` line in the same log is an INFO, not the failure. That version is on PyPI as an sdist only, and the bulk resolve runs `--only-binary :all:` so it never executes a setup.py, which is the scanner's whole security model. "from versions: none" is what pip says about an sdist-only package under that flag, and the per-spec fallback plus the direct sdist fetch that follow are the designed path working. The scipy `assert_raises(ValueError, CubicSpline, ...)` evidence quoted alongside it is a MEDIUM and cannot fail the gate; it is a false positive on `bc1`, a boundary condition variable in scipy's own test file that looks like a bech32 address. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the openai pin off Python 3.9, which the first version of it broke `openai==3.2.0` on its own was a regression, caught simulating the install rather than reading it. openai 3.x is `requires-python >=3.10` and this project is `>=3.9,<3.15`, so an exact pin resolves to nothing at all on 3.9, where `>=2.7.2` had quietly been selecting 2.48.0. Fixing a CI gate by breaking an install is not a fix. Split on the marker instead, which is what the rest of this file already does for soundfile and tabulate. 2.48.0 is the newest release that accepts 3.9, so the 3.9 branch pins exactly what 3.9 was resolving to before. Resolved with uv against every interpreter in requires-python, --no-deps, three ways (main, the bad pin, this): spec 3.9 3.10 3.12 3.13 3.14 >=2.7.2 2.48.0 3.2.0 3.2.0 3.2.0 3.2.0 ==3.2.0 NO SOLUTION 3.2.0 3.2.0 3.2.0 3.2.0 split 2.48.0 3.2.0 3.2.0 3.2.0 3.2.0 So this is behaviour-preserving: identical to main on every supported Python, and the whole of extras.txt resolves to the same 31 packages on 3.10, 3.12, 3.13 and 3.14 before and after, differing only in the `# via -r` filename. 3.9 fails on both sides for an unrelated and pre-existing reason (scikit-learn==1.7.1 is itself >=3.10), so nothing here changes that either. The security audit is unaffected: it runs on 3.12, markers are evaluated by pip, and the scan of the two-line spec fetches only 3.2.0 and exits 0. The 3.9 branch is never scanned, exactly as the floating spec's 3.9 resolution never was. The guard is widened to the second half of the same invariant: the exact pins for a digest-pinned package must cover every supported Python between them, so a marker partition with a hole in it fails. Its limit is now written down rather than implied. It is marker-only and offline, so it catches a `>= "3.11"` beside a `< "3.10"`, but it cannot catch a single unmarked pin whose release does not support 3.9, because that means asking PyPI for the release's requires-python. The resolution simulation above is what covers that case, and the test module stays network-free. Verified: reverting to `openai>=2.7.2` still fails the guard naming extras.txt. tests/security/test_scan_packages.py: 121 passed. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Widen the pin guard to the pyproject inputs, and make `==` mean one version Two review findings, both real, both reproduced before fixing. The guard read `studio/backend/requirements/*.txt` and stopped there, but security-audit.yml builds `audit-reqs/unsloth-deps.txt` out of pyproject's `project.dependencies` plus the `huggingfacenotorch` extra and feeds that to the hf-stack shard. A digest-pinned package declared there was invisible to the guard, which is the whole failure mode it exists to catch, one file over. It now reads both halves, from one helper that mirrors the workflow's own input transform. Reading pyproject turns up the counterexample the finding named: unsloth_zoo is digest-pinned (hf_xet_health.py, the credential send that motivated #8104) and declared `unsloth_zoo>=2026.8.12`. That one is deliberately not version-pinned and must not be. The recurrence being prevented is an upstream release WE DO NOT CONTROL changing the bytes and reddening main on a day nobody touched the repo; unsloth_zoo is ours, released in lockstep, and an exact pin would break that. When its digest reopens, the change is one of ours and re-reviewing it is the point of the pin. So it is named in an exemption rather than quietly skipped, and the exemption is itself asserted: it must contain only names that really are digest pinned, and at least one third-party package must remain, so the guard cannot be defused by growing the list. That assertion earned its keep immediately, failing on `unsloth` until it was removed for having no pinned entry. Second, `==` has to mean one version. The old prefix regex accepted `openai==3.*`, which pip resolves as a prefix match to whatever 3.x is newest, recreating the exact release-day failure. The specifier is now parsed with packaging.requirements rather than pattern-matched: exactly one clause, operator `==`, version not ending in `.*`. Parsing also drops the hand-rolled name and marker splitting, so extras and whitespace stop being edge cases. Four negative controls, each red with its own message and green after: - `openai>=2.7.2` in extras.txt: not pinned to one version, names extras.txt:68. - `openai==3.*`: same assertion, names the wildcard spec. - `< "3.9"` instead of `< "3.10"`: openai uncovered on 3.9. - `openai>=2.7.2` added to pyproject's dependencies: names pyproject.toml, which is the finding above and was green before this commit. tests/security/test_scan_packages.py: 121 passed. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Read pyproject through tomli when tomllib is not there pyproject sets requires-python >=3.9 and testpaths ["tests/security"], so a bare pytest from the repo root collects this module on 3.9 and 3.10, where tomllib does not exist (it landed in 3.11, PEP 680). The two helpers added here imported it unguarded, so on those interpreters the file raised ModuleNotFoundError instead of reporting a result. tomli is already pinned for python_version < 3.11 in extras-no-deps.txt, and four other suites in this repo already use exactly this fallback. The regression test supplies the backport rather than requiring it. The tests-security job installs only pytest and PyYAML, so a test that leaned on a real tomli being importable would importorskip its way to green there and never execute the branch at all. Registering the parser this interpreter does have under the name the fallback looks for keeps it load-bearing everywhere, while import tomllib is made to fail for the duration so the fallback is provably what gets consulted. Checked red on the unfixed helpers and green on the fixed ones, including with tomli hidden. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
1 participant
RE_NETWORKmatchedhttpx.but nothttpx2..httpx2is not a submodule ofhttpx, it is the pydantic-maintained successor under a separate import name.openai3.0.0 requireshttpx2<3,>=2.7.0and routes every call through it, so the entire OpenAI SDK's HTTP surface was invisible to the scanner.That matters because several checks are deliberately combined: they only fire when a file does something sensitive and makes a network call. With the network half unmatched,
secrets + network,IMDS + networkandarchive + networkcould not fire on any package using httpx2.The visible symptom was already in the logs.
openai/auth/_workload.pyreported as a standalone HIGH "accesses cloud metadata / IMDS endpoints" instead of the combined CRITICAL, because the scanner could not correlate the IMDS URLs with thehttpx2.Client()calls immediately beside them:The change
Explicit alternation rather than
httpx2?so the intent reads at a glance. It stays anchored:myhttpx.get,httpx23.getandhttpx2.Timeout(5)do not match, and all three are asserted.What it surfaces
Four findings, all in
openai3.0.0, all benign, all baselined here. The package archive was already confirmed byte-identical to the upstreamopenai/openai-pythonv3.0.0 tag when its earlier findings were reviewed, so this is a triage of new checks firing on known-good code, not a new package.openai/_client.pyOPENAI_API_KEY,OPENAI_ADMIN_KEY,OPENAI_WEBHOOK_SECREThttpx2.URL/httpx2.Clientannotationsopenai/lib/azure.pyAZURE_OPENAI_API_KEY,AZURE_OPENAI_AD_TOKENhttpx2.Client/AsyncClientannotationsopenai/lib/bedrock.pyAWS_BEARER_TOKEN_BEDROCKhttpx2.URLannotationsopenai/auth/_workload.pyhttpx2.Client().get(...)callsWorth being precise about the first three: their network half is entirely type annotations, not calls. The combined check fires on a file that reads a credential and mentions
httpx2.Clientin a signature. That is the check working as designed on a coarse signal, not evidence of anything.The fourth is the real one, and I read it rather than assuming. Both providers fetch a subject token from the instance metadata service and
returnit to the caller for exchange attoken_exchange_url; the only network destinations in the file are the two metadata endpoints and that exchange. This is documented workload identity federation.Verification
Baseline edit is purely additive: the 224 existing entries are byte-identical and in order, 4 appended,
_commentandversionuntouched. Entries were produced by loadingscan_packages.pyand calling its ownscan_archiveand_finding_key, not by--write-baseline, which would have rewritten the whole file from one shard's run.audit-reqs/was rebuilt exactly as the workflow's "Build filtered requirements set" step does and is byte-identical to the artifacts CI produced.The studio shard exits 1 on my machine for a reason that cannot occur in CI: this host is Python 3.13 and CI pins 3.12, and
multiprocess0.70.19 ships a different wheel per minor version. I downloaded thepy312wheel and ran the scanner against it: all three of its CRITICAL findings are already baselined, so CI's shard has nothing non-baselined. I did not baseline the py313 variants, since CI never fetches that archive and adding them would be unreviewed suppression.pytest tests/security/test_scan_packages.py: 113 passed, including the two added here. Those two were mutation-checked against the old regex, where all three httpx2 spellings go undetected, so they fail without the fix rather than passing vacuously.Not covered
Widening this one pattern does not make the network check complete. Still unmatched, and each would need its own triage of whatever it surfaces:
aiohttp.ClientSession,urllib3.PoolManager/urllib3.request,websockets.connect,pycurl,grpc.insecure_channel,niquests,paramiko,ftplib,smtplib.Of those,
urllib3andwebsocketsare in the declared dependency set.urllib3is partly covered in practice because its usual call shape isconn.urlopen(...), which theurlopen\s*\(alternative already matches. I have not measured the others, so I am not claiming they are or are not a problem, only that they are not in scope here.