fix(fs): refuse credential writes through symlinks - #36360
Conversation
auth.json and other JSON state writers use Filesystem.write / FSUtil.writeJson, which followed existing symlinks via writeFile. A planted symlink under the data directory could redirect secrets to an attacker-controlled path. Minimal fix: - lstat before write; throw if the path is a symlink - same guard in core FSUtil.writeJson (Auth/McpAuth paths) - regression test for Filesystem.write
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
github-actions
Bot
added
the
needs:compliance
|
The following comment was made by an LLM, it may be inaccurate: Based on my search, I found one potentially related PR but it's not a duplicate: PR #28532:
The other results either matched the current PR (#36360) itself or were unrelated to the symlink write security issue. No duplicate PRs found |
|
Ready for review — minimal security hardening for credential JSON writers (auth/MCP). Happy to iterate. — @1837620622 |
github-actions
Bot
removed
needs:issue
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
Review ping 🙏 Refuse credential/state writes through symlinks ( |
Addresses CodeRabbit TOCTOU finding on the anomalyco#36360 port: the leaf symlink check now happens at open time via O_NOFOLLOW (lstat retained as the Windows fallback). Symlinked parent directories remain allowed by design for dotfile-manager setups.
* fix(fs): refuse credential writes through symlinks Ported from upstream anomalyco#36360. * fix(fs): bind symlink refusal to O_NOFOLLOW descriptor writes Addresses CodeRabbit TOCTOU finding on the anomalyco#36360 port: the leaf symlink check now happens at open time via O_NOFOLLOW (lstat retained as the Windows fallback). Symlinked parent directories remain allowed by design for dotfile-manager setups.
Gentle follow-upStill ready for review. Refuse credential/state writes through symlinks ( Security-hardening only — happy to tighten further if needed. |
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
1 participant
Issue for this PR
Closes #36367
Type of change
What does this PR do?
Credential/state writers (
FSUtil.writeJson,Filesystem.write) followed existing symlinks, so a replacedauth.json(or similar) could redirect writes outside the intended store.Refuse writes through an existing symlink; leave the link target untouched. Regression test for
Filesystem.write.How did you verify your code works?
filesystem.test.tscase: write through symlink is rejected and target content unchangedFSUtil.writeJsonwhich now has the same guardScreenshots / recordings
N/A — non-UI
Checklist