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

fix(instrumentation-http): skip malformed forwarded headers #5099

Conversation

pmlanger
Copy link
Contributor

@pmlanger pmlanger commented Oct 30, 2024

Which problem is this PR solving?

Skips malformed forwarded headers instead of throwing an uncaught exception.

Fixes #5095

Short description of the changes

Introduce parseForwardedHeader in experimental/packages/opentelemetry-instrumentation-http/src/utils.ts which wraps forwarded-parse and returns an empty array on any caught exception.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

First, wrote a test that broke the instrumentation in the expected way (throwing ParseError: Unexpected end of input).
Then, fixed it and confirmed the test passes.

Checklist:

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

Copy link

linux-foundation-easycla bot commented Oct 30, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@omBratteng
Copy link

I think this would solve an issue we stumbled upon, where the forwarded header contains a base64 encoded value that ends with a =, and technically makes it malformed.

Could you add a test case for this value?

for=127.0.0.1;host=something.vercel.app;proto=https;sig=0QmVhcmVyIDc3MGNlY2M5YTIyZjJmNTg1ZGNlOTQ1NTUzMTIwMGFkMmZhOGEzZGNiM2M2NWQ4ZDk2ZGI5MDExMDkzMjkwYWM=;exp=1730556049

@pmlanger
Copy link
Contributor Author

pmlanger commented Nov 4, 2024

@omBratteng not sure there's much value adding tests for different kinds of malformed Forwarded headers.
fwiw, forwarded-parse throws on your input:

> fp = require('forwarded-parse')
[Function: parse]
> fp('for=127.0.0.1;host=something.vercel.app;proto=https;sig=0QmVhcmVyIDc3MGNlY2M5YTIyZjJmNTg1ZGNlOTQ1NTUzMTIwMGFkMmZhOGEzZGNiM2M2NWQ4ZDk2ZGI5MDExMDkzMjkwYWM=;exp=1730556049')
Uncaught ParseError: Unexpected character '=' at index 152
    at parse (/Users/philipp.langer/dev/opentelemetry-js/node_modules/forwarded-parse/index.js:121:17) {
  input: 'for=127.0.0.1;host=something.vercel.app;proto=https;sig=0QmVhcmVyIDc3MGNlY2M5YTIyZjJmNTg1ZGNlOTQ1NTUzMTIwMGFkMmZhOGEzZGNiM2M2NWQ4ZDk2ZGI5MDExMDkzMjkwYWM=;exp=1730556049'

so it should be covered by this fix as well.

@pmlanger pmlanger force-pushed the instrumentation-http-malformed-forwarded-headers branch from 61f4855 to bc159fb Compare November 4, 2024 13:57
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

looks good - thanks for taking care of this 👍

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.18%. Comparing base (ce5bbfb) to head (bc159fb).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5099   +/-   ##
=======================================
  Coverage   93.18%   93.18%           
=======================================
  Files         315      315           
  Lines        8086     8086           
  Branches     1617     1617           
=======================================
  Hits         7535     7535           
  Misses        551      551           

@pichlermarc pichlermarc added this pull request to the merge queue Nov 5, 2024
Merged via the queue into open-telemetry:main with commit 67d7718 Nov 5, 2024
21 checks passed
@kiyaGu
Copy link

kiyaGu commented Nov 5, 2024

@pichlermarc is it possible to know when this will be released? One of our app uses the version with the issue and we are wondering either downgrade or wait for the new release - Thanks

@pichlermarc
Copy link
Member

pichlermarc commented Nov 5, 2024

@kiyaGu I published it just now (@opentelemetry/[email protected], GitHub Release)

@pmlanger pmlanger deleted the instrumentation-http-malformed-forwarded-headers branch November 7, 2024 16:53
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.

opentelemetry-instrumentation-http throws on requests with malformed Forwarded headers
6 participants