Skip to content

C#: Re-factor Xss to use the new data flow API. #12845

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

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

michaelnebel
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the C# label Apr 17, 2023
@michaelnebel michaelnebel force-pushed the csharp/xssrefactor branch 2 times, most recently from ff09ed8 to 5a83e1a Compare April 19, 2023 07:38
@michaelnebel
Copy link
Contributor Author

DCA looks good, except for a failure on mono (both variants failed - so most likely a spurious failure).

@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Apr 20, 2023
@michaelnebel michaelnebel marked this pull request as ready for review April 20, 2023 07:21
@michaelnebel michaelnebel requested a review from a team as a code owner April 20, 2023 07:21
@michaelnebel
Copy link
Contributor Author

michaelnebel commented Apr 20, 2023

After re-running DCA for the mono project (failed in the first attempt), we get an extra alert.
However, this is the same alert that disappeared during the last quarter: https://github.com/github/codeql-dca-main/issues/12334.
I think we should accept this.

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

One comment, otherwise LGTM.

}
}

private newtype TXssNode =
TXssDataFlowNode(DataFlow2::PathNode node) or
TXssDataFlowNode(DataFlow2::PathNode node) or // Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want this, as it means we basically have to compute the flow graph twice. Instead, I think for this case we should simply break backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - that is fair enough.
I will do that instead and add a change node (I suppose that is breaking change which requires bumping the major version number?)

---
category: majorAnalysis
---
* C#: Extending `TaintTrackingConfiguration` in `XSSQuery.qll` no longer affects query results.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change note is needed, TBH. It was never intended for 3rd parties to customize the query by overriding the configuration itself; instead, they could add new sources or sinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright - I will go ahead and remove it.

@michaelnebel michaelnebel merged commit 239a763 into github:main Apr 21, 2023
@michaelnebel michaelnebel deleted the csharp/xssrefactor branch April 21, 2023 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants