Skip to content

Skip SSH config dependency roots in Windows sandbox#18415

Closed
efrazer-oai wants to merge 1 commit intoefrazer/windows-sandbox-skip-tshfrom
efrazer/windows-sandbox-ssh-dependency-roots
Closed

Skip SSH config dependency roots in Windows sandbox#18415
efrazer-oai wants to merge 1 commit intoefrazer/windows-sandbox-skip-tshfrom
efrazer/windows-sandbox-ssh-dependency-roots

Conversation

@efrazer-oai
Copy link
Copy Markdown
Contributor

@efrazer-oai efrazer-oai commented Apr 17, 2026

Summary

  • Add a small OpenSSH-config dependency scanner for the Windows sandbox profile scan.
  • Follow Include directives recursively, including globbed includes, and record top-level profile folders named by file-bearing SSH directives.
  • Exclude those folders from profile read roots before ACLs are granted to CodexSandboxUsers.

Why

Adding .tsh covers the known Teleport failure. This follow-up handles the same class of bug when .ssh/config points at other profile folders through IdentityFile, CertificateFile, UserKnownHostsFile, ControlPath, IdentityAgent, PKCS11Provider, SecurityKeyProvider, XAuthLocation, or included config files.

The scanner is intentionally narrower than a general SSH client config parser. It answers one question: which top-level folders under the human user profile are named by the .ssh/config dependency chain?

Testing

  • just fmt
  • cargo test -p codex-windows-sandbox --lib
  • just fix -p codex-windows-sandbox

Copy link
Copy Markdown
Contributor

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

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: 11bd91e876

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

""
} else {
let prefix = format!("{profile}/");
path.strip_prefix(&prefix).unwrap_or_default()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Badge Normalize case before stripping profile prefix

record_profile_entry compares the full path case-insensitively, but uses a case-sensitive strip_prefix for "{profile}/". On Windows, a config path like c:\users\ALICE\.keys\id with USERPROFILE=C:\Users\Alice won't match, so no entry is recorded. That leaves .keys in read roots and ACLs can still be granted on SSH-dependent folders, defeating this fix.

Useful? React with 👍 / 👎.

@efrazer-oai
Copy link
Copy Markdown
Contributor Author

Closing in favor of a smaller Windows sandbox fix that prevents USERPROFILE itself from becoming an ACL root.

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