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

ASP.NET Core request body capturing erases the body #460

Closed
gregkalapos opened this issue Aug 26, 2019 · 11 comments
Closed

ASP.NET Core request body capturing erases the body #460

gregkalapos opened this issue Aug 26, 2019 · 11 comments
Labels
bug Something isn't working from a real user This issue is from a real user (i.e., not an issue we found during internal testing)
Milestone

Comments

@gregkalapos
Copy link
Contributor

Reported here by @brezaie.

Reproducer also included in the comment - copying here:

[HttpPost("send")]
public JsonResult Send([FromBody] BaseReportFilter<SendMessageFilter> filter)
{
	//Do something
	return new JsonResult("");
}


public class BaseReportFilter<T>
{
    public T ReportFilter { get; set; }
}

public class SendMessageFilter{
	public string SenderApplicationCode { get; set; }
	public string MediaType { get; set; }
	public string Body { get; set; }
	public List<string> Recipients { get; set; }
}

//appsettings.json:
"ElasticApm": {
    "ServerUrls": "http://localhost:8200",
    "ServiceName": "ServiceName",
    "CaptureHeaders": true,
    "CaptureBody": "all",
    "CaptureBodyContentTypes": "application/x-www-form-urlencoded*, text/*, application/json*, application/xml*",
    "LogLevel": "Info",
    "StackTraceLimit": -1
  }

"CaptureBodyContentTypes": "application/x-www-form-urlencoded*, text/*, application/json*, application/xml*",

Without agent the filter parameter is populated, once the agent is activated it became null.

@gregkalapos gregkalapos added the bug Something isn't working label Aug 26, 2019
@gregkalapos
Copy link
Contributor Author

Here we have the test that covers this feature - the test case is a bit simpler, I don't see yet how it makes a difference, but as it seems it does make a difference.

@SergeyKleyman SergeyKleyman added the from a real user This issue is from a real user (i.e., not an issue we found during internal testing) label Sep 20, 2019
@lreuven lreuven added this to the 7.5 milestone Sep 26, 2019
@minbyulcat
Copy link

This works fine if I comment out request.Body = initialBody in ExtractRequestBody method in RequestExtentions.cs. I hope this helps solve the problem.

@iquirino
Copy link
Contributor

iquirino commented Oct 8, 2019

Hi!

I am using the latest version and continues with this problem.

"ElasticApm": {
"ServerUrls": "#####",
"TransactionSampleRate": 1.0,
"CaptureBody": "all",
"CaptureBodyContentTypes": "application/x-www-form-urlencoded*, text/, application/json, application/xml*"
},

@iquirino
Copy link
Contributor

iquirino commented Oct 9, 2019

You are right!

All tests passed when comment this line.

I think is not necessary to do this and an accepted pull request.

https://devblogs.microsoft.com/aspnet/re-reading-asp-net-core-request-bodies-with-enablebuffering/

I need this ASAP so i'll pull a request, okay?

Thank you!

This works fine if I comment out request.Body = initialBody in ExtractRequestBody method in RequestExtentions.cs. I hope this helps solve the problem.

@iquirino
Copy link
Contributor

iquirino commented Oct 9, 2019

This solves: #539

@gregkalapos
Copy link
Contributor Author

Thanks for the help everyone!

The PR from @iquirino (#539) seems ok to me, very much appreciated!

I also opened #542 - as mentioned our existing automated test did not discover this issue. I was unable to repro it with the way we tested previously. On the other hand hosting the sample app with WebHost reproduced the problem, so #542 also adds a test covering this scenario.

@iquirino
Copy link
Contributor

iquirino commented Oct 9, 2019

When it will be available on nugget?

@gregkalapos
Copy link
Contributor Author

When it will be available on nugget?

We don't have a fix date for the next release, so unfortunately I can't tell you a specific date.

@iquirino
Copy link
Contributor

iquirino commented Oct 10, 2019

I'm running a version from my computer because this broken my production environment.
Yes... Yes.... I need to evaluate it first..... ^^

gregkalapos added a commit that referenced this issue Oct 10, 2019
Add test covering #460 - making sure capturing request body does not erase the body
@gregkalapos
Copy link
Contributor Author

I'm running a version from my computer because this broken my production environment.
Yes... Yes.... I need to evaluate it first..... ^^

I'm very sorry about that! To be fair: this feature is off by default and you could immediately recover by turning the feature off, right? Or do I miss something? Not that I want to make this sound like a non-issue - it was indeed an important fix-, but I want to make sure I see the impact of this correctly.

The release is out: https://github.com/elastic/apm-agent-dotnet/releases/tag/1.1.2

Version 1.1.2 packages include the fix. Also updated on nuget.org.

@iquirino
Copy link
Contributor

Uhull! You are awesome! Thank you Greg!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working from a real user This issue is from a real user (i.e., not an issue we found during internal testing)
Projects
None yet
Development

No branches or pull requests

5 participants