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

Wrong http.request.url reported when host header is set #401

Open
danilobuerger opened this issue Apr 4, 2021 · 3 comments
Open

Wrong http.request.url reported when host header is set #401

danilobuerger opened this issue Apr 4, 2021 · 3 comments
Assignees

Comments

@danilobuerger
Copy link

Sometimes it is useful to connect to a url and have a different host header:

   The "Host" header field in a request provides the host and port
   information from the target URI, enabling the origin server to
   distinguish among resources while servicing requests for multiple
   host names on a single IP address.

https://tools.ietf.org/html/rfc7230#section-5.4

When this is the case, the reported http.request.url contains the host header instead of the host from the connecting url.

For example:

fetch("http://url.example/path", { headers: { host: "host.example" }})

would be reported as http://host.example/path instead of http://url.example/path.

@willarmiros
Copy link
Contributor

Hi @danilobuerger,

That's a fair calllout, we do parse the host header for the name of the subsegment here, and for the http.request.url field as you said here. I would be open to a PR to prefer parsing the hostname from the URL instead.

@danilobuerger
Copy link
Author

@willarmiros Thanks for your response. Sadly I don't have the time to submit a proper PR at this time. However, here is my patch file if somebody is willing to submit it as a PR or runs into the same issue:

diff --git a/node_modules/aws-xray-sdk-core/lib/patchers/http_p.js b/node_modules/aws-xray-sdk-core/lib/patchers/http_p.js
index 7743ecd..6ea5fc7 100644
--- a/node_modules/aws-xray-sdk-core/lib/patchers/http_p.js
+++ b/node_modules/aws-xray-sdk-core/lib/patchers/http_p.js
@@ -135,7 +135,7 @@ function enableCapture(module, downstreamXRayEnabled, subsegmentCallback) {
       } else {
         var madeItToDownstream = (e.code !== 'ECONNREFUSED');
 
-        subsegment.addRemoteRequestData(this, null, madeItToDownstream && downstreamXRayEnabled);
+        subsegment.addRemoteRequestData(this, options, null, madeItToDownstream && downstreamXRayEnabled);
         subsegment.close(e);
       }
 
@@ -164,7 +164,7 @@ function enableCapture(module, downstreamXRayEnabled, subsegmentCallback) {
         if (cause)
           subsegment[cause] = true;
 
-        subsegment.addRemoteRequestData(res.req, res, !!downstreamXRayEnabled);
+        subsegment.addRemoteRequestData(res.req, options, res, !!downstreamXRayEnabled);
         subsegment.close();
       });
 
diff --git a/node_modules/aws-xray-sdk-core/lib/segments/attributes/remote_request_data.js b/node_modules/aws-xray-sdk-core/lib/segments/attributes/remote_request_data.js
index 6986794..bdb987b 100644
--- a/node_modules/aws-xray-sdk-core/lib/segments/attributes/remote_request_data.js
+++ b/node_modules/aws-xray-sdk-core/lib/segments/attributes/remote_request_data.js
@@ -12,9 +12,9 @@ function RemoteRequestData(req, res, downstreamXRayEnabled) {
   this.init(req, res, downstreamXRayEnabled);
 }
 
-RemoteRequestData.prototype.init = function init(req, res, downstreamXRayEnabled) {
+RemoteRequestData.prototype.init = function init(req, options, res, downstreamXRayEnabled) {
   this.request = {
-    url: (req.agent && req.agent.protocol) ? (req.agent.protocol + '//' + req.getHeader('host') +  stripQueryStringFromPath(req.path)) : '',
+    url: (req.agent && req.agent.protocol) ? (req.agent.protocol + '//' + options.host +  stripQueryStringFromPath(req.path)) : '',
     method: req.method || '',
   };
 
diff --git a/node_modules/aws-xray-sdk-core/lib/segments/attributes/subsegment.js b/node_modules/aws-xray-sdk-core/lib/segments/attributes/subsegment.js
index 5e717ef..5923cfe 100644
--- a/node_modules/aws-xray-sdk-core/lib/segments/attributes/subsegment.js
+++ b/node_modules/aws-xray-sdk-core/lib/segments/attributes/subsegment.js
@@ -217,8 +217,8 @@ Subsegment.prototype.addError = function addError(err, remote) {
  * @param {boolean} downstreamXRayEnabled - when true, adds a "traced": true hint to generated subsegments such that the AWS X-Ray service expects a corresponding segment from the downstream service.
  */
 
-Subsegment.prototype.addRemoteRequestData = function addRemoteRequestData(req, res, downstreamXRayEnabled) {
-  this.http = new RemoteRequestData(req, res, downstreamXRayEnabled);
+Subsegment.prototype.addRemoteRequestData = function addRemoteRequestData(req, options, res, downstreamXRayEnabled) {
+  this.http = new RemoteRequestData(req, options, res, downstreamXRayEnabled);
   if ('traced' in this.http.request) {
     this.traced = this.http.request.traced;
     delete this.http.request.traced;

@willarmiros
Copy link
Contributor

@danilobuerger thanks for adding this! We can't reorder the parameters like that to avoid a breaking change, but adding the options parameter last is an acceptable change if we do make a quick PR.

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

No branches or pull requests

3 participants