From 3842cb31d8b9ce896f1ba81d5233a255782cc7ef Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Tue, 23 Jul 2019 17:14:39 +0200 Subject: [PATCH 1/4] Update flask middleware to not include full URL with query params as part of the span name. Including query strings means that the span can potentially contain sensitive data (a lot of times query params can contain things such as API keys, etc). --- .../opencensus/ext/flask/flask_middleware.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/opencensus-ext-flask/opencensus/ext/flask/flask_middleware.py b/contrib/opencensus-ext-flask/opencensus/ext/flask/flask_middleware.py index 328b53f10..9c1b69209 100644 --- a/contrib/opencensus-ext-flask/opencensus/ext/flask/flask_middleware.py +++ b/contrib/opencensus-ext-flask/opencensus/ext/flask/flask_middleware.py @@ -142,11 +142,11 @@ def _before_request(self): # Set the span name as the name of the current module name span.name = '[{}]{}'.format( flask.request.method, - flask.request.url) + flask.request.base_url) tracer.add_attribute_to_current_span( HTTP_METHOD, flask.request.method) tracer.add_attribute_to_current_span( - HTTP_URL, str(flask.request.url)) + HTTP_URL, str(flask.request.base_url)) execution_context.set_opencensus_attr( 'blacklist_hostnames', self.blacklist_hostnames) From a4fa8f252cce672cb1772a869578292ab767e342 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Tue, 23 Jul 2019 18:09:56 +0200 Subject: [PATCH 2/4] Add test case for it. --- .../tests/test_flask_middleware.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/contrib/opencensus-ext-flask/tests/test_flask_middleware.py b/contrib/opencensus-ext-flask/tests/test_flask_middleware.py index 1855e721f..38d9eb799 100644 --- a/contrib/opencensus-ext-flask/tests/test_flask_middleware.py +++ b/contrib/opencensus-ext-flask/tests/test_flask_middleware.py @@ -246,6 +246,33 @@ def test_header_is_none(self): self.assertEqual(span.attributes, expected_attributes) assert isinstance(span.parent_span, base.NullContextManager) + def test_query_params_are_not_included_as_part_of_span_name(self): + # query params can contain sensitive data so they are not included as + # part of span name + app = self.create_app() + flask_middleware.FlaskMiddleware(app=app, + sampler=samplers.AlwaysOnSampler()) + context = app.test_request_context( + path='/path/value?foo=bar&bar=baz') + + with context: + app.preprocess_request() + tracer = execution_context.get_opencensus_tracer() + self.assertIsNotNone(tracer) + + span = tracer.current_span() + + expected_attributes = { + 'http.url': u'http://localhost/path/value', + 'http.method': 'GET', + } + print(span.name) + + self.assertEqual(span.name, + '[GET]http://localhost/path/value') + self.assertEqual(span.attributes, expected_attributes) + assert isinstance(span.parent_span, base.NullContextManager) + def test__after_request_not_sampled(self): flask_trace_header = 'traceparent' trace_id = '2dd43a1d6b2549c6bc2a1a54c2fc0b05' From e383d22eefe3617612caf2f5ef5a8d9dec46f231 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Tue, 23 Jul 2019 18:11:39 +0200 Subject: [PATCH 3/4] Add changelog entry. --- contrib/opencensus-ext-flask/CHANGELOG.md | 11 +++++++++++ .../tests/test_flask_middleware.py | 1 - 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/contrib/opencensus-ext-flask/CHANGELOG.md b/contrib/opencensus-ext-flask/CHANGELOG.md index 498fd8eeb..065d216e4 100644 --- a/contrib/opencensus-ext-flask/CHANGELOG.md +++ b/contrib/opencensus-ext-flask/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## in development + +- Don't use full URL with query parameters for the span name and span HTTP_URL + span attribute. + + Query params can contain sensitive values which shouldn't be logged. Now just + the url without the query parameters is used. + + Before: ``http://example.com/path/bar?foo=bar&bar=baz``, now: + ``http://example.com/path/bar``. + ## Unreleased - Make ProbabilitySampler default diff --git a/contrib/opencensus-ext-flask/tests/test_flask_middleware.py b/contrib/opencensus-ext-flask/tests/test_flask_middleware.py index 38d9eb799..d0537b0a4 100644 --- a/contrib/opencensus-ext-flask/tests/test_flask_middleware.py +++ b/contrib/opencensus-ext-flask/tests/test_flask_middleware.py @@ -266,7 +266,6 @@ def test_query_params_are_not_included_as_part_of_span_name(self): 'http.url': u'http://localhost/path/value', 'http.method': 'GET', } - print(span.name) self.assertEqual(span.name, '[GET]http://localhost/path/value') From 55447e21da6fd9532e01df4ba5967e1271ca7f2c Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Tue, 23 Jul 2019 23:14:21 +0200 Subject: [PATCH 4/4] Per spec, full url with query params needs to be part of the "http.url" span attribute. --- .../opencensus/ext/flask/flask_middleware.py | 2 +- contrib/opencensus-ext-flask/tests/test_flask_middleware.py | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/contrib/opencensus-ext-flask/opencensus/ext/flask/flask_middleware.py b/contrib/opencensus-ext-flask/opencensus/ext/flask/flask_middleware.py index 9c1b69209..ffc03d8f8 100644 --- a/contrib/opencensus-ext-flask/opencensus/ext/flask/flask_middleware.py +++ b/contrib/opencensus-ext-flask/opencensus/ext/flask/flask_middleware.py @@ -146,7 +146,7 @@ def _before_request(self): tracer.add_attribute_to_current_span( HTTP_METHOD, flask.request.method) tracer.add_attribute_to_current_span( - HTTP_URL, str(flask.request.base_url)) + HTTP_URL, str(flask.request.url)) execution_context.set_opencensus_attr( 'blacklist_hostnames', self.blacklist_hostnames) diff --git a/contrib/opencensus-ext-flask/tests/test_flask_middleware.py b/contrib/opencensus-ext-flask/tests/test_flask_middleware.py index d0537b0a4..4873deaeb 100644 --- a/contrib/opencensus-ext-flask/tests/test_flask_middleware.py +++ b/contrib/opencensus-ext-flask/tests/test_flask_middleware.py @@ -263,7 +263,11 @@ def test_query_params_are_not_included_as_part_of_span_name(self): span = tracer.current_span() expected_attributes = { - 'http.url': u'http://localhost/path/value', + # NOTE: Query params need to be include as per spec + # https://github.com/census-instrumentation/opencensus-specs + # TODO: Open feedback PR to spec and suggestion making query + # params optional since they can contain sensitive data + 'http.url': u'http://localhost/path/value?foo=bar&bar=baz', 'http.method': 'GET', }