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.

Sign up for GitHub

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

[ASM] Catch HttpException when reading the request body in .net461 #6459

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

[ASM] Catch HttpException when reading the request body in .net461 #6459

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22c3375
Catch HttpException
NachoEchevarria Dec 17, 2024
16fdae4
Merge branch 'master' into nacho/RequestComExceptionReadingBody
NachoEchevarria Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tracer/src/Datadog.Trace/Util/RequestDataHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ internal static QueryString GetQueryString(HttpRequest request)
{
return request.Form;
}
catch (HttpRequestValidationException)
catch (HttpException)
Copy link
Member

Choose a reason for hiding this comment

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

At this point, I have to wonder why we're not just catching all exceptions here 🤔 What exceptions would we want to bubble up, if we're catching "valid" issues?

The one concern I have is that it's likely we'll trigger behaviour that only happens the first time a form (or any of the other request data tbh) is accessed.

For example, take this case. We call EnsureForm() and that "fills" the form, which triggers an exception. What happens if the customer calls request.Form later? Is EnsureForm called again and the exception thrown again? If so, that's great. But if not (because it's only called once), then we're potentially introducing hard-to-debug issues...

I think the behaviour is what really matters here. e.g. what happens if a customer calls request.Form again in this scenario? Is that something we can test (or just confirm from the code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

    internal HttpValueCollection EnsureForm()
    {
        if (_form == null)
        {
            _form = new HttpValueCollection();
            if (_wr != null)
            {
                FillInFormCollection();
            }

            _form.MakeReadOnly();
        }

        return _form;
    }

Good point. FillInFormCollection() can potentially throw an HttpException (confirmed by looking at the code) and the next time, _form would not be null anymore so, yes, we might encounter situations where we could be modifying the result obtained by the customers code.

This is something that will happen anyway. If we catch or not the exception will not change that. On the other hand, we really need to read the body of the request. I wonder if there is any possible way that we could read the body of the request without causing this potential behavior change.

About catching exceptions, I tried to be restrictive here, catching only the exceptions types that we have reviewed. If a new exception type is thrown, it would probably be worth to log it and check if we should just catch it or if we should do anything else.

{
Log.Debug("Error reading Form (body) from the request.");
return null;
Expand Down
Loading