Add redirect URL and docfx file path checks - #736
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and security issues in the new docfx path validation (URI detection and path traversal), plus a mismatch with the PR’s “modified docfx.json file(s)” scope and solution-file formatting issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds new verification steps to the docs-verifier action to (a) validate redirect target URLs in modified redirection JSON files and (b) validate file-path references in modified docfx.json files, with accompanying unit tests.
Changes:
- Add
RedirectionVerifier.RedirectTargetVerifierto validateredirect_urlvalues (including 404 checks) in redirection files. - Add
DocfxVerifier.PathVerifierto validate that configured docfx path entries point to existing files/directories. - Wire both checks into
ActionRunner, and add unit tests + project references.
File summaries
| File | Description |
|---|---|
| actions/docs-verifier/tests/GitHub.UnitTests/RedirectTargetVerifierTests.cs | Adds unit tests for redirect URL validation (valid/invalid/404). |
| actions/docs-verifier/tests/GitHub.UnitTests/PathVerifierTests.cs | Adds unit tests for docfx path validation (valid paths, fileMetadata keys, subdir docfx.json). |
| actions/docs-verifier/tests/GitHub.UnitTests/GitHub.UnitTests.csproj | Adds project references needed by the new unit tests. |
| actions/docs-verifier/src/RedirectionVerifier/RedirectTargetVerifier.cs | Introduces redirect target URL validation logic with network checks. |
| actions/docs-verifier/src/ActionRunner/Program.cs | Integrates docfx path checks and redirect URL checks into the action runner flow. |
| actions/docs-verifier/src/ActionRunner/ActionRunner.csproj | Adds reference to the new DocfxVerifier project. |
| actions/docs-verifier/MSDocsBuildVerifier.sln | Adds DocfxVerifier to the solution and updates solution metadata. |
| actions/docs-verifier/DocfxVerifier/PathVerifier.cs | Introduces JSON traversal + path validation for docfx.json fields. |
| actions/docs-verifier/DocfxVerifier/DocfxVerifier.csproj | Adds the new verifier project. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fcb63d6 to
26810f6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new validation has SSRF exposure and incorrectly handles several valid Docfx configuration forms.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
actions/docs-verifier/src/DocfxVerifier/PathVerifier.cs:193
- Path resolution loses the surrounding file-mapping context. In Docfx,
filesandexcludeare relative to their siblingsrc, but every value here is checked only against the repository root or config directory. For example,src: "content"withfiles: ["guides/a.md"]incorrectly fails when the valid file is atcontent/guides/a.md; conversely, an unrelated root-level path can incorrectly make it pass. Track each mapping's effectivesrcand validate its patterns against that base.
bool existsRelativeToRoot = ExistsInRepository(repositoryRoot, nonWildcardPrefix);
bool existsRelativeToConfig = ExistsInRepository(configurationDirectory, nonWildcardPrefix);
if (!existsRelativeToRoot && !existsRelativeToConfig)
actions/docs-verifier/src/DocfxVerifier/PathVerifier.cs:208
- This containment check rejects every
../path relative to a nested docfx.json, even when the resolved target remains inside the repository. Docfx paths are config-relative, so a validdocs/docfx.jsonentry such as../sharedis reported invalid. Resolve from the config directory, then enforce containment againstrepositoryRootrather than against the resolution base.
if (relative.Equals("..", StringComparison.Ordinal)
|| relative.StartsWith(".." + Path.DirectorySeparatorChar, StringComparison.Ordinal)
|| relative.StartsWith(".." + Path.AltDirectorySeparatorChar, StringComparison.Ordinal))
{
return false;
actions/docs-verifier/src/DocfxVerifier/PathVerifier.cs:139
- This array dispatch does not support Docfx's two valid file-mapping forms. String shorthand under
contentis ignored becausecontentis absent froms_pathArrayPropertyNames, while object entries underresourceoroverwriteenter this branch but are silently skipped. Handlecontent,resource, andoverwriteas file-mapping arrays that validate string items and recurse into object items.
if (propertyName is not null && s_pathArrayPropertyNames.Contains(propertyName))
{
int index = 0;
foreach (JsonElement item in element.EnumerateArray())
{
if (item.ValueKind == JsonValueKind.String)
{
ValidatePath(item.GetString(), $"{jsonPath}[{index}]", repositoryRoot, configurationDirectory, errors);
}
actions/docs-verifier/src/DocfxVerifier/PathVerifier.cs:95
- The recursive walk interprets matching property names anywhere in the JSON as Docfx path fields. Sections such as
globalMetadataand extension/plugin settings permit arbitrary user-defined keys, so a metadata value namedsrc,dest, orfilescan be incorrectly rejected as a missing path. Traverse schema-defined path locations instead of matching property names globally.
foreach (JsonProperty property in element.EnumerateObject())
{
string childPath = $"{jsonPath}.{property.Name}";
ValidateElement(property.Value, property.Name, childPath, repositoryRoot, configurationDirectory, errors);
actions/docs-verifier/src/DocfxVerifier/PathVerifier.cs:65
- This parser uses strict JSON even though the existing configuration reader explicitly accepts trailing commas (
BuildVerifier.IO.Abstractions/BaseConfigurationReader.cs:8-11). A modified docfx.json that the action already accepts can now throwJsonExceptionhere and abort the run instead of being verified. Parse with matchingJsonDocumentOptions.
using JsonDocument json = await JsonDocument.ParseAsync(stream);
- Files reviewed: 9/9 changed files
- Comments generated: 5
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
There was a problem hiding this comment.
🟡 Changes recommended
DocFX schema handling can reject valid configurations or miss paths, while redirect checks introduce SSRF and runtime-scaling risks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 10
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Redirect fetching remains SSRF-bypassable, and DocFX validation does not cover all promised path-bearing sections.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
actions/docs-verifier/src/DocfxVerifier/PathVerifier.cs:124
fileMetadataproperty names are file glob patterns, but this branch accepts every absolute HTTP(S) URL without checking a repository path. For example, a key namedhttps://invalid.example/file.mdis reported as valid even though it cannot identify a DocFX input file. Remove this URL exemption so such entries fail path validation.
if (Uri.TryCreate(path, UriKind.Absolute, out Uri? uri)
&& uri is not null
&& (uri.Scheme == Uri.UriSchemeHttp || uri.Scheme == Uri.UriSchemeHttps))
{
return;
actions/docs-verifier/src/RedirectionVerifier/RedirectTargetVerifier.cs:78
- The public-address check does not constrain the subsequent HTTP connection. The default
HttpClientautomatically follows redirects, so an attacker-controlled public URL can redirect to a loopback/private endpoint; DNS can also return a public address during this check and a private address whenHttpClientresolves it again. Because PR authors control these URLs, disable automatic redirects and validate every hop while binding the connection to the validated address (or otherwise perform the request through an SSRF-safe client).
HttpStatusCode? statusCode = await statusCodeProvider(uri!);
actions/docs-verifier/src/RedirectionVerifier/RedirectTargetVerifier.cs:46
- Each target is awaited serially, so verification time is the sum of every network request; with the 15-second timeout (and possible HEAD-then-GET fallback), a large redirection file can take hours and exceed the action timeout. Deduplicate targets and check them with bounded concurrency while collecting diagnostics deterministically.
for (int i = 0; i < redirections.Length; i++)
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
Uh oh!
There was an error while loading. Please reload this page.