Skip to content

fix: normalisePath and normalisePathWin incorrectly return changed=true when path is unmodified - #1672

Open
fzipi with Copilot wants to merge 2 commits into
mainfrom
copilot/normalise-path-bug-fix
Open

fix: normalisePath and normalisePathWin incorrectly return changed=true when path is unmodified#1672
fzipi with Copilot wants to merge 2 commits into
mainfrom
copilot/normalise-path-bug-fix

Conversation

Copilot AI commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

normalisePath hardcoded changed = true for any path ending with /, even when filepath.Clean(data) + "/" equals the original input. normalisePathWin had a related bug: it discarded the changed flag from normalisePath and never accounted for whether the backslash→slash replacement itself constituted a change.

Changes

  • normalise_path.go: Replace hardcoded true in the trailing-slash branch with a proper comparison:

    // Before
    return clean + "/", true, nil
    
    // After
    result := clean + "/"
    return result, data != result, nil
  • normalise_path_win.go: Compare the final normalized result against the original input (pre-replacement) so both path normalization and backslash conversion are reflected in changed:

    replaced := strings.ReplaceAll(data, "\\", "/")
    result, _, err := normalisePath(replaced)
    return result, data != result, err
  • Test data: Added "//" cases (ret: 0) to normalisePath.json and normalisePathWin.json.

Examples of corrected behavior

Input Output changed (before) changed (after)
// // true false
../ ../ true false
../../ ../../ true false
dir/../foo/ foo/ true true
\\foo\\bar (win) /foo/bar false true

Summary by CodeRabbit

  • Bug Fixes

    • Improved path normalization so unchanged paths are correctly reported as unchanged.
    • Preserved Windows path handling without modifying the original input.
    • Correctly handles paths consisting of double slashes.
  • Tests

    • Added coverage for double-slash paths on Unix-style and Windows-style systems.

Copilot AI changed the title [WIP] Fix bug in normalisePath function for accurate change reporting fix: normalisePath and normalisePathWin incorrectly return changed=true when path is unmodified Jul 26, 2026
Copilot finished work on behalf of fzipi July 26, 2026 14:59
Copilot AI requested a review from fzipi July 26, 2026 14:59
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.78%. Comparing base (15ceac3) to head (da99412).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1672   +/-   ##
=======================================
  Coverage   87.78%   87.78%           
=======================================
  Files         178      178           
  Lines        9151     9153    +2     
=======================================
+ Hits         8033     8035    +2     
  Misses        854      854           
  Partials      264      264           
Flag Coverage Δ
coraza.no_memoize 87.87% <100.00%> (+<0.01%) ⬆️
coraza.rule.case_sensitive_args_keys 87.75% <100.00%> (+<0.01%) ⬆️
coraza.rule.mandatory_rule_id_check 87.77% <100.00%> (+<0.01%) ⬆️
coraza.rule.multiphase_evaluation 87.54% <100.00%> (+<0.01%) ⬆️
coraza.rule.no_regex_multiline 87.74% <100.00%> (+<0.01%) ⬆️
coraza.rule.rx_prefilter 87.78% <100.00%> (+<0.01%) ⬆️
default 87.78% <100.00%> (+<0.01%) ⬆️
examples+ 16.40% <0.00%> (-0.01%) ⬇️
examples+coraza.no_memoize 85.74% <100.00%> (+<0.01%) ⬆️
examples+coraza.rule.case_sensitive_args_keys 85.72% <100.00%> (+<0.01%) ⬆️
examples+coraza.rule.mandatory_rule_id_check 85.83% <100.00%> (+<0.01%) ⬆️
examples+coraza.rule.multiphase_evaluation 87.54% <100.00%> (+<0.01%) ⬆️
examples+coraza.rule.no_regex_multiline 85.65% <100.00%> (+<0.01%) ⬆️
examples+coraza.rule.rx_prefilter 86.00% <100.00%> (+<0.01%) ⬆️
examples+no_fs_access 85.07% <100.00%> (+<0.01%) ⬆️
ftw 87.78% <100.00%> (+<0.01%) ⬆️
no_fs_access 87.12% <100.00%> (+<0.01%) ⬆️
tinygo 87.78% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fzipi
fzipi marked this pull request as ready for review July 26, 2026 15:35
@fzipi
fzipi requested a review from a team as a code owner July 26, 2026 15:35
@fzipi

fzipi commented Jul 26, 2026

Copy link
Copy Markdown
Member

@coderabbitai full_review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

@fzipi: I’ll perform a full review of the pull request, including the changed normalization logic and its test coverage.

✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6c62b554-3b7d-46e4-9c39-478112df1248

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The path normalization functions now compute the changed flag by comparing the normalized output with the original input. Test data adds unchanged // cases for both Unix and Windows normalization.

Changes

Path normalization

Layer / File(s) Summary
Correct base path change detection
internal/transformations/normalise_path.go, internal/transformations/testdata/normalisePath.json
normalisePath compares the trailing-slash result with the original input, and test data verifies that // returns unchanged.
Propagate original-input comparison
internal/transformations/normalise_path_win.go, internal/transformations/testdata/normalisePathWin.json
normalisePathWin retains the original input while normalizing separators and compares it with the result; // coverage is added.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

  • corazawaf/coraza#1660: Updates Windows path normalization to derive the changed flag from the original and normalized paths.

Suggested labels: v3.8

Suggested reviewers: fzipi

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the bug fix for changed-flag reporting in both path normalization functions.
Linked Issues check ✅ Passed The changes address issue #1670 by correcting changed detection in both functions and adding unchanged-path test cases.
Out of Scope Changes check ✅ Passed The PR stays focused on the path-normalization bug fix and matching tests, with no unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/transformations/testdata/normalisePath.json`:
- Around line 218-224: Update transformations_test.go to capture the
normalisePath changed boolean and assert it against data.Ret != 0. In
internal/transformations/testdata/normalisePath.json lines 218-224, retain the
unchanged // vector with ret: 0; in
internal/transformations/testdata/normalisePathWin.json lines 218-224, add
unchanged // and backslash-separator conversion vectors, with the latter using
ret: 1 to verify changed=true.
🪄 Autofix (Beta)

✅ Autofix completed


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 94086f40-ce46-48ec-8dbd-c54cc493dea5

📥 Commits

Reviewing files that changed from the base of the PR and between 2cea9a0 and 9522666.

📒 Files selected for processing (4)
  • internal/transformations/normalise_path.go
  • internal/transformations/normalise_path_win.go
  • internal/transformations/testdata/normalisePath.json
  • internal/transformations/testdata/normalisePathWin.json

Comment thread internal/transformations/testdata/normalisePath.json
@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 2 file(s) based on 1 unresolved review comment.

Files modified:

  • internal/transformations/testdata/normalisePathWin.json
  • internal/transformations/transformations_test.go

Commit: 80eda40bcea887bfa57457eddb95ca31f645ccfd

The changes have been pushed to the copilot/normalise-path-bug-fix branch.

Time taken: 6m 30s

Copilot AI and others added 2 commits August 4, 2026 08:31
…h is unchanged

- normalisePath: when path ends with '/', compare result to original instead of always returning true
- normalisePathWin: compare result against original input (before backslash replacement) so backslash-only changes are correctly reported
- Add // test cases to both JSON test data files

Fixes #1670
Fixed 2 file(s) based on 1 unresolved review comment.

Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
@fzipi
fzipi force-pushed the copilot/normalise-path-bug-fix branch from 80eda40 to da99412 Compare August 4, 2026 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

normalise path and normalise path win too transformations return changed true for some cases even if the path not touched

2 participants