Fix BasicAuth default handler fail-open behavior - #34
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90bba36fa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Initializes a new instance of the <see cref="BasicAuthenticateRequestHandler"/> class with default settings. | ||
| /// </summary> | ||
| public BasicAuthenticateRequestHandler () : this ( ( a, b ) => null, null ) { | ||
| public BasicAuthenticateRequestHandler () : this ( ( _, _ ) => CreateDefaultUnauthorizedResponse ( DefaultRealm ), null ) { |
There was a problem hiding this comment.
Honor configured Realm in default validator response
The new parameterless constructor wires validateDefault to always call CreateDefaultUnauthorizedResponse(DefaultRealm), which hard-codes the challenge realm. If a caller constructs new BasicAuthenticateRequestHandler(), then sets Realm (e.g., for a custom protection scope), requests with an Authorization header now return 401 with the default realm instead of the configured one, while other unauthorized paths still use Realm; this creates inconsistent auth challenges and breaks runtime realm customization.
Useful? React with 👍 / 👎.
Motivation
BasicAuthenticateRequestHandlerconstructor previously wired a validator that returnednull, andnullis interpreted as successful authentication, causing a fail-open where any syntactically valid Basic auth blob granted access.OnValidating, so the default should be deny-by-default.Description
401 Unauthorizedinstead of returningnull, makingnew BasicAuthenticateRequestHandler()fail-closed.CreateDefaultUnauthorizedResponseto produce the401response used by the default delegate.OnValidatingvirtual method semantics unchanged.Testing
dotnet build extensions/Sisk.BasicAuth/Sisk.BasicAuth.csproj -c Debug -f net6.0 /p:RestoreIgnoreFailedSources=true, which failed in this environment due to missing restored assets fornet6.0.dotnet build extensions/Sisk.BasicAuth/Sisk.BasicAuth.csproj -c Debug /p:RestoreIgnoreFailedSources=true, which completed successfully and produced the extension assembly, indicating the change compiles.Codex Task