JS: add initial support for import.meta expressions in TypeScript by erik-krogh · Pull Request #2169 · github/codeql

erik-krogh · 2019-10-22T14:03:52Z

Adding initial support for import.meta in TypeScript.

This is only for TypeScript, and the recently reported FP is therefore not fixed.

Adding support for import.meta in JavaScript should just be a case of emitting a new MetaProperty(..) instead of unexpected token.

erik-krogh · 2019-10-22T14:07:07Z

javascript/extractor/lib/typescript/src/main.ts

 }

-function handleParseCommand(command: ParseCommand) {
+function handleParseCommand(command: ParseCommand, checkPending = true) {


This change is not needed to support import.meta.
But it was required for me to run the "standalone" commands, so I kept the change.

erik-krogh · 2019-10-22T14:57:18Z

It might need a db upgrade script. I forgot about that.

xiemaisi

Looks plausible, though I can't comment on the TypeScript extractor change. I'd also suggest making import.meta expressions source nodes by adding astNode instanceof ImportMetaExpr to the characteristic predicate of SourceNode::DefaultRange in Sources.qll.

As you say, this will need a dbscheme upgrade script.

xiemaisi · 2019-10-22T15:59:15Z

javascript/ql/src/semmle/javascript/ES2015Modules.qll

+/**
+ * An `import.meta` expression.
+ */
+class ImportMeta extends @importmetaexpr, Expr {


I think this class would feel more at home in Expr.qll, and should probably be called ImportMetaExpr by analogy with other similar classes. Also, it looks like import.meta expressions are pure, so it may be worth adding an override of isImpure().

asger-semmle

We'll also need to open an internal PR to bump the dbscheme hash.

asger-semmle · 2019-10-22T16:01:22Z

javascript/extractor/src/com/semmle/js/parser/TypeScriptASTConverter.java

        new Position(metaStart.getLine(), metaStart.getColumn() + 3, metaStart.getOffset() + 3);
-    SourceLocation metaLoc = new SourceLocation("new", metaStart, metaEnd);
-    Identifier meta = new Identifier(metaLoc, "new");
+    String keyWordKind = syntaxKinds.get(node.getAsJsonPrimitive("keywordToken").getAsInt() + "").toString();


Use getAsString instead of toString.

Also keyWordKind -> keywordKind

asger-semmle · 2019-10-22T16:03:50Z

javascript/extractor/src/com/semmle/js/parser/TypeScriptASTConverter.java

-    SourceLocation metaLoc = new SourceLocation("new", metaStart, metaEnd);
-    Identifier meta = new Identifier(metaLoc, "new");
+    String keyWordKind = syntaxKinds.get(node.getAsJsonPrimitive("keywordToken").getAsInt() + "").toString();
+    String identifier = keyWordKind.equals("\"ImportKeyword\"") ? "import" : "new";


The quotes \" are unnecessary with getAsString

asger-semmle · 2019-10-22T16:04:46Z

javascript/extractor/src/com/semmle/js/parser/TypeScriptASTConverter.java

@@ -1582,8 +1582,10 @@ private Node convertMetaProperty(JsonObject node, SourceLocation loc) throws Par
    Position metaStart = loc.getStart();
    Position metaEnd =
        new Position(metaStart.getLine(), metaStart.getColumn() + 3, metaStart.getOffset() + 3);


The magic constant 3 here seems to have been outdated, as it should presumably be 4 for import.meta.

asger-semmle · 2019-10-22T16:09:07Z

javascript/ql/src/semmle/javascript/ES2015Modules.qll

 }
+
+/**
+ * An `import.meta` expression.


Our AST classes have concrete syntax examples:

Suggested change

* An `import.meta` expression.

* An `import.meta` expression.

*

* Example:

* ```js

* let url = import.meta.url;

* ```

asger-semmle · 2019-10-22T16:10:39Z

change-notes/1.23/analysis-javascript.md

  - [rate-limiter-flexible](https://www.npmjs.com/package/rate-limiter-flexible)

 * The call graph has been improved to resolve method calls in more cases. This may produce more security alerts.
+* import.meta is now supported in TypeScript.


With this change I believe we have complete support for 3.6 so I'd rather just mention that we support TypeScript 3.6 now.

Bonus nitpick: add linebreak before the bullet point

javascript/upgrades/ab0dab3b55b386f366e9904e10139edebca0495c/upgrade.properties

asger-semmle · 2019-10-23T11:32:19Z

Some tests are failing (in the internal PR) due to old.dbscheme not corresponding to the old dbscheme. The only diff seems be a newline at the end. Could you make sure it's an exact copy of the previous dbscheme?

Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>

erik-krogh · 2019-10-24T09:55:34Z

Internal PR just turned green.

erik-krogh added the JS label Oct 22, 2019

erik-krogh requested a review from asger-semmle October 22, 2019 14:03

erik-krogh requested a review from a team as a code owner October 22, 2019 14:03

erik-krogh changed the title ~~JS: add initial support for expressions in TypeScript~~ JS: add initial support for import.meta expressions in TypeScript Oct 22, 2019

erik-krogh commented Oct 22, 2019

View reviewed changes

xiemaisi suggested changes Oct 22, 2019

View reviewed changes

asger-semmle reviewed Oct 22, 2019

View reviewed changes

max-schaefer reviewed Oct 23, 2019

View reviewed changes

javascript/upgrades/ab0dab3b55b386f366e9904e10139edebca0495c/upgrade.properties Outdated Show resolved Hide resolved

erik-krogh and others added 3 commits October 24, 2019 10:17

add initial support for expressions in TypeScript

834b572

change update script description

a584d7c

Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>

fix line end at end of dbscheme

ab42b5d

erik-krogh force-pushed the importMeta branch from 9387c11 to ab42b5d Compare October 24, 2019 08:17

max-schaefer approved these changes Oct 24, 2019

View reviewed changes

asger-semmle merged commit 7dd7463 into github:master Oct 24, 2019

erik-krogh mentioned this pull request Dec 18, 2019

JS: add support for import.meta expressions in JavaScript #2552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS: add initial support for import.meta expressions in TypeScript#2169

JS: add initial support for import.meta expressions in TypeScript#2169
asger-semmle merged 3 commits intogithub:masterfrom
erik-krogh:importMeta

erik-krogh commented Oct 22, 2019

Uh oh!

erik-krogh Oct 22, 2019

Uh oh!

erik-krogh commented Oct 22, 2019

Uh oh!

xiemaisi left a comment

Uh oh!

xiemaisi Oct 22, 2019

Uh oh!

asger-semmle left a comment

Uh oh!

asger-semmle Oct 22, 2019

Uh oh!

asger-semmle Oct 22, 2019

Uh oh!

asger-semmle Oct 22, 2019

Uh oh!

asger-semmle Oct 22, 2019

Uh oh!

asger-semmle Oct 22, 2019

Uh oh!

Uh oh!

asger-semmle commented Oct 23, 2019

Uh oh!

erik-krogh commented Oct 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

erik-krogh commented Oct 22, 2019

Uh oh!

erik-krogh Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Oct 22, 2019

Uh oh!

xiemaisi left a comment

Choose a reason for hiding this comment

Uh oh!

xiemaisi Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

asger-semmle left a comment

Choose a reason for hiding this comment

Uh oh!

asger-semmle Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

asger-semmle Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

asger-semmle Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

asger-semmle Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

asger-semmle Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asger-semmle commented Oct 23, 2019

Uh oh!

erik-krogh commented Oct 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants