Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

Protect from exceptions in decodeURIComponent; do not url-encode span context #105

Merged
merged 2 commits into from
Mar 29, 2017

Conversation

yurishkuro
Copy link
Member

decodeURIComponent can throw URIError: URI malformed when unable to parse the string.

The string representation of the context is #:#:#:# (# is hex digits), so it is "safe" to be used as is in the http header value without URL encoding.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.6%) to 98.604% when pulling 2efd91f on improve-http-encoding into 2f77eeb on master.

import SpanContext from '../src/span_context';

describe ('TextMapCodec', () => {
it('should not URL-decode value that has no % meta-characters', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is insufficient because decodeURIComponent(abc) returns abc. We should explicitly verify that decodeURIComponent('abc') has never been called.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, but I am not sure how to do that. @saminzadeh is there a way to mock standard functions?

if (this._urlEncoding && value.indexOf('%') > -1) {
// unfortunately, decodeURIComponent() can throw 'URIError: URI malformed'
try {
return decodeURIComponent(value);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saminzadeh do you know if there's an alternative std function that never throws exceptions?

Copy link
Contributor

@vprithvi vprithvi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If decodeURIComponent is hard to mock, I'm ok with merging this

@coveralls
Copy link

Coverage Status

Coverage increased (+2.6%) to 98.606% when pulling b079954 on improve-http-encoding into 2f77eeb on master.

@yurishkuro yurishkuro merged commit 99ecf02 into master Mar 29, 2017
@yurishkuro yurishkuro deleted the improve-http-encoding branch February 26, 2018 20:24
Iuriy-Budnikov pushed a commit to agile-pm/jaeger-client-node that referenced this pull request Sep 25, 2021
* Adds Metrics API

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/metrics-api.md

adds gauge and counter types

checkpoint

checkpoint

update docs

update index.ts

move todo

* yarn check
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants