Skip to content

Conversation

@asgerf
Copy link
Contributor

@asgerf asgerf commented Oct 9, 2020

Adds a model of Angular, also known as Angular2 (not to be confused with AngularJS; we already have a model of that).

The model relies heavily on type annotations, as Angular's dependency injection depends on type information (which seemed impossible until I read this blog post). This mean types aren't just a substitute for missing data flow in this model.

The model doesn't currently use API graphs as there isn't currently a drop-in replacement for hasUnderlyingType which is pretty important for this model. Some improvements for this exact use-case are underway, though given the number of evaluation I've had to run I'd prefer to postpone integration to a later PR.

It also includes two commits to resolve some performance issues that arose since some more Customizations files are now transitively reachable from javascript.qll. In particular, we should avoid creating flow labels in such files as they affect all data flow configurations.

Evaluations: (internal links)

@asgerf asgerf added the JS label Oct 9, 2020
@asgerf asgerf requested a review from a team as a code owner October 9, 2020 13:14
import UnvalidatedDynamicMethodCallCustomizations::UnvalidatedDynamicMethodCall
private import DataFlow::FlowLabel

// Materialize flow labels
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add the below as a test somewhere:

import javascript

select any(DataFlow::FlowLabel label)

That way we have a failing test if new flow-labels are added globally.

Also, this PR could use a merge.
And do you intend to rebase this PR on top of the recent ApiGraph changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And do you intend to rebase this PR on top of the recent ApiGraph changes?

No, mentioned in the PR description, it took a lot of evaluations to get here, so I'll defer integration to a later PR.

@asgerf
Copy link
Contributor Author

asgerf commented Oct 19, 2020

I split the CWE-079 tests into separate folders to avoid a problem with the BDD node limit. @erik-krogh PTAL

@codeql-ci codeql-ci merged commit 5ead424 into github:main Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants