Dataflow: Deprecate FlowStateString. #15062
Conversation
aschackmull
added
the
no-change-note-required
aschackmull
force-pushed
the
dataflow/deprecate-flowstatestring
branch
from
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). |
aschackmull
dismissed stale reviews from yoff, geoffw0, atorralba, and owen-mc
via
f2dea89
aschackmull
force-pushed
the
dataflow/deprecate-flowstatestring
branch
from
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 |
aschackmull
force-pushed
the
dataflow/deprecate-flowstatestring
branch
from
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.
| /** 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?
There was a problem hiding this comment.
Yes, but such a hypothetical source wouldn't make much sense. Its default state would not match any sink, so it wouldn't work. Hence, I find it unlikely that this extension point has been used in the wild. Really, as the query is written, we need a key size state to be specified for the sources and the sinks, and with the way that I've reimplemented the logic I found it hard to provide a straight-forward deprecation path from the old interface.
I guess we could put in some work provide a better deprecation path by renaming the hasState predicate and providing a translation from the old to the new, but it's not going to be neither pretty nor simple, do you think that's worth it? I was leaning towards risking breakage here as my preferred option.
There was a problem hiding this comment.
I was just making sure this was a deliberate decision. I also favor the cleanest path forward (if we are allowed to break things when we believe it makes sense, which I didn't know 😄).
There was a problem hiding this comment.
It's a calculated risk and one that we generally try hard to avoid. But adding a new abstract predicate isn't really possible to do without potential breakage.
Co-authored-by: Tony Torralba <[email protected]>
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Followup for #14983. I don't think we need a change note for this, as this was effectively part of the old api.