Skip to content

Run pull request code only inside a Modal Sandbox#8170

Merged
tohtana merged 2 commits into
masterfrom
tohtana/fix/secure-modal-pr-sandbox
Jul 24, 2026
Merged

Run pull request code only inside a Modal Sandbox#8170
tohtana merged 2 commits into
masterfrom
tohtana/fix/secure-modal-pr-sandbox

Conversation

@tohtana

@tohtana tohtana commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

GitHub recently changed actions/checkout to reject fork pull request checkouts by default in privileged pull_request_target workflows. This protection was introduced in actions/checkout v7 and backported to supported floating major versions, including actions/checkout@v4. As a result, the current workflow's checkout of ${{ github.event.pull_request.head.sha }} is now rejected for fork pull requests.

Setting allow-unsafe-pr-checkout: true would restore the previous behavior, but it would deliberately bypass this protection. A pull_request_target workflow runs trusted default-branch code with access to the base repository's token and secrets. Because this workflow needs MODAL_TOKEN_ID and MODAL_TOKEN_SECRET to start GPU CI, the code that handles those credentials must remain trusted code from master; pull request code must not execute in the same GitHub runner context.

Modal documents Sandboxes as secure containers for executing untrusted code, including checking out a repository and running its test suite. This change follows that model: GitHub runs only the trusted selector and Modal controller, while the pull request is treated as data during test selection. The controller passes only a validated public repository name, exact commit SHA, and validated test selection to a Modal Sandbox. The Sandbox receives no GitHub, Modal, or Hugging Face credentials, and performs the candidate checkout, dependency installation, DeepSpeed installation, and pytest execution itself.

tohtana added 2 commits July 23, 2026 14:24
Signed-off-by: Masahiro Tanaka <mtanaka@anyscale.com>
Signed-off-by: Masahiro Tanaka <mtanaka@anyscale.com>
@tohtana
tohtana marked this pull request as ready for review July 23, 2026 23:15
@tohtana
tohtana requested a review from loadams as a code owner July 23, 2026 23:15

@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: 0c4a7800fc

ℹ️ 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 ci/torch_latest.py
def run_sandbox_command(sandbox: Any, modal_module: Any, command: RemoteCommand) -> str:
process = sandbox.exec(
*command.argv,
stderr=modal_module.stream_type.StreamType.STDOUT,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Route stderr through the capped log path

When an untrusted install or pytest command writes to stderr, StreamType.STDOUT sends that stream straight to the runner stdout instead of through the loop that prefixes, escapes, and enforces MAX_DISPLAY_BYTES_PER_COMMAND; only process.stdout is drained by this code. That lets PR-controlled stderr emit raw GitHub workflow commands or flood the job log despite the cap, so keep stderr piped and drain/cap it with the same sanitizer as stdout.

Useful? React with 👍 / 👎.

@tohtana
tohtana added this pull request to the merge queue Jul 24, 2026
Merged via the queue into master with commit 39adc2b Jul 24, 2026
15 of 17 checks passed
@tohtana
tohtana deleted the tohtana/fix/secure-modal-pr-sandbox branch July 24, 2026 17:53
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.

2 participants