-
Notifications
You must be signed in to change notification settings - Fork 1
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
Cover CAP's Fluent API to construct cds.ql
queries
#29
Conversation
…curity/codeql-sap-js into jeongsolee09/CAP-first-draft
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.
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll
Outdated
Show resolved
Hide resolved
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll
Outdated
Show resolved
Hide resolved
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll
Outdated
Show resolved
Hide resolved
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll
Outdated
Show resolved
Hide resolved
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.
First iteration. Most important part is that we need to properly represent CQL expression clauses as we are now mixing those with expressions themselves.
The parent relationship between CQL expression clauses is not correct and that needs to be addressed. The predicate getAnAncestorCqlExpr
is not equal to the transitive closure of getCqlParentExpr
.
comment style modules addition
add tests simplify models
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.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
isolate each test type
for case when objects are obtained through ql member of cds module
add extra sink for await on sql stmt and improve test for that also add metadata to query
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.
The modelling of CQL and CDS looks good to me as well as the tests.
The sqlinjection query can be improved in future PRs
Maybe you could add QLDoc to CQL.qll entities?
Model CAP's Fluent API that constructs CQL query objects. This PR aims to cover all API combinations of the following:
SELECT
.one
.distinct
.columns()
(includingSELECT
itself behaving as a shortcut for it).from()
.alias()
.where()
.having()
.groupBy()
.orderBy()
.limit()
.forUpdate()
.forShareLock()
INSERT
.into()
.entries()
(includingINSERT
itself behaving as a shortcut for it).values()
.rows()
.as()
DELETE
.from()
.where()
UPDATE
.entity()
(includingUPDATE
itself behaving as a shortcut for it).set()
.with()
.where()
UPSERT
.into()
.entries()
(includingUPSERT
itslef behaving as a shortcut for it)What this PR does not cover:
CQL `SELECT col1, col2, col3 from Table`;
This PR also adds some modelling for the CDS custom in source Service modelling
It covers:
and request handlers registered in user definitions of services, but remains to be tested on if it will miss ones that are added later, for example
one consideration for this is , for
RequestSource
implementation, do we need to actually ever know which service the handler is associated to?It does not cover:
Lastly this PR adds a basic SQL injection query that uses the above models (ie request objects received in event handlers that end up in CQL SQL statements and are run)