Run the price importers with least privilege and refuse implausible responses - #541
Run the price importers with least privilege and refuse implausible responses#541dsfaccini wants to merge 4 commits into
Conversation
Closes #538. The helicone importer was the only code-execution vector in the repo. pull.sh cloned Helicone/helicone at unpinned main, and main.ts *imports* cost/providers/mappings.ts - so third-party TypeScript executed locally, under deno run --allow-write with no path scope. That is an arbitrary-write primitive as the invoking user, and arbitrary write becomes code execution through a shell rc file or a git hook. No CI job runs any *-get target, so the exposure was a maintainer's laptop rather than a runner. Two changes close it, verified by probing the real config: inside scope (../source_prices): ALLOWED home dir: blocked (NotCapable) repo root: blocked (NotCapable) git hooks dir: blocked (NotCapable) - Scope --allow-write to ../source_prices. main.ts writes exactly one path, so this costs nothing. Deno already denies net/run/read/env, so with write scoped a compromised upstream can only put bad numbers in a gitignored JSON file - the same trust level as every other source, already gated by review. - Pin the clone to a commit SHA and fetch over HTTPS rather than handing it an authenticated SSH agent. Bumping the pin is now a reviewable commit instead of something that happens silently on every run. Data-integrity guards, for the four importers that overwrite tracked provider YAML - which is what the published artifacts are built from, gated only by someone reading the diff: - New source_guard module. check_non_empty refuses an import that produced no models; check_no_sharp_drop refuses a rewrite that would delete more than half of a provider's recorded models. GENAI_PRICES_ALLOW_IMPLAUSIBLE_IMPORT=1 overrides when a shrink is genuine. - Wired into write_source_prices, which is the chokepoint for every importer that writes a JSON source file, and into the huggingface and ovhcloud paths that write provider YAML directly. Also, from the same review: - ovhcloud swallowed every exception into a silent return, so a failed fetch looked exactly like a successful no-op. It raises now. - huggingface called .json()['data'] with no raise_for_status, turning an error page into a KeyError. - The four importers with no HTTP timeout now have one.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ers before writing any All three findings were valid, and the first two share a root cause: the guards counted the raw response instead of what parsing actually produced, so the one failure mode they exist for - an upstream shape change - would have walked straight past them. A large `models` list that yields zero usable ModelInfos passed `check_non_empty` and then wrote nothing, silently, exit 0. - huggingface: count the ModelInfos built from the response, not `len(models)`. - ovhcloud: same, and move the check after `get_model_infos`. The old `if not model_infos: print(...); return` path exited 0 on a total parse failure, which is indistinguishable from a successful import. - huggingface: build every provider's YAML into a pending list, validate all of them, then write. Validating inside the write loop meant a sharp drop on a later provider left earlier ones already overwritten - a partial update that is worse than either outcome. Verified against the live APIs: both importers still complete and the guards do not false-positive on real responses (huggingface even picked up two new providers, whose data churn is deliberately not included here). Verified the guards fire on the real recorded counts - 61 models on record, 0 or 2 returned is refused, 60 is written.
Marcelo is right, and the change was worse than redundant. httpx2 2.2.0 ships DEFAULT_TIMEOUT_CONFIG = Timeout(timeout=5.0), applied to connect/read/write/pool individually, so the importers were never unbounded. Passing timeout=30.0 made them six times slower to give up than the default they already had. The claim in #538 that a hung upstream hangs the command indefinitely was simply wrong; corrected there. source_ovhcloud.py keeps its pre-existing timeout=30.0 - that predates this branch and is someone's deliberate choice, not mine to reverse here.
Two pieces of review feedback. The pin added recurring friction - someone has to bump it - for a speculative benefit, so pull.sh goes back to tracking main. The remote stays on HTTPS rather than SSH: this is a public repo, so there is no reason for the clone to use an authenticated agent, and that costs nothing either way. The scoped --allow-write stays. main.ts writes exactly one path, so the narrow form is free, and it keeps an unexpected upstream change contained to the gitignored output file every other price source writes. Also reworded the deno.json note, the README and the source_guard docstring in terms of least privilege and upstream breakage rather than adversarial framing, per Marcelo - that discussion belongs in a private channel, not in a public repo.
dsfaccini
changed the title
|
This comment was posted by Claude Code using claude-opus-5 on behalf of David. @Kludex — both points taken, PR updated. Timeouts: you're right, and it was worse than redundant. Scope: dropped the SHA pin, kept the write scoping. The pin added recurring friction — someone has to bump it — for a speculative benefit, so Wording: retitled the PR and rewrote both it and #538 in terms of least privilege and upstream breakage. Agreed that the framing didn't belong in a public repo — the in-repo comments are reworded too. Worth flagging one thing I can't clean up: the earlier commit messages on this branch still carry the original framing, and rewriting them would mean a force-push. Happy to squash-merge so only the PR title lands on On whether it saves anyone — fair challenge, and the honest answer is it doesn't protect any user of the package, since nobody but us runs |
1 participant
This pull request was posted by Claude Code using claude-opus-5 on behalf of David. David reviewed it lightly.
Closes #538. Independent of #533/#536 — branched from
main, no overlap.Housekeeping on the six price importers: run the one that executes upstream code with least privilege, and make the ones that rewrite tracked files refuse an implausible response instead of writing it.
Least privilege for the helicone task
prices/helicone_get/main.tsimportscost/providers/mappings.ts, which is Helicone's TypeScript copied in bypull.sh. Importing it means upstream code runs duringmake helicone-get. None of the other five importers do that — they parse inert JSON.The task ran with a bare
--allow-write, which grants write access to every path the invoking user has.main.tswrites exactly one path (../source_prices/helicone.json), so the broad permission bought nothing. It is now--allow-write=../source_prices.Deno denies net, read, run and env by default and none are granted, so with write scoped this task can only touch the file it is meant to produce. Verified from
prices/helicone_get:pull.shswitches the clone from SSH to HTTPS — it is a public repo, so there is no reason to use an authenticated agent.prices/helicone_get/README.mdrecords why the write scope is narrow so it does not get widened later by accident.Refuse implausible imports
Four importers overwrite tracked
prices/providers/*.ymlfrom live responses, and those files are what the published artifacts are built from. The only gate is someone readinggit diff, which is least reliable exactly when the diff is large.New
prices/src/prices/source_guard.py:check_non_empty— refuse an import that produced no models. This is the failure mode from Post-v2 audit: four broken pipeline paths + the verification gaps that let them ship green #533: an upstream changed shape, the parser dropped everything, and the importer wrote{}and exited 0.check_no_sharp_drop— refuse a rewrite that would delete more than half of a provider's recorded models.GENAI_PRICES_ALLOW_IMPLAUSIBLE_IMPORT=1downgrades both to warnings, for when a provider genuinely did retire its catalogue.Wired into
write_source_prices— the chokepoint every JSON-writing importer passes through, covering simonw, litellm, openrouter and aws in one place — and into thesource_huggingface/source_ovhcloudpaths that write provider YAML directly. Both build and validate all their output before writing any of it, so a failure on one provider cannot leave others half-updated.These catch the two loudest upstream failure modes and nothing subtler. A plausible-looking response with wrong numbers still needs a human to notice.
Also
source_ovhcloud.pyswallowed every exception into a silentreturn— a failed fetch printed a line and exited 0, indistinguishable from a successful no-op. It raises now.source_huggingface.pycalled.json()['data']with noraise_for_status(), so an error page surfaced as aKeyError.Verification
766 tests pass, including 9 new ones for the guards. ruff, basedpyright strict and the full pre-commit set pass.
make helicone-get,make ovhcloud-getandmake huggingface-getall run against their live APIs: the guards do not false-positive on real responses, and they do fire on the real recorded counts —huggingface_novitahas 61 models on record, so a response yielding 0 or 2 is refused and 60 is written.Running those importers surfaced genuine price churn plus two new HuggingFace providers (
deepinfra,scaleway). That is deliberately not included here — it belongs in its own data PR.Review feedback applied
timeout=30.0to four importers on the belief they were unbounded. They were not:httpx22.2.0 shipsDEFAULT_TIMEOUT_CONFIG = Timeout(timeout=5.0)applied per operation, so the change made them six times slower to give up than the default. Reverted, and the claim corrected on Importer housekeeping: least privilege for the helicone task, plausibility checks before writing provider YAML #538. Thanks @Kludex.source_ovhcloud.pykeeps its pre-existingtimeout=30.0, which predates this branch.pull.shtracksmainagain as before.Scope decisions, flagged as deliberate
packages/costwithout importing it, and it is authored as TypeScript modules with imports — a rewrite, not a tweak. Narrowing the permission makes it a data-trust question, which is the same question every other source already poses.source_aws.pyuntouched. It runslist(get_available_models())and a baremain()at module scope, so importing it fires live AWS calls. That is a restructure, already tracked in Post-v2 audit: four broken pipeline paths + the verification gaps that let them ship green #533.AGENTS.mdnote. Refresh agent-facing docs for the v2 data contract #534 rewrites that file; adding to it here would conflict for no benefit.