Repository navigation
fix: untrusted-input guards for the seed guard, contractId resolver and YAML loader - #725
Merged
Merged
Conversation
resolve_at_path indexed the candidate with cursor[i], so a candidate with fewer exposes than the seed raised IndexError, a dict or number in a list's place raised KeyError or TypeError, and a string returned one of its characters. diff_against_seed raised with it, and generate_copilot_artifacts catches every guard exception, logs structural_seed_guard_failed and keeps the candidate: the guard failed open on the mutation it exists to catch. An index past the end of a list, or into anything that is not a list, is now a missing value, so a dropped expose is a mismatch and the repair loop asks the model to restore it.
…le with another id was linked contractId is document text, and the resolver probed paths with it unchecked. A NUL byte raised ValueError from Path.resolve (only OSError and RuntimeError were caught), a non-string raised TypeError, and an unparsable URL such as "http://[::1" raised ValueError from urlparse. BitolOdpsProvider._safe_resolve catches only ContractNotFound, so `fluid opds import` failed even with --lenient and on input ports, which always resolve leniently. A contractId that is not a non-empty string without NUL bytes now raises ContractNotFound with the reason, which lenient imports skip; _looks_like_url treats an unparsable URL as not a URL, and the path probe also skips a candidate that raises ValueError, as util/ref_confinement does. The last-segment probe tries orders.odcs.yaml for "commerce.orders" and accepted whatever contract that file held. A probed file is now used only when it declares the contractId as its id, as ODPS ports reference ODCS ids; a mismatch is skipped with a warning naming both ids, so a later candidate can still match. Index lookups match by id already and explicit hints are unchanged.
… with the guard load_yaml_safe counted alias events (at most 50) on the theory that a billion-laughs payload needs more. It does not: a list of two aliases to the level below doubles per level, so 22 levels are 44 aliases and 2**22 nodes. Composition shares the anchored node, so the load itself is fast, but every later walk sees the expanded tree: a 685-byte contract shaped like that held `fluid validate` for more than 90 s. A document that has aliases is now composed and walked once, iteratively, sizing each node's expanded tree with memoisation, before it is constructed. It is refused when its aliases add more than MAX_YAML_ALIAS_EXPANSION (100,000) nodes, or grow it more than 100-fold past 1,000 nodes. The design follows OmegaConf 2.4's YAML alias limits (omegaconf/_yaml.py, BSD-3-Clause) with two differences: the budget is the nodes aliases add, not the total, so a large contract without aliases still loads; and a recursive alias counts once instead of being refused, so the contract API keeps reporting a document that contains itself as its documented contract_load_failed. A document without aliases skips the walk and loads exactly as before. The ODCS reader (odcs.io.read_input, behind the Bitol import, the contractId resolver and --seed-from), the resolver's fetched-body parse and both `fluid odps-bitol validate` paths used a plain yaml.safe_load, so none of them had even the count cap. They now go through load_yaml_safe.
…a failed validation `fluid odps-bitol validate` runs _run_odps_validate, which had no handler around the load. With the file now read through load_yaml_safe, an over-cap document ended as "CLI unhandled exception" (exit 2). It now prints "Validation failed" with the reason and returns 1, as the click command and every other validation failure in it do.
A malformed contractId now reaches the lenient skip in _safe_resolve instead of aborting the import, and the warning formatted it with %s, so a NUL byte (or any control character the document carries) went to the terminal as is. Format it with %r, as ContractNotFound already does for the same id.
…and import tests The resolve() guard reads as one named predicate, _is_valid_contract_id, instead of three inline conditions. The malformed-id test now checks each case's reason in one parametrized test, the fetched-bomb test leaves TestRemoteResolution for a module-level test, and the two malformed-port import tests are one parametrized test. No behaviour change.
…er report a refused file load_yaml_safe read a stream whole before measuring it against the 5 MiB cap, so a huge file was buffered just to be refused; the readers this branch routes through it pass open files. It now reads one unit past the cap. A bytes input is measured before it is decoded. The OpenAPI validator, the artifact fan-out (reference-only check and schedule contract) and the federation manifest reader caught yaml.YAMLError but not UnsafeYamlError. The expansion budget refuses inputs the 50-alias count never did, so each one now crashed where it reports bad YAML (OAS-PARSE, an unreadable contract, an empty manifest); they catch UnsafeYamlError too, as upstream_discovery and artifact_validators already did. Tests pin what the audit found unpinned: the safe constructor on the composed alias path (a Loader mutant ran !!python/object/apply), the ratio rule on its own (13 levels, under the node budget), the 100,000 default (39 vs 45 aliases to a 2,501-node block), the budget counting added nodes rather than document size, and the bounded read.
…refused file is reported `fluid odcs validate` read through cli/_common.load_yaml_json, a plain yaml.safe_load, so it still loaded an expansion bomb the Bitol readers now refuse. load_yaml_json uses load_yaml_safe, and both `fluid odcs` argparse paths report a refused file (validate: "Validation failed", import: "Error importing", exit 1) instead of an unhandled exception. import_directory classified files through _sniff_kind, which swallowed every error: a product file over the caps was skipped and the rest of the directory imported as ODCS-only with no error. A refused file is now skipped with a warning naming it, and a directory with no readable ODPS doc fails when the caps refused a file that may be it; an unrelated refused file next to a readable product still imports. A fetched body over the caps now names the reason (sizes and counts only, never body text), the probe-mismatch warning prints the path with %r, and the read_input and resolver docstrings state the caps and the probe id rule.
generate_copilot_artifacts with a scripted model: attempt 1 drops a seeded expose, attempt 2 returns the seed. On origin/main attempt 1 is kept after "structural_seed_guard_failed ... list index out of range"; now it carries "Seed ground-truth violation at exposes[1]..." and the final contract keeps every seeded expose.
… the guard fixes The ODCS/ODPS readers now refuse files the caps refuse and a probed sibling must declare the contractId, so both get an Upgrade note. The YAML entry moves to Security, quotes the count the guard computes (25,165,822 nodes, not 2^22), names `fluid odps` instead of the deprecated `fluid opds`, and lists every reader and caller it covers; the contractId entry quotes "not a valid contractId" only where it applies, and the seed entry no longer promises a "restore" prompt.
…not an unhandled exception `fluid generate artifacts` loads the contract through artifact_fanout._load_contract in every emitter, and only a FanoutError reaches its generate_artifacts_failed report. A contract the YAML guard refuses, or one that is not YAML, escaped as UnsafeYamlError or yaml.YAMLError; it is now a FanoutError naming the file. The two pre-checks already treated it as unreadable. `fluid odcs validate` and `fluid odps-bitol validate` reported a file over the caps as a failed validation but still ended in "CLI unhandled exception" on malformed YAML; both now report that as a failed validation too, as `fluid odcs import` does. The ratio-rule test pins the sizes the guard computes (30 to 49,150 nodes) instead of a bare raises, and a test docstring counts leaves and aliases the way the CHANGELOG does.
import_directory now calls _classify_bundle_files, which sorts the directory into ODPS docs, ODCS contracts and files the YAML caps refused, and keeps only the policy (fail when nothing readable is the product). The end-to-end seed test and the refused-file tests move to their own classes. No behaviour change.
…pty contractIds are ignored The Upgrade note said the ODCS/ODPS readers got the caps FLUID contracts already had, but the 100,000-node expansion cap is new for FLUID contracts too; it now says so and lists `fluid odps validate`. The contractId entry no longer claims an empty id fails a strict import: the provider skips a falsy contractId, as before.
The expansion budget counted nodes, so one anchored 1 MB string repeated through two nested lists of four aliases added 20 nodes and 20 MB of text; a 400 KB .odcs.yaml shaped like that made `fluid odcs import` run 143 s and write 230 MB. The walk now sizes each node's expanded scalar text too, and refuses a document whose aliases add more than MAX_YAML_ALIAS_EXPANSION_CHARS (the 5 MiB input cap again). The pre-scan refuses nesting past MAX_YAML_DEPTH = 128 (serde_json's default recursion limit; the deepest YAML file in the repo nests 9). PyYAML's scanner slows with flow depth, so a 64 KB file nested 32,000 deep took over 40 s in the readers this branch routes through the guard, and its recursive composer fails with RecursionError a few hundred levels down, sooner under a deep caller stack. A stream is read past short reads up to the cap (a raw stream can return fewer units than asked before its end), and the size cap counts UTF-8 bytes for text as it already did for bytes. The module docstring no longer claims every consumer refuses a recursive alias: the FLUID loader and the contract API do; the ODCS/ODPS readers do not. load_yaml_safe keeps only the flow; reading and measuring the source and the event pre-scan are their own helpers.
…read The schedule emitter skips a contract it cannot read, leaving the other emitters to report it. With `--emit schedule` alone there are none, so a contract that did not parse exited 0 with no files. run_fanout now loads such a contract through _load_contract, which raises the FanoutError every other emitter would. A contract that was not UTF-8 escaped every except tuple as UnicodeDecodeError, and a list or scalar contract raised AttributeError in the reference-only check; both ended in "Unexpected error". The check now returns False for either, and _load_contract reports an unreadable or undecodable file as a FanoutError naming it.
…opped import_directory skipped a product doc with a YAML syntax error without a word and imported the directory as ODCS-only; only a file the YAML caps refused was reported. Any file that does not parse is now skipped with a warning naming it, and a directory left without a readable ODPS doc fails. _sniff_kind is folded into _classify_bundle_files. The ODCS reader, load_yaml_json and both `fluid odps-bitol validate` readers tested the suffix case-sensitively, so PRODUCT.ODPS.YAML was parsed as JSON and never reached the caps. They lower-case it. `if not cid` skipped a falsy non-string contractId (false, 0, []), so a strict import succeeded with the port unresolved while a truthy one failed as not a valid contractId; only None and "" are skipped now. A contractId segment longer than a file name may be made the probe's exists() raise OSError, which aborted even a lenient import; the probe skips that candidate. The probe loop moves out of resolve() into _probe_local. `fluid odcs validate --report` returned before writing the report when the file did not parse, so a CI gate reading it found nothing; it now records a failed parse pass.
The end-to-end seed-guard test patches call_llm, but streaming and the judge pass call the provider directly, so the test made DNS and HTTP requests to api.openai.com, raw.githubusercontent.com and localhost:11434 and took 10-12 s. It sets FLUID_LLM_STREAMING=0 and FLUID_COPILOT_JUDGE=0; with sockets blocked it makes no network attempt and runs in 1.5 s.
The Security entry no longer claims the guard caps what aliases expand to while it counted nodes only: it now names the text cap and the depth cap. The Upgrade note lists both and the UTF-8 byte count, and says an unparsable bundle file also fails a directory without a product. The contractId entry covers the long-segment OSError and the falsy non-string ids a strict import used to skip, and three Fixed entries cover the bundle directory, `fluid generate artifacts` and `fluid odcs validate --report`.
… that will not open Dropping UnicodeDecodeError from _load_schedule_contract, or the OSError branch from _load_contract, left every test green while run_fanout on a latin-1 contract, or _load_contract on a directory, crashed with the raw exception. The run_fanout test now covers malformed and latin-1 contracts, with `--emit schedule` alone and the default set, and a directory passed to _load_contract is a FanoutError.
… for YAML only The text cap counts scalar characters (5,242,880), not bytes, so the entries say so instead of "5 MiB of text". `fluid odcs validate --report` writes the report when the YAML does not parse; a malformed JSON or non-UTF-8 file still exits 2 without one, so the entry no longer says "the file".
github-actions
Bot
added
provider
📄 Documentation ReminderThis PR appears to be missing a documentation reference. Our docs live in a separate repo. Please update the PR description with one of:
See the Contributing Guide for details. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
The --report helper re-imported json and Path, and the validate test re-imported json; both are imported at module level (code-quality review on #725).
github-actions
Bot
removed
the
needs-docs
litellm 1.106.0 adds Reka to llms/openai_like/providers.json with api_base_env REKA_API_BASE. The vendored LITELLM_ENDPOINT_NAMES did not have it, so test_the_vendored_names_cover_the_installed_litellm failed on every CI leg, and a project .env could set the endpoint FLUID's LLM key is sent to. It is listed now, and the docstring names 1.106.0 among the wheels the list covers.
5 of 9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Three small bugs found while comparing forge against the Bitol ODCS/ODPS examples, plus the untrusted-input holes the reviews found around them.
fluid forge --seed-from). When the model dropped an expose that the seed declares,resolve_at_pathraisedIndexError.generate_copilot_artifactsloggedstructural_seed_guard_failedand kept the contract, so the expose was lost without an error. An index past the end of a list, or into a value that is not a list, now counts as a missing value. The guard reports a mismatch and the repair loop asks the model to restore the expose.http://[::1) or a path segment too long for a file name. It crashed even under--lenientand on input ports. It also linked a probed file that declared a differentid.ContractNotFound, which lenient imports skip.idmatches.false,0) is no longer skipped silently under a strict import.load_yaml_safecapped the alias count but not what aliases expand to. 22 levels of[*a, *a]are 44 aliases that expand to 25,165,822 nodes. A 685-byte contract shaped like that heldfluid validatefor over 90 s. The ODCS/ODPS readers bypassed the guard entirely.RecursionErrora few hundred levels down.odcs/io.read_input, the resolver's fetch,fluid odcs validate/import,fluid odps-bitol validate,cli/_common.load_yaml_json. A refused or malformed file is reported as a failed validation or import, not as an unhandled exception.The reviews turned up more failures on the same paths, all fixed here:
fluid generate artifacts --emit scheduleexited 0 on a contract that did not parse. A non-UTF-8 or non-mapping contract ended in "Unexpected error".fluid odps import <dir>silently dropped a product doc it could not parse, or one with an upper-case.YAMLsuffix (that file was parsed as JSON).fluid odcs validate --reportwrote no report when the YAML did not parse.Recursive aliases are deliberately not refused by the guard.
api/contract.pydocuments a self-referencing document ascontract_load_failed. The module docstring now says which consumers refuse one and which do not.CHANGELOG
[Unreleased]has Upgrade notes covering:contractId;Also in this PR, unrelated to the fixes above: CI installs the newest litellm, and 1.106.0 reads
REKA_API_BASE, which the vendoredLITELLM_ENDPOINT_NAMESdid not list.test_the_vendored_names_cover_the_installed_litellmfailed on every leg, andmainwould fail the same way. 81a084d lists the name and adds a Security entry: a committed.envcould have pointed that endpoint at the FLUID LLM key.Borrowed, not built
omegaconf/_yaml.py, BSD-3-Clause), with two changes:serde_json's default recursion limit. The deepest YAML file in this repo nests 9 levels, and all 219 repo YAML files load identically under the guard andyaml.safe_load.ValueError: the same pattern asutil/ref_confinement.Validation
Full suite (
-n auto --dist loadscope): 26,572 passed, 4 failed.main: dbt not installed (test_dotenv_unfiltered_readers), two duckdb temp-root tests, andtest_coding_agent_live.tests/perf: 38 passed, 7 xfailed.Lint and format:
ruff check fluid_build/ tests/andblack==24.10.0 --check;check_license_headers.pyandlint-imports.Red, then green: every new test from the last fix round fails on the pre-fix commit and passes here. The one exception is the bytes case of the UTF-8 size test, a regression guard.
Mutation: 18 mutants of this round's fixes; 14 caught, every one of the planned ones among them. Two of the four survivors are now pinned: a non-UTF-8 contract through
run_fanout, and a path_load_contractcannot open. The other two are a suffix fix in a Click group thatfluiddoes not wire in, and an input-port change whose only effect is a warning.Live through the
fluidCLI. Each was reproduced on the pre-fix code and checked on this branch:fluid validatefluid odcs validatefluid generate artifacts --emit schedulegenerate_artifacts_failedfluid generate artifactsgenerate_artifacts_failedfluid odcs validate --reportfluid odps import <dir>PRODUCT.ODPS.YAMLbundlefluid odps import <dir>fluid odps import --lenientfluid odps importfluid odcs importon a 400 KB text-expansion file ran past 20 s before (an earlier measurement: 143 s, 230 MB written). It is refused in 0.7 s.ripwire --quality-delta, measured against a no-op control onmainto drop environmental rows. Accepted:load_yaml_safetakes 7 parameters, one keyword-only cap each, mirroringmax_bytes/max_aliases;ContractResolverclass LOC 267→297 (class size is a tracked follow-up);TestRunFanoutLOC;pytest.raisestests matching unrelated tests of the same shape.The complexity growth in
resolve()andload_yaml_safewas fixed by extracting_probe_local,_bounded_textand_count_aliases.Review verdicts
--reportparse error, carry PyYAML's ~40-character excerpt of the bad line (stdout already shows it);odcs validate --report) was pre-existing and is left as a follow-up.--emit schedulealone;odcs validatestill exits 2 on malformed JSON.Documentation
contractIdresolution, bundle files it cannot read, the seed guard; merge after this PR)Follow-ups (not in this PR)
yaml.safe_loadsites on untrusted input through the guard, and add a semgrep rule.UnsafeYamlErrorsubclassyaml.YAMLError.<unicode string>instead of the file name.UnsafeYamlErrorescape.!!float/!!intbody value in a rawValueError. This is already onmain.