#11997 Fix for Twisted web client support of trailer Server-Timing

[taroved]

Scope and purpose

Fixes #11997

Replaces simple check of trailer chars (CRLF) with additional check for trailer Server-Timing header like data.
Trailer Server-Timing should be handled without excepion. CURL or Browsers handle such data without errors.

Read more about trailer Server-Timing here: https://w3c.github.io/server-timing/

This fix will not have affect on efficiency.

Contributor Checklist:

This process applies to all pull requests - no matter how small.
Have a look at our developer documentation before submitting your Pull Request.

Below is a non-exhaustive list (as a reminder):

• The title of the PR should describe the changes and starts with the associated issue number, like “#9782 Remove twisted.news. #1234 Brief description”.
• A release notes news fragment file was create in src/twisted/newsfragments/ (see: Release notes fragments docs.)
• The automated tests were updated.
• Once all checks are green, request a review by leaving a comment that contains exactly the string please review.
  Our bot will trigger the review process, by applying the pending review label
  and requesting a review from the Twisted dev team.

[taroved]

please review

[adiroiban]

Many thanks for your PR.
It looks good.

I left a few comments.
Most probable because I am not familiar with the spec.

I only did a quick read about this header at
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Server-Timing

I haven't yet read the whole specification.

Also, I haven't executed a manual test yet to see the before and after part. I saw you mention a manual test in the initial ticket.

I will try to continue the review a bit later,
but I hope the current comments will help to get this ready for merge.

Thanks again

[twm]

Awesome, thanks for diving in here!

Before we can merge you'll have to fix the missing type annotation. As you're not doing anything with the trailers yet perhaps drop the ivar for now? (If you want to keep it, please add a test that inspects its value!)

Otherwise this looks good to me! Please re-request review when everything's green.

[adiroiban]

Changes look good.

I am not very familiar with how Twisted web chunk encoding works, but I think that with this implementation we can cause a remote deny of sevice error, by consuming all the available memory

this can be done by a malicious HTTP client or HTTP server

[adiroiban]

sorry for the late review. crazy days over here.

feel free to ping me or check over Gitter to request a review

Is my concern for accepting an infinite amount of trailing headers valid ?

[adiroiban]

Sorry for the late reply.

I left a few minor comments.

I think that this is better than what we have in trunk and can be merge.

Thanks again.

[adiroiban]

Sorry for the late review. It looks good. Only minor comments.

[adiroiban]

The merge of this PR is blocked as @twm has requested changed. Tom, do you have time to check the latest changes and see your are happy with this PR? Thanks!

[adiroiban]

Thanks Alexandr for the PR.
I plan to have a release soon ... if we don't get another review until then, I think it's ok to have this merged and released.

We can then get more feedback for this feature as people are using it.

I have tagged it as a "Release-blocker" as a reminder.

Regards

[adiroiban]

@glyph I would like to do a new Twisted release soon .

I think that this PR is in good shape.

The blocking comment from Tom was about type annotation. As long as mypy is green, I think that we are file.

I will apply my suggestion, as they are only code comments and I will merge.

[adiroiban]

I am merging this as I am preparing a new release.
Pypy pass, so I think that Tom's request was addressed.

Thanks for your help with this.