Un-escape url coding when parsing baggage. #2529
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MadVikingGod
requested review from
Aneurysm9,
dashpole,
evantorrie,
jmacd,
MrAlias,
paivagustavo,
pellared and
XSAM
as code owners
Codecov Report
@@ Coverage Diff @@
## main #2529 +/- ##
=======================================
- Coverage 76.0% 75.9% -0.2%
=======================================
Files 174 174
Lines 12088 12093 +5
=======================================
- Hits 9198 9189 -9
- Misses 2647 2658 +11
- Partials 243 246 +3
|
Aneurysm9
approved these changes
Jan 21, 2022
MrAlias
approved these changes
Jan 22, 2022
jmacd
approved these changes
Jan 24, 2022
Merged
MrAlias
added a commit
that referenced
this pull request
Mar 21, 2022
* Allow setting the Sampler via environment variables (#2305) * Add changelog entry. * Replace t.Setenv with internaltest/SetEnvVariables for Go <= 1.6. * Handle the lack of a sampler argument without logging errors. * Add additional test cases and error checks. * Refactor documentation. Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com> * emitBatchOverhead should only be used for splitting spans into batches (#2512) * emitBatchOverhead should only be used for splitting spans into batches (#2503) * limit max packet size parameter * Add additional errors types, simplify abstractions and error handling * Make error comparisons less fragile. * Fix typo in jaeger example (#2524) * Fix some typos in docs for Go libraries (#2520) Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Fix getting-started.md Run function (#2527) * Fix getting-started.md Run function, it assigns this new context to a variable shared between connections in to accept loop. Thus creating a growing chain of contexts. so every calculate fibonacci request, all spans in a trace. * add a comment explaining the reason for that new variable * update example fib * Bump github.com/google/go-cmp from 0.5.6 to 0.5.7 across the project (#2545) * update go-cmp to 0.5.7 * fixes go.sums Co-authored-by: Aaron Clawson <MadVikingGod@users.noreply.github.com> * Un-escape url coding when parsing baggage. (#2529) * un-escape url coding when parsing baggage. * Added changelog Co-authored-by: Aaron Clawson <MadVikingGod@users.noreply.github.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Bump go.opentelemetry.io/proto/otlp from 0.11.0 to 0.12.0 (#2546) * Update go.opentelemetry.io/proto/otlp to v0.12.0 * Changelog * Update CHANGELOG.md Fix's md linting Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> Co-authored-by: Aaron Clawson <MadVikingGod@users.noreply.github.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Remove unused sdk/internal/santize (#2549) * Add links to code examples and docs (#2551) * Bump github.com/prometheus/client_golang from 1.11.0 to 1.12.0 in /exporters/prometheus (#2541) * Bump github.com/prometheus/client_golang in /exporters/prometheus Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.11.0 to 1.12.0. - [Release notes](https://github.com/prometheus/client_golang/releases) - [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md) - [Commits](prometheus/client_golang@v1.11.0...v1.12.0) --- updated-dependencies: - dependency-name: github.com/prometheus/client_golang dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * go mod tidy Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Tyler Yahn <codingalias@gmail.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Optimize evictedQueue implementation and use (#2556) * Optimize evictedQueue impl and use Avoid unnecessary allocations in the recordingSpan by using an evictedQueue type instead of a pointer to one. Lazy allocate the evictedQueue queue to prevent unnecessary operations for spans without any use of the queue. Document the evictedQueue * Fix grammar * Add env support for batch span processor (#2515) * Add env support for batch span processor * Update changelog * lint * Bump golang.org/x/tools from 0.1.8 to 0.1.9 in /internal/tools (#2566) * Bump golang.org/x/tools from 0.1.8 to 0.1.9 in /internal/tools Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.1.8 to 0.1.9. - [Release notes](https://github.com/golang/tools/releases) - [Commits](golang/tools@v0.1.8...v0.1.9) --- updated-dependencies: - dependency-name: golang.org/x/tools dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: MrAlias <MrAlias@users.noreply.github.com> * Bump github.com/golangci/golangci-lint from 1.43.0 to 1.44.0 in /internal/tools (#2567) * Bump github.com/golangci/golangci-lint in /internal/tools Bumps [github.com/golangci/golangci-lint](https://github.com/golangci/golangci-lint) from 1.43.0 to 1.44.0. - [Release notes](https://github.com/golangci/golangci-lint/releases) - [Changelog](https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md) - [Commits](golangci/golangci-lint@v1.43.0...v1.44.0) --- updated-dependencies: - dependency-name: github.com/golangci/golangci-lint dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: MrAlias <MrAlias@users.noreply.github.com> * Bump github.com/prometheus/client_golang from 1.12.0 to 1.12.1 in /exporters/prometheus (#2570) * Bump github.com/prometheus/client_golang in /exporters/prometheus Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.12.0 to 1.12.1. - [Release notes](https://github.com/prometheus/client_golang/releases) - [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md) - [Commits](prometheus/client_golang@v1.12.0...v1.12.1) --- updated-dependencies: - dependency-name: github.com/prometheus/client_golang dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: MrAlias <MrAlias@users.noreply.github.com> * Fix TestBackoffRetry in otlp/internal/retry package (#2562) * Fix TestBackoffRetry in otlp retry pkg The delay of the retry is within two times a randomization factor (the back-off time is delay * random number within [1 - factor, 1 + factor]. This means the waitFunc in TestBackoffRetry needs to check the delay is within an appropriate delta, not equal to configure initial delay. * Fix delta value * Fix delta Co-authored-by: Aaron Clawson <Aaron.Clawson@gmail.com> * Bump google.golang.org/grpc from 1.43.0 to 1.44.0 in /exporters/otlp/otlptrace (#2568) * Bump google.golang.org/grpc in /exporters/otlp/otlptrace Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.43.0 to 1.44.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.43.0...v1.44.0) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: MrAlias <MrAlias@users.noreply.github.com> * Bump google.golang.org/grpc from 1.43.0 to 1.44.0 in /example/otel-collector (#2565) * Bump google.golang.org/grpc in /example/otel-collector Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.43.0 to 1.44.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.43.0...v1.44.0) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: MrAlias <MrAlias@users.noreply.github.com> * Bump google.golang.org/grpc from 1.43.0 to 1.44.0 in /exporters/otlp/otlpmetric (#2572) * Bump google.golang.org/grpc in /exporters/otlp/otlpmetric Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.43.0 to 1.44.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.43.0...v1.44.0) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: MrAlias <MrAlias@users.noreply.github.com> * Change Options to accept type not pointer (#2558) * Change trace options to accept type not pointer Add benchmark to show allocation improvement. * Update CONTRIBUTING.md guidelines * Update all Option iface * Fix grammar in CONTRIBUTING * Do not store TracerProvider or Tracer fields in SDK recordingSpan (#2575) * Do not store TracerProvider fields in span Instead of keeping a reference to the span's Tracer, and therefore also it's TracerProvider, and the associated resource and spanLimits just keep the reference to the Tracer. Refer to the TracerProvider fields when needed instead. * Make span refer to the inst lib via the Tracer Instead of holding a field in the span, refer to the field in the parent Tracer. * [website_docs] fix page meta-links (#2580) Contributes to open-telemetry/opentelemetry.io#1096 /cc @cartermp @austinlparker * Validate members once, in `NewMember` (#2522) * use NewMember, or specify if the member is not validated when creating new ones * expect members to already be validated when creating a new package * add changelog entry * add an isEmpty field to member and property for quick validation * rename isEmpty to hasData So by default, an empty struct really is marked as having no data * Update baggage/baggage_test.go Co-authored-by: Aaron Clawson <Aaron.Clawson@gmail.com> * don't validate the member in parseMember, we alredy ran that validation We also don't want to use NewMember, as that runs the property validation again, making the benchmark quite slower * move changelog entry to the fixed section * provide the member/property data when returning an invalid error Co-authored-by: Aaron Clawson <Aaron.Clawson@gmail.com> * Fix link to Zipkin exporter (#2581) Currently it is linked to the old package that was moved. * Unexport EnvBatchSpanProcessor* constants (#2583) * Move BSP env support to internal * Use pkg name * Update env test * Use internal/env in sdk/trace * Avoid an extra allocation in applyTracerProviderEnvConfigs. * Add additional errors for ratio > 1.0. * Add test cases for ratio > 1.0. * Update CHANGELOG.md Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com> Co-authored-by: jaychung <ken8203@gmail.com> Co-authored-by: Ben Wells <b.v.wells@gmail.com> Co-authored-by: Jeremy Kaplan <jeremy@stytch.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> Co-authored-by: thinkgo <49174849+thinkgos@users.noreply.github.com> Co-authored-by: Aaron Clawson <Aaron.Clawson@gmail.com> Co-authored-by: Aaron Clawson <MadVikingGod@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Tyler Yahn <codingalias@gmail.com> Co-authored-by: Chao Weng <19381524+sincejune@users.noreply.github.com> Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com> Co-authored-by: Damien Mathieu <42@dmathieu.com>
yurishkuro
reviewed
Feb 8, 2023
| @@ -339,6 +339,13 @@ func TestBaggageParse(t *testing.T) { | |||
| "foo": {Value: "2"}, | |||
| }, | |||
| }, | |||
| { | |||
| name: "url encoded value", | |||
| in: "key1=val%252", | |||
There was a problem hiding this comment.
This test was insufficient. The regexp check now happens after QueryUnescape, not at all what was intended in the original code. As a result, valid strings like val%27 do not pass the check. This specific test does pass because the decoded string val%2 still "looks like" URL-encoded (i.e. it passes the regex).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2519
The W3c Sepc indicates that the value of a baggage member is a URL encoded string. We currently escape properly when we encode into the header, but we don't unescape when we praise from a header.
This change fixes that and adds tests at the propagator, and baggage level to ensure.