Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUG] contrib/dimfeld/httptreemux.v5: trailing slash requests are redirected with raw URL being tracked #2663

Closed
devillecodes opened this issue Apr 18, 2024 · 1 comment · Fixed by #2685
Closed

[BUG] contrib/dimfeld/httptreemux.v5: trailing slash requests are redirected with raw URL being tracked #2663

devillecodes opened this issue Apr 18, 2024 · 1 comment · Fixed by #2685
Assignees
@darccio
Labels
ack apm:ecosystem contrib/* related feature requests or bugs bug unintended behavior that has to be fixed

Comments

@devillecodes
Copy link
Contributor

Version of dd-trace-go v1.59.1 (and newer)

Describe what happened:

This issue is specific to contrib/dimfeld/httptreemux.v5 and related to #2293.

From the description of #2293:

The dimfeld/httptreemux router has logic to handle trailing slashes which redirects to the URL without the trailing slash. This happens without the URL parameterized and sanitized of any sensitive information. This results in a second request being done, which is parameterized, but more importantly, it means that the first request is traced with its raw URL rather than the parametrized one. This happens because while looking up the resource with a trailing slash, the parameters are not detected.

A partial fix for the issue was included in #2332, but it only catered for one of the two scenarios. The same problematic behaviour is observed when a request without a trailing slash matches a registered route that has a trailing slash.

The following examples will both trigger the redirect behaviour in the dimfeld/httptreemux.v5 router:

  1. request without trailing slash matching to route with trailing slash:
    • request URL: GET /api/v1/:email (without trailing slash)
    • route path: GET /api/v1/:email/ (with trailing slash)
  2. request with trailing slash matching to route without trailing slash:
    • request URL: GET /api/v1/:email/ (with trailing slash)
    • route path: GET /api/v1/:email (without trailing slash)

Describe what you expected:

Both requests, redirect and post redirect, should be parameterized in both scenarios listed in the previous section.

Steps to reproduce the issue:

Call an endpoint with at least one path parameter with a trailing slash, while using the dimfeld/httptreemux.v5 router.

Additional environment details (Version of Go, Operating System, etc.):

This issue isn't dependent on any environmental details.
@devillecodes devillecodes added the bug unintended behavior that has to be fixed label Apr 18, 2024
devillecodes added a commit to agilebits/dd-trace-go that referenced this issue Apr 18, 2024
@devillecodes
contrib/dimfeld/httptreemux.v5: fix resource name for 301 redirects
d5ec356 
The httptreemux router has a set of redirect behaviours that is invoked
when a request URL and matched route only differs in a trailing slash.
The default behaviour is to respond with a 301 (moved permanently) to
redirect the client to the exact path of the matched handler. We
previously patched one of the two scenarios where this occurs in DataDog#2332.
The changes in this commit addresses the other scenario.

We also standardize the resource name in the event that there is a
trailing slash missmatch between request URL and matched handler. We
always truncate the trailing slash in the resource name.

Fixes DataDog#2663
@github-actions github-actions bot added apm:ecosystem contrib/* related feature requests or bugs needs-triage New issues that have not yet been triaged labels Apr 18, 2024
devillecodes added a commit to agilebits/dd-trace-go that referenced this issue Apr 18, 2024
@devillecodes
contrib/dimfeld/httptreemux.v5: fix resource name for 30X redirects
fcb9846 
The httptreemux router has a set of redirect behaviours that is invoked
when a request URL and matched route only differs in a trailing slash.
The default behaviour is to respond with a 301 (moved permanently) to
redirect the client to the exact path of the matched handler. We
previously patched one of the two scenarios where this occurs in DataDog#2332.
The changes in this commit addresses the other scenario for the 301
redirect, but also the 307 and 308 redirect options. Several tests
were included to cover the various scenarios.

We also standardize the resource name in the event that there is a
trailing slash missmatch between request URL and matched handler. We
always truncate the trailing slash in the resource name.

Fixes DataDog#2663
devillecodes added a commit to agilebits/dd-trace-go that referenced this issue Apr 18, 2024
@devillecodes
contrib/dimfeld/httptreemux.v5: fix route and name for 30X redirects
ca7b437 
The httptreemux router has a redirect behaviour that is invoked when
a request URL and matched route only differs in a trailing slash. The
default behaviour is to respond with a 301 (moved permanently) to
redirect the client to the exact path of the matched handler. We
previously patched one of the two scenarios where this occurs in DataDog#2332.
The changes in this commit addresses the other scenario for the 301
redirect, but also the 307 and 308 redirect options. Several tests
were included to cover the various scenarios.

We also standardize the resource name in the event that there is a
trailing slash missmatch between request URL and matched handler. We
always truncate the trailing slash in the resource name.

Fixes DataDog#2663
@devillecodes devillecodes mentioned this issue Apr 18, 2024
Closed
6 tasks
@rarguelloF
Copy link
Contributor

Hi @devillecodes

Thanks for the bug report and opening a PR with the fix! I was able to confirm the bug is indeed happening, so I will proceed with the PR review soon.

@rarguelloF rarguelloF added ack and removed needs-triage New issues that have not yet been triaged labels Apr 24, 2024
devillecodes added a commit to agilebits/dd-trace-go that referenced this issue Apr 25, 2024
@devillecodes
contrib/dimfeld/httptreemux.v5: fix route and name for 30X redirects
41f1176 
The httptreemux router has a redirect behaviour that is invoked when
a request URL and matched route only differs in a trailing slash. The
default behaviour is to respond with a 301 (moved permanently) to
redirect the client to the exact path of the matched handler. We
previously patched one of the two scenarios where this occurs in DataDog#2332.
The changes in this commit addresses the other scenario for the 301
redirect, but also the 307 and 308 redirect options. Several tests
were included to cover the various scenarios.

We also standardize the resource name in the event that there is a
trailing slash missmatch between request URL and matched handler. We
always truncate the trailing slash in the resource name.

Fixes DataDog#2663
@darccio darccio self-assigned this Apr 30, 2024
@darccio darccio mentioned this issue May 8, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@darccio darccio

Labels
ack apm:ecosystem contrib/* related feature requests or bugs bug unintended behavior that has to be fixed
Projects
None yet
Milestone
No milestone
3 participants
@darccio @rarguelloF @devillecodes