Extend aspnetcore controller definition

[JarLob]

AspNetCore is more flexible deciding what are controllers than AspNet - https://docs.microsoft.com/en-us/aspnet/core/mvc/controllers/actions?view=aspnetcore-3.1

The controller class class doesn't have to be derived from a specific parent, it is enough to suffix it or any parent class with Controller. Also [Controller] and [NonController] attributes should be taken into account.

This should add potentially missed taint sources.

[michaelnebel]

The linked documentation states

By convention, controller classes:

(1)
Reside in the project's root-level Controllers folder.
Inherit from Microsoft.AspNetCore.Mvc.Controller.

(2)
A controller is an instantiable class, usually [public](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/public),
in which at least one of the following conditions is true:
- The class name is suffixed with Controller.
- The class inherits from a class whose name is suffixed with Controller.
- The [Controller] attribute is applied to the class.

(3)
A controller class must not have an associated [NonController] attribute.

Isn't this a conjunction between the three paragraphs?
That is, it should always hold that controller class inherits from Inherit from Microsoft.AspNetCore.Mvc.Controller., either directly or transitively.
Furthermore, at least one of the properties in (2) should hold and (3) should always hold.

[michaelnebel]

Have added some comments and questions.
Also, it would be great, if it is possible to extend the unit test coverage.

[JarLob]

That's the point. The controller can be any class suffixed with Controller and it is convenient, but not obligatory to be in the root folder. See the project attached. There are two endpoints localhost/Home and localhost/Test

WebApplication2.zip

[michaelnebel]

Microsoft.AspNetCore.Mvc.Controller

Alright. I am not well versed in ASP.NET.
I can see that the zipped source contains HomeController and TestController. What you are saying is that these are "real" controllers even though they don't inherit the abstract Mvc.Controller class?

[JarLob]

Correct.

[michaelnebel]

Correct.
Added another comment on a possible side effect of derived remote flow sources.
This Controller suffix requirement needs more scope of the remote flow source code will also need to be changed.

[tamasvajk]

@JarLob Can you point me to some documentation on convention based controllers?
We would need to implement the same logic for controller lookup that asp.net core does. I assume that we would need to consider Controller suffixed classes in asp.net core projects. So we should probably check for the existence of some aspnetcore references, or look for the existence of some asp.net core entry points, such as a call to WebHostBuilderExtensions.UseStartup.
I think in some corner cases we will have trouble identifying controllers. For example it seems to me that you can define assemblies dynamically that should be considered for controller lookup: https://stackoverflow.com/a/59121354/1410281. This means that controllers can probably reside in assemblies that have no asp.net core references at all.

[JarLob]

@JarLob Can you point me to some documentation on convention based controllers? We would need to implement the same logic for controller lookup that asp.net core does. I assume that we would need to consider Controller suffixed classes in asp.net core projects. So we should probably check for the existence of some aspnetcore references, or look for the existence of some asp.net core entry points, such as a call to WebHostBuilderExtensions.UseStartup. I think in some corner cases we will have trouble identifying controllers. For example it seems to me that you can define assemblies dynamically that should be considered for controller lookup: https://stackoverflow.com/a/59121354/1410281. This means that controllers can probably reside in assemblies that have no asp.net core references at all.

You mean the link in my first comment? https://docs.microsoft.com/en-us/aspnet/core/mvc/controllers/actions?view=aspnetcore-3.1

Current implementation probably covers 95% of use cases (99% with the PR). I'm not concerned about covering the last 1% if they are loaded dynamically etc. The question is if this commit introduces more FPs. Please see if cf561ed is sufficient.

[tamasvajk]

You mean the link in my first comment? https://docs.microsoft.com/en-us/aspnet/core/mvc/controllers/actions?view=aspnetcore-3.1

Current implementation probably covers 95% of use cases. I'm not concerned about covering the last 1% if they are loaded dynamically etc. The question is if this commit introduces more FPs. Please see if cf561ed is sufficient.

Looks reasonable to me.
I'm not entirely sure what "instantiable class" means here. Filtering abstract and static classes would probably not have any major impact on false positives.

[JarLob]

Could you please help me with the tests? I tried to find something similar and used \ql\csharp\ql\test\query-tests\Stubs\References\ as an example. However when I try to run test it fails with an error error CS0579: Duplicate 'global::System.Runtime.Versioning.TargetFrameworkAttribute' attribute, both the new tests and the old ones, so it makes me think I'm running it with wrong parameters. I'm running it as codeql.exe test run --show-extractor-output --search-path=\vscode-codeql-starter\ql "\vscode-codeql-starter\ql\csharp\ql\test\library-tests\frameworks\microsoft"

[tamasvajk]

Could you please help me with the tests? I tried to find something similar and used \ql\csharp\ql\test\query-tests\Stubs\References\ as an example. However when I try to run test it fails with an error error CS0579: Duplicate 'global::System.Runtime.Versioning.TargetFrameworkAttribute' attribute, both the new tests and the old ones, so it makes me think I'm running it with wrong parameters. I'm running it as codeql.exe test run --show-extractor-output --search-path=\vscode-codeql-starter\ql "\vscode-codeql-starter\ql\csharp\ql\test\library-tests\frameworks\microsoft"

I've ran codeql test run ql/csharp/ql/test/library-tests/frameworks/microsoft, which produced 0 results in the expected file, but no extraction errors. I think there are no results, because when tests are run, the sources added through --load-sources-from-project are actually not added as reference assemblies, but instead as normal input source files.

Regarding the duplicate TargetFrameworkAttribute: I can't repro this. Can you try with -vvvv appended to your test command, maybe that reveals something. For example, I have these in my logs:

Semmle.Extraction.CSharp.Driver: called with --load-sources-from-project:../../../resources/stubs/_frameworks/Microsoft.AspNetCore.App/Microsoft.AspNetCore.App.csproj, --qltest, -unsafe, -target:library, /noconfig, /nostdlib, --console, --verbosity, 5, AspNetCore.cs

and

Arguments to Roslyn: -unsafe -target:library /noconfig /nostdlib AspNetCore.cs ...

The latter one has a lot more source files from the stubs folder, but I only found one class TargetFrameworkAttribute in those files, namely in ql/csharp/ql/test/resources/stubs/_frameworks/Microsoft.NETCore.App/System.Runtime.cs.

[JarLob]

I have added the tests. There are two issues though:

1. The code https://github.com/github/codeql/pull/9406/files#diff-19ef3a656171752739b8876631ea7527435a6bf09eaa8b1ab2e7664e3fe9c0bfR199-R205 makes sense in prod, but doesn't work for tests because there are no referenced assemblies, just stubs.
2. https://github.com/github/codeql/pull/9406/files#diff-2267e9eedf92ab85f06da1a348918911fb13c14a58ded8f18f0721be251ce2c4R77-R84 gets detected as a controller although I have added https://github.com/github/codeql/pull/9406/files#diff-19ef3a656171752739b8876631ea7527435a6bf09eaa8b1ab2e7664e3fe9c0bfR208. Either in CodeQL it differs from Roslyn's https://github.com/dotnet/aspnetcore/blob/b3c93967ba508b8ef139add27132d9483c1a9eb4/src/Mvc/Mvc.Core/src/Controllers/ControllerFeatureProvider.cs#L58 or I don't understand what is the generic parameter, although I have tried different combinations of using generics in the class, but it was always detected as a controller.

[michaelnebel]

I have added the tests. There are two issues though:

1. The code https://github.com/github/codeql/pull/9406/files#diff-19ef3a656171752739b8876631ea7527435a6bf09eaa8b1ab2e7664e3fe9c0bfR199-R205 makes sense in prod, but doesn't work for tests because there are no referenced assemblies, just stubs.
2. https://github.com/github/codeql/pull/9406/files#diff-2267e9eedf92ab85f06da1a348918911fb13c14a58ded8f18f0721be251ce2c4R77-R84 gets detected as a controller although I have added https://github.com/github/codeql/pull/9406/files#diff-19ef3a656171752739b8876631ea7527435a6bf09eaa8b1ab2e7664e3fe9c0bfR208. Either in CodeQL it differs from Roslyn's https://github.com/dotnet/aspnetcore/blob/b3c93967ba508b8ef139add27132d9483c1a9eb4/src/Mvc/Mvc.Core/src/Controllers/ControllerFeatureProvider.cs#L58 or I don't understand what is the generic parameter, although I have tried different combinations of using generics in the class, but it was always detected as a controller.

I have taken the liberty to force push some commits to the branch (hope that is ok).
@1: Besides using the dll usage as a hint to indicate, when we should perceive controller classes as ASP.NET controllers, we now also use namespace usage as a hint (this will work for tests and I suppose it also makes sense as production code).
@2: The cotainsTypeParameters will not work for phantom typed classes as the one if the test (might also be other cases where it doesn't work). There is a class called GenericType, this is probably the one we want to use.

[michaelnebel]

@JarLob : Are the pushed changes aligned with your thinking on the topic?

[JarLob]

Totally. Sorry for the late reply and thank you for fixing it for me! Tests are green. Is anything else needed to be done for it to be merged? Updating the branch?

[michaelnebel]

No problem - happy to help. 👍

The following needs to be done:

• Rebase the PR as things has changed in some of the surrounding code.
• Make a change note stating the changes as this will impact query results.
• Request review from an extra person (otherwise I will do an extra review - recommending @tamasvajk or @hvitved)
• Run DCA to see if we get any new alerts (and take a look at the diff if there are any new alerts in our existing projects)

Let me know, if you need any assistance with the above.

[JarLob]

• Rebase the PR as things has changed in some of the surrounding code.

Done, but it is a moving target.

• Make a change note stating the changes as this will impact query results.

Added.

• Request review from an extra person (otherwise I will do an extra review - recommending @tamasvajk or @hvitved)

I don't have permissions to add reviewers. Could you please request the review from @tamasvajk ?

• Run DCA to see if we get any new alerts (and take a look at the diff if there are any new alerts in our existing projects)

What is DCA and how to run it?

[michaelnebel]

LGTM, but let's await the second review as well. 👍

[michaelnebel]

What is DCA and how to run it?
DCA or Dist Compare Action is our test tool. It allows us test for performance regressions and new/missing alerts and much.
I will trigger an execution of DCA on this PR.

[tamasvajk]

LGTM apart from a minor issue in the change notes.

[tamasvajk]

I'm not a native speaker, but doesn't ASP.NET Core controller definition has been made more precise. sound better?

[JarLob]

Neither am I. I have definitely forgot to add was at least. Fixed.

[tamasvajk]

LGTM, let's wait for the DCA results before merging.

[michaelnebel]

LGTM, let's wait for the DCA results before merging.

I must admit I forgot about this PR.
Just inspected the DCA execution
(1) There doesn't appear to be any performance regressions.
(2) The alerts that have disappeared is as expected as these had methods in "private" controller like classes in their paths (only public controller like classes are perceived as controllers now).

Before merging, I would like to re-base the PR and just re-execute the unit tests.

[michaelnebel]

LGTM.
No manual changes required for rebasing.

[michaelnebel]

Everything still looks good.
Merging now.