JS: Add SharedTaintStep (again)#5396
Conversation
esbena
left a comment
There was a problem hiding this comment.
Nice!
And bravo for keeping this to so many atomic and simple commits!
My concern is that we are not making our users properly aware of this change. It is not obvious to outsiders why we are doing this major surgery, which could look like a simple renaming exercise that is optional to respect.
I suppose a change note and an update to the tutorials would suffice. A more glaring deprecation warning would be great, but I can see how that may be tricky.
| * of the standard library. Override `Configuration::isAdditionalTaintStep` | ||
| * for analysis-specific taint steps. | ||
| */ | ||
| abstract class AdditionalTaintStep extends DataFlow::Node { |
There was a problem hiding this comment.
Could we mark this as deprecated, or at least add some prominent text about SharedTaintSted? Otherwise, the users will keep using it.
(I suppose the reason that this isn't deprecated is that we use it ourselves in legacyAdditionalTaintStep, it would be nice if we could juggle our way out of that)
There was a problem hiding this comment.
Oh I thought it actually was deprecated. It's properly deprecated now.
I've also updated the tutorial. In the process of doing that, I introduced a forward-compatible version of DataFlow::SharedFlowStep as one paragraph simply couldn't be updated without distracting the reader with the details of how these two classes differ. Actually migrating to SharedFlowStep is for a future PR.
| /** | ||
| * Holds if `pred -> succ` is an edge used by all taint-tracking configurations. | ||
| */ | ||
| predicate sharedTaintStep(DataFlow::Node pred, DataFlow::Node succ) { |
There was a problem hiding this comment.
Could we add a comment at the relevant locations about the need to manually maintain this large disjunction?
esbena
left a comment
There was a problem hiding this comment.
Do we wait for a change note labelling decision, or do we add that afterwards?
8ed0579 to
ccc879d
Compare
|
Rebased to resolve conflicts |
erik-krogh
left a comment
There was a problem hiding this comment.
LGTM.
But shouldn't we add a change note?
(Or are you waiting until DataFlow::SharedFlowStep is fully implemented?)
|
@esbena any final comments from your end? |
For anyone else seeing this, we addressed this question offline and decided not to add a change note for now. |
Revival of #3603 without the string base-type stuff.
Converts
AdditionalTaintStepto a unit-type class calledSharedTaintStep:AdditionalTaintStepremains as a deprecated class.AdditionalTaintStepinto a unit type would be a breaking change, hence the new class.Evaluation looks good