Repository navigation
📦🔧：check the file the importer opened - #143
Merged
openinf-commit-queue[bot] merged 1 commit intoNov 2, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Validate opened destination before writing packages/gh-file-importer/src/index.ts, packages/gh-file-importer/README.md, packages/gh-file-importer/test/index.ts, .changeset/importer-opened-file.md |
The importer opens the destination without truncation. On Linux, it checks the opened descriptor’s physical path before truncating or writing. Documentation describes destination-tree requirements and the limits of the check. Tests cover shorter overwrites, a directory-swap race, and descriptor lookup errors. |
Printable download URLs
| Layer / File(s) | Summary |
|---|---|
Encode control characters in displayed URLs packages/gh-file-importer/src/index.ts, packages/gh-file-importer/test/index.ts |
Download-start logs and HTTP failure messages use URLs with C0 and C1 control characters percent-encoded. A test checks the encoded output. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Possibly related PRs
- OpenINF/sdk#130: Changes the same importer destination-write path. This PR adds a check of the opened destination descriptor before truncation and writing.
Merge Risk: 
25b50
-
On restricted Linux systems, a directory swap can still overwrite a file outside the destination. Handle unexpected lookup failures before merging.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the main change: checking the file that the importer opened before writing. The emojis are unnecessary, but they do not make the title unrelated or misleading. Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
CodeRabbit Verify
- Run CodeRabbit Verify to test the changes in this pull request.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Member
Author
|
@coderabbitai review Generated by Claude Code |
Action not completed
|
DerekNonGeneric
force-pushed
the
claude/project-thread-fqaph2-importer
branch
from
November 2, 2026 23:27
679fac5 to
1e5bded
Compare
Every check writeWithinDestDir made was of a path. A directory swapped for a symbolic link after the last check still redirected the open, and O_TRUNC emptied whatever file it reached before a byte was written. The file is now opened without truncating it, and on Linux /proc/self/fd names the file the descriptor actually reached; one outside destDir is refused before any of it changes. Where /proc is mounted but the lookup fails, the import fails rather than writing unchecked. A swap that wins the race costs at most an empty file created outside. No other platform, and no Linux system without /proc, says where a descriptor leads, so the README now asks for a destDir nobody less trusted than the caller can change mid-import. The download log line and the failed-download error printed the URL as given, so a line break in it could forge a log line and an escape could restyle the terminal. Control characters in it are now percent-encoded, the way a URL carries them. Signed-off-by: Derek Lewis <DerekNonGeneric@inf.is> Assisted-by: Claude-Code:claude-opus-5
DerekNonGeneric
force-pushed
the
claude/project-thread-fqaph2-importer
branch
from
November 2, 2026 23:33
1e5bded to
25b50b8
Compare
DerekNonGeneric
added
the
🚀 Status: Commit Queue
Claude
openinf-commit-queue
Bot
removed
the
🚀 Status: Commit Queue
openinf-commit-queue
Bot
deleted the
claude/project-thread-fqaph2-importer
branch
November 2, 2026 23:44
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by DerekNonGeneric
Before: every destination check in
writeWithinDestDirchecked a path, not the file actually opened. Suppose a directory was swapped for a symbolic link after the last check. Theopencall would then follow the link, andO_TRUNCwould empty whatever file it reached before anything was written (CWE-367). Separately, the download log line and the error for a failed download printed the URL exactly as given. A URL containing a line break could fake an extra log line, and one containing a terminal escape could change how the terminal displays output (CWE-117). CodeRabbit's security deepscan reported both.After: on Linux, a swapped-in link is refused before any byte of the outside file changes. The worst a winning race can do is create an empty file outside
destDir. Other platforms offer no way to ask where an open file descriptor points. For them, the README now asks callers to use adestDirthat no less trusted user can change during an import. Control characters in a URL are percent-encoded in both the log line and the error message.How: the destination file is now opened with
O_CREAT | O_WRONLY | O_NOFOLLOW, withoutO_TRUNC. On Linux, the code reads the descriptor's real path from/proc/self/fd/<fd>and refuses the write if it is outside the physicaldestDir. Only after that check does it callhandle.truncate(0)and write.printableUrlpercent-encodes C0 and C1 control characters. I added three tests:openso the parent directory is swapped for a link at the last moment. The file outsidedestDirkeeps its contents. This test is skipped on platforms other than Linux.The race test and the URL test fail against the old code. All 69 tests pass on Node 24.
🤖 Generated with Claude Code
Summary by CodeRabbit