Skip to content

feat: add bounded artifact reader#40

Closed
Pigbibi wants to merge 2 commits into
mainfrom
codex/p0-safe-artifact-reader
Closed

feat: add bounded artifact reader#40
Pigbibi wants to merge 2 commits into
mainfrom
codex/p0-safe-artifact-reader

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a descriptor-relative bounded artifact reader
  • allow a symlinked base directory while rejecting source path symlinks and escapes
  • require regular files, enforce a 4 MiB default limit, and close all descriptors

Scope

Library primitive only. No strict linkage validator, CLI/workflow, freshness, expiry, data-quality, policy, or trading changes.

Validation

  • ........... [100%]
    11 passed in 0.08s — 11 passed
  • ...................................................... [100%]
    54 passed in 0.29s — 54 passed
  • All checks passed! — passed
  • — passed

Co-Authored-By: Codex <noreply@openai.com>
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

🤖 Codex PR Review

🚫 Merge blocked: 1 serious issue(s) found in high-risk files

🚫 Blocking Issues

These issues must be fixed before this PR can be merged:

1. 🟠 [HIGH] Security in src/research_signal_context_pipelines/artifact_io.py

The code resolves base_dir to an absolute pathname and then re-opens that pathname later with os.open(canonical_base, ...). That split is not race-safe: an attacker who can rename or replace a parent component of canonical_base between resolve(strict=True) and os.open(...) can redirect the open into a different tree, because O_NOFOLLOW only protects the final path component. In that case, subsequent descriptor-relative opens operate under the attacker's substituted directory instead of the originally validated base. (line 24)

Suggestion: Anchor the base directory without reopening a resolved pathname string. Open the base through a trusted parent descriptor chain with O_NOFOLLOW on every component, or otherwise obtain and validate a stable directory descriptor before deriving child opens from it. Avoid Path.resolve() + later path reopen as the security boundary.


Review by Codex PR Review bot • PR

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c955b5860a

ℹ️ 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".

Comment thread src/research_signal_context_pipelines/artifact_io.py Outdated
Co-Authored-By: Codex <noreply@openai.com>
@Pigbibi

Pigbibi commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Stopped by the bounded-rollout rule after the single remediation exposed another filesystem race contract. This low-level safe-reader direction is no longer proportional to the report-quality P0 outcome, so the PR is closed without merge and will not receive further automated fixes.

@Pigbibi Pigbibi closed this Jul 13, 2026
@Pigbibi
Pigbibi deleted the codex/p0-safe-artifact-reader branch July 13, 2026 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant