fix: harden Russell Top50 trust strings - #229
Conversation
Co-Authored-By: Codex <noreply@openai.com>
|
@codex review Frozen F3 exact-string trust correction. Please review this exact head only. |
![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: 72db04fc11
ℹ️ 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".
| _invalid() | ||
| pd.read_csv(__import__("io").BytesIO(members["inputs/prices.csv"])) | ||
| pd.read_csv(__import__("io").BytesIO(members["inputs/universe.csv"])) | ||
| pd.read_csv(__import__("io").BytesIO(members["snapshot/feature_snapshot.csv"])) |
There was a problem hiding this comment.
For a re-canonicalized package whose digest is supplied to the verifier, this only proves that the feature snapshot is parseable CSV. Because both the inner snapshot_sha256 and outer manifest_id are derived from the submitted bytes, an attacker can replace the snapshot with an arbitrary nine-row frame, update the manifests and member hashes, and have bind_russell_top50_research_input evaluate the forged data. The verifier should rebuild the derived artifacts from the hash-pinned inputs or validate them against equally strict frozen semantics.
Useful? React with 👍 / 👎.
| or | ||
| _sha256(members["inputs/prices.csv"]) != _PRICES_SHA256 | ||
| or _sha256(members["inputs/universe.csv"]) != _UNIVERSE_SHA256 | ||
| or manifest["research_input_contract_id"] != CONTRACT_VERSION |
There was a problem hiding this comment.
When a package is re-canonicalized with a newly supplied digest, effective_at can be changed to any schema-valid timestamp while as_of, the inner snapshot date, and the calendar session remain unchanged. _outer_manifest always sets effective_at equal to as_of, but _validate_claims never checks that invariant, so contradictory timing metadata is accepted as valid research evidence.
Useful? React with 👍 / 👎.
| _require_descriptor_capabilities() | ||
| root_fd = os.open(root, flags) | ||
| try: | ||
| if {entry.name for entry in os.scandir(root_fd)} != {_MANIFEST, "inputs", "snapshot"}: |
There was a problem hiding this comment.
For an untrusted package directory containing a very large number of entries, this set comprehension enumerates and retains every name before discovering that the directory is invalid. The member byte limits therefore do not prevent a local memory/CPU denial of service; enumerate with a count limit and reject immediately after the expected number of entries, as is already done by the hardened snapshot reader.
Useful? React with 👍 / 👎.
| _BASE = "b5306a972724105273efebbd22834afde1294e4e" | ||
| _TREE = "9c5c6bf08b7dcd70a7ff091b606fe09e4ee04b5c" |
There was a problem hiding this comment.
Every package is forced to claim producer commit b5306a9 and tree 9c5c6bf, but that commit does not contain russell_top50_research_input_binding.py; the module is introduced only by this reviewed commit, whose tree is different. Consequently, all successfully materialized evidence names a producer tool that cannot be inspected or reproduced at its asserted revision. Use a commit/tree containing this implementation, and keep the older revision only as a separate fixture-source revision if needed.
Useful? React with 👍 / 👎.
| "contract_version": "russell_top50_leader_rotation.feature_snapshot.v1", | ||
| "generated_at": observed, | ||
| "input_artifacts": { | ||
| "prices": {"as_of": cutoff[:10], "path": "inputs/prices.csv", "row_count": 2880, "sha256": _PRICES_SHA256}, |
There was a problem hiding this comment.
The hash-pinned price fixture ends on 2025-03-24, but any canonical UTC as_of is accepted and then recorded here as the price artifact's observation date. For example, an as_of in 2030 makes the builder reuse the last 2025 price row while the rewritten inner and outer manifests claim 2030 price and snapshot dates, allowing stale data to masquerade as contemporaneous research evidence. Require the cutoff to match a supported session in the frozen input, or retain the actual maximum source date in these claims.
Useful? React with 👍 / 👎.
| verify_russell_top50_research_input(temporary, expected_manifest_sha256=digest) | ||
| _publish_noreplace(temporary, destination) |
There was a problem hiding this comment.
If the output parent is writable by an adversary, the verified temporary directory can be renamed away and replaced at the same basename after verification but before this path-based rename. _publish_noreplace then publishes the replacement while the returned digest and members still describe the original verified directory. Retain and check an identity for the source directory, or perform verification and publication through anchored descriptors so the object being renamed is the one that was verified.
Useful? React with 👍 / 👎.
| except (UnicodeDecodeError, json.JSONDecodeError, TypeError, ValueError): | ||
| _invalid() |
There was a problem hiding this comment.
For a re-sealed package containing a deeply nested inner manifest, json.loads can raise RecursionError, which is not caught here and therefore escapes verify_russell_top50_research_input instead of producing the documented ResearchInputBindingError. The two-megabyte member limit still permits nesting well beyond Python's recursion limit, so include RecursionError in the parser rejection path or enforce a bounded JSON depth.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Frozen F3 publication: exact two paths, 679 lines; no post-publication correction is authorized.