fix(security): narrow alternate write root to a plugin temp subdir (R2-11) - #343
Merged
Merged
Conversation
…a plugin subdir (R2-11) IsOutputPathAllowed accepted any path under the system temp dir as a legitimate write root (besides the configured DownloadPath). %TEMP% is world-writable and shared across processes, so that allowed a caller-supplied outputPath to land anywhere in it. Narrow the alternate root to a plugin-owned %TEMP%/tidalarr subdir (PluginTempRoot), and stage GetTempFilePath there so the legitimate staging path stays inside the allowed root. Containment stays canonical-form (PathTraversalGuard), so the sibling-prefix case "%TEMP%/tidalarr_evil" is refused, not just "%TEMP%/other". TDD: under-subdir accepted; elsewhere-in-%TEMP% and sibling-prefix rejected. 51 download tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /// not all of <c>%TEMP%</c> — is accepted as the alternate write root, so a caller-supplied path landing | ||
| /// elsewhere in the (world-writable, shared) temp dir is refused. Staging (<see cref="GetTempFilePath"/>) | ||
| /// writes here.</summary> | ||
| internal static string PluginTempRoot { get; } = Path.Combine(Path.GetTempPath(), "tidalarr"); |
| return Path.Combine(Path.GetTempPath(), $"tidalarr_{safeName}{extension}"); | ||
| // R2-11: stage inside the plugin-owned temp subdir (the only alternate write root IsOutputPathAllowed | ||
| // accepts) rather than directly in the shared %TEMP%. | ||
| return Path.Combine(PluginTempRoot, $"tidalarr_{safeName}{extension}"); |
RicherTunes
added a commit
that referenced
this pull request
Sep 7, 2026
Picks up Common #343 (smoke test review fixes). Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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
2 participants
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.
Round-2 review R2-11
IsOutputPathAllowedaccepted any path under the system temp dir as a legitimate write root (besides the configuredDownloadPath).%TEMP%is world-writable and shared across processes, so that allowed a caller-suppliedoutputPathto land anywhere in it.Fix
Narrow the alternate root to a plugin-owned
%TEMP%/tidalarrsubdir (PluginTempRoot), and stageGetTempFilePaththere so the legitimate staging path stays inside the allowed root. Containment stays canonical-form (PathTraversalGuard), so the sibling-prefix case%TEMP%/tidalarr_evilis refused — not just%TEMP%/other.Tests
TDD: under-subdir accepted; elsewhere-in-
%TEMP%and sibling-prefix rejected. 51 download-client tests green.🤖 Generated with Claude Code