fix: sanitize attributes inputs

CHANGELOG.md

@@ -10,6 +10,8 @@ All notable changes to this project will be documented in this file.
 
 ### :bug: (Bug Fix)
 
+* [#2881](https://github.com/open-telemetry/opentelemetry-js/pull/2881) fix: sanitize attributes inputs ([@legendecas](https://githuc.com/legendecas))
+
 ### :books: (Refine Doc)
 
 * [#2860](https://github.com/open-telemetry/opentelemetry-js/pull/2860) docs(sdk): update earliest support node version ([@svetlanabrennan](https://github.com/svetlanabrennan))

packages/opentelemetry-core/src/common/attributes.ts

@@ -13,17 +13,19 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { SpanAttributeValue, SpanAttributes } from '@opentelemetry/api';
+import { AttributeValue, Attributes } from '@opentelemetry/api';
 
-export function sanitizeAttributes(attributes: unknown): SpanAttributes {
-  const out: SpanAttributes = {};
+export function sanitizeAttributes(attributes: unknown): Attributes {
+  const out: Attributes = {};
 
-  if (attributes == null || typeof attributes !== 'object') {
+  if (typeof attributes !== 'object' || attributes == null) {
     return out;
   }
 
-  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-  for (const [k, v] of Object.entries(attributes!)) {
+  for (const [k, v] of Object.entries(attributes)) {
+    if (!isAttributeKey(k)) {
+      continue;
+    }
     if (isAttributeValue(v)) {
       if (Array.isArray(v)) {
         out[k] = v.slice();
@@ -36,7 +38,11 @@ export function sanitizeAttributes(attributes: unknown): SpanAttributes {
   return out;
 }
 
-export function isAttributeValue(val: unknown): val is SpanAttributeValue {
+export function isAttributeKey(key: unknown): key is string {
+  return typeof key === 'string' && key.length > 0;
+}
+
+export function isAttributeValue(val: unknown): val is AttributeValue {
   if (val == null) {
     return true;
   }

packages/opentelemetry-core/src/trace/sampler/ParentBasedSampler.ts

@@ -15,7 +15,7 @@
  */
 
 import {
-  SpanAttributes,
+  Attributes,
   Context,
   isSpanContextValid,
   Link,
@@ -64,7 +64,7 @@ export class ParentBasedSampler implements Sampler {
     traceId: string,
     spanName: string,
     spanKind: SpanKind,
-    attributes: SpanAttributes,
+    attributes: Attributes,
     links: Link[]
   ): SamplingResult {
     const parentContext = trace.getSpanContext(context);

packages/opentelemetry-exporter-zipkin/src/transform.ts

@@ -66,9 +66,9 @@ export function toZipkinSpan(
   return zipkinSpan;
 }
 
-/** Converts OpenTelemetry SpanAttributes and SpanStatus to Zipkin Tags format. */
+/** Converts OpenTelemetry Attributes and SpanStatus to Zipkin Tags format. */
 export function _toZipkinTags(
-  attributes: api.SpanAttributes,
+  attributes: api.Attributes,
   status: api.SpanStatus,
   statusCodeTagName: string,
   statusErrorTagName: string,

packages/opentelemetry-resources/src/Resource.ts

@@ -68,7 +68,7 @@ export class Resource {
   merge(other: Resource | null): Resource {
     if (!other || !Object.keys(other.attributes).length) return this;
 
-    // SpanAttributes from resource overwrite attributes from other resource.
+    // Attributes from resource overwrite attributes from other resource.
     const mergedAttributes = Object.assign(
       {},
       this.attributes,

packages/opentelemetry-sdk-trace-base/src/Span.ts

@@ -22,6 +22,7 @@ import {
   InstrumentationLibrary,
   isTimeInput,
   timeInputToHrTime,
+  sanitizeAttributes,
 } from '@opentelemetry/core';
 import { Resource } from '@opentelemetry/resources';
 import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
@@ -30,7 +31,7 @@ import { TimedEvent } from './TimedEvent';
 import { Tracer } from './Tracer';
 import { SpanProcessor } from './SpanProcessor';
 import { SpanLimits } from './types';
-import { SpanAttributeValue, Context } from '@opentelemetry/api';
+import { AttributeValue, Context } from '@opentelemetry/api';
 import { ExceptionEventName } from './enums';
 
 /**
@@ -42,7 +43,7 @@ export class Span implements api.Span, ReadableSpan {
   private readonly _spanContext: api.SpanContext;
   readonly kind: api.SpanKind;
   readonly parentSpanId?: string;
-  readonly attributes: api.SpanAttributes = {};
+  readonly attributes: api.Attributes = {};
   readonly links: api.Link[] = [];
   readonly events: TimedEvent[] = [];
   readonly startTime: api.HrTime;
@@ -88,7 +89,7 @@ export class Span implements api.Span, ReadableSpan {
     return this._spanContext;
   }
 
-  setAttribute(key: string, value?: SpanAttributeValue): this;
+  setAttribute(key: string, value?: AttributeValue): this;
   setAttribute(key: string, value: unknown): this {
     if (value == null || this._isSpanEnded()) return this;
     if (key.length === 0) {
@@ -111,7 +112,7 @@ export class Span implements api.Span, ReadableSpan {
     return this;
   }
 
-  setAttributes(attributes: api.SpanAttributes): this {
+  setAttributes(attributes: api.Attributes): this {
     for (const [k, v] of Object.entries(attributes)) {
       this.setAttribute(k, v);
     }
@@ -127,7 +128,7 @@ export class Span implements api.Span, ReadableSpan {
    */
   addEvent(
     name: string,
-    attributesOrStartTime?: api.SpanAttributes | api.TimeInput,
+    attributesOrStartTime?: api.Attributes | api.TimeInput,
     startTime?: api.TimeInput
   ): this {
     if (this._isSpanEnded()) return this;
@@ -148,9 +149,11 @@ export class Span implements api.Span, ReadableSpan {
     if (typeof startTime === 'undefined') {
       startTime = hrTime();
     }
+
+    const attributes = sanitizeAttributes(attributesOrStartTime);
     this.events.push({
       name,
-      attributes: attributesOrStartTime as api.SpanAttributes,
+      attributes,
       time: timeInputToHrTime(startTime),
     });
     return this;
@@ -193,7 +196,7 @@ export class Span implements api.Span, ReadableSpan {
   }
 
   recordException(exception: api.Exception, time: api.TimeInput = hrTime()): void {
-    const attributes: api.SpanAttributes = {};
+    const attributes: api.Attributes = {};
     if (typeof exception === 'string') {
       attributes[SemanticAttributes.EXCEPTION_MESSAGE] = exception;
     } else if (exception) {
@@ -217,7 +220,7 @@ export class Span implements api.Span, ReadableSpan {
       attributes[SemanticAttributes.EXCEPTION_TYPE] ||
       attributes[SemanticAttributes.EXCEPTION_MESSAGE]
     ) {
-      this.addEvent(ExceptionEventName, attributes as api.SpanAttributes, time);
+      this.addEvent(ExceptionEventName, attributes, time);
     } else {
       api.diag.warn(`Failed to record an exception ${exception}`);
     }
@@ -260,7 +263,7 @@ export class Span implements api.Span, ReadableSpan {
    * @param value Attribute value
    * @returns truncated attribute value if required, otherwise same value
    */
-  private _truncateToSize(value: SpanAttributeValue): SpanAttributeValue {
+  private _truncateToSize(value: AttributeValue): AttributeValue {
     const limit = this._attributeValueLengthLimit;
     // Check limit
     if (limit <= 0) {

packages/opentelemetry-sdk-trace-base/src/TimedEvent.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { HrTime, SpanAttributes } from '@opentelemetry/api';
+import { HrTime, Attributes } from '@opentelemetry/api';
 
 /**
  * Represents a timed event.
@@ -25,5 +25,5 @@ export interface TimedEvent {
   /** The name of the event. */
   name: string;
   /** The attributes of the event. */
-  attributes?: SpanAttributes;
+  attributes?: Attributes;
 }

packages/opentelemetry-sdk-trace-base/src/Tracer.ts

@@ -92,7 +92,12 @@ export class Tracer implements api.Tracer {
     }
 
     const spanKind = options.kind ?? api.SpanKind.INTERNAL;
-    const links = options.links ?? [];
+    const links = (options.links ?? []).map(link => {
+      return {
+        context: link.context,
+        attributes: sanitizeAttributes(link.attributes),
+      };
+    });
     const attributes = sanitizeAttributes(options.attributes);
     // make sampling decision
     const samplingResult = this._sampler.shouldSample(
@@ -124,8 +129,10 @@ export class Tracer implements api.Tracer {
       links,
       options.startTime
     );
-    // Set default attributes
-    span.setAttributes(Object.assign(attributes, samplingResult.attributes));
+    // Set initial span attributes. The attributes object may have been mutated
+    // by the sampler, so we sanitize the merged attributes before setting them.
+    const initAttributes = sanitizeAttributes(Object.assign(attributes, samplingResult.attributes));
+    span.setAttributes(initAttributes);
     return span;
   }
 

packages/opentelemetry-sdk-trace-base/src/export/ReadableSpan.ts

@@ -17,7 +17,7 @@
 import {
   SpanKind,
   SpanStatus,
-  SpanAttributes,
+  Attributes,
   HrTime,
   Link,
   SpanContext,
@@ -34,7 +34,7 @@ export interface ReadableSpan {
   readonly startTime: HrTime;
   readonly endTime: HrTime;
   readonly status: SpanStatus;
-  readonly attributes: SpanAttributes;
+  readonly attributes: Attributes;
   readonly links: Link[];
   readonly events: TimedEvent[];
   readonly duration: HrTime;

packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts

@@ -22,6 +22,8 @@ import {
   SpanKind,
   TraceFlags,
   HrTime,
+  Attributes,
+  AttributeValue,
 } from '@opentelemetry/api';
 import {
   DEFAULT_ATTRIBUTE_COUNT_LIMIT,
@@ -35,6 +37,7 @@ 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';
 
 const performanceTimeOrigin: HrTime = [1, 1];
 
@@ -237,28 +240,14 @@ describe('Span', () => {
           SpanKind.CLIENT
         );
 
-        span.setAttribute('string', 'string');
-        span.setAttribute('number', 0);
-        span.setAttribute('bool', true);
-        span.setAttribute('array<string>', ['str1', 'str2']);
-        span.setAttribute('array<number>', [1, 2]);
-        span.setAttribute('array<bool>', [true, false]);
-
-        //@ts-expect-error invalid attribute type object
-        span.setAttribute('object', { foo: 'bar' });
-        //@ts-expect-error invalid attribute inhomogenous array
-        span.setAttribute('non-homogeneous-array', [0, '']);
-        // This empty length attribute should not be set
-        span.setAttribute('', 'empty-key');
+        for (const [k, v] of Object.entries(validAttributes)) {
+          span.setAttribute(k, v);
+        }
+        for (const [k, v] of Object.entries(invalidAttributes)) {
+          span.setAttribute(k, v as unknown as AttributeValue);
+        }
 
-        assert.deepStrictEqual(span.attributes, {
-          string: 'string',
-          number: 0,
-          bool: true,
-          'array<string>': ['str1', 'str2'],
-          'array<number>': [1, 2],
-          'array<bool>': [true, false],
-        });
+        assert.deepStrictEqual(span.attributes, validAttributes);
       });
 
       it('should be able to overwrite attributes', () => {
@@ -623,43 +612,42 @@ describe('Span', () => {
         SpanKind.CLIENT
       );
 
-      span.setAttributes({
-        string: 'string',
-        number: 0,
-        bool: true,
-        'array<string>': ['str1', 'str2'],
-        'array<number>': [1, 2],
-        'array<bool>': [true, false],
-        //@ts-expect-error invalid attribute type object
-        object: { foo: 'bar' },
-        //@ts-expect-error invalid attribute inhomogenous array
-        'non-homogeneous-array': [0, ''],
-        // This empty length attribute should not be set
-        '': 'empty-key',
-      });
+      span.setAttributes(validAttributes);
+      span.setAttributes(invalidAttributes as unknown as Attributes);
 
-      assert.deepStrictEqual(span.attributes, {
-        string: 'string',
-        number: 0,
-        bool: true,
-        'array<string>': ['str1', 'str2'],
-        'array<number>': [1, 2],
-        'array<bool>': [true, false],
-      });
+      assert.deepStrictEqual(span.attributes, validAttributes);
     });
   });
 
-  it('should set an event', () => {
-    const span = new Span(
-      tracer,
-      ROOT_CONTEXT,
-      name,
-      spanContext,
-      SpanKind.CLIENT
-    );
-    span.addEvent('sent');
-    span.addEvent('rev', { attr1: 'value', attr2: 123, attr3: true });
-    span.end();
+  describe('addEvent', () => {
+    it('should add an event', () => {
+      const span = new Span(
+        tracer,
+        ROOT_CONTEXT,
+        name,
+        spanContext,
+        SpanKind.CLIENT
+      );
+      span.addEvent('sent');
+      span.addEvent('rev', { attr1: 'value', attr2: 123, attr3: true });
+      span.end();
+    });
+
+    it('should sanitize attribute values', () => {
+      const span = new Span(
+        tracer,
+        ROOT_CONTEXT,
+        name,
+        spanContext,
+        SpanKind.CLIENT
+      );
+      span.addEvent('rev', { ...validAttributes, ...invalidAttributes } as unknown as Attributes);
+      span.end();
+
+      assert.strictEqual(span.events.length, 1);
+      assert.deepStrictEqual(span.events[0].name, 'rev');
+      assert.deepStrictEqual(span.events[0].attributes, validAttributes);
+    });
   });
 
   it('should set a link', () => {
@@ -836,14 +824,14 @@ describe('Span', () => {
     assert.strictEqual(span.events.length, 1);
     const [event] = span.events;
     assert.deepStrictEqual(event.name, 'sent');
-    assert.ok(!event.attributes);
+    assert.deepStrictEqual(event.attributes, {});
     assert.ok(event.time[0] > 0);
 
     span.addEvent('rev', { attr1: 'value', attr2: 123, attr3: true });
     assert.strictEqual(span.events.length, 2);
     const [event1, event2] = span.events;
     assert.deepStrictEqual(event1.name, 'sent');
-    assert.ok(!event1.attributes);
+    assert.deepStrictEqual(event1.attributes, {});
     assert.ok(event1.time[0] > 0);
     assert.deepStrictEqual(event2.name, 'rev');
     assert.deepStrictEqual(event2.attributes, {