Resolve CA2201: No Reserved Exceptions #47873

dahlbyk · 2023-04-25T04:40:03Z

Resolve CA2201: No Reserved Exceptions

~~Progress on~~ Resolves Avoid CA2201 (Generic Exception) Violations #47708
Adds Microsoft.AspNetCore.Authentication.AuthenticationFailureException
- cc Custom RemoteAuthenticationHandler Exceptions #46212

Description

Addresses ~~initial list from @danmoseley~~ all the CA2201 errors in published projects.

ghost · 2023-04-25T04:40:11Z

Thanks for your PR, @dahlbyk. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

src/Security/Authentication/Negotiate/src/Internal/NegotiateState.cs

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointParameterEmitter.cs

BrennanConroy · 2023-04-25T15:51:06Z

Wondering if there's an easy way to turn CA2201 on just for production projects?

You add it here https://github.com/dotnet/aspnetcore/blob/main/.editorconfig#L80
And then also here with lower severity for tests/samples/etc. https://github.com/dotnet/aspnetcore/blob/main/.editorconfig#L319

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointParameterEmitter.cs

src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointParameterEmitter.cs

src/Servers/Kestrel/Core/src/Internal/Http3/Http3PendingStream.cs

src/Security/Authentication/Negotiate/src/Internal/NegotiateState.cs

dahlbyk · 2023-04-29T20:05:54Z

Change the new exception name not to conflict. AuthenticationFailureException?

Renamed and tried to fix failing tests.

UnreachableException not available in .NET Standard

All but src/Components/benchmarkapps covered by perf

dahlbyk · 2023-04-30T05:08:44Z

Ended up just finishing the job and marking CA2201 as an error for production projects. Glad to split into multiple PRs if that's preferred - each commit is meant to be atomic.

Co-authored-by: Chris Ross <Tratcher@Outlook.com>

dahlbyk · 2023-04-30T05:15:25Z

src/Middleware/Rewrite/src/IISUrlRewrite/ConditionCollection.cs

@@ -33,7 +33,7 @@ public ConditionCollection(LogicalGrouping grouping, bool trackAllCaptures)
            {
                return _conditions[index];
            }
-            throw new IndexOutOfRangeException($"Cannot access condition at index {index}. Only {_conditions.Count} conditions were captured.");
+            throw new ArgumentOutOfRangeException(nameof(index), $"Cannot access condition at index {index}. Only {_conditions.Count} conditions were captured.");


This is another spot where the custom message is good enough that paramName: null might be preferred.

Suggested change

throw new ArgumentOutOfRangeException(nameof(index), $"Cannot access condition at index {index}. Only {_conditions.Count} conditions were captured.");

throw new ArgumentOutOfRangeException(null, $"Cannot access condition at index {index}. Only {_conditions.Count} conditions were captured.");

I agree that the existing custom message was good here, but how does passing in a null parameter name help? The custom message still gets used with nameof(index), right?

how does passing in a null parameter name help?

Non-null/empty paramName is appended to the Message, which adds nothing to this message.

I went ahead and switched to null.

I missed that you changed this before approving. Can you change it back? I think the slight redundancy in the error message is worth keeping ArgumentOutOfRangeException.ParamName non-null. Can we change our ArgumentOutOfRange exceptions to pass in the actualValue where it makes sense too?

Edit: Never mind. We can do this as a follow up if anyone actually cares.

src/SignalR/common/Shared/ClientResultsManager.cs

src/SignalR/server/Core/src/StreamTracker.cs

halter73

Looking good! The API proposal for AuthenticationFailureException was also approved.

src/SignalR/common/Shared/ClientResultsManager.cs

src/SignalR/server/Core/src/StreamTracker.cs

halter73 · 2023-05-02T19:55:53Z

.editorconfig

@@ -240,6 +240,9 @@ dotnet_diagnostic.CA2016.severity = warning
 # CA2200: Rethrow to preserve stack details
 dotnet_diagnostic.CA2200.severity = warning

+# CA2201: Do not raise reserved exception types
+dotnet_diagnostic.CA2201.severity = error


Do we really need to enable this for our test projects? Isn't the addition on line 394 enough? Conversely, if we really do want to enable this for test, do we need line 394?

Do we really need to enable this for our test projects? Isn't the addition on line 394 enough? Conversely, if we really do want to enable this for test, do we need line 394?

This line promotes to warning for all projects, then line 394 demotes back to suggestion for test/perf projects.

halter73 · 2023-05-02T19:58:59Z

src/Middleware/Rewrite/src/IISUrlRewrite/ConditionCollection.cs

@@ -33,7 +33,7 @@ public ConditionCollection(LogicalGrouping grouping, bool trackAllCaptures)
            {
                return _conditions[index];
            }
-            throw new IndexOutOfRangeException($"Cannot access condition at index {index}. Only {_conditions.Count} conditions were captured.");
+            throw new ArgumentOutOfRangeException(nameof(index), $"Cannot access condition at index {index}. Only {_conditions.Count} conditions were captured.");


I agree that the existing custom message was good here, but how does passing in a null parameter name help? The custom message still gets used with nameof(index), right?

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointParameterEmitter.cs

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointParameter.cs

.editorconfig

src/SignalR/common/Shared/ClientResultsManager.cs

Promote CA2201 to warning (as error)

dahlbyk

I believe all feedback has been addressed by the last few commits. Should I rebase onto main?

dahlbyk · 2023-05-03T05:46:50Z

.editorconfig

@@ -240,6 +240,9 @@ dotnet_diagnostic.CA2016.severity = warning
 # CA2200: Rethrow to preserve stack details
 dotnet_diagnostic.CA2200.severity = warning

+# CA2201: Do not raise reserved exception types
+dotnet_diagnostic.CA2201.severity = error


Do we really need to enable this for our test projects? Isn't the addition on line 394 enough? Conversely, if we really do want to enable this for test, do we need line 394?

This line promotes to warning for all projects, then line 394 demotes back to suggestion for test/perf projects.

halter73

LGTM. Thanks!

dahlbyk · 2023-05-03T17:44:18Z

cc Custom RemoteAuthenticationHandler Exceptions #46212

I noticed GitHub didn't add a reference from that issue to this PR or #47708. Might be nice if a collaborator could add a note there about AuthenticationFailureException.

ghost · 2023-05-03T17:44:22Z

Hi @dahlbyk. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

dahlbyk requested review from BrennanConroy, halter73, Tratcher, JamesNK, mgravell and captainsafia as code owners April 25, 2023 04:40

ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 25, 2023

dotnet-issue-labeler bot added the area-runtime label Apr 25, 2023

dahlbyk force-pushed the 47708-no-System-Exception branch from 283465b to 4bdd3bf Compare April 25, 2023 04:52

dahlbyk commented Apr 25, 2023

View reviewed changes

src/Security/Authentication/Negotiate/src/Internal/NegotiateState.cs Show resolved Hide resolved

dahlbyk mentioned this pull request Apr 25, 2023

Avoid CA2201 (Generic Exception) Violations #47708

Closed

1 task

campersau reviewed Apr 25, 2023

View reviewed changes

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointParameterEmitter.cs Outdated Show resolved Hide resolved

mkArtakMSFT assigned halter73 Apr 25, 2023

halter73 reviewed Apr 25, 2023

View reviewed changes

src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointParameterEmitter.cs Outdated Show resolved Hide resolved

src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt Outdated Show resolved Hide resolved

Tratcher reviewed Apr 25, 2023

View reviewed changes

Avoid System.Exception in dotnet-user-jwts

32a155a

dahlbyk force-pushed the 47708-no-System-Exception branch from 4bdd3bf to 095dacc Compare April 29, 2023 19:09

dahlbyk requested a review from a team as a code owner April 29, 2023 19:09

dahlbyk added 9 commits April 29, 2023 22:32

Avoid System.Exception in Hosting

c22655d

Avoid System.Exception in Kestrel

327c449

Use UnreachableException for sentinel

cff23f7

Avoid System.Exception in SignalR Clients

b6ae69a

Avoid System.Exception in Http.Extensions

591b9da

UnreachableException not available in .NET Standard

Add/throw AuthenticationFailureException

b365ad3

Replace NullReferenceException in JSInterop

f5c65b9

Avoid IndexOutOfRangeException

81e7fac

Allow OutOfMemoryException

10c6d10

dahlbyk added 2 commits April 29, 2023 23:44

Lax rules for benchmarkapps

b8f01a2

All but src/Components/benchmarkapps covered by perf

Promote CA2201 to error

4222688

dahlbyk force-pushed the 47708-no-System-Exception branch from 095dacc to 4222688 Compare April 30, 2023 05:06

dahlbyk requested review from a team and javiercn as code owners April 30, 2023 05:06

fixup! Avoid System.Exception in Kestrel

e2ade1e

Co-authored-by: Chris Ross <Tratcher@Outlook.com>

dahlbyk commented Apr 30, 2023

View reviewed changes

dahlbyk changed the title ~~Fewer System.Exception~~ Resolve CA2201: No Reserved Exceptions Apr 30, 2023

fixup! Avoid more System.Exception in SignalR

2dd9c5c

halter73 reviewed May 2, 2023

View reviewed changes

halter73 mentioned this pull request May 2, 2023

Add initial support for parameters with AsParameters attribute #47914

Merged

BrennanConroy reviewed May 3, 2023

View reviewed changes

.editorconfig Outdated Show resolved Hide resolved

src/SignalR/common/Shared/ClientResultsManager.cs Outdated Show resolved Hide resolved

dahlbyk added 4 commits May 3, 2023 00:52

Use HubException

91ffe2e

fixup! Avoid IndexOutOfRangeException

a1fa850

fixup! Avoid System.Exception in Http.Extensions

dea39bf

squash! Promote CA2201 to error

1a9035b

Promote CA2201 to warning (as error)

dahlbyk commented May 3, 2023

View reviewed changes

halter73 approved these changes May 3, 2023

View reviewed changes

halter73 merged commit 0c8a569 into dotnet:main May 3, 2023

ghost added this to the 8.0-preview5 milestone May 3, 2023

dahlbyk deleted the 47708-no-System-Exception branch May 3, 2023 17:40

Tratcher linked an issue May 3, 2023 that may be closed by this pull request

Custom RemoteAuthenticationHandler Exceptions #46212

Closed

1 task

amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

mkArtakMSFT mentioned this pull request Dec 1, 2023

Unclear how to catch "System.Exception: Correlation failed." and prevent HTTP 500 #45620

Closed

1 task

asos-jasperdeluna mentioned this pull request Nov 4, 2024

Correlation failure #58780

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve CA2201: No Reserved Exceptions #47873

Resolve CA2201: No Reserved Exceptions #47873

dahlbyk commented Apr 25, 2023 •

edited

Loading

ghost commented Apr 25, 2023

BrennanConroy commented Apr 25, 2023

dahlbyk commented Apr 29, 2023

dahlbyk commented Apr 30, 2023

dahlbyk Apr 30, 2023

halter73 May 2, 2023

dahlbyk May 3, 2023 •

edited

Loading

halter73 May 3, 2023 •

edited

Loading

halter73 left a comment

halter73 May 2, 2023

dahlbyk May 3, 2023

halter73 May 2, 2023

dahlbyk left a comment

dahlbyk May 3, 2023

halter73 left a comment

dahlbyk commented May 3, 2023

ghost commented May 3, 2023

	throw new ArgumentOutOfRangeException(nameof(index), $"Cannot access condition at index {index}. Only {_conditions.Count} conditions were captured.");
	throw new ArgumentOutOfRangeException(null, $"Cannot access condition at index {index}. Only {_conditions.Count} conditions were captured.");

Resolve CA2201: No Reserved Exceptions #47873

Resolve CA2201: No Reserved Exceptions #47873

Conversation

dahlbyk commented Apr 25, 2023 • edited Loading

Resolve CA2201: No Reserved Exceptions

Description

ghost commented Apr 25, 2023

BrennanConroy commented Apr 25, 2023

dahlbyk commented Apr 29, 2023

dahlbyk commented Apr 30, 2023

dahlbyk Apr 30, 2023

Choose a reason for hiding this comment

halter73 May 2, 2023

Choose a reason for hiding this comment

dahlbyk May 3, 2023 • edited Loading

Choose a reason for hiding this comment

halter73 May 3, 2023 • edited Loading

Choose a reason for hiding this comment

halter73 left a comment

Choose a reason for hiding this comment

halter73 May 2, 2023

Choose a reason for hiding this comment

dahlbyk May 3, 2023

Choose a reason for hiding this comment

halter73 May 2, 2023

Choose a reason for hiding this comment

dahlbyk left a comment

Choose a reason for hiding this comment

dahlbyk May 3, 2023

Choose a reason for hiding this comment

halter73 left a comment

Choose a reason for hiding this comment

dahlbyk commented May 3, 2023

ghost commented May 3, 2023

dahlbyk commented Apr 25, 2023 •

edited

Loading

dahlbyk May 3, 2023 •

edited

Loading

halter73 May 3, 2023 •

edited

Loading