Skip to content

fix(parameters): avoid panic on double-slash request paths#275

Open
SAY-5 wants to merge 7 commits into
pb33f:mainfrom
SAY-5:fix-double-slash-panic
Open

fix(parameters): avoid panic on double-slash request paths#275
SAY-5 wants to merge 7 commits into
pb33f:mainfrom
SAY-5:fix-double-slash-panic

Conversation

@SAY-5

@SAY-5 SAY-5 commented May 21, 2026

Copy link
Copy Markdown

Fixes #274.

Request paths containing a leading double slash (e.g. //test/path/x) make strings.Split produce an extra empty segment, so submittedSegments ends up longer than pathSegments. The loop then indexes submittedSegments[x] against a segment the request never actually had, the regex returns nil, and matches[1:] panics with slice bounds out of range [1:0].

The patch skips iterations where x is out of bounds for submittedSegments, and treats a nil regex match as a non-match rather than panicking. Regression test in parameters/path_parameters_test.go exercises the exact panic stack from the issue.

@codecov

codecov Bot commented May 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.85714% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.21%. Comparing base (f309f59) to head (5c81fea).

Files with missing lines Patch % Lines
parameters/path_parameters.go 82.85% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #275      +/-   ##
==========================================
- Coverage   98.27%   98.21%   -0.06%     
==========================================
  Files          75       75              
  Lines        9192     9223      +31     
==========================================
+ Hits         9033     9058      +25     
- Misses        132      135       +3     
- Partials       27       30       +3     
Flag Coverage Δ
unittests 98.21% <82.85%> (-0.06%) ⬇️

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.

}

matches := rgx.FindStringSubmatch(submittedSegments[x])
if matches == nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this avoids the panic, but it can silently accept the bad request. For //test/path/fubar against /test/path/{param}, the extra empty segment shifts indexing, so {param} is validated against "path" instead of "fubar", and ValidateHttpRequestWithPathItem returns valid=true, errs=0. This should either normalize segments consistently and validate fubar, or return a validation error; current behavior is neither.

@daveshanley daveshanley May 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here is a suggestion.

submittedSegments := nonEmptyPathSegments(paths.StripRequestPath(request, v.document))
  pathSegments := nonEmptyPathSegments(pathValue)

  if len(submittedSegments) != len(pathSegments) {
        return false, []*errors.ValidationError{
                errors.PathParameterMissing(p, pathValue, request.URL.Path),
        }
  }

  for x := range pathSegments {
        var rgx *regexp.Regexp
        // existing regex cache/build code...

        matches := rgx.FindStringSubmatch(submittedSegments[x])
        if matches == nil {
                continue
        }

        matches = matches[1:]
        // existing match validation code...
  }

  Helper:

  func nonEmptyPathSegments(path string) []string {
        raw := strings.Split(path, helpers.Slash)
        segments := make([]string, 0, len(raw))

        for _, segment := range raw {
                if segment == "" {
                        continue
                }
                segments = append(segments, segment)
        }

        return segments
  }

The key is: don’t keep the original indexes after dropping/skipping empty template segments.
Either compact both sides first, or return a path/missing-param error when the segment counts
differ.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in the latest push: the approach now uses to drop empty segments from both sides before aligning them, so correctly validates against . For paths where segment counts still differ after stripping, the validator now returns a error (modeled on your suggestion). Test at line 2383 asserts the exact outcomes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Almost there... don’t silently continue on matches == nil for a templated segment. Return a path/parameter validation error there, and update the regression test to assert invalid/no panic, not only no panic.

Comment thread parameters/path_parameters_test.go Outdated
@SAY-5

SAY-5 commented May 24, 2026

Copy link
Copy Markdown
Author

Reworked per your suggestion: both sides now compact empty segments via a nonEmptyPathSegments helper, and a segment-count mismatch returns PathParameterMissing instead of silently passing. The regression test now asserts the request validates fubar (valid, no errors) and that a missing-segment path is rejected.

@SAY-5

SAY-5 commented May 25, 2026

Copy link
Copy Markdown
Author

Added a test covering the nil-match guard: TestValidatePathParamsWithPathItem_RegexNoMatchContinues uses a constrained template ({id:[0-9]+}) with a non-matching submitted value (abc), which triggers FindStringSubmatch returning nil and exercises the guard directly.

@SAY-5

SAY-5 commented May 26, 2026

Copy link
Copy Markdown
Author

Updated: matches==nil now appends a PathParameterMissing error and breaks rather than continuing silently. Test asserts valid=false, errs non-empty.

@daveshanley

Copy link
Copy Markdown
Member

matches == nil fix still misattributes errors on multi-param routes. In parameters/path_parameters.go line 92, the code appends PathParameterMissing(p, ...) before it knows whether the failing segment belongs to the current outer-loop parameter.

Running: /items/{id:[0-9]+}/{slug} with /items/abc/foo returns both the id and slug missing when the slug is present.

The fix should identify the path segment’s parameter first or refactor segment-first, and add a regression test.

@SAY-5

SAY-5 commented Jun 2, 2026

Copy link
Copy Markdown
Author

You're right, the misattribution was real. Pushed 3fa334d which only flags the current outer-loop parameter as missing when the failing template segment actually declares it; otherwise the regex failure is left for that segment's owning parameter to report. Added a regression test on /items/{id:[0-9]+}/{slug} with /items/abc/foo that asserts slug is not flagged missing.

Signed-off-by: Sai Asish Y <[email protected]>

# Conflicts:
#	parameters/path_parameters.go
@SAY-5

SAY-5 commented Jul 18, 2026

Copy link
Copy Markdown
Author

Rebased onto latest main to clear the conflict. The multi-param misattribution fix and its regression test are intact; go test ./parameters/ is green locally.

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.

ValidateHttpRequestWithPathItem panics for path with double slash

2 participants