Skip to content

C#: Blazor: Add route parameters as remote flow sources #18664

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

Conversation

egregius313
Copy link
Contributor

Adds the variables parsed from the @page directive of a Blazor component as remote flow sources

@Copilot Copilot AI review requested due to automatic review settings February 3, 2025 17:17
@egregius313 egregius313 requested a review from a team as a code owner February 3, 2025 17:17
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@github-actions github-actions bot added the C# label Feb 3, 2025
/** Provides classes for working with `Microsoft.AspNetCore.Components` */

import csharp
import semmle.code.csharp.frameworks.Microsoft

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
semmle.code.csharp.frameworks.microsoft.AspNetCore
.
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 102 to 108
Property getARouteParameterProperty() {
result = this.getAParameterProperty() and
exists(string urlParamName | urlParamName = this.getARouteParameter() |
result.getName().toLowerCase() = urlParamName.toLowerCase()
)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Joining on names is always a candidate for a bad join, so it is usually safer to force multi-column joins, e.g.

  pragma[nomagic]
  private Property getAParameterProperty(string name) {
    result = this.getAProperty() and
    result.getAnAttribute() instanceof MicrosoftAspNetCoreComponentsParameterAttribute and
    name = result.getName().toLowerCase()
  }

  pragma[nomagic]
  private string getARouteParameter() {
    exists(string s |
      s = this.getRouteAttributeUrl().splitAt("{").regexpCapture("\\*?([^:?}]+)[:?}](.*)", 1) and
      result = s.toLowerCase()
    )
  }

  Property getARouteParameterProperty() {
    exists(string name |
      result = this.getAParameterProperty(name) and
      name = this.getARouteParameter()
    )
  }

@@ -0,0 +1,7 @@
import semmle.code.csharp.security.dataflow.flowsources.Remote
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the motivation for using integration tests instead of normal QL tests? I find it quite difficult to run
(specific) integration tests locally...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was mostly because the test was originally from Tamás's WIP PR. And I wasn't sure about testing Blazor components as a QL test (I'm unsure if I'll need to have a full project). I'll experiment with moving it.

You are correct about the integration test being a more complicated way to run the test locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not easy to do, then let's do only the integration test.

from RemoteFlowSource source, File f
where
source.getLocation().getFile() = f and
(f.fromSource() or f.getExtension() = "razor")
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the f.getExtension() = "razor" disjunct can be removed?

@egregius313 egregius313 merged commit c965024 into github:main Feb 7, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants