-
Notifications
You must be signed in to change notification settings - Fork 1.7k
JS: Initial models-as-data implementation #7171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Updated with some new access path tokens.
|
|
|
||
/** Holds if `path` has the form `basePath.token` where `token` is a single token. */ | ||
bindingset[path] | ||
private predicate decomposePath(string path, string basePath, string token) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way of decomposing is quadratic in the number of tokens in path
. Might as well use a linear decomposition. I.e. for "a.b.c.d"
just construct each of "a"
, "b"
, "c"
, and "d"
, instead of also constructing "a.b.c"
and "a.b"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably doesn't matter because of the package-level pruning, but yeah we might as well clean it up.
We previously needed a value to represent each step of the access path, which we don't get by simply splitting into individual tokens. The resolution predicates now take an int n
parameter and resolve the first n
tokens of the given full access path, with no sharing of common prefixes.
To simplify accessing the tokens of an access path I added the AccessPath
class as well.
Evaluation still looks OK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking really good 👍
I only have a few minor comments.
javascript/ql/lib/semmle/javascript/frameworks/data/internal/Impl.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll
Outdated
Show resolved
Hide resolved
Thanks for the review @erik-krogh! I've addressed the comments and force-pushed to resolve the conflict in SQL.qll. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
There is just an auto-format issue.
And some implicit-this QL-for-QL warnings.
I've fixed all of that in the suggestions below.
javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll
Outdated
Show resolved
Hide resolved
Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
Initial implementation of models-as-data. Please use the backlinked issue for high-level discussion of the format itself.
The implementation is split in three files:
Shared.qll
is intended to be shared between languages (some day; currently only works for JS)Impl.qll
is intended to contain the langauge-specific details.ModelsAsData.qll
is the public interface from the rest of the librarySome predicates from
Shared.qll
must be usable fromImpl.qll
, but should not be public in the library, soModelsAsData.qll
acts as a gatekeeper toward the public API.The API exposes two modules:
ModelInput
is used for contributing modelsModelInput::SinkModelCsv
to add new sinksModelOutput
is used for accessing interpreted models in terms of API nodesModelOutput::getASinkNode("sql-injection")
to get SQL injection sinksIn this PR I have ported Sequelize and Spanner, as initial validation.
Evaluation from a slightly earlier version.
getSourceType()
returned a different string for those source, not because they were truly new)