ATM: Test for endpoints scored at inference time

[tiferet]

Adds a test to detect changes in the endpoints that get scored at inference time.

Note that the queries' .ql files (e.g. src/NosqlInjectionATM.ql) can't be called directly here, because they use the model and compute a score.

Closeshttps://github.com/github/ml-ql-adaptive-threat-modeling/issues/2135

[jhelie]

Thanks @tiferet, LGTM. I've added XssThroughDom but this didn't require updating the .expected files : I guess our some small examples do not contain instances of source for Xss and not for XssThroughDom (or vice versa).

[jhelie]

ah I see we were both working on the PR: I can push my change or you can update the ExtractEndpointDataInference.ql file yourself after rebasing on main.

(I'm de-approving in case you are still working on this and I'll refrain pushing my branch unless you tell me to do so)

changes since review

[tiferet]

Thanks @tiferet, LGTM. I've added XssThroughDom but this didn't require updating the .expected files : I guess our some small examples do not contain instances of source for Xss and not for XssThroughDom (or vice versa).

I don't see any commits from you, but don't worry about it -- I'll make the needed 1-line change and push it 😄

[tiferet]

@kaeluka / @henrymercer Does it make sense that there are no XssThroughDom sink candidates with flow from a source in endpoint_large_scale? Was that set created specifically for our existing four queries? If so, do we need to add to it each time we boost a new query?

[kaeluka]

LGTM, but I think we don't need to rely on the heavier PathNode class here. Node suffices.

Also suggested a name change to explicitly mention that the predicate only returns endpoints WITH FLOW.

[kaeluka]

Suggested change

	AtmConfig::AtmConfigqueryConfig, JS::DataFlow::PathNode sink
	AtmConfig::AtmConfigqueryConfig, JS::DataFlow::Node sink

[tiferet]

I want to keep the test as similar as possible to the actual extraction queries. The extraction queries use DataFlow::PathNode (e.g. javascript/ql/experimental/adaptivethreatmodeling/src/SqlInjectionATM.ql). I don't know if there's a reason they do this or not, but if we want to change those to DataFlow::Node (in which case we can change these as well), we should do so in a separate PR.

[kaeluka]

The extraction queries use the PathNodes for a specific reason - namely, that the UI should be able to list a specific path from source to sink. This is not needed in this test.

[tiferet]

See my comment here

[kaeluka]

Does it make sense that there are no XssThroughDom sink candidates with flow from a source in endpoint_large_scale?

No, IMO

Was that set created specifically for our existing four queries?

No, IMO (but I wasn't there)

If so, do we need to add to it each time we boost a new query?

I think so, yes.

[jhelie]

Please update the issue template if we need to consider updating endpoint_large_scale when adding a new query.

[tiferet]

Please update the issue template if we need to consider updating endpoint_large_scale when adding a new query.

@jhelie I added a line about it here.

[henrymercer]

@kaeluka / @henrymercer Does it make sense that there are no XssThroughDom sink candidates with flow from a source in endpoint_large_scale? Was that set created specifically for our existing four queries? If so, do we need to add to it each time we boost a new query?

Stephan is correct, and I'll add some more context. See https://github.com/github/codeql/tree/main/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/autogenerated for a description of how endpoint_large_scale is generated. Ideally, the files copied from javascript/ql/test/query-tests/Security/CWE-079 would contain some sink candidates with flow for XSS through DOM, but it looks like the model isn't finding anything new there.

The test set was not created specifically for the existing four queries, but in general we will need to check it whenever we boost a new query to ensure it covers the new query.

[tiferet]

@kaeluka / @henrymercer Does it make sense that there are no XssThroughDom sink candidates with flow from a source in endpoint_large_scale? Was that set created specifically for our existing four queries? If so, do we need to add to it each time we boost a new query?

Stephan is correct, and I'll add some more context. See https://github.com/github/codeql/tree/main/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/autogenerated for a description of how endpoint_large_scale is generated. Ideally, the files copied from javascript/ql/test/query-tests/Security/CWE-079 would contain some sink candidates with flow for XSS through DOM, but it looks like the model isn't finding anything new there.

The test set was not created specifically for the existing four queries, but in general we will need to check it whenever we boost a new query to ensure it covers the new query.

Thanks! @jhelie I linked this answer in the issue template as well, for when you get back to the XssThroughDom work.

[kaeluka]

The PR LGTM, although I'm a bit unclear whether the discussion between @tiferet and Jean has been resolved already.

• I'm approving this, assuming that all things related to the conversation will be resolved in a different PR. This is how I understood the conversation.
• The discussion about PathNodes is not worth losing time over (but it somewhat has bumped the urgency with which I want to look into that class's implementation ^^)

Also: thanks, @henrymercer for weighing in! I never had to look "inside" those tests before, so I appreciate the background info. I actually thought they were hand-crafted for the individual queries.

[tiferet]

The PR LGTM, although I'm a bit unclear whether the discussion between @tiferet and Jean has been resolved already.

That was actually a conversation about the addition of XssThroughDom, that ended up here just because this PR revealed that our test set lacks XssThroughDom examples 😄. That's part of the XssThroughDom work, though, unrelated to this PR.