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.

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

[httplib, requests] Sanitize urls in span metadata #688

Merged
merged 9 commits into from Nov 6, 2018

Conversation

majorgreys
Copy link
Collaborator

Use urlparse in Python 2 and Python 3 standard library to only set the path (without parameters and query string) in the http.url metadata.

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine, but would you be able to add some test cases for ensuring this works?

Also can we add a test case for the requests integration as well, I want to make sure that requests.get('https://.../?query=string') does not leak as well.

ddtrace/compat.py Outdated Show resolved Hide resolved
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests/contrib/httplib/test_httplib.py Show resolved Hide resolved
@majorgreys majorgreys changed the title [httplib] Strip all but path from url [httplib, requests] Sanitize urls in span metadata Nov 2, 2018
Kyle-Verhoog
Kyle-Verhoog previously approved these changes Nov 5, 2018
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very very minor things. otherwise this is good to go

@@ -55,6 +53,15 @@ def _wrap_request(func, instance, args, kwargs):
url = kwargs.get('url') or args[1]
headers = kwargs.get('headers', {})
parsed_uri = parse.urlparse(url)
# sanitize url of query
sanitized_url = parse.urlunparse((
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I was originally thinking just parsed_uri.path or something, but this is much better.

ddtrace/contrib/httplib/patch.py Show resolved Hide resolved
tests/contrib/httplib/test_httplib.py Show resolved Hide resolved
tests/contrib/httplib/test_httplib.py Show resolved Hide resolved
brettlangdon
brettlangdon previously approved these changes Nov 5, 2018
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brettlangdon
Copy link
Member

might need to update from 0.16-dev first

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@brettlangdon brettlangdon added this to the 0.16.0 milestone Nov 6, 2018
@brettlangdon brettlangdon merged commit d48ed31 into 0.16-dev Nov 6, 2018
brettlangdon pushed a commit that referenced this pull request Nov 8, 2018
* [httplib] Strip all but path from url

* [httplib] Fix tests

* [requests] Sanitize url

* [httplib] Add comment

* Correct comment

* Make httlib and requests consistent
@brettlangdon brettlangdon deleted the tahir/httplib-fix branch November 20, 2018 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants