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 error due to missing header in the response in resolveSampling method #706

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

jj22ee
Copy link
Contributor

@jj22ee jj22ee commented Jan 9, 2025

Issue #, if available:

The following error can occur when an incoming request header x-amzn-trace-id has Sampled=?. This can cause an error to occur for this logic specific for Sampled=?.

TypeError: Cannot set properties of undefined (setting 'x-amzn-trace-id')
    at Object.resolveSampling (/ <...> /aws-xray-sdk-node/packages/core/dist/lib/middleware/mw_utils.js:93:37)
    at Object.traceRequestResponseCycle (/ <...> /aws-xray-sdk-node/packages/core/dist/lib/middleware/mw_utils.js:153:14)

The root cause is that the traceRequestResponseCycle method is creating a copy of the response with Object.assign({}, res, { req: req }), but in doing so, res.header is not copied via Object.assign since it is a prototype method. Thus when this copied response is passed into resolveSampling, the error occurs when res.header is accessed.

The bug is introduced in this old commit - See Before and After.

  • This old commit makes a copied value of the response to modify it, but doesn't do anything with it later. So it is not a useful change and is likely unintended. Since there doesn't seem to be any good reason for this specific change, this part could be reverted.

This traceRequestResponseCycle logic is used by aws-xray-sdk-express, aws-xray-sdk-restify, and aws-xray-sdk-fastify.

Also, res.header is a method, not an object which the SDK currently assumes.

Description of changes:

  • The response that is passed into the resolveSampling method in mw_utils.js is now the original response, which was the previous/original logic for Express and Restify. This is also the logic currently used in aws-xray-sdk-hapi and aws-xray-sdk-koa2.
  • Add truthy check for res.header before it is modified in resolveSampling. This is needed because res.header is not guaranteed to exist, as is the case when this logic is used in aws-xray-sdk-fastify for Fastify users.
  • Correctly use res.header as a method instead of treating it like an object.

Testing:

  • Tested with local Express, Restify, and Fastify sample apps
    • Tested scenarios include incoming trace headers with and without Sampled=? cases. Ensure error occurs before the the fix, and there is no error after the fix. Ensure that the response headers include the XRay Context header when request headers have Sampled=? (not working for Fastify which doesn't provide header method in reply.raw).
  • Add unit test for resolveSampling and traceRequestResponseCycle where Sampled=?.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jj22ee jj22ee requested a review from a team as a code owner January 9, 2025 21:20
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.28%. Comparing base (72d523d) to head (eb8ae16).

Files with missing lines Patch % Lines
packages/core/lib/middleware/mw_utils.js 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #706      +/-   ##
==========================================
+ Coverage   83.56%   84.28%   +0.72%     
==========================================
  Files          36       36              
  Lines        1813     1814       +1     
==========================================
+ Hits         1515     1529      +14     
+ Misses        298      285      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jj22ee jj22ee force-pushed the fix-missing-header-error branch from eb8ae16 to 88b0991 Compare January 9, 2025 22:06
@jj22ee jj22ee merged commit d27bfd2 into aws:master Jan 10, 2025
13 checks passed
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.

3 participants