chore: sanitize http.url attribute values #3487

daniel-white · 2022-12-14T03:13:44Z

Which problem is this PR solving?

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

I added a map of semantic attributes to a value sanitizing function. also made this work for one off values.

Errata: I'm not sure if you want to classify this as a breaking change or not. Also, I'm not sure why my tests are failing. Any vscode+mocha tips appreciated!

Fixes #2000

Short description of the changes

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

added unit tests

Checklist:

Followed the style guidelines of this project
Unit tests have been added
[] Documentation has been updated

linux-foundation-easycla · 2022-12-14T03:13:49Z

The committers listed above are authorized under a signed CLA.

✅ login: daniel-white / name: Daniel A. White (b4c4cbc)

daniel-white · 2022-12-14T03:15:22Z

i'm not sure what i need to do - i accepted as an individual contributor for the cla, but it says it is not enabled for this project.

legendecas · 2022-12-15T03:15:40Z

Are you signing the CLA with the email used to commit?

osherv · 2022-12-19T16:34:58Z

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

+        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');
+        });


Can you add to the test log message validations?

osherv · 2022-12-19T16:51:54Z

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

+  let copyVal: AttributeValue;
+
+  if (Array.isArray(val)) {
+    copyVal = val.slice();


In this case, we also want to return, no?

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

osherv · 2022-12-19T16:57:54Z

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

+  if (!isValidPrimitiveAttributeValue(val)) {
+    diag.warn(
+      `Invalid attribute value set for key: ${SemanticAttributes.HTTP_URL}. Unable to sanitize complex value.`
+    );
+
+    out = val;
+
+    return out;
+  }


why do you need to check that if you only accept strings?

the URL constructor only takes a string or other URL instances.

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

daniel-white · 2022-12-19T18:27:00Z

thanks @osherv for the review - i'm working on your feedback.

do you have any tips on how that i can debug in vscode?

osherv · 2022-12-19T18:32:46Z

@daniel-white sure! unfortunately i'm working in Webstorm, i can help you debug with Webstorm 😅

daniel-white · 2022-12-27T21:28:14Z

i know my tests/lint are failing. however, i'd like to confirm that this is the pattern you all would like to go with

legendecas

Sorry for the late review. Thank you for working on this!

However, as the attribute http.url is part of the semantic conventions, but not part of the Trace SDK specification, I believe it would be more straightforward and performant to explicitly sanitize the credentials of the semantic convention attribute http.url in the instrumentations instead.

We can still export sanitizeHttpUrl in @opentelemetry/core to reduce duplications.

legendecas · 2022-12-28T08:26:18Z

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

@@ -94,3 +122,34 @@ function isValidPrimitiveAttributeValue(val: unknown): boolean {

  return false;
 }
+
+function sanitizeHttpUrl(val: AttributeValue): AttributeValue | undefined {


Would this return undefined?

legendecas · 2022-12-28T08:32:21Z

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

@@ -279,7 +285,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 {


In the SDK, we still keep the deprecated SpanAttributeValue because the SDK is required to support the older version of @opentelemetry/api.

Would you mind reverting these unrelated changes? Thank you!

legendecas · 2022-12-28T08:36:06Z

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

+      describe('http.url', () => {
+        it('should remove username and password from the url', () => {
+          const out = sanitizeAttribute(
+            SemanticAttributes.HTTP_URL,


Would you mind converting those test cases into table-based tests to reduce the repetition between tests?

const testData = [ { name: 'should remove username and password from the url', input: 'http://user:pass@host:9000/path?query#fragment', expect: 'http://host:9000/path?query#fragment', }, ... ]; for (const item of testData) { it(item.name, () => { ... }); }

sure that would be possible.

legendecas · 2022-12-28T08:43:26Z

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

+  let out: AttributeValue | undefined;
+
+  if (typeof val !== 'string') {
+    diag.warn(


I'd find these diag warnings in the value sanitizer unexpected. I think it should be still valid for trace-sdk users to set arbitrary valid attribute values as http.url. http.url is part of the semantic conventions, not part of the SDK specifications.

@open-telemetry/javascript-approvers WDYT?

according to #2000, its part of the specification. i'm open to all of your guidance on how to approach this

Yes, it's part of the semantic convention specification, not the trace sdk specification. So IMHO the constraints should only be applied to instrumentations that collect those attributes.

github-actions · 2023-04-03T06:34:38Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions · 2023-06-12T06:34:52Z

This PR was closed because it has been stale for 14 days with no activity.

daniel-white requested a review from a team December 14, 2022 03:13

daniel-white force-pushed the chore/sanitize-url branch 3 times, most recently from 89aea3a to e74790b Compare December 17, 2022 01:24

osherv reviewed Dec 19, 2022

View reviewed changes

packages/opentelemetry-core/src/common/attributes.ts Outdated Show resolved Hide resolved

osherv reviewed Dec 19, 2022

View reviewed changes

packages/opentelemetry-core/src/common/attributes.ts Outdated Show resolved Hide resolved

daniel-white requested a review from osherv December 19, 2022 18:59

daniel-white force-pushed the chore/sanitize-url branch 3 times, most recently from 90a1bc7 to 6b9e85e Compare December 21, 2022 21:27

legendecas reviewed Dec 28, 2022

View reviewed changes

daniel-white force-pushed the chore/sanitize-url branch from 6b9e85e to daec69f Compare January 7, 2023 16:33

daniel-white added 2 commits January 15, 2023 11:05

feat: sanitize http.url attribute values

5f3b6ae

revert changes, new strategy for function

de9e811

daniel-white force-pushed the chore/sanitize-url branch from daec69f to de9e811 Compare January 15, 2023 16:20

daniel-white marked this pull request as draft January 15, 2023 16:20

github-actions bot added the stale label Apr 3, 2023

github-actions bot closed this Jun 12, 2023

legendecas mentioned this pull request Jul 13, 2023

HTTP Span Attributes: http.url must not contain username / password #2000

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: sanitize http.url attribute values #3487

chore: sanitize http.url attribute values #3487

daniel-white commented Dec 14, 2022

linux-foundation-easycla bot commented Dec 14, 2022 •

edited

Loading

daniel-white commented Dec 14, 2022

legendecas commented Dec 15, 2022

osherv Dec 19, 2022

osherv Dec 19, 2022

osherv Dec 19, 2022

daniel-white Dec 19, 2022

daniel-white commented Dec 19, 2022

osherv commented Dec 19, 2022

daniel-white commented Dec 27, 2022

legendecas left a comment

legendecas Dec 28, 2022

legendecas Dec 28, 2022

daniel-white Jan 7, 2023

legendecas Dec 28, 2022

daniel-white Jan 7, 2023

legendecas Dec 28, 2022

daniel-white Jan 7, 2023

legendecas Jan 31, 2023

github-actions bot commented Apr 3, 2023

github-actions bot commented Jun 12, 2023

chore: sanitize http.url attribute values #3487

chore: sanitize http.url attribute values #3487

Conversation

daniel-white commented Dec 14, 2022

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Dec 14, 2022 • edited Loading

daniel-white commented Dec 14, 2022

legendecas commented Dec 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-white commented Dec 19, 2022

osherv commented Dec 19, 2022

daniel-white commented Dec 27, 2022

legendecas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 3, 2023

github-actions bot commented Jun 12, 2023

linux-foundation-easycla bot commented Dec 14, 2022 •

edited

Loading