Skip to content

Fix neutral definition for CodeQL 2.13.3#2467

Merged
koesie10 merged 2 commits intomainfrom
koesie10/fix-neutral-definition
Jun 2, 2023
Merged

Fix neutral definition for CodeQL 2.13.3#2467
koesie10 merged 2 commits intomainfrom
koesie10/fix-neutral-definition

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Jun 1, 2023

In CodeQL 2.13.3, the definition of the neutralModel predicate has changed to include the kind. This updates the definition of the data extensions editor to match the new definition.

One caveat is that when selecting a kind other than summary, the method will not be shown as supported. This is because a NeutralCallable only calls into neutralSummaryElement. This matches the previous behavior because setting the kind to source or sink only says that the method is either not a source or not a sink, but not both. Only summary fully models the method.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

In CodeQL 2.13.3, the definition of the neutralModel predicate has
changed to include the `kind`. This updates the definition of the
data extensions editor to match the new definition.

One caveat is that when selecting a `kind` other than `summary`, the
method will not be shown as supported. This is because a
`NeutralCallable` only calls into `neutralSummaryElement`. This matches
the previous behavior because setting the `kind` to `source` or `sink`
only says that the method is either not a source or not a sink, but not
both. Only `summary` fully models the method.

See: github/codeql#12931
See: https://github.com/github/codeql/blob/ff78ac98d27c7b9f1adffcf235c56855f8348ad0/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll#L338
See: https://github.com/github/codeql/blob/ff78ac98d27c7b9f1adffcf235c56855f8348ad0/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImplSpecific.qll#L160
@koesie10 koesie10 added the secexp label Jun 1, 2023
@starcke starcke self-requested a review June 1, 2023 13:12
@koesie10 koesie10 marked this pull request as ready for review June 1, 2023 13:19
@koesie10 koesie10 requested a review from a team as a code owner June 1, 2023 13:19
Copy link
Copy Markdown
Contributor

@starcke starcke left a comment

Choose a reason for hiding this comment

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

Nice! I am impressed that the change is so small.

One caveat is that when selecting a kind other than summary, the method will not be shown as supported. This is because a NeutralCallable only calls into neutralSummaryElement. This matches the previous behavior because setting the kind to source or sink only says that the method is either not a source or not a sink, but not both. Only summary fully models the method.

Good to know, this ties into the whole discussion of whether we should model functions or arguments and how to deal with things that have have multiple classifications (e.g. an argument that is both a sink and a flow summary)

@koesie10 koesie10 enabled auto-merge June 1, 2023 14:59
@koesie10 koesie10 merged commit 26d27f8 into main Jun 2, 2023
@koesie10 koesie10 deleted the koesie10/fix-neutral-definition branch June 2, 2023 07:46
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