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.

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

[release/8.0] Add namespace and assembly check before interception #51276

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Oct 10, 2023

Backport of #51243 to release/8.0

/cc @captainsafia

Add namespace and assembly check before interception

Resolves an issue where the Request Delegate Generator would inadvertently intercept methods that looks like the targeted Map{Verb} methods from the framework but were not. For example:

var builder = WebApplication.CreateSlimBuilder(args);

// Should not be intercepted
builder.Services.Map(() => Task.CompletedTask);

var app = builder.Build();

// Should be intercepted
app.Map("route", () => Results.Text("Working"));

app.Run();

static class Ext
{
    public static IServiceCollection Map(this IServiceCollection services, Delegate @delegate) => services;
}

Description

Adds a check to verify that the intercepted method is defined in our frameworks assemblies and namespace to avoid intercepting unexpected methods.

Fixes #51233

Customer Impact

Without this bug fix, customer code might inadvertently register interceptors due to the greedy nature of the check we use to confirm if an invocation if one that should be intercepted. Although the number of impacted users is likely low, there is no workaround and the fix contributes to the overall hardening of our source generator.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

Low risk because the impacted segments are small and the change makes the codebase more restrictive than previously.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Oct 10, 2023
@captainsafia captainsafia added feature-rdg area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc Servicing-approved Shiproom has approved the issue and removed area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels Oct 10, 2023
@ghost
Copy link

ghost commented Oct 13, 2023

Hi @github-actions[bot]. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@captainsafia
Copy link
Member

Approved via email.

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Non-blocking question.

@wtgodbe wtgodbe merged commit 2750ee7 into release/8.0 Oct 20, 2023
25 checks passed
@wtgodbe wtgodbe deleted the backport/pr-51243-to-release/8.0 branch October 20, 2023 16:08
@ghost ghost added this to the 8.0.0 milestone Oct 20, 2023
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Oct 20, 2023
@jeremy-allocate
Copy link

Does not seem to have been included in the initial public release of dotnet 8.

@ghost
Copy link

ghost commented Nov 15, 2023

Hi @jeremy-allocate. 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.

@captainsafia
Copy link
Member

Does not seem to have been included in the initial public release of dotnet 8.

I see your comment on the other issue. That one deals particularly with the exception occurring in our analyzers. The fix here only applied to the Request Delegate Generator (which is off by default). TY for flagging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-rdg Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants