From ad32fb6ce4c0ea69c1d77dc55f7b50c25caf810b Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 18 Oct 2022 11:59:35 +0800 Subject: [PATCH] fix(resources): strict OTEL_RESOURCE_ATTRIBUTES decoding --- CHANGELOG.md | 2 ++ .../opentelemetry-sdk-node/test/sdk.test.ts | 2 +- .../src/detectors/EnvDetector.ts | 11 +++--- .../detectors/browser/EnvDetector.test.ts | 35 ++++++++++++++++++- .../test/detectors/node/EnvDetector.test.ts | 25 ++++++++++++- 5 files changed, 67 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99a4cef09f..e447399eeb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,8 @@ All notable changes to this project will be documented in this file. [#3327](https://github.com/open-telemetry/opentelemetry-js/pull/3327) @dyladan * fix(resources): fix EnvDetector throwing errors when attribute values contain spaces [#3295](https://github.com/open-telemetry/opentelemetry-js/issues/3295) +* fix(resources): strict OTEL_RESOURCE_ATTRIBUTES baggage octet decoding + [#3341](https://github.com/open-telemetry/opentelemetry-js/pull/3341) @legendecas ### :books: (Refine Doc) diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index 12a3833744..bb78a3f7d3 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -416,7 +416,7 @@ describe('Node SDK', () => { describe('with a faulty environment variable', () => { beforeEach(() => { - process.env.OTEL_RESOURCE_ATTRIBUTES = 'bad=~attribute'; + process.env.OTEL_RESOURCE_ATTRIBUTES = 'bad=\\attribute'; }); it('prints correct error messages when EnvDetector has an invalid variable', async () => { diff --git a/packages/opentelemetry-resources/src/detectors/EnvDetector.ts b/packages/opentelemetry-resources/src/detectors/EnvDetector.ts index 44f15dcdfe..b7a98ff4e1 100644 --- a/packages/opentelemetry-resources/src/detectors/EnvDetector.ts +++ b/packages/opentelemetry-resources/src/detectors/EnvDetector.ts @@ -117,7 +117,7 @@ class EnvDetector implements Detector { if (!this._isValid(value)) { throw new Error(`Attribute value ${this._ERROR_MESSAGE_INVALID_VALUE}`); } - attributes[key] = value; + attributes[key] = decodeURIComponent(value); } return attributes; } @@ -130,13 +130,14 @@ class EnvDetector implements Detector { * @returns Whether the String is valid. */ private _isValid(name: string): boolean { - return name.length <= this._MAX_LENGTH && this._isPrintableString(name); + return name.length <= this._MAX_LENGTH && this._isBaggageOctetString(name); } - private _isPrintableString(str: string): boolean { + // https://www.w3.org/TR/baggage/#definition + private _isBaggageOctetString(str: string): boolean { for (let i = 0; i < str.length; i++) { - const ch: string = str.charAt(i); - if (ch < ' ' || ch >= '~') { + const ch = str.charCodeAt(i); + if (ch < 0x21 || ch === 0x2C || ch === 0x3B || ch === 0x5C || ch > 0x7E) { return false; } } diff --git a/packages/opentelemetry-resources/test/detectors/browser/EnvDetector.test.ts b/packages/opentelemetry-resources/test/detectors/browser/EnvDetector.test.ts index 940b5b07c0..da4354f89e 100644 --- a/packages/opentelemetry-resources/test/detectors/browser/EnvDetector.test.ts +++ b/packages/opentelemetry-resources/test/detectors/browser/EnvDetector.test.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import * as assert from 'assert'; import { RAW_ENVIRONMENT } from '@opentelemetry/core'; import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'; import { envDetector, Resource } from '../../../src'; @@ -27,7 +28,7 @@ describeBrowser('envDetector() on web browser', () => { describe('with valid env', () => { before(() => { (globalThis as typeof globalThis & RAW_ENVIRONMENT).OTEL_RESOURCE_ATTRIBUTES = - 'webengine.name="chromium",webengine.version="99",webengine.description="Chromium"'; + 'webengine.name="chromium",webengine.version="99",webengine.description="Chromium",custom.key="custom%20value"'; }); after(() => { @@ -41,6 +42,38 @@ describeBrowser('envDetector() on web browser', () => { [SemanticResourceAttributes.WEBENGINE_VERSION]: '99', [SemanticResourceAttributes.WEBENGINE_DESCRIPTION]: 'Chromium', }); + assert.strictEqual(resource.attributes['custom.key'], 'custom value'); + }); + }); + + + describe('with invalid env', () => { + const values = [ + 'webengine.description="with spaces"', + ]; + + for (const value of values) { + describe(`value: '${value}'`, () => { + before(() => { + (globalThis as typeof globalThis & RAW_ENVIRONMENT).OTEL_RESOURCE_ATTRIBUTES = value; + }); + + after(() => { + delete (globalThis as typeof globalThis & RAW_ENVIRONMENT).OTEL_RESOURCE_ATTRIBUTES; + }); + + it('should return empty resource', async () => { + const resource: Resource = await envDetector.detect(); + assertEmptyResource(resource); + }); + }); + } + }); + + describe('with empty env', () => { + it('should return empty resource', async () => { + const resource: Resource = await envDetector.detect(); + assertEmptyResource(resource); }); }); diff --git a/packages/opentelemetry-resources/test/detectors/node/EnvDetector.test.ts b/packages/opentelemetry-resources/test/detectors/node/EnvDetector.test.ts index 486de2b4bb..3f6b63a1f1 100644 --- a/packages/opentelemetry-resources/test/detectors/node/EnvDetector.test.ts +++ b/packages/opentelemetry-resources/test/detectors/node/EnvDetector.test.ts @@ -25,7 +25,7 @@ describeNode('envDetector() on Node.js', () => { describe('with valid env', () => { before(() => { process.env.OTEL_RESOURCE_ATTRIBUTES = - 'k8s.pod.name="pod-xyz-123",k8s.cluster.name="c1",k8s.namespace.name="default",k8s.deployment.name="deployment name"'; + 'k8s.pod.name="pod-xyz-123",k8s.cluster.name="c1",k8s.namespace.name="default",k8s.deployment.name="deployment%20name"'; }); after(() => { @@ -43,6 +43,29 @@ describeNode('envDetector() on Node.js', () => { }); }); + describe('with invalid env', () => { + const values = [ + 'k8s.deployment.name="with spaces"', + ]; + + for (const value of values) { + describe(`value: '${value}'`, () => { + before(() => { + process.env.OTEL_RESOURCE_ATTRIBUTES = value; + }); + + after(() => { + delete process.env.OTEL_RESOURCE_ATTRIBUTES; + }); + + it('should return empty resource', async () => { + const resource: Resource = await envDetector.detect(); + assertEmptyResource(resource); + }); + }); + } + }); + describe('with empty env', () => { it('should return empty resource', async () => { const resource: Resource = await envDetector.detect();