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

Jump to bottom

Analyzer for misusage of MaxResponseHeadersLength #6796

Merged
merged 20 commits into from
Oct 2, 2023
Merged

Analyzer for misusage of MaxResponseHeadersLength #6796

merged 20 commits into from
Oct 2, 2023

Conversation

amiru3f
Copy link
Contributor

@amiru3f amiru3f commented Jul 24, 2023 •
edited
Loading

Fixes #75137

The analyzer tries to

  • Find the exact property assignment
  • Check the value assigned

If it's more than a specifically configured limit, raises a message with Information severity

  • It seems I don't need to write any CodeFixProvider based on the corresponding issue

@amiru3f
Copy link
Contributor Author

amiru3f commented Jul 24, 2023

@dotnet-policy-service agree

@amiru3f amiru3f changed the title Analyzer of misusage of MaxResponseHeadersLength Analyzer for misusage of MaxResponseHeadersLength Jul 24, 2023
@amiru3f amiru3f force-pushed the main branch 6 times, most recently from 49c62ae to eb6dc9b Compare July 24, 2023 20:53
@amiru3f amiru3f marked this pull request as ready for review July 26, 2023 11:32
@amiru3f amiru3f requested a review from a team as a code owner July 26, 2023 11:32
@amiru3f amiru3f force-pushed the main branch 4 times, most recently from 21583f2 to 541ce67 Compare July 26, 2023 22:21
@amiru3f amiru3f closed this Jul 26, 2023
@amiru3f amiru3f force-pushed the main branch 2 times, most recently from 541ce67 to b499986 Compare July 26, 2023 22:23
@amiru3f amiru3f reopened this Jul 26, 2023
@codecov
Copy link

codecov bot commented Aug 1, 2023 •
edited
Loading

Codecov Report

Merging #6796 (6b9c383) into main (84fb81c) will decrease coverage by 0.01%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##             main    #6796      +/-   ##
==========================================
- Coverage   96.43%   96.42%   -0.01%     
==========================================
  Files        1408     1410       +2     
  Lines      335751   335881     +130     
  Branches    11081    11090       +9     
==========================================
+ Hits       323772   323888     +116     
- Misses       9193     9201       +8     
- Partials     2786     2792       +6

@amiru3f amiru3f requested a review from Youssef1313 August 1, 2023 13:27
@buyaa-n buyaa-n requested a review from MihaZupan August 3, 2023 04:52
MihaZupan
MihaZupan reviewed Aug 3, 2023
View reviewed changes
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @amiru3f, this looks good to me.

cc: @dotnet/ncl

...oft.NetCore.Analyzers/Usage/ProvideHttpClientHandlerMaxResponseHeaderLengthValueCorrectly.cs Outdated Show resolved Hide resolved
...oft.NetCore.Analyzers/Usage/ProvideHttpClientHandlerMaxResponseHeaderLengthValueCorrectly.cs Outdated Show resolved Hide resolved
@amiru3f
Copy link
Contributor Author

amiru3f commented Aug 7, 2023

Hello guys @Youssef1313 @MihaZupan,
Just a quick question, what would be the next step for this PR?

buyaa-n
buyaa-n reviewed Sep 28, 2023
View reviewed changes
Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Just a quick question, what would be the next step for this PR?

Thank you for your contribution @amiru3f and sorry for late review, left a few comments, next steps will apply/respond them and please merge the PR conflicts.

...etCore.Analyzers/Usage/ProvideHttpClientHandlerMaxResponseHeaderLengthValueCorrectlyTests.cs Show resolved Hide resolved
...etCore.Analyzers/Usage/ProvideHttpClientHandlerMaxResponseHeaderLengthValueCorrectlyTests.cs Show resolved Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved
...oft.NetCore.Analyzers/Usage/ProvideHttpClientHandlerMaxResponseHeaderLengthValueCorrectly.cs Outdated Show resolved Hide resolved
buyaa-n
buyaa-n reviewed Sep 30, 2023
View reviewed changes
src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md Outdated
CA1867 | Performance | Disabled | UseStringMethodCharOverloadWithSingleCharacters, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1867)
CA1868 | Performance | Info | DoNotGuardSetAddOrRemoveByContains, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1865)
CA2261 | Usage | Warning | DoNotUseConfigureAwaitWithSuppressThrowing, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2250)
CA1510 | Maintainability | Info | UseExceptionThrowHelpers, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1510)
Copy link
Contributor

Choose a reason for hiding this comment

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

These analyzers shipped with 8.0, looks the PR need to pull the latest and run msbuild /t:pack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird, it was mistakenly (auto) generated by msbuild packing.
Fixed.

buyaa-n
buyaa-n approved these changes Oct 2, 2023
View reviewed changes
Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Looks great, thank you @amiru3f!

@buyaa-n buyaa-n merged commit 7099c1d into dotnet:main Oct 2, 2023
11 checks passed
@amiru3f
Copy link
Contributor Author

amiru3f commented Oct 2, 2023

@buyaa-n 🙏
Thanks for your help guys @MihaZupan @Youssef1313

@gewarren gewarren mentioned this pull request Dec 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MihaZupan MihaZupan MihaZupan approved these changes

@buyaa-n buyaa-n buyaa-n approved these changes

@Youssef1313 Youssef1313 Awaiting requested review from Youssef1313

Assignees
No one assigned
Labels
Community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an analyzer warning of erroneous usage of MaxResponseHeadersLength
4 participants
@amiru3f @MihaZupan @Youssef1313 @buyaa-n