feat: stabilize local dev paths with ContentRoot-based resolution#1
feat: stabilize local dev paths with ContentRoot-based resolution#1joirunner wants to merge 53 commits intov2.0.0-devfrom
Conversation
0acd0f8 to
bec8050
Compare
- Add MotelyPaths static resolver using ASP.NET Core ContentRootPath - Set ContentRoot to repo root in MotelyApiHost for consistent paths - Fix hardcoded Windows path in Endpoints.GetFilters() - Fix Filters/JamlFilters directory mismatch in FilterService - Fix WordLists/SeedSources directory mismatch in Endpoints - Replace all relative path usages with MotelyPaths properties - Add path traversal protection in FilterService.GetFilterJaml() - Support config overrides via Motely:Paths section - Add LOCAL_DEV.md documentation for path configuration Resolves cross-platform path issues on macOS/Linux/Windows
bec8050 to
9d3fe04
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a centralized path resolution system for Motely.API to address cross-platform path issues on macOS, Linux, and Windows. The implementation uses ASP.NET Core's ContentRootPath as the base for all directory paths, with optional configuration overrides.
Changes:
- Adds MotelyPaths static class for centralized path resolution using ContentRootPath
- Replaces hardcoded and relative paths with MotelyPaths properties throughout the codebase
- Implements path traversal protection in FilterService.GetFilterJaml()
- Adds configuration support for custom path overrides via appsettings.json
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Motely.API/MotelyPaths.cs | New centralized path resolver with ContentRoot-based resolution and config override support |
| Motely.API/MotelyApiHost.cs | Sets ContentRoot to repo root and initializes MotelyPaths at startup |
| Motely.API/Services/FilterService.cs | Adds path traversal protection and updates to use MotelyPaths.JamlFiltersDir |
| Motely.API/SearchManager.cs | Updates all path references to use MotelyPaths static properties |
| Motely.API/Endpoints.cs | Removes hardcoded Windows path and updates to use MotelyPaths properties |
| Motely.API/appsettings.example.json | Adds configuration structure for path overrides |
| Motely.API/LOCAL_DEV.md | New documentation for local development path configuration |
| BalatroGameMechanicsDatasetDocum.md | Removed (appears to be unrelated documentation cleanup) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@joirunner I've opened a new pull request, #4, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@joirunner I've opened a new pull request, #5, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@joirunner I've opened a new pull request, #6, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@joirunner I've opened a new pull request, #7, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@joirunner I've opened a new pull request, #8, to work on those changes. Once the pull request is ready, I'll request review from you. |
[WIP] Address feedback on stabilizing local dev paths
[WIP] WIP to address feedback on local dev paths stabilization PR
Co-authored-by: joirunner <6373131+joirunner@users.noreply.github.com>
[WIP] Address feedback on local dev paths stabilization
Eliminate duplicate FindMotelyRoot() call in MotelyApiHost
Co-authored-by: joirunner <6373131+joirunner@users.noreply.github.com>
Co-authored-by: joirunner <6373131+joirunner@users.noreply.github.com>
Add defense-in-depth path validation to prevent directory traversal attacks
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@joirunner I've opened a new pull request, #16, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ce condition Co-authored-by: joirunner <6373131+joirunner@users.noreply.github.com>
[WIP] Fix local dev paths stabilization with ContentRoot resolution
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static bool IsPathWithinDirectory(string filePath, string baseDirectory, out string fullFilePath) | ||
| { | ||
| fullFilePath = Path.GetFullPath(filePath); | ||
| var fullBaseDir = Path.GetFullPath(baseDirectory); | ||
|
|
||
| // Normalize the directory path to end with a separator for accurate StartsWith check | ||
| // This prevents false positives like "/app/JamlFilters" matching "/app/JamlFiltersEvil" | ||
| fullBaseDir = Path.TrimEndingDirectorySeparator(fullBaseDir) + Path.DirectorySeparatorChar; | ||
|
|
||
| // Defense in depth: Check using both StartsWith and GetRelativePath | ||
| // StartsWith check: Verify the full path is within the expected directory | ||
| // Using Ordinal comparison for case-sensitive security on case-sensitive file systems | ||
| if (!fullFilePath.StartsWith(fullBaseDir, StringComparison.Ordinal)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // GetRelativePath check: Verify no ".." path traversal attempts | ||
| var relativePath = Path.GetRelativePath(fullBaseDir, fullFilePath); | ||
| if (relativePath.StartsWith("..", StringComparison.Ordinal) || Path.IsPathRooted(relativePath)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Sanitizes a filter name to prevent path traversal attacks. | ||
| /// Extracts just the filename stem (no path separators, no extension) and replaces invalid characters. | ||
| /// </summary> | ||
| /// <param name="name">The filter name to sanitize</param> | ||
| /// <param name="safeName">The sanitized filename stem, or null if the name is invalid</param> | ||
| /// <returns>True if the name is valid and was sanitized, false otherwise</returns> | ||
| public static bool TrySanitizeFilterName(string? name, out string? safeName) | ||
| { | ||
| safeName = null; | ||
|
|
||
| if (string.IsNullOrEmpty(name)) | ||
| return false; | ||
|
|
||
| // Extract just the filename stem (no path separators, no extension) | ||
| var sanitized = Path.GetFileNameWithoutExtension(name); | ||
| if (string.IsNullOrWhiteSpace(sanitized)) | ||
| return false; | ||
|
|
||
| // Remove any remaining path separators or invalid characters | ||
| var invalidChars = Path.GetInvalidFileNameChars(); | ||
| foreach (var c in invalidChars) | ||
| { | ||
| sanitized = sanitized.Replace(c, '_'); | ||
| } | ||
|
|
||
| safeName = sanitized; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The new path traversal protection functions (IsPathWithinDirectory and TrySanitizeFilterName) are security-critical but lack test coverage. The repository has comprehensive test coverage for other functionality (in Motely.Tests), and these security functions should have tests verifying they correctly prevent path traversal attacks with various malicious inputs like "../", absolute paths, and edge cases on different platforms.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Motely.API/MotelyPaths.cs
Outdated
| if (config != null) | ||
| { | ||
| _jamlFiltersOverride = config["Motely:Paths:JamlFiltersDir"]; | ||
| _seedSourcesOverride = config["Motely:Paths:SeedSourcesDir"]; | ||
| _searchResultsOverride = config["Motely:Paths:SearchResultsDir"]; | ||
| } |
There was a problem hiding this comment.
The path configuration allows arbitrary absolute paths via config overrides, which could be exploited to point to sensitive system directories. Consider adding validation in Initialize to ensure configured paths are within safe boundaries or at least document this security consideration in appsettings.example.json. This is especially important since directory creation happens automatically for configured paths.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| public static void Initialize(IWebHostEnvironment env, IConfiguration? config = null) | ||
| { | ||
| _contentRoot = env.ContentRootPath; | ||
|
|
||
| if (config != null) | ||
| { | ||
| _jamlFiltersOverride = config["Motely:Paths:JamlFiltersDir"]; | ||
| _seedSourcesOverride = config["Motely:Paths:SeedSourcesDir"]; | ||
| _searchResultsOverride = config["Motely:Paths:SearchResultsDir"]; | ||
| } | ||
|
|
||
| // Ensure directories exist (using ResolvePath directly to avoid EnsureInitialized check) | ||
| Directory.CreateDirectory(ResolvePath(_jamlFiltersOverride, "JamlFilters")); | ||
| Directory.CreateDirectory(ResolvePath(_seedSourcesOverride, "SeedSources")); | ||
| Directory.CreateDirectory(ResolvePath(_searchResultsOverride, "SearchResults")); | ||
|
|
||
| // Mark as initialized after all setup is complete | ||
| _isInitialized = true; | ||
| } |
There was a problem hiding this comment.
The volatile keyword alone is insufficient for thread safety in the Initialize method. Multiple threads could potentially call Initialize concurrently, leading to race conditions where directories might be created multiple times or _isInitialized could be set true before all initialization is complete. Consider using a lock or Interlocked operations to ensure thread-safe initialization, or document that Initialize must only be called once from a single thread during application startup.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@joirunner I've opened a new pull request, #17, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@joirunner I've opened a new pull request, #18, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@joirunner I've opened a new pull request, #19, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: joirunner <6373131+joirunner@users.noreply.github.com>
Co-authored-by: joirunner <6373131+joirunner@users.noreply.github.com>
…hrows Co-authored-by: joirunner <6373131+joirunner@users.noreply.github.com>
Co-authored-by: joirunner <6373131+joirunner@users.noreply.github.com>
Add thread-safe double-checked locking to MotelyPaths.Initialize
Add test coverage for FilterService path security functions
|
what the fuck bro |
Resolves cross-platform path issues on macOS/Linux/Windows