Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure base64 encoded values can be used as headers from env #943

Merged
merged 3 commits into from
Mar 26, 2023

Conversation

cedricziel
Copy link
Contributor

@cedricziel cedricziel commented Mar 25, 2023

This change instructs explode to extract at most 2 parts.

Given an example value of

OTEL_EXPORTER_OTLP_HEADERS=Authorization=Basic bla64=

The config parser would have created three elements, where only two are required. The trailing = in base64 headers would then have been effectively discarded, leading to invalid configuration.

This change instructs explode to extract at most 2 parts.

Given an example value of 
```
OTEL_EXPORTER_OTLP_HEADERS=Authorization=Basic bla64=
```

The config parser would have created the elements, where only two are required. The trailing `=` in base64 headers would then have been effectively discarded, leading to invalid configuration.
@cedricziel cedricziel requested a review from a team March 25, 2023 10:25
@welcome
Copy link

welcome bot commented Mar 25, 2023

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

@codecov
Copy link

codecov bot commented Mar 25, 2023

Codecov Report

Merging #943 (089dfd9) into main (4194522) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 089dfd9 differs from pull request most recent head d368da7. Consider uploading reports for the commit d368da7 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #943   +/-   ##
=========================================
  Coverage     83.39%   83.39%           
  Complexity     2122     2122           
=========================================
  Files           277      277           
  Lines          6096     6096           
=========================================
  Hits           5084     5084           
  Misses         1012     1012           
Flag Coverage Δ
7.4 82.04% <100.00%> (ø)
8.0 83.35% <100.00%> (ø)
8.1 83.47% <100.00%> (ø)
8.2 83.47% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/SDK/Common/Configuration/Parser/MapParser.php 94.44% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4194522...d368da7. Read the comment docs.

Copy link
Collaborator

@brettmc brettmc left a comment

Choose a reason for hiding this comment

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

LGTM - could you add a test case for this?

@cedricziel
Copy link
Contributor Author

@brettmc added a test

@cedricziel cedricziel requested a review from brettmc March 25, 2023 22:09
@brettmc
Copy link
Collaborator

brettmc commented Mar 26, 2023

@cedricziel one linting issue to fix (make style should do it), then it's good to go.

@cedricziel
Copy link
Contributor Author

@brettmc done

@brettmc brettmc merged commit a2a4d4d into open-telemetry:main Mar 26, 2023
@brettmc
Copy link
Collaborator

brettmc commented Mar 26, 2023

Thanks for your contribution, @cedricziel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants