diff --git a/remote/client.go b/remote/client.go index acf91b7..9eed42c 100644 --- a/remote/client.go +++ b/remote/client.go @@ -154,34 +154,81 @@ func (c *Client) executeWithRetry(ctx context.Context, method, path string, body return lastResp, lastErr } -func (c *Client) executeOnce(ctx context.Context, method, path string, body []byte, extraHeaders map[string]string) (*http.Response, error) { - finalURL := path - - // Handle URL construction and verification +// resolveURL turns a caller-supplied path into the absolute URL to request. +// +// An absolute path is used verbatim once its scheme and host are checked +// against the configured service. A relative one is resolved against the base +// URL with net/url rather than string concatenation, so separators, escaping +// and dot-segments follow the same rules the rest of the world uses. +func (c *Client) resolveURL(path string) (string, error) { if strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") { - // Absolute URL validation - if c.baseURL != "" { - base, err := url.Parse(c.baseURL) - if err != nil { - return nil, fmt.Errorf("remote: invalid base URL: %w", err) - } + if c.baseURL == "" { + return path, nil + } - provided, err := url.Parse(path) - if err != nil { - return nil, fmt.Errorf("remote: invalid absolute URL: %w", err) - } + base, err := url.Parse(c.baseURL) + if err != nil { + return "", fmt.Errorf("remote: invalid base URL: %w", err) + } + provided, err := url.Parse(path) + if err != nil { + return "", fmt.Errorf("remote: invalid absolute URL: %w", err) + } - // Ensure Scheme and Host match to prevent SSRF or credential leakage - if base.Scheme != provided.Scheme || base.Host != provided.Host { - return nil, fmt.Errorf("remote: absolute URL host %q does not match configured service host %q", provided.Host, base.Host) - } + // Ensure Scheme and Host match to prevent SSRF or credential leakage + if base.Scheme != provided.Scheme || base.Host != provided.Host { + return "", fmt.Errorf("remote: absolute URL host %q does not match configured service host %q", provided.Host, base.Host) } - } else { - // Relative path handling - // Ensure baseURL ends with / and path doesn't start with / to avoid double slashes or missing slashes - base := strings.TrimSuffix(c.baseURL, "/") - p := strings.TrimPrefix(path, "/") - finalURL = base + "/" + p + return path, nil + } + + base, err := url.Parse(c.baseURL) + if err != nil { + return "", fmt.Errorf("remote: invalid base URL: %w", err) + } + // Parsing splits the path from any query or fragment the caller appended to + // it (JSONRequest appends the encoded Query this way), so each part can be + // carried on the URL it belongs to instead of being spliced into a string. + ref, err := url.Parse(path) + if err != nil { + return "", fmt.Errorf("remote: invalid request path %q: %w", path, err) + } + + // EscapedPath, not Path: joining the decoded form would turn an escaped + // separator ("x%2Fy") into a real one, silently addressing a different + // resource than the caller asked for. + p := ref.EscapedPath() + + // JoinPath preserves a trailing separator, so a root-only path would + // produce "/". That names a different resource than "" and + // some servers reject it, and a caller reaching the service URL itself has + // no other way to say so — treat it as empty. + if p == "/" { + p = "" + } + + u := base.JoinPath(p) + + // A query on the base URL is unusual but not illegal, so merge rather than + // let either side silently win. + switch { + case ref.RawQuery == "": + case u.RawQuery == "": + u.RawQuery = ref.RawQuery + default: + u.RawQuery += "&" + ref.RawQuery + } + if ref.Fragment != "" { + u.Fragment = ref.Fragment + } + + return u.String(), nil +} + +func (c *Client) executeOnce(ctx context.Context, method, path string, body []byte, extraHeaders map[string]string) (*http.Response, error) { + finalURL, err := c.resolveURL(path) + if err != nil { + return nil, err } var bodyReader io.Reader diff --git a/remote/client_test.go b/remote/client_test.go index fa3b0b6..e61448a 100644 --- a/remote/client_test.go +++ b/remote/client_test.go @@ -161,6 +161,100 @@ func TestClient_BaseURL_Logic(t *testing.T) { assert.Error(t, err2) assert.Contains(t, err2.Error(), "does not match configured service host") }) + + // An empty path addresses the service URL itself. Appending a separator + // would request a different resource, and some servers answer the trailing + // form with 405. + t.Run("relative path joins without a trailing separator", func(t *testing.T) { + for _, tc := range []struct { + name string + basePath string + path string + query url.Values + wantPath string + wantQuery string + }{ + {name: "empty path posts to the service URL itself", basePath: "/api/reports", path: "", wantPath: "/api/reports"}, + {name: "root path is treated as empty", basePath: "/api/reports", path: "/", wantPath: "/api/reports"}, + {name: "non-empty path is appended", basePath: "/api", path: "reports", wantPath: "/api/reports"}, + {name: "leading separator is not doubled", basePath: "/api", path: "/reports", wantPath: "/api/reports"}, + + // Query parameters are appended to the path before it reaches the + // join, so the empty-path rule has to look past them: a path of + // "?a=1" still addresses the service URL itself. + { + name: "empty path with query keeps the service URL itself", + basePath: "/api/reports", + path: "", + query: url.Values{"id": {"42"}}, + wantPath: "/api/reports", + wantQuery: "id=42", + }, + { + name: "root path with query is treated as empty", + basePath: "/api/reports", + path: "/", + query: url.Values{"id": {"42"}}, + wantPath: "/api/reports", + wantQuery: "id=42", + }, + { + name: "non-empty path with query is unaffected", + basePath: "/api", + path: "/reports", + query: url.Values{"id": {"42"}}, + wantPath: "/api/reports", + wantQuery: "id=42", + }, + { + name: "query already in the path is preserved", + basePath: "/api/reports", + path: "?first=1", + query: url.Values{"second": {"2"}}, + wantPath: "/api/reports", + wantQuery: "first=1&second=2", + }, + + // Resolution goes through net/url, so escaping and dot-segments + // follow the usual rules rather than whatever concatenation left. + { + name: "an escaped separator is not turned into a real one", + basePath: "/api", + path: "x%2Fy", + wantPath: "/api/x%2Fy", + }, + { + name: "a space in a segment is escaped", + basePath: "/api", + path: "sp ace", + wantPath: "/api/sp%20ace", + }, + { + name: "dot segments are resolved before sending", + basePath: "/api/reports", + path: "../other", + wantPath: "/api/other", + }, + } { + t.Run(tc.name, func(t *testing.T) { + var gotPath, gotQuery string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // EscapedPath, not Path: the difference between a literal + // separator and an escaped one is the point of two cases here. + gotPath = r.URL.EscapedPath() + gotQuery = r.URL.RawQuery + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + client := NewClient(server.URL + tc.basePath) + err := client.JSONRequest(context.Background(), Request{Method: "GET", Path: tc.path, Query: tc.query}, nil) + assert.NoError(t, err) + assert.Equal(t, tc.wantPath, gotPath) + assert.Equal(t, tc.wantQuery, gotQuery) + }) + } + }) } func TestClient_NoContentResponse(t *testing.T) {