From 5f3b6aee61f9d775a2ebba1b6c6c9b97ea3c11b4 Mon Sep 17 00:00:00 2001 From: "Daniel A. White" Date: Tue, 13 Dec 2022 21:44:53 -0500 Subject: [PATCH 1/2] feat: sanitize http.url attribute values --- CHANGELOG.md | 2 + .../src/common/attributes.ts | 91 +++++++++++++++---- .../test/common/attributes.test.ts | 88 ++++++++++++++++++ .../test/common/Span.test.ts | 15 ++- .../test/common/util.ts | 8 ++ 5 files changed, 184 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40647a188a..60215bc594 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ ### :rocket: (Enhancement) +* feat(opentelemetry-core): sanitize `http.url` attributes to remove usernames and passwords [#3487](https://github.com/open-telemetry/opentelemetry-js/pull/3487) @daniel-white + ### :bug: (Bug Fix) ### :books: (Refine Doc) diff --git a/packages/opentelemetry-core/src/common/attributes.ts b/packages/opentelemetry-core/src/common/attributes.ts index 0726acd581..41b7378d24 100644 --- a/packages/opentelemetry-core/src/common/attributes.ts +++ b/packages/opentelemetry-core/src/common/attributes.ts @@ -14,28 +14,56 @@ * limitations under the License. */ -import { diag, SpanAttributeValue, SpanAttributes } from '@opentelemetry/api'; +import { diag, AttributeValue, Attributes } from '@opentelemetry/api'; +import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; + +const valueSanitizers: ReadonlyMap< + string, + (val: AttributeValue) => AttributeValue | undefined +> = new Map([[SemanticAttributes.HTTP_URL, sanitizeHttpUrl]]); + +export function sanitizeAttribute( + key: string, + val: AttributeValue +): AttributeValue | undefined { + let out: AttributeValue | undefined = undefined; + + if (!isAttributeKey(key)) { + diag.warn(`Invalid attribute key: ${key}`); + return out; + } + if (!isAttributeValue(val)) { + diag.warn(`Invalid attribute value set for key: ${key}`); + return out; + } + + let copyVal: AttributeValue; + + if (Array.isArray(val)) { + copyVal = val.slice(); + } else { + copyVal = val; + } + + const valueSanitizer = valueSanitizers.get(key); + if (typeof valueSanitizer === 'undefined') { + return copyVal; + } -export function sanitizeAttributes(attributes: unknown): SpanAttributes { - const out: SpanAttributes = {}; + return valueSanitizer(copyVal); +} + +export function sanitizeAttributes(attributes: unknown): Attributes { + const out: Attributes = {}; if (typeof attributes !== 'object' || attributes == null) { return out; } for (const [key, val] of Object.entries(attributes)) { - if (!isAttributeKey(key)) { - diag.warn(`Invalid attribute key: ${key}`); - continue; - } - if (!isAttributeValue(val)) { - diag.warn(`Invalid attribute value set for key: ${key}`); - continue; - } - if (Array.isArray(val)) { - out[key] = val.slice(); - } else { - out[key] = val; + const sanitizedVal = sanitizeAttribute(key, val); + if (typeof sanitizedVal !== 'undefined') { + out[key] = sanitizedVal; } } @@ -46,7 +74,7 @@ export function isAttributeKey(key: unknown): key is string { return typeof key === 'string' && key.length > 0; } -export function isAttributeValue(val: unknown): val is SpanAttributeValue { +export function isAttributeValue(val: unknown): val is AttributeValue { if (val == null) { return true; } @@ -94,3 +122,34 @@ function isValidPrimitiveAttributeValue(val: unknown): boolean { return false; } + +function sanitizeHttpUrl(val: AttributeValue): AttributeValue | undefined { + let out: AttributeValue | undefined; + + if (typeof val !== 'string') { + diag.warn( + `Invalid attribute value set for key: ${ + SemanticAttributes.HTTP_URL + }. Unable to sanitize ${Array.isArray(val) ? 'array' : typeof val} value.` + ); + + return val; + } + + try { + const valUrl = new URL(val); + + valUrl.username = ''; + valUrl.password = ''; + + out = valUrl.toString(); + } catch (err: unknown) { + diag.warn( + `Invalid attribute value set for key: ${SemanticAttributes.HTTP_URL}. Unable to sanitize invalid URL.` + ); + + out = val; + } + + return out; +} diff --git a/packages/opentelemetry-core/test/common/attributes.test.ts b/packages/opentelemetry-core/test/common/attributes.test.ts index 8524135b18..4ad917e9b5 100644 --- a/packages/opentelemetry-core/test/common/attributes.test.ts +++ b/packages/opentelemetry-core/test/common/attributes.test.ts @@ -14,13 +14,28 @@ * limitations under the License. */ +import { diag } from '@opentelemetry/api'; +import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import * as assert from 'assert'; +import * as sinon from 'sinon'; import { isAttributeValue, + sanitizeAttribute, sanitizeAttributes, } from '../../src/common/attributes'; describe('attributes', () => { + const warnStub = sinon.fake(); + + beforeEach(() => { + diag.setLogger({ + warn: warnStub, + } as any); + + // diag.warn is used when the logger is set + warnStub.resetHistory(); + }); + describe('#isAttributeValue', () => { it('should allow primitive values', () => { assert.ok(isAttributeValue(0)); @@ -102,5 +117,78 @@ describe('attributes', () => { assert.ok(Array.isArray(attributes.arr)); assert.strictEqual(attributes.arr[0], 'unmodified'); }); + + describe('value sanitizers', () => { + describe('http.url', () => { + it('should remove username and password from the url', () => { + const out = sanitizeAttribute( + SemanticAttributes.HTTP_URL, + 'http://user:pass@host:9000/path?query#fragment' + ); + + assert.strictEqual(out, 'http://host:9000/path?query#fragment'); + assert.ok(warnStub.notCalled, 'should not log warning'); + }); + + it('should return the same url', () => { + const out = sanitizeAttribute( + SemanticAttributes.HTTP_URL, + 'http://host:9000/path?query#fragment' + ); + + assert.strictEqual(out, 'http://host:9000/path?query#fragment'); + assert.ok(warnStub.notCalled, 'should not log warning'); + }); + + it('should return the input string when the value is not a valid url', () => { + const out = sanitizeAttribute( + SemanticAttributes.HTTP_URL, + 'invalid url' + ); + + assert.strictEqual(out, 'invalid url'); + assert.ok( + warnStub.calledWithExactly( + `Invalid attribute value set for key: ${SemanticAttributes.HTTP_URL}. Unable to sanitize invalid URL.` + ) + ); + }); + + it('should return the input when the value is a number', () => { + const out = sanitizeAttribute(SemanticAttributes.HTTP_URL, 27); + + assert.strictEqual(out, 27); + assert.ok( + warnStub.calledWithExactly( + `Invalid attribute value set for key: ${SemanticAttributes.HTTP_URL}. Unable to sanitize number value.` + ) + ); + }); + + it('should return the input when the value is boolean', () => { + const out = sanitizeAttribute(SemanticAttributes.HTTP_URL, false); + + assert.strictEqual(out, false); + assert.ok( + warnStub.calledWithExactly( + `Invalid attribute value set for key: ${SemanticAttributes.HTTP_URL}. Unable to sanitize boolean value.` + ) + ); + }); + + it('should return the input when an array is supplied', () => { + const out = sanitizeAttribute(SemanticAttributes.HTTP_URL, [ + 'http://host/path?query#fragment', + ]); + + assert.deepStrictEqual(out, ['http://host/path?query#fragment']); + assert.ok( + warnStub.calledWithExactly( + `Invalid attribute value set for key: ${SemanticAttributes.HTTP_URL}. Unable to sanitize array value.` + ) + ); + }); + }); + }); }); }); diff --git a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts index dac8728138..bedf50fefd 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts @@ -37,7 +37,11 @@ import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import * as assert from 'assert'; import * as sinon from 'sinon'; import { BasicTracerProvider, Span, SpanProcessor } from '../../src'; -import { invalidAttributes, validAttributes } from './util'; +import { + invalidAttributes, + sanitizedValidAttributes, + validAttributes, +} from './util'; const performanceTimeOrigin: HrTime = [1, 1]; @@ -264,7 +268,7 @@ describe('Span', () => { span.setAttribute(k, v as unknown as SpanAttributeValue); } - assert.deepStrictEqual(span.attributes, validAttributes); + assert.deepStrictEqual(span.attributes, sanitizedValidAttributes); }); it('should be able to overwrite attributes', () => { @@ -712,7 +716,7 @@ describe('Span', () => { span.setAttributes(validAttributes); span.setAttributes(invalidAttributes as unknown as SpanAttributes); - assert.deepStrictEqual(span.attributes, validAttributes); + assert.deepStrictEqual(span.attributes, sanitizedValidAttributes); }); }); @@ -746,7 +750,10 @@ describe('Span', () => { assert.strictEqual(span.events.length, 1); assert.deepStrictEqual(span.events[0].name, 'rev'); - assert.deepStrictEqual(span.events[0].attributes, validAttributes); + assert.deepStrictEqual( + span.events[0].attributes, + sanitizedValidAttributes + ); }); }); diff --git a/packages/opentelemetry-sdk-trace-base/test/common/util.ts b/packages/opentelemetry-sdk-trace-base/test/common/util.ts index ab34f2ab1b..eed1c57b2d 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/util.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/util.ts @@ -14,6 +14,8 @@ * limitations under the License. */ +import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; + export const validAttributes = { string: 'string', number: 0, @@ -21,6 +23,12 @@ export const validAttributes = { 'array': ['str1', 'str2'], 'array': [1, 2], 'array': [true, false], + [SemanticAttributes.HTTP_URL]: 'http://user:pass@host:port/path?query', +}; + +export const sanitizedValidAttributes = { + ...validAttributes, + [SemanticAttributes.HTTP_URL]: 'http://host:port/path?query', }; export const invalidAttributes = { From de9e811593b5994efbe70aa06b8f43339ccad416 Mon Sep 17 00:00:00 2001 From: "Daniel A. White" Date: Sun, 15 Jan 2023 11:19:39 -0500 Subject: [PATCH 2/2] revert changes, new strategy for function --- .../src/common/attributes.ts | 101 ++++++++---------- 1 file changed, 46 insertions(+), 55 deletions(-) diff --git a/packages/opentelemetry-core/src/common/attributes.ts b/packages/opentelemetry-core/src/common/attributes.ts index 41b7378d24..45477f0370 100644 --- a/packages/opentelemetry-core/src/common/attributes.ts +++ b/packages/opentelemetry-core/src/common/attributes.ts @@ -14,56 +14,29 @@ * limitations under the License. */ -import { diag, AttributeValue, Attributes } from '@opentelemetry/api'; +import { diag, SpanAttributeValue, SpanAttributes } from '@opentelemetry/api'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; -const valueSanitizers: ReadonlyMap< - string, - (val: AttributeValue) => AttributeValue | undefined -> = new Map([[SemanticAttributes.HTTP_URL, sanitizeHttpUrl]]); - -export function sanitizeAttribute( - key: string, - val: AttributeValue -): AttributeValue | undefined { - let out: AttributeValue | undefined = undefined; - - if (!isAttributeKey(key)) { - diag.warn(`Invalid attribute key: ${key}`); - return out; - } - if (!isAttributeValue(val)) { - diag.warn(`Invalid attribute value set for key: ${key}`); - return out; - } - - let copyVal: AttributeValue; - - if (Array.isArray(val)) { - copyVal = val.slice(); - } else { - copyVal = val; - } - - const valueSanitizer = valueSanitizers.get(key); - if (typeof valueSanitizer === 'undefined') { - return copyVal; - } - - return valueSanitizer(copyVal); -} - -export function sanitizeAttributes(attributes: unknown): Attributes { - const out: Attributes = {}; +export function sanitizeAttributes(attributes: unknown): SpanAttributes { + const out: SpanAttributes = {}; if (typeof attributes !== 'object' || attributes == null) { return out; } for (const [key, val] of Object.entries(attributes)) { - const sanitizedVal = sanitizeAttribute(key, val); - if (typeof sanitizedVal !== 'undefined') { - out[key] = sanitizedVal; + if (!isAttributeKey(key)) { + diag.warn(`Invalid attribute key: ${key}`); + continue; + } + if (!isAttributeValue(val)) { + diag.warn(`Invalid attribute value set for key: ${key}`); + continue; + } + if (Array.isArray(val)) { + out[key] = val.slice(); + } else { + out[key] = val; } } @@ -74,7 +47,7 @@ export function isAttributeKey(key: unknown): key is string { return typeof key === 'string' && key.length > 0; } -export function isAttributeValue(val: unknown): val is AttributeValue { +export function isAttributeValue(val: unknown): val is SpanAttributeValue { if (val == null) { return true; } @@ -123,18 +96,8 @@ function isValidPrimitiveAttributeValue(val: unknown): boolean { return false; } -function sanitizeHttpUrl(val: AttributeValue): AttributeValue | undefined { - let out: AttributeValue | undefined; - - if (typeof val !== 'string') { - diag.warn( - `Invalid attribute value set for key: ${ - SemanticAttributes.HTTP_URL - }. Unable to sanitize ${Array.isArray(val) ? 'array' : typeof val} value.` - ); - - return val; - } +function sanitizeAttributeUsingURL(val: string): string { + let out: string; try { const valUrl = new URL(val); @@ -153,3 +116,31 @@ function sanitizeHttpUrl(val: AttributeValue): AttributeValue | undefined { return out; } + +function sanitizeHttpUrlUsingRegExp(val: string): string { + let out = val.replace(/^(https?:\/\/)([^@]+@)?(.*)/, '$1$3'); + + return out; +} + +const sanitizeHttpUrlImpl = + typeof URL === 'function' + ? sanitizeAttributeUsingURL + : sanitizeHttpUrlUsingRegExp; + +export function sanitizeHttpUrl(val: SpanAttributeValue): SpanAttributeValue { + let out: SpanAttributeValue; + + if (typeof val !== 'string') { + diag.warn( + `Invalid attribute value set for key: ${ + SemanticAttributes.HTTP_URL + }. Unable to sanitize ${Array.isArray(val) ? 'array' : typeof val} value.` + ); + + return val; + } + + out = sanitizeHttpUrlImpl(val); + return out; +}