Skip to content

ATM: Boost XssThroughDOM#11486

Merged
jhelie merged 8 commits intomainfrom
tiferet/boost-xss-through-dom
Dec 2, 2022
Merged

ATM: Boost XssThroughDOM#11486
jhelie merged 8 commits intomainfrom
tiferet/boost-xss-through-dom

Conversation

@tiferet
Copy link
Copy Markdown
Contributor

@tiferet tiferet commented Nov 30, 2022 •
edited
Loading

This is a new version of #11204 based on top of the CodeQL refactoring, of which #11323 is the final PR.

@jhelie Because rebasing was too hard, I created this new version of your PR based on top of the new CodeQL framework. As I did this, though, I noticed that, as I understand it, there are more changes that need to be made when boosting a new query that are missing from your PR. Searching the repo for the substring XssAtm I found changes that I think need to be made for XssThroughDOM in the following files:

  • javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/EndpointFeatures.ql
  • javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/FilteredTruePositives.ql
  • javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/DebugResultInclusion.ql
  • javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll (this one will go away if we deprecate hasFlowFromSource from the training data)

The changes to the tests will also require changes to the .expected files, of course.

@github-actions github-actions Bot added the ATM label Nov 30, 2022
@tiferet tiferet mentioned this pull request Nov 30, 2022
Closed
Base automatically changed from tiferet/simplify-configs to main November 30, 2022 01:39
@tiferet tiferet force-pushed the tiferet/boost-xss-through-dom branch from a2921cb to b885249 Compare November 30, 2022 01:40
@owen-mc owen-mc changed the title Boost XssThroughDOM ATM: Boost XssThroughDOM Nov 30, 2022
github-advanced-security[bot]
github-advanced-security AI found potential problems Dec 1, 2022
View reviewed changes
Comment thread javascript/ql/experimental/adaptivethreatmodeling/src/XssThroughDomATM.ql Fixed
@jhelie
Copy link
Copy Markdown
Contributor

jhelie commented Dec 1, 2022

I'm not sure why the tests are failing, I can run the query on my machine and the tests pass.

@henrymercer
Copy link
Copy Markdown
Contributor

henrymercer commented Dec 1, 2022

The tests run on a merge commit, and there have been changes on main since this PR. So I think we can resolve this by merging in main and updating the code, e.g. the getAlerts predicate specifically.

@jhelie
Copy link
Copy Markdown
Contributor

jhelie commented Dec 1, 2022 •
edited
Loading

I've merged main but I'm not sure which piece of code I'm supposed to update?

I see it, ignore me

@henrymercer
Copy link
Copy Markdown
Contributor

henrymercer commented Dec 1, 2022

Can you now reproduce the test failures locally?

@jhelie jhelie marked this pull request as ready for review December 1, 2022 22:09
@jhelie jhelie requested a review from a team December 1, 2022 22:09
@tiferet
Copy link
Copy Markdown
Contributor Author

tiferet commented Dec 1, 2022

Note: When #11532 gets merged XssThroughDOM should be added to that test as well.

adityasharad
adityasharad approved these changes Dec 1, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Code changes look good. Assuming no other concerns from the team, I recommend merging this as is, so we don't accumulate conflicts with other PRs, and evaluating the combined state of main.

Comment thread javascript/ql/experimental/adaptivethreatmodeling/src/XssThroughDomATM.ql
@@ -0,0 +1,25 @@
/**
* For internal use only.
Copy link
Copy Markdown
Collaborator

@adityasharad adityasharad Dec 1, 2022

Choose a reason for hiding this comment

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

Shall we just remove this comment now, so we don't forget later? Here and in the corresponding QLL file.

Copy link
Copy Markdown
Contributor

@jhelie jhelie Dec 2, 2022

Choose a reason for hiding this comment

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

This comment is present in all ATM-related .ql and .qll so probably best done in a separate PR given the churn. I'll open one.

@jhelie
Copy link
Copy Markdown
Contributor

jhelie commented Dec 2, 2022

Merging now, and will evaluate new main state.

@jhelie jhelie merged commit 3f203ea into main Dec 2, 2022
@jhelie jhelie deleted the tiferet/boost-xss-through-dom branch December 2, 2022 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityasharad adityasharad adityasharad approved these changes

+1 more reviewer

@jhelie jhelie jhelie left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ATM

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tiferet @jhelie @henrymercer @adityasharad @github-advanced-security