fix(resources): strict OTEL_RESOURCE_ATTRIBUTES decoding (#3341)

Co-authored-by: Daniel Dyla <[email protected]>
open-telemetry · Oct 28, 2022 · f31448e · f31448e
1 parent e9347e4
commit f31448e
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,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
@@ -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
@@ -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
@@ -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
@@ -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();