Skip to content

Conversation

jhelie
Copy link
Contributor

@jhelie jhelie commented Mar 24, 2022

As discussed with @henrymercer this PR adds a defensive check in the getAnUnknown() predicate in ExtractEndpointData.qll. This should now ensure that the label of an endpoint has a unique value in {NotASink, Sink, Unkknown} for a given query.

The root problem should be fixed upstream by improving the endpoint filters, see https://github.com/github/ml-ql-adaptive-threat-modeling/issues/1818.

PR checks:

  • here is the dev pipeline triggered using the proposed change in the QL library
  • here is the model trained on the output of the above dev pipeline and here is its end-to-end evaluation

I therefore propose this PR is good to merge.

Relates to https://github.com/github/ml-ql-adaptive-threat-modeling/issues/1757

@jhelie jhelie requested a review from a team March 24, 2022 12:34
@github-actions github-actions bot added the JS label Mar 24, 2022
@jhelie jhelie force-pushed the jhelie/enforce-unknown-incompatibiliy-with-notasink branch 3 times, most recently from 1c8011c to cf820e7 Compare March 24, 2022 14:35
@jhelie jhelie marked this pull request as draft March 24, 2022 14:36
@owen-mc owen-mc changed the title add defensive check to ensure Unknown endpoints cannot also be NotASink ML: add defensive check to ensure Unknown endpoints cannot also be NotASink Mar 24, 2022
@jhelie
Copy link
Contributor Author

jhelie commented Mar 24, 2022

(thanks for the rename @owen-mc , will do so next time 👍 )

@jhelie jhelie force-pushed the jhelie/enforce-unknown-incompatibiliy-with-notasink branch from cf820e7 to 5abd885 Compare March 28, 2022 10:56
@jhelie jhelie force-pushed the jhelie/enforce-unknown-incompatibiliy-with-notasink branch from 5abd885 to c080ee9 Compare April 8, 2022 11:47
@jhelie jhelie marked this pull request as ready for review April 11, 2022 12:46
@henrymercer henrymercer removed their assignment Apr 11, 2022
@jhelie jhelie force-pushed the jhelie/enforce-unknown-incompatibiliy-with-notasink branch from c080ee9 to cb88b26 Compare April 13, 2022 11:53
@jhelie jhelie force-pushed the jhelie/enforce-unknown-incompatibiliy-with-notasink branch from cb88b26 to b46cbad Compare April 13, 2022 12:02
@jhelie
Copy link
Contributor Author

jhelie commented Apr 13, 2022

Thanks for the review @henrymercer . I agree it's a good idea to add a regression test but I will need your help to implement it. I don't think you have any more office hours this week and then you are on holidays, so do you think we can schedule something this week? I think it'd be good for this PR to be merged now rather than in 2 weeks.

@jhelie
Copy link
Contributor Author

jhelie commented Apr 13, 2022

Here is an example of a sink that is currently labelled as both Unknown and NotASink (there are 8 others that can be found in sudheeshshetty-chat when running the test pipeline).

cc @annarailton

@jhelie jhelie force-pushed the jhelie/enforce-unknown-incompatibiliy-with-notasink branch from b46cbad to 95b2ce3 Compare April 13, 2022 16:08
@jhelie jhelie force-pushed the jhelie/enforce-unknown-incompatibiliy-with-notasink branch from 95b2ce3 to 1e39a9c Compare April 13, 2022 16:14
@jhelie
Copy link
Contributor Author

jhelie commented Apr 13, 2022

@henrymercer : thanks to @annarailton's help I could update the tests. I've added 2 commits: one pre and post fix so that we can easily verify that the change in getAnUnknown modified the outcome of that new regression test (by removing the Unknown label).

I think this addresses all your comments.

@jhelie jhelie requested a review from henrymercer April 13, 2022 16:19
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for implementing that regression test and ✨ to @annarailton for 🍐ing on it!

@jhelie jhelie merged commit d094bbc into main Apr 14, 2022
@jhelie jhelie deleted the jhelie/enforce-unknown-incompatibiliy-with-notasink branch April 14, 2022 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants