Skip to content

Commit c068920

Browse files
lvrachlvrach
authored
fix: reporting common client path with query (#5842)
# Description url.JoinPath has a side effect of encoding query parameter. Causing `/metrics?version=v1` to be used as path, rather than `/metrics` with query `?version=v1`. Added a test to cover this. ## Linear Ticket Resolves PIPE-2062 https://linear.app/rudderstack/issue/PIPE-2062/fix-reporting-client-path-with-query ## Security - [x] The code changed/added as part of this pull request won't create any security issues with how the software is being used.
1 parent 695cf53 commit c068920

File tree

5 files changed

+194
-26
lines changed

5 files changed

+194
-26
lines changed

‎enterprise/reporting/client/client.go‎

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -31,16 +31,37 @@ const (
3131
)
3232

3333
const (
34-
PathMetrics Path = "/metrics?version=v1"
35-
PathRecordErrors Path = "/recordErrors"
36-
PathTrackedUsers Path = "/trackedUser"
34+
RouteMetrics Route = "/metrics?version=v1"
35+
RouteRecordErrors Route = "/recordErrors"
36+
RouteTrackedUsers Route = "/trackedUser"
3737
)
3838

39-
type Path string
39+
// Route contains the HTTP path and query string for the service.
40+
type Route string
41+
42+
// URL returns the absolute URL for the route, given a base URL.
43+
// * baseURL provides only the scheme, host, and port.
44+
// * Route provides path and query parameters.
45+
func (p Route) URL(baseURL string) (url.URL, error) {
46+
u, err := url.Parse(baseURL)
47+
if err != nil {
48+
return url.URL{}, fmt.Errorf("parsing base URL: %w", err)
49+
}
50+
51+
pathURL, err := url.Parse(string(p))
52+
if err != nil {
53+
return url.URL{}, fmt.Errorf("parsing service endpoint: %w", err)
54+
}
55+
56+
u.Path = pathURL.Path
57+
u.RawQuery = pathURL.RawQuery
58+
59+
return *u, nil
60+
}
4061

4162
// Client handles sending metrics to the reporting service
4263
type Client struct {
43-
path Path
64+
route Route
4465
reportingServiceURL string
4566

4667
httpClient *http.Client
@@ -68,7 +89,7 @@ func backOffFromConfig(conf *config.Config) backoff.BackOff {
6889
}
6990

7091
// New creates a new reporting client
71-
func New(path Path, conf *config.Config, log logger.Logger, stats stats.Stats) *Client {
92+
func New(path Route, conf *config.Config, log logger.Logger, stats stats.Stats) *Client {
7293
reportingServiceURL := conf.GetString("REPORTING_URL", "https://reporting.dev.rudderlabs.com")
7394
reportingServiceURL = strings.TrimSuffix(reportingServiceURL, "/")
7495

@@ -78,7 +99,7 @@ func New(path Path, conf *config.Config, log logger.Logger, stats stats.Stats) *
7899
},
79100
reportingServiceURL: reportingServiceURL,
80101
backoff: backOffFromConfig(conf),
81-
path: path,
102+
route: path,
82103
instanceID: conf.GetString("INSTANCE_ID", "1"),
83104
moduleName: conf.GetString("clientName", ""),
84105
stats: stats,
@@ -92,15 +113,15 @@ func (c *Client) Send(ctx context.Context, payload any) error {
92113
return err
93114
}
94115

95-
u, err := url.JoinPath(c.reportingServiceURL, string(c.path))
116+
u, err := c.route.URL(c.reportingServiceURL)
96117
if err != nil {
97-
return err
118+
return fmt.Errorf("constructing URL for service endpoint (%q, %q): %w", c.route, c.reportingServiceURL, err)
98119
}
99120

100121
o := func() error {
101-
req, err := http.NewRequestWithContext(ctx, "POST", u, bytes.NewBuffer(payloadBytes))
122+
req, err := http.NewRequestWithContext(ctx, http.MethodPost, u.String(), bytes.NewBuffer(payloadBytes))
102123
if err != nil {
103-
return err
124+
return fmt.Errorf("constructing HTTP request: %w", err)
104125
}
105126
req.Header.Set("Content-Type", "application/json; charset=utf-8")
106127
httpRequestStart := time.Now()
@@ -126,11 +147,11 @@ func (c *Client) Send(ctx context.Context, payload any) error {
126147
defer func() { httputil.CloseResponse(resp) }()
127148
respBody, err := io.ReadAll(resp.Body)
128149
if err != nil {
129-
return fmt.Errorf("reading response body from reporting service: %q: %w", c.path, err)
150+
return fmt.Errorf("reading response body: %q: %w", c.route, err)
130151
}
131152

132153
if !c.isHTTPRequestSuccessful(resp.StatusCode) {
133-
err = fmt.Errorf("received unexpected response from reporting service: %q: statusCode: %d body: %v", c.path, resp.StatusCode, string(respBody))
154+
err = fmt.Errorf("received unexpected response: %q: statusCode: %d body: %v", c.route, resp.StatusCode, string(respBody))
134155
}
135156
return err
136157
}
@@ -152,7 +173,7 @@ func (c *Client) getTags() stats.Tags {
152173
"module": c.moduleName,
153174
"instanceId": c.instanceID,
154175
"endpoint": serverURL.Host,
155-
"path": string(c.path),
176+
"path": string(c.route),
156177
}
157178
}
158179

‎enterprise/reporting/client/client_test.go‎

Lines changed: 156 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,19 @@ func TestClientSendMetric(t *testing.T) {
3737
// Create a test server to mock the reporting service
3838
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
3939
var err error
40+
41+
if r.URL.Path != "/metrics" {
42+
w.WriteHeader(http.StatusNotFound)
43+
t.Logf("received request with path: %s", r.URL.Path)
44+
return
45+
}
46+
47+
if r.URL.Query().Get("version") != "v1" {
48+
w.WriteHeader(http.StatusBadRequest)
49+
t.Logf("received request with version: %s", r.URL.Query().Get("version"))
50+
return
51+
}
52+
4053
receivedPayload, err = io.ReadAll(r.Body)
4154
require.NoError(t, err)
4255
payloadChan <- receivedPayload
@@ -53,9 +66,10 @@ func TestClientSendMetric(t *testing.T) {
5366
conf.Set("INSTANCE_ID", "test-instance")
5467
conf.Set("clientName", "test-client")
5568
conf.Set("REPORTING_URL", server.URL)
69+
conf.Set("Reporting.httpClient.backoff.maxRetries", 1)
5670

5771
// Create the client
58-
c := client.New(client.PathMetrics, conf, logger.NOP, statsStore)
72+
c := client.New(client.RouteMetrics, conf, logger.NOP, statsStore)
5973

6074
bucket, _ := reporting.GetAggregationBucketMinute(28017690, 10)
6175

@@ -138,15 +152,15 @@ func TestClientSendMetric(t *testing.T) {
138152
t.Run("ensure metrics are recorded", func(t *testing.T) {
139153
// Expected tags for all metrics
140154
expectedTags := stats.Tags{
141-
"path": string(client.PathMetrics),
155+
"path": string(client.RouteMetrics),
142156
"module": "test-client",
143157
"instanceId": instanceID,
144158
"endpoint": serverURL.Host,
145159
}
146160

147161
// Expected tags for HTTP metrics
148162
expectedHttpTags := stats.Tags{
149-
"path": string(client.PathMetrics),
163+
"path": string(client.RouteMetrics),
150164
"module": "test-client",
151165
"instanceId": instanceID,
152166
"endpoint": serverURL.Host,
@@ -196,7 +210,7 @@ func TestClientSendErrorMetric(t *testing.T) {
196210
conf.Set("REPORTING_URL", server.URL)
197211

198212
// Create the client
199-
c := client.New(client.PathMetrics, conf, logger.NOP, statsStore)
213+
c := client.New(client.RouteMetrics, conf, logger.NOP, statsStore)
200214

201215
// Create sample event as json.RawMessage
202216
sampleEvent := json.RawMessage(`{"event": "test_event", "properties": {"test": "value"}}`)
@@ -258,15 +272,15 @@ func TestClientSendErrorMetric(t *testing.T) {
258272

259273
// Expected tags for all metrics
260274
expectedTags := stats.Tags{
261-
"path": string(client.PathMetrics),
275+
"path": string(client.RouteMetrics),
262276
"module": "test-client",
263277
"instanceId": "test-instance",
264278
"endpoint": serverURL.Host,
265279
}
266280

267281
// Expected tags for HTTP metrics
268282
expectedHttpTags := stats.Tags{
269-
"path": string(client.PathMetrics),
283+
"path": string(client.RouteMetrics),
270284
"module": "test-client",
271285
"instanceId": "test-instance",
272286
"endpoint": serverURL.Host,
@@ -327,19 +341,19 @@ func TestClient5xx(t *testing.T) {
327341
conf.Set("REPORTING_URL", server.URL)
328342
conf.Set("Reporting.httpClient.backoff.maxRetries", 1)
329343

330-
c := client.New(client.PathMetrics, conf, logger.NOP, statsStore)
344+
c := client.New(client.RouteMetrics, conf, logger.NOP, statsStore)
331345

332346
// Send the metric and expect an error
333347
err = c.Send(context.Background(), metric)
334348
require.Error(t, err)
335-
require.Contains(t, err.Error(), fmt.Sprintf("received unexpected response from reporting service: \"/metrics?version=v1\": statusCode: %d body: error with status %d", statusCode, statusCode))
349+
require.Contains(t, err.Error(), fmt.Sprintf("received unexpected response: \"/metrics?version=v1\": statusCode: %d body: error with status %d", statusCode, statusCode))
336350

337351
// Get server hostname
338352
serverURL, _ := url.Parse(server.URL)
339353

340354
// Expected tags for HTTP metrics
341355
expectedHttpTags := stats.Tags{
342-
"path": string(client.PathMetrics),
356+
"path": string(client.RouteMetrics),
343357
"module": "test-client",
344358
"instanceId": "2",
345359
"endpoint": serverURL.Host,
@@ -353,3 +367,136 @@ func TestClient5xx(t *testing.T) {
353367
})
354368
}
355369
}
370+
371+
func TestRouteURL(t *testing.T) {
372+
tests := []struct {
373+
name string
374+
route client.Route
375+
baseURL string
376+
wantURL string
377+
wantErr bool
378+
errContains string
379+
}{
380+
{
381+
name: "valid metrics endpoint",
382+
route: client.RouteMetrics,
383+
baseURL: "https://reporting.dev.rudderlabs.com",
384+
wantURL: "https://reporting.dev.rudderlabs.com/metrics?version=v1",
385+
wantErr: false,
386+
},
387+
{
388+
name: "valid record errors endpoint",
389+
route: client.RouteRecordErrors,
390+
baseURL: "https://reporting.dev.rudderlabs.com",
391+
wantURL: "https://reporting.dev.rudderlabs.com/recordErrors",
392+
wantErr: false,
393+
},
394+
{
395+
name: "valid tracked users endpoint",
396+
route: client.RouteTrackedUsers,
397+
baseURL: "https://reporting.dev.rudderlabs.com",
398+
wantURL: "https://reporting.dev.rudderlabs.com/trackedUser",
399+
wantErr: false,
400+
},
401+
{
402+
name: "invalid base URL",
403+
route: client.RouteMetrics,
404+
baseURL: "://invalid-url",
405+
wantErr: true,
406+
errContains: "parsing base URL",
407+
},
408+
{
409+
name: "invalid service endpoint",
410+
route: client.Route("://invalid-endpoint"),
411+
baseURL: "https://reporting.dev.rudderlabs.com",
412+
wantErr: true,
413+
errContains: "parsing service endpoint",
414+
},
415+
{
416+
name: "base URL with trailing slash",
417+
route: client.RouteMetrics,
418+
baseURL: "https://reporting.dev.rudderlabs.com/",
419+
wantURL: "https://reporting.dev.rudderlabs.com/metrics?version=v1",
420+
wantErr: false,
421+
},
422+
{
423+
name: "base URL with trailing dot",
424+
route: client.RouteMetrics,
425+
baseURL: "https://reporting.dev.rudderlabs.com./",
426+
wantURL: "https://reporting.dev.rudderlabs.com./metrics?version=v1",
427+
wantErr: false,
428+
},
429+
430+
{
431+
name: "base URL with existing path",
432+
route: client.RouteMetrics,
433+
baseURL: "https://reporting.dev.rudderlabs.com/api",
434+
wantURL: "https://reporting.dev.rudderlabs.com/metrics?version=v1",
435+
wantErr: false,
436+
},
437+
{
438+
name: "base URL with multiple trailing slashes",
439+
route: client.RouteMetrics,
440+
baseURL: "https://reporting.dev.rudderlabs.com///",
441+
wantURL: "https://reporting.dev.rudderlabs.com/metrics?version=v1",
442+
wantErr: false,
443+
},
444+
{
445+
name: "base URL with relative path up",
446+
route: client.RouteMetrics,
447+
baseURL: "https://reporting.dev.rudderlabs.com/api/../",
448+
wantURL: "https://reporting.dev.rudderlabs.com/metrics?version=v1",
449+
wantErr: false,
450+
},
451+
{
452+
name: "base URL with relative path current",
453+
route: client.RouteMetrics,
454+
baseURL: "https://reporting.dev.rudderlabs.com/api/./",
455+
wantURL: "https://reporting.dev.rudderlabs.com/metrics?version=v1",
456+
wantErr: false,
457+
},
458+
{
459+
name: "base URL with complex relative path",
460+
route: client.RouteMetrics,
461+
baseURL: "https://reporting.dev.rudderlabs.com/api/v1/../v2/./v3/",
462+
wantURL: "https://reporting.dev.rudderlabs.com/metrics?version=v1",
463+
wantErr: false,
464+
},
465+
{
466+
name: "base URL with encoded path",
467+
route: client.RouteMetrics,
468+
baseURL: "https://reporting.dev.rudderlabs.com/api%2Fv1",
469+
wantURL: "https://reporting.dev.rudderlabs.com/metrics?version=v1",
470+
wantErr: false,
471+
},
472+
{
473+
name: "base URL with query parameters",
474+
route: client.RouteMetrics,
475+
baseURL: "https://reporting.dev.rudderlabs.com?param=value",
476+
wantURL: "https://reporting.dev.rudderlabs.com/metrics?version=v1",
477+
wantErr: false,
478+
},
479+
{
480+
name: "base URL with port",
481+
route: client.RouteMetrics,
482+
baseURL: "https://reporting.dev.rudderlabs.com:8080",
483+
wantURL: "https://reporting.dev.rudderlabs.com:8080/metrics?version=v1",
484+
wantErr: false,
485+
},
486+
}
487+
488+
for _, tt := range tests {
489+
t.Run(tt.name, func(t *testing.T) {
490+
got, err := tt.route.URL(tt.baseURL)
491+
if tt.wantErr {
492+
require.Error(t, err)
493+
if tt.errContains != "" {
494+
require.Contains(t, err.Error(), tt.errContains)
495+
}
496+
return
497+
}
498+
require.NoError(t, err)
499+
require.Equal(t, tt.wantURL, got.String())
500+
})
501+
}
502+
}

‎enterprise/reporting/error_reporting.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ func NewErrorDetailReporter(
165165
config: conf,
166166

167167
useCommonClient: useCommonClient,
168-
commonClient: client.New(client.PathRecordErrors, conf, log, stats),
168+
commonClient: client.New(client.RouteRecordErrors, conf, log, stats),
169169
}
170170
}
171171

‎enterprise/reporting/flusher/factory.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ func CreateRunner(ctx context.Context, table string, log logger.Logger, stats st
4545
return &NOPCronRunner{}, nil
4646
}
4747

48-
commonClient := client.New(client.PathTrackedUsers, conf, log, stats)
48+
commonClient := client.New(client.RouteTrackedUsers, conf, log, stats)
4949

5050
// DEPRECATED: Remove this after migration to commonClient.
5151
reportingBaseURL := config.GetString("REPORTING_URL", "https://reporting.rudderstack.com/")

‎enterprise/reporting/reporting.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ func NewDefaultReporter(ctx context.Context, conf *config.Config, log logger.Log
146146
eventSamplingDuration: eventSamplingDuration,
147147
eventSampler: eventSampler,
148148
useCommonClient: useCommonClient,
149-
commonClient: client.New(client.PathMetrics, conf, log, stats),
149+
commonClient: client.New(client.RouteMetrics, conf, log, stats),
150150
}
151151
}
152152

0 commit comments

Comments
 (0)