Dataflow: Deprecate FlowStateString. #15062
Conversation
a026252
to
edd6583
Compare
|
Fix for the coding standards tests: github/codeql-coding-standards#473 |
|
@aschackmull The coding standards tests are now fixed. |
|
Oh, one of the test failures is in Swift. There was briefly a version of |
Yes. I'm aware I need to rebase, and that should fix the swift test. But there's also a Java failure, so I was planning to fix that before rebasing. (No need to rerun CI twice). |
f2dea89
7b4c0db
to
f2dea89
Compare
|
@atorralba could you take a look at the last commit - I've fixed the 3 java queries that had accidental exposure of |
f2dea89
to
7623432
Compare
There was a problem hiding this comment.
Ok, so the last commit mostly LGTM.
ImplictPendingIntents👍- Turns out that we didn't even use
FlowStates inTemplateInjection, and I made those extension points "just in case" 😅. Anyway, 👍. - I think I followed your changes in
InsufficientKeySize. AFAIU there aren't changes in the semantics (and the green tests probably confirm this), so we should be good. Still, I added a few minor comments/questions.
| @@ -3,32 +3,76 @@ | |||
| private import semmle.code.java.security.Encryption | |||
| private import semmle.code.java.dataflow.DataFlow | |||
| private import semmle.code.java.security.internal.EncryptionKeySizes | |||
| import codeql.util.Either | |||
There was a problem hiding this comment.
Make this private?
| import codeql.util.Either | |
| private import codeql.util.Either |
| MinimumKeySize() { any() } | ||
|
|
||
| /** Gets a textual representation of this element. */ | ||
| string toString() { result = super.toString() } |
There was a problem hiding this comment.
OOC: why do we need this implementation of toString? And same question for AlgorithmKind.
| /** Holds if this sink has the specified `state`. */ | ||
| predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty } | ||
| /** Holds if this sink accepts the specified `state`. */ | ||
| final predicate hasState(KeySizeState state) { |
There was a problem hiding this comment.
Couldn't this break existing implementations of InsufficientKeySizeSink overriding hasState?
|
|
||
| /** A source for an insufficient key size. */ | ||
| abstract class InsufficientKeySizeSource extends DataFlow::Node { | ||
| /** Holds if this source has the specified `state`. */ | ||
| predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty } | ||
| abstract predicate hasState(KeySizeState state); |
There was a problem hiding this comment.
Couldn't this break existing implementations of InsufficientKeySizeSource that don't override hasState?
Followup for #14983. I don't think we need a change note for this, as this was effectively part of the old api.