Conversation
Path.Join is preferred over Path.Combine because it does not treat absolute path segments as rooted, preventing potential path traversal issues. See dotnet/runtime#24263 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR replaces many uses of Path.Combine/fs.Path.Combine with Path.Join/fs.Path.Join across source and test files, altering path-segmentation behavior consistently. It adds Elastic.Documentation.Extensions.UrlPath with Join and JoinUrl helpers and introduces Paths.ValidateSinglePathSegment. Several places add or tighten path-containment and relative-path validation (e.g., GitLinkIndexReader, StaticWebHost, DocumentationWebHost). Control flow, public APIs, and error handling are otherwise preserved. Sequence Diagram(s)sequenceDiagram
participant Client as Client (HTTP)
participant Host as StaticWebHost
participant FS as FileSystem
participant Res as Result
Client->>Host: GET /<slug>
Host->>Host: normalize contentRoot (Path.GetFullPath)
Host->>Host: build localPath = Path.Join(contentRoot, slug or slug/index.html)
Host->>Host: resolve fullLocalPath = Path.GetFullPath(localPath)
Host->>FS: FileExists(fullLocalPath)?
alt not inside contentRoot (traversal)
Host->>Res: Results.NotFound()
Res-->>Client: 404 Not Found
else file exists
Host->>FS: OpenRead(fullLocalPath)
FS-->>Host: Stream
Host-->>Client: 200 OK with file stream
end
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/Elastic.Changelog/Rendering/BundleValidationService.cs (1)
243-247:⚠️ Potential issue | 🟠 MajorValidate file path references against bundle directory boundary.
Line 243 joins untrusted
entry.File.Name(sourced from bundle YAML) directly without containment checks.Path.Joinpreserves..segments—an attacker can craftentry.File.Name: ../../../etc/passwdto escapebundleDirectory. No tests exist for this scenario, and no validation guards follow the join.Suggested fix
- var filePath = fileSystem.Path.Join(bundleDirectory, entry.File.Name); + var bundleRoot = fileSystem.Path.GetFullPath(bundleDirectory); + var filePath = fileSystem.Path.GetFullPath(fileSystem.Path.Join(bundleRoot, entry.File.Name)); + var relativePath = fileSystem.Path.GetRelativePath(bundleRoot, filePath); + if (relativePath.StartsWith("..", StringComparison.Ordinal) || fileSystem.Path.IsPathRooted(relativePath)) + { + collector.EmitError(bundleFile, $"Referenced changelog file '{entry.File.Name}' resolves outside bundle directory"); + return false; + } if (!fileSystem.File.Exists(filePath)) { collector.EmitError(bundleFile, $"Referenced changelog file '{entry.File.Name}' does not exist at path: {filePath}"); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/Elastic.Changelog/Rendering/BundleValidationService.cs` around lines 243 - 247, The code builds filePath by joining bundleDirectory and untrusted entry.File.Name without containment checks, allowing path traversal; update the validation in BundleValidationService (around filePath, bundleDirectory, entry.File.Name usage) to compute canonical/absolute paths (e.g., Path.GetFullPath on both bundleDirectory and the joined path), reject if the resulting filePath is outside the bundleDirectory boundary or if entry.File.Name is an absolute path, and call collector.EmitError with a clear message and return false when the check fails; ensure the containment check is robust to OS path-casing rules and trailing separators before proceeding to File.Exists.
🧹 Nitpick comments (1)
src/services/Elastic.Documentation.Assembler/Configuration/ConfigurationCloneService.cs (1)
68-68: Usefs.Path.Joinfor consistency with the file system abstraction.The rest of this file uses the injected
FileSystem fsabstraction (fs.DirectoryInfo,fs.File,fs.Directory), but this line uses the staticPath.Join. For testability and consistency, use the abstraction.Suggested fix
- var gitRefInformationFile = Path.Join(checkoutFolder.FullName, "config", "git-ref.txt"); + var gitRefInformationFile = fs.Path.Join(checkoutFolder.FullName, "config", "git-ref.txt");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/Elastic.Documentation.Assembler/Configuration/ConfigurationCloneService.cs` at line 68, Replace the static System.IO.Path.Join usage with the injected file system abstraction to keep testability consistent: in ConfigurationCloneService, change the creation of the gitRefInformationFile variable (named gitRefInformationFile) to use the injected FileSystem (fs.Path.Join) instead of Path.Join, keeping the same inputs (checkoutFolder.FullName and "config/git-ref.txt") so the rest of the method continues to operate against the FileSystem abstraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Elastic.Documentation.LinkIndex/GitLinkIndexReader.cs`:
- Around line 52-54: The current check only blocks rooted paths but allows '..'
to traverse out of CloneDirectory; update the validation in GitLinkIndexReader
to compute the absolute path of the joined path (e.g.,
Path.GetFullPath(Path.Join(CloneDirectory, _environment, "link-index.json"))),
compute the absolute path of CloneDirectory, and ensure the file path starts
with the CloneDirectory full path (or otherwise rejects when the resolved path
is outside CloneDirectory); also apply the same fix to the other block that
constructs the path (lines referencing Path.Join and _environment around the
66–69 area). Ensure you still keep rejecting rooted inputs and fold both checks
into the same secure full-path containment validation.
In `@src/Elastic.Markdown/Extensions/DetectionRules/DetectionRuleFile.cs`:
- Around line 113-116: The relative path trimming only checks for "../" (using
Path.GetRelativePath result), which fails on Windows where the parent-separator
is "..\..."; update the logic that computes relative (the variable from
Path.GetRelativePath(rulePath.FullName, build.OutputDirectory.FullName) or
equivalent) to detect and remove a leading parent-directory segment using both
separators (e.g., check StartsWith("../") OR StartsWith("..\\") or normalize
separators via Path.DirectorySeparatorChar/Path.AltDirectorySeparatorChar)
before calling Path.Join to produce newPath; ensure the check references the
existing variables relative, rulePath and build.OutputDirectory and preserves
cross-platform behavior.
In `@src/Elastic.Markdown/HtmlWriter.cs`:
- Around line 83-90: The code uses Path.Join (filesystem paths) to build URLs
which yields backslashes on Windows; update the URL construction in the
HtmlWriter to use URL-safe forward‑slash concatenation instead of Path.Join:
build the editUrl by joining relativeSourcePath and markdown.RelativePath with
"/" (not Path.Join) before interpolating into editUrl, and when creating
reportLinkParameter/reportUrl join DocumentationSet.Context.UrlPathPrefix and
current.Url with "/" (or normalize to ensure a single leading/trailing slash) so
the Uri and GitHub link are valid across platforms; adjust any related helpers
in HtmlWriter.cs that produce these strings.
In
`@src/services/Elastic.Documentation.Assembler/Sourcing/RepositorySourcesFetcher.cs`:
- Line 38: The code builds filesystem paths using repo.Name (e.g., when creating
checkoutFolder via Path.Join) without validating or sanitizing it, which allows
path traversal like "../outside"; update the logic that uses repo.Name in
RepositorySourcesFetcher (the checkoutFolder and the other Path.Join usage) to
validate or sanitize the name before joining: reject or normalize names
containing path-separators or "..", or replace repo.Name with
Path.GetFileName(repo.Name) (or a safe-sanitization function) and throw/log an
error for invalid values so callers cannot escape the intended checkout
directory. Ensure the same validation is applied to both places where repo.Name
is joined into paths.
In `@src/tooling/docs-builder/Http/DocumentationWebHost.cs`:
- Around line 216-217: The code builds a file path from user-controlled slug and
then reads it; to prevent path traversal ensure the resolved path stays inside
holder.ApiPath.FullName: after joining holder.ApiPath.FullName and slug (and
"index.html"), call Path.GetFullPath on the resulting path and also
Path.GetFullPath on holder.ApiPath.FullName (append a trailing separator) and
verify the resolved path starts with the canonical base path; if it does not,
reject the request (return 404/forbidden) instead of calling
_writeFileSystem.FileInfo.New(path). Update the logic in DocumentationWebHost
where path is constructed (the block using holder.ApiPath.FullName,
slug.Trim('/'), and _writeFileSystem.FileInfo.New) to perform this
canonicalization and containment check before any file reads.
In `@src/tooling/docs-builder/Http/StaticWebHost.cs`:
- Around line 114-118: The code constructs localPath from user-controlled slug
and may allow path traversal; compute the canonical absolute paths using
Path.GetFullPath for both the joined slug path and _contentRoot, normalize
separators, then verify the slug-derived full path starts with the content-root
full path (use StringComparison.OrdinalIgnoreCase on Windows) before creating
FileInfo/DirectoryInfo or reading files; if the check fails, treat it as not
found or return a 403/404 instead of accessing the filesystem (also apply the
same check when resolving index.html).
In `@tests-integration/Elastic.Assembler.IntegrationTests/SiteNavigationTests.cs`:
- Line 54: The code uses redundant nested Path.Join calls in the expression
fs.DirectoryInfo.New(fs.Path.Join(Path.Join(CheckoutDirectory.FullName, name)));
— simplify by removing the inner Path.Join and pass both segments to
fs.Path.Join directly (e.g., change to
fs.DirectoryInfo.New(fs.Path.Join(CheckoutDirectory.FullName, name))); so the
path is built in one call; update the expression where it appears (the
fs.DirectoryInfo.New(...) invocation) accordingly.
---
Outside diff comments:
In `@src/services/Elastic.Changelog/Rendering/BundleValidationService.cs`:
- Around line 243-247: The code builds filePath by joining bundleDirectory and
untrusted entry.File.Name without containment checks, allowing path traversal;
update the validation in BundleValidationService (around filePath,
bundleDirectory, entry.File.Name usage) to compute canonical/absolute paths
(e.g., Path.GetFullPath on both bundleDirectory and the joined path), reject if
the resulting filePath is outside the bundleDirectory boundary or if
entry.File.Name is an absolute path, and call collector.EmitError with a clear
message and return false when the check fails; ensure the containment check is
robust to OS path-casing rules and trailing separators before proceeding to
File.Exists.
---
Nitpick comments:
In
`@src/services/Elastic.Documentation.Assembler/Configuration/ConfigurationCloneService.cs`:
- Line 68: Replace the static System.IO.Path.Join usage with the injected file
system abstraction to keep testability consistent: in ConfigurationCloneService,
change the creation of the gitRefInformationFile variable (named
gitRefInformationFile) to use the injected FileSystem (fs.Path.Join) instead of
Path.Join, keeping the same inputs (checkoutFolder.FullName and
"config/git-ref.txt") so the rest of the method continues to operate against the
FileSystem abstraction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c25fdfa-eb08-452e-9b5e-b65cbe2bbf29
📒 Files selected for processing (103)
src/Elastic.ApiExplorer/OpenApiGenerator.cssrc/Elastic.Codex/Building/CodexBuildService.cssrc/Elastic.Codex/CodexContext.cssrc/Elastic.Codex/CodexGenerator.cssrc/Elastic.Codex/Sourcing/CodexCloneService.cssrc/Elastic.Codex/Sourcing/CodexGitRepository.cssrc/Elastic.Documentation.Configuration/Assembler/AssemblyConfiguration.cssrc/Elastic.Documentation.Configuration/BuildContext.cssrc/Elastic.Documentation.Configuration/Builder/ConfigurationFile.cssrc/Elastic.Documentation.Configuration/Builder/RedirectFile.cssrc/Elastic.Documentation.Configuration/ConfigurationFileProvider.cssrc/Elastic.Documentation.Configuration/Paths.cssrc/Elastic.Documentation.Configuration/ReleaseNotes/BundleLoader.cssrc/Elastic.Documentation.Configuration/Toc/DocumentationSetFile.cssrc/Elastic.Documentation.LegacyDocs/LegacyPageService.cssrc/Elastic.Documentation.LegacyDocs/PagesProvider.cssrc/Elastic.Documentation.LinkIndex/GitLinkIndexReader.cssrc/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cssrc/Elastic.Documentation.Links/InboundLinks/LinkIndexLinkChecker.cssrc/Elastic.Documentation.Navigation/Isolated/Node/DocumentationSetNavigation.cssrc/Elastic.Documentation.Site/FileProviders/EmbeddedOrPhysicalFileProvider.cssrc/Elastic.Documentation/Extensions/IFileInfoExtensions.cssrc/Elastic.Documentation/GitCheckoutInformation.cssrc/Elastic.Markdown/DocumentationGenerator.cssrc/Elastic.Markdown/Exporters/ConfigurationExporter.cssrc/Elastic.Markdown/Exporters/LlmMarkdownExporter.cssrc/Elastic.Markdown/Extensions/DetectionRules/DetectionRule.cssrc/Elastic.Markdown/Extensions/DetectionRules/DetectionRuleFile.cssrc/Elastic.Markdown/HtmlWriter.cssrc/Elastic.Markdown/IO/DocumentationSet.cssrc/Elastic.Markdown/Myst/Directives/CsvInclude/CsvIncludeBlock.cssrc/Elastic.Markdown/Myst/Directives/Include/IncludeBlock.cssrc/Elastic.Markdown/Myst/Directives/Settings/SettingsBlock.cssrc/Elastic.Markdown/Myst/InlineParsers/DiagnosticLinkInlineParser.cssrc/authoring/Elastic.Documentation.Refactor/Move.cssrc/authoring/Elastic.Documentation.Refactor/Tracking/LocalChangesService.cssrc/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cssrc/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cssrc/services/Elastic.Changelog/Bundling/ChangelogRemoveService.cssrc/services/Elastic.Changelog/Configuration/ChangelogConfigurationLoader.cssrc/services/Elastic.Changelog/Creation/ChangelogFileWriter.cssrc/services/Elastic.Changelog/Evaluation/ChangelogPrEvaluationService.cssrc/services/Elastic.Changelog/GithubRelease/GitHubReleaseChangelogService.cssrc/services/Elastic.Changelog/Rendering/Asciidoc/ChangelogAsciidocRenderer.cssrc/services/Elastic.Changelog/Rendering/BundleDataResolver.cssrc/services/Elastic.Changelog/Rendering/BundleValidationService.cssrc/services/Elastic.Changelog/Rendering/FeatureHidingLoader.cssrc/services/Elastic.Changelog/Rendering/Markdown/MarkdownRendererBase.cssrc/services/Elastic.Documentation.Assembler/AssembleContext.cssrc/services/Elastic.Documentation.Assembler/Building/AssemblerBuildService.cssrc/services/Elastic.Documentation.Assembler/Building/AssemblerBuilder.cssrc/services/Elastic.Documentation.Assembler/Building/SitemapBuilder.cssrc/services/Elastic.Documentation.Assembler/Configuration/ConfigurationCloneService.cssrc/services/Elastic.Documentation.Assembler/Deploying/Synchronization/AwsS3SyncApplyStrategy.cssrc/services/Elastic.Documentation.Assembler/Deploying/Synchronization/AwsS3SyncPlanStrategy.cssrc/services/Elastic.Documentation.Assembler/Navigation/AssemblerDocumentationSet.cssrc/services/Elastic.Documentation.Assembler/Navigation/GlobalNavigationPathProvider.cssrc/services/Elastic.Documentation.Assembler/Navigation/NavigationPrefixChecker.cssrc/services/Elastic.Documentation.Assembler/Sourcing/GitFacade.cssrc/services/Elastic.Documentation.Assembler/Sourcing/RepositorySourcesFetcher.cssrc/services/Elastic.Documentation.Isolated/IsolatedBuildService.cssrc/tooling/docs-builder/Arguments/BundleInputParser.cssrc/tooling/docs-builder/Commands/ChangelogCommand.cssrc/tooling/docs-builder/Commands/Codex/CodexCommands.cssrc/tooling/docs-builder/Filters/CheckForUpdatesFilter.cssrc/tooling/docs-builder/Http/DocumentationWebHost.cssrc/tooling/docs-builder/Http/LiveReload.cssrc/tooling/docs-builder/Http/ParcelWatchService.cssrc/tooling/docs-builder/Http/ReloadableGeneratorState.cssrc/tooling/docs-builder/Http/StaticWebHost.cstests-integration/Elastic.Assembler.IntegrationTests/AssemblerConfigurationTests.cstests-integration/Elastic.Assembler.IntegrationTests/DocsSyncTests.cstests-integration/Elastic.Assembler.IntegrationTests/SiteNavigationTests.cstests/Elastic.Changelog.Tests/Changelogs/BundleAmendTests.cstests/Elastic.Changelog.Tests/Changelogs/BundleChangelogsTests.cstests/Elastic.Changelog.Tests/Changelogs/BundleProfileGitHubReleaseTests.cstests/Elastic.Changelog.Tests/Changelogs/BundleReleaseVersionTests.cstests/Elastic.Changelog.Tests/Changelogs/ChangelogConfigurationTests.cstests/Elastic.Changelog.Tests/Changelogs/ChangelogRemoveTests.cstests/Elastic.Changelog.Tests/Changelogs/Create/CreateChangelogTestBase.cstests/Elastic.Changelog.Tests/Changelogs/Create/PrIntegrationTests.cstests/Elastic.Changelog.Tests/Changelogs/Create/ReleaseVersionTests.cstests/Elastic.Changelog.Tests/Changelogs/RemoveReleaseVersionTests.cstests/Elastic.Changelog.Tests/Changelogs/Render/BasicRenderTests.cstests/Elastic.Changelog.Tests/Changelogs/Render/BundleValidationTests.cstests/Elastic.Changelog.Tests/Changelogs/Render/ChecksumValidationTests.cstests/Elastic.Changelog.Tests/Changelogs/Render/DuplicateHandlingTests.cstests/Elastic.Changelog.Tests/Changelogs/Render/ErrorHandlingTests.cstests/Elastic.Changelog.Tests/Changelogs/Render/HideFeaturesTests.cstests/Elastic.Changelog.Tests/Changelogs/Render/HighlightsRenderTests.cstests/Elastic.Changelog.Tests/Changelogs/Render/OutputFormatTests.cstests/Elastic.Changelog.Tests/Changelogs/Render/TitleTargetTests.cstests/Elastic.Documentation.Build.Tests/SitemapTests.cstests/Elastic.Documentation.Configuration.Tests/PhysicalDocsetTests.cstests/Elastic.Documentation.Configuration.Tests/VersionInferenceTests.cstests/Elastic.Markdown.Tests/Directives/DirectiveBaseTests.cstests/Elastic.Markdown.Tests/DocSet/BreadCrumbTests.cstests/Elastic.Markdown.Tests/DocSet/NestedTocTests.cstests/Elastic.Markdown.Tests/MockFileSystemExtensions.cstests/Elastic.Markdown.Tests/Mover/MoverTests.cstests/Elastic.Markdown.Tests/SettingsInclusion/IncludeTests.cstests/Navigation.Tests/Isolation/FileInfoValidationTests.cstests/Navigation.Tests/Isolation/PhysicalDocsetTests.cs
…RemoveService Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… fixes, and shared utilities - Add UrlPath static class for URL-safe path joining (always '/', normalises backslashes) - Add Paths.ValidateSinglePathSegment shared utility; remove duplicated copies - GitLinkIndexReader: replace IsPathRooted check with full-path containment validation to block '..' traversal segments - DetectionRuleFile: handle Windows path separator in relative path prefix trim - HtmlWriter: use UrlPath.Join/JoinUrl instead of Path.Join for URL construction - RepositorySourcesFetcher: use Paths.ValidateSinglePathSegment before joining repo.Name into filesystem paths - DocumentationWebHost: verify slug-derived path stays under ApiPath - StaticWebHost: verify slug-derived path stays under content root - SiteNavigationTests: remove redundant nested Path.Join call Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/services/Elastic.Documentation.Assembler/Sourcing/RepositorySourcesFetcher.cs`:
- Around line 139-146: The CloneRef method in RepositorySourcesFetcher drops
non-default flags when it retries (appendRepositoryName and assumeCloned),
causing retries to use different checkoutFolder and behavior; update all retry
paths inside CloneRef (and any recursive/internal retry calls referenced around
CloneRef) to forward the original appendRepositoryName and assumeCloned
parameters (and pull/attempt as appropriate) so the same checkoutFolder
resolution is used on retry, and ensure any constructed checkoutFolder uses
those forwarded values rather than defaulting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e71ace6-13b1-45b0-bb64-c639208579fe
📒 Files selected for processing (9)
src/Elastic.Documentation.Configuration/Paths.cssrc/Elastic.Documentation.LinkIndex/GitLinkIndexReader.cssrc/Elastic.Documentation/Extensions/UrlPath.cssrc/Elastic.Markdown/Extensions/DetectionRules/DetectionRuleFile.cssrc/Elastic.Markdown/HtmlWriter.cssrc/services/Elastic.Documentation.Assembler/Sourcing/RepositorySourcesFetcher.cssrc/tooling/docs-builder/Http/DocumentationWebHost.cssrc/tooling/docs-builder/Http/StaticWebHost.cstests-integration/Elastic.Assembler.IntegrationTests/SiteNavigationTests.cs
✅ Files skipped from review due to trivial changes (3)
- src/Elastic.Markdown/HtmlWriter.cs
- src/Elastic.Markdown/Extensions/DetectionRules/DetectionRuleFile.cs
- src/Elastic.Documentation/Extensions/UrlPath.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- tests-integration/Elastic.Assembler.IntegrationTests/SiteNavigationTests.cs
- src/tooling/docs-builder/Http/StaticWebHost.cs
- src/tooling/docs-builder/Http/DocumentationWebHost.cs
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Retry calls were dropping non-default arguments, causing retries to resolve a different checkout folder than the initial attempt. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Path.CombinewithPath.Joinacross 103 C# files (src, tests, integration tests)Path.Joinis preferred because it does not treat absolute path segments as rooted, avoiding potential path traversal issues (see Need Span based path join API dotnet/runtime#24263)System.IO.Abstractionsv22.1.0 in use supportsIPath.JoinTest plan
🤖 Generated with Claude Code