fix(remote): resolve request URLs with net/url so an empty path keeps no trailing separator

[Aravinda-HWK]

Summary

Fixes the URL the remote client sends when the request path is empty or root, and replaces the hand-rolled join that caused it with net/url resolution.

The problem

Client.executeOnce joined the base URL and the request path by concatenation, always inserting a separator:

base := strings.TrimSuffix(c.baseURL, "/")
p := strings.TrimPrefix(path, "/")
finalURL = base + "/" + p

With an empty path that yields <base>/. /api/reports and /api/reports/ are different resources as far as HTTP is concerned, and some servers answer the trailing form with 405 Method Not Allowed — so a caller configured with the full service URL as its base, addressing that URL itself, could not reach the service.

The same concatenation carried two more defects, both surfaced during review:

1. Query parameters defeated the fix. JSONRequest appends the encoded Query to the path before the join runs, so an empty path arrives as "?id=42" — not empty. Any empty-path special case never fires and the trailing slash returns for every call carrying a query.
2. Path segments were not escaped. A space went out raw rather than percent-encoded.

The bug only appears when the base URL itself has a path: with a bare origin, net/http normalises an empty path to / on the wire either way. Every pre-existing test used a path-less httptest base URL, which is why none of them caught it.

Closes #146.

The solution

Resolution moved into a Client.resolveURL method built on net/url, so no part of the URL is assembled with string arithmetic:

ref, err := url.Parse(path)   // splits path from query and fragment
p := ref.EscapedPath()
if p == "/" { p = "" }
u := base.JoinPath(p)         // separators, escaping, dot-segments
// query and fragment carried on the URL, not spliced into a string

Why this shape:

• The query never needs splitting off. url.Parse puts it in RawQuery where it belongs, so the empty-path decision looks at the path alone. That removes defect (1) structurally rather than by special-casing ?.
• EscapedPath(), not Path. Joining the decoded form would turn an escaped separator (x%2Fy) into a real one, addressing a different resource than the caller asked for. This is the trap in the obvious version of this refactor, and there is a test pinning it.
• ResolveReference is not used, despite being the usual answer for URL joining. It follows RFC 3986 relative-reference semantics and replaces the last path segment: "send" against …/api/reports yields …/api/send, silently dropping reports.
• Query merges when both the base URL and the request carry one, rather than letting either side silently win.

One rule stays explicit, because no library can decide it: a root-only path resolves to the base URL itself. JoinPath preserves a trailing separator by design, so "/" would otherwise reproduce the original bug. Whether / means "the service URL" or "the collection under it" is a decision about this client's API, and it is commented as such.

Beyond the reported bug

Two behaviours the rewrite fixes for free, both covered by tests:

Input	Before	After
"sp ace"	raw space in the path	sp%20ace
"../other"	…/reports/../other sent literally	/api/other

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Refactoring (no functional changes)
• Performance improvement
• Other (please describe):

Changes Made

• remote/client.go — extracts URL resolution from executeOnce into Client.resolveURL, rewritten on net/url. The absolute-path branch keeps its existing scheme/host check against the configured service (the SSRF guard) and is unchanged in behaviour; the relative branch is the rewrite.
• remote/client_test.go — the table below.

Testing

• I have tested this change locally
• I have added tests that prove my fix is effective or that my feature works
• I have tested edge cases
• All existing tests pass

Ten table-driven cases under TestClient_BaseURL_Logic, asserting r.URL.EscapedPath() and r.URL.RawQuery as observed by an httptest server rather than re-deriving the join in the test:

Case	Path	Query	Expected path	Expected query
empty path posts to the service URL itself	""	—	/api/reports	—
root path is treated as empty	"/"	—	/api/reports	—
non-empty path is appended	reports	—	/api/reports	—
leading separator is not doubled	/reports	—	/api/reports	—
empty path with query	""	id=42	/api/reports	id=42
root path with query	"/"	id=42	/api/reports	id=42
non-empty path with query	/reports	id=42	/api/reports	id=42
query already in the path merges	?first=1	second=2	/api/reports	first=1&second=2
escaped separator stays escaped	x%2Fy	—	/api/x%2Fy	—
space in a segment is escaped	sp ace	—	/api/sp%20ace	—
dot segments resolved before sending	../other	—	/api/other	—

The query cases were written before the fix and observed to fail (expected "/api/reports", actual "/api/reports/"), so they demonstrate the defect rather than merely describing it.

go build ./..., go vet ./... and go test -race ./... all pass in the remote module, including the mTLS and manager suites, which exercise resolveURL through RawRequest.

Checklist

• My code follows the project's style guidelines
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have checked that there are no merge conflicts

Docs unticked: no user-facing documentation describes the join behaviour, and the reasoning lives in comments at the decision points.

Related Issues

Closes #146

Additional Notes

Lint not verified locally. The installed golangci-lint v2.11.2 is built against Go 1.25 and refuses the repo's go 1.26 target, so the lint gate is left to CI.

Behaviour changes worth naming:

1. Dot segments are now resolved rather than sent literally, and spaces are escaped. Both are corrections, but a caller depending on the previous literal passthrough would see a different request.
2. With no base URL configured and an empty path the final URL is now "" where it was "/". Both are unusable and both fail; the error moves from the transport into http.NewRequestWithContext.

[coderabbitai]

Important

Review available on request

• 🔍 Trigger review

Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment @coderabbitai review to review the latest changes. For a full review, comment @coderabbitai full review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 18827d13-4ff7-45e1-a5f2-a8e1a04a7836

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[ginaxu1]

what happens if a caller issues a request with an empty or root path AND query parameters (req.Query)? add a unit test for that

[Aravinda-HWK]

what happens if a caller issues a request with an empty or root path AND query parameters (req.Query)? add a unit test for that

@ginaxu1 I fixed this issue and added the unit test cases as well.

[ginaxu1]

LGTM

[sthanikan2000]

Verified the fix for the reported trailing-separator bug and the SSRF guard — both hold up. Found one confirmed regression that should block merge (inline below), plus one minor efficiency note.

[sthanikan2000]

url.Parse here reads a path segment like TASK:123 or ns:foo as scheme:opaque per RFC 3986, not as a literal path — and since resolveURL only reads EscapedPath()/RawQuery/Fragment off the result, an opaque parse means EscapedPath() comes back empty and the segment is silently dropped, sending the request to the bare base URL instead of the intended resource with no error at all. A digit-led variant (12:34) fails the other way — url.Parse rejects it outright ("first path segment in URL cannot contain colon"), breaking a path that worked before this change. Verified directly against this branch's resolveURL: path="TASK:123" resolves to http://example.com/api/reports (path silently dropped, no error); path="12:34" returns an error. Neither case is in the new test table.

Suggested fix: parse "./"+path instead of path. This isn't a workaround — RFC 3986 §4.2 documents this exact ambiguity and prescribes exactly this escape: "A path segment that contains a colon character... cannot be used as the first segment of a relative-path reference, as it would be mistaken for a scheme name. Such a segment must be preceded by a dot-segment (e.g., ./this:that)." JoinPath's internal path.Clean strips the ./ back out for free, so no extra stripping is needed. The root-path check at line 206 would need p == "./" instead of p == "/" to match. Confirmed this fix against all 10 existing test cases plus TASK:123, ns:foo, and 12:34 — all resolve correctly.

[sthanikan2000]

Minor: this now parses c.baseURL on every request in the relative-path branch, where the previous code did no parsing at all here — and it's on the hot path, since most calls are relative rather than absolute.

Suggested fix: baseURL is immutable after NewClient (no Option in options.go touches it), and url.URL.JoinPath never mutates its receiver, so a single parsed *url.URL can safely be cached on Client and reused across concurrent requests with no extra locking. One wrinkle: NewClient has no error return today, so this isn't quite "fail fast at construction" — the practical version is to parse once, store both the *url.URL and any parse error, and return the stored error from resolveURL on first use, same as today's behavior but without re-parsing every call.