Repository navigation
fix(seclang): use slash paths for fs.FS include roots on Windows - #1784
Conversation
FromFile joined relative Include paths and derived the include directory with filepath, so on Windows an fs.Sub, embed or mergefs root was handed backslash names that io/fs rejects. The same directory becomes ConfigDir, so @pmFromFile data next to an included file could not be found either. Convert those names to slashes unless the root is io.OSFS, which takes OS paths. UNC and device paths are left as they are. Nothing changes where the separator is already a slash.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely identifies the main change: using slash-separated paths for fs.FS include roots on Windows. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… |
| 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. |
| Adr Required For Structural Changes | |
Passed — not applicable. The diff changes only internal/seclang/parser.go and fixes Windows path handling in Parser.FromFile (internal/seclang/parser.go:58-105). It adds no directive, operator, … |
| Test Layer Discipline & Duplication | |
Not applicable. The pull request changes only internal/seclang/parser.go; it adds no _test.go file and does not touch testing/engine/ or testing/profile/. The custom check therefore has no tes… |
| Generated Content Not Hand-Edited | |
Not applicable. The pull request changes only internal/seclang/parser.go; it does not touch any generated .gen.go file or the specified English/Spanish Seclang documentation files. |
| Documentation Translation Parity | |
Not applicable. The pull request changes only internal/seclang/parser.go in the current repository and does not touch the coraza.io repository or any file under content/en/. |
| Cgo Memory Safety | |
Not applicable. The pull request changes only internal/seclang/parser.go; it does not change a .go file under libcoraza/ or any .c, .h, or .i file. |
| Breaking Changes To Public Api & Compatibility | |
No breaking public API or compatibility change is introduced. The PR changes only internal/seclang/parser.go: it adds unexported fsPath and normalizes relative include paths and ConfigDir for no… |
| Security Fix Process Compliance | |
Not applicable. The PR title, description, commit message, and changed code describe a Windows io/fs path correctness fix. They do not describe an exploitable vulnerability, attack sequence, working… |
| Owasp Security (Web & Supply Chain) | |
No OWASP security failure is introduced. The PR only normalizes parser include paths and ConfigDir for fs.FS roots in internal/seclang/parser.go:55-67. It does not add secrets, cryptography, dep… |
| Unpinned Dependencies & Actions | |
Passed: not applicable. The pull request changes only internal/seclang/parser.go; it does not change a dependency manifest, lockfile, Dockerfile, or workflow file covered by this check. |
| New Dependency Scrutiny | |
The pull request changes only internal/seclang/parser.go (+13/-2). The reviewed diff adds no entry to go.mod or package.json, and adds no workflow uses: step. The dependency scrutiny trigger d… |
| Install & Build-Time Code Execution | |
PASS. The pull request changes only internal/seclang/parser.go. The diff adds Parser.fsPath and updates path handling; it adds no installer, Docker build fetch, CI setup, shell script, Go checksum… |
| Secrets, Payloads & Pii In Logs | |
No logging or telemetry code changed. The added code only normalizes filesystem paths in internal/seclang/parser.go; it does not emit request/response data, credentials, tokens, base64 blobs, or aud… |
| Renovate: Config Present And Valid | |
The pull request changes only internal/seclang/parser.go. It does not touch any Renovate configuration path, and renovate.json exists at the PR head with the required $schema and `github>corazaw… |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.
Full details: Adr Required For Structural Changes
-
Explanation
Passed — not applicable. The diff changes only
internal/seclang/parser.goand fixes Windows path handling inParser.FromFile(internal/seclang/parser.go:58-105). It adds no directive, operator, action, transformation, variable, collection, subsystem, plugin, public API change, or performance-oriented algorithm change. This is an explicit bug fix, which the check exempts. No ADR file was added, and no ADR is required.
Full details: Test Layer Discipline & Duplication
-
Explanation
Not applicable. The pull request changes only
internal/seclang/parser.go; it adds no_test.gofile and does not touchtesting/engine/ortesting/profile/. The custom check therefore has no test-layer or duplication condition to evaluate.
Full details: Breaking Changes To Public Api & Compatibility
-
Explanation
No breaking public API or compatibility change is introduced. The PR changes only
internal/seclang/parser.go: it adds unexportedfsPathand normalizes relative include paths andConfigDirfor non-io.OSFSroots.Parser.FromFileandWithDirectivesFromFilesignatures remain unchanged, and OS-backed paths remain unchanged. The behavior fixes Windowsfs.FSincludes rather than invalidating a previously working configuration. Existing coverage includesTestEmbedFSandTestGlobIncludeFromSubdirectoryininternal/seclang/parser_test.go. No documentation update is required by this check.
Full details: Security Fix Process Compliance
-
Explanation
Not applicable. The PR title, description, commit message, and changed code describe a Windows
io/fspath correctness fix. They do not describe an exploitable vulnerability, attack sequence, working payload, impact, CVE, or GHSA. The repository guidance reserves private advisories for exploitable bugs (AGENTS.md:724-734), and this PR contains no such security disclosure.
Full details: Owasp Security (Web & Supply Chain)
-
Explanation
No OWASP security failure is introduced. The PR only normalizes parser include paths and
ConfigDirforfs.FSroots ininternal/seclang/parser.go:55-67. It does not add secrets, cryptography, dependencies, network requests, logging changes, or request-derived command/SQL/path construction. Include paths remain configuration input, and the existingfilepath.Joinbehavior is unchanged in purpose.
Full details: New Dependency Scrutiny
-
Explanation
The pull request changes only
internal/seclang/parser.go(+13/-2). The reviewed diff adds no entry togo.modorpackage.json, and adds no workflowuses:step. The dependency scrutiny trigger does not apply.
Full details: Install & Build-Time Code Execution
-
Explanation
PASS. The pull request changes only
internal/seclang/parser.go. The diff addsParser.fsPathand updates path handling; it adds no installer, Docker build fetch, CI setup, shell script, Go checksum bypass, or npm lifecycle script. No custom-check failure condition is introduced.
Full details: Secrets, Payloads & Pii In Logs
-
Explanation
No logging or telemetry code changed. The added code only normalizes filesystem paths in
internal/seclang/parser.go; it does not emit request/response data, credentials, tokens, base64 blobs, or audit records. Existing directive debug logging and read/parse error handling are unchanged. The pull request adds no fixtures or log files.
Full details: Renovate: Config Present And Valid
-
Explanation
The pull request changes only
internal/seclang/parser.go. It does not touch any Renovate configuration path, andrenovate.jsonexists at the PR head with the required$schemaandgithub>corazawaf/renovate-configextension. The trigger condition is not met.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
fzipi
left a comment
fzipi
left a comment
There was a problem hiding this comment.
Thanks, this is the right spot: every include path (Include, WithDirectivesFromFile) goes through FromFile, and the operators already path.Join ConfigDir, so this also fixes @pmFromFile data under an fs.FS root. On Linux and macOS fsPath is a no-op, so nothing changes there. Checked go test ./internal/seclang/... plus GOOS=windows go vet/go test -c locally.
Our CI only runs on Linux, so no test here fails without the fix; noting that the Windows result was checked by hand.
Non-blocking follow-up: in fsPath, the UNC/device check (len(filepath.VolumeName(name)) > 2) only runs for roots that are not io.OSFS, but the comment says only an OS-backed root can open those paths. If no non-OS root can open them, the condition can go; otherwise the comment could name the root type it protects.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1784 +/- ##
=======================================
Coverage 88.58% 88.58%
=======================================
Files 180 180
Lines 9601 9605 +4
=======================================
+ Hits 8505 8509 +4
Misses 824 824
Partials 272 272
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
On Windows,
Parser.FromFilebuilt relativeIncludepaths and the include directory withfilepath, sofs.Sub,embedand mergefs roots (coraza-caddy loads CRS this way) got backslash names, whichio/fsrejects. That directory also becomesConfigDir, which broke@pmFromFiledata next to an included file.Both are now converted to slashes unless the root is
io.OSFS, which takes OS paths. UNC and device paths are left alone, and nothing changes on Linux or macOS. This follows #1135 by @jabdr, without the build-tagged reader.TestEmbedFSandTestGlobIncludeFromSubdirectoryalready fail on Windows before this change and pass after, so I did not add a test. Tested with the seclang tests on Windows, and on Linux under the default, no_memoize, multiphase and no_regex_multiline tags.Part of #1140.
Summary by CodeRabbit