search

aws / aws-xray-sdk-dotnet

The official AWS X-Ray SDK for .NET.
Apache License 2.0
112 stars 64 forks source link

[EF6] Incorrectly sanitised subsegment name #213

Open yngndrw opened 3 years ago

yngndrw commented 3 years ago

Hello,

When using the EF6 XRay interceptor (AWSXRayEntityFramework6.AddXRayInterceptor()), the subsegment name is based upon the database name and the connection data source.

The port number is removed from the connection data source but no other sanitisation is performed.

It is valid for the connection data source to contain characters which are invalid for an XRay subsection name as there's a special alias for the current machine which includes parenthesis: (local) https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/connection-string-syntax#windows-authentication-with-sqlclient

In my case I end up with a section name which looks like this: my_database_name@(local)

...resulting in the daemon error: Invalid subsegment. ErrorCode: InvalidName, Cause: null

I suspect the same issue also applies to the DbCommandInterceptor, as the code is duplicated there.

As the subsection name is generated from an uncontrolled source, I'd suggest fully sanitising it to ensure that only the valid characters are included.

srprash commented 3 years ago

Hi @yngndrw Currently we are only sanitizing the port number from the connection string, but as you pointed out in your case, if the data-source contains ( or ), it will be rejected by the X-Ray service since the segment/subsegment names can only contain these valid characters.

I looked into the X-Ray Python SDK and noticed that we sanitize all the segment and subsegment names within the SDK before sending them out to the Daemon. We can probably do the same here if it doesn't affect the performance.

yngndrw commented 2 years ago

I'd forgotten all about this issue, moved onto another project and ran into it again - This time using TraceableSqlCommand. It's very annoying.

Are there any plans to resolve this issue for the next release?

srprash commented 2 years ago

Hi @yngndrw Sorry to hear that you ran into this issue again. I think we can implement sanitization of invalid segment/subsegment names with ease. However, I am slightly concerned if this could be a breaking change for someone who might be purposely using invalid name to avoid sending the segment. I know it's an uncommon case and likely not being used, but I will get back to you on this soon. Thanks.