Skip to content

fix(transport/http): escape path parameters safely - #3856

Open
ZYZ666-RGB wants to merge 1 commit into
go-kratos:mainfrom
ZYZ666-RGB:fix/kratos-path-escaping
Open

fix(transport/http): escape path parameters safely#3856
ZYZ666-RGB wants to merge 1 commit into
go-kratos:mainfrom
ZYZ666-RGB:fix/kratos-path-escaping

Conversation

@ZYZ666-RGB

Copy link
Copy Markdown

Description (what this PR does / why we need it):

BuildPath previously substituted path parameter values without escaping them. Values containing spaces, ?, or # could therefore corrupt the URL structure or inject query/fragment delimiters.

This change:

  • Escapes each slash-separated path segment with url.PathEscape.
  • Preserves / for resource names such as publishers/go/books/kratos.
  • Preserves RFC 3986 sub-delimiters that were already accepted by BuildPath, avoiding compatibility regressions.
  • Adds regression coverage for query/fragment characters, spaces, resource-name slashes, and valid path-segment characters.

Which issue(s) this PR fixes (resolves / be part of):

Fixes #3853

Other special notes for the reviewers:

Focused tests pass:

go test ./transport/http -run '^TestBuildPath' -count=1

@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jul 21, 2026

Copy link
Copy Markdown
Author

Hi maintainers, the Non-English Comments Check failed before the scanning step ran.

The failure occurred in actions/checkout@v6: it refused to check out fork code because this workflow uses pull_request_target, which runs in the trusted base-repository context. The subsequent non-English comment scan was skipped.

I also reproduced the workflow's grep check locally against the two changed files, and it found no non-English comments.

Failed run: https://github.com/go-kratos/kratos/actions/runs/29814185538

Could you please review the workflow configuration or re-run the appropriate check after it is adjusted? Thank you!

Copilot AI 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.

Pull request overview

This PR hardens transport/http URL path construction by escaping substituted path parameters in BuildPath, preventing injected ?, #, and spaces from corrupting the resulting request URL while preserving / for resource-name style parameters.

Changes:

  • Escape each slash-separated path segment via url.PathEscape, joining segments to preserve / in resource names.
  • Restore a small set of RFC 3986 path-segment sub-delimiters that url.PathEscape would otherwise escape to avoid compatibility regressions.
  • Add regression tests covering query/fragment injection, spaces, resource-name slashes, and valid path-segment characters.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
transport/http/path.go Escapes substituted path parameter values safely (segment-wise) while preserving / and selected valid sub-delimiters.
transport/http/path_test.go Adds test coverage to prevent regressions for escaping behavior and preserved characters.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Author

Hi maintainers, just following up on this PR. The Go and Lint checks are green, and the focused tests cover query/fragment characters, spaces, resource-name slashes, and preserved valid path-segment characters. Please let me know if there are any compatibility cases or changes you would like me to address. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: URL path parameters can break URL structure due to lack of partial escaping

2 participants