JS: Add dot.js support

[jorgectf]

No description provided.

[asgerf]

Looks reasonable, but I'm thinking there are probably a few other types of .dot files that aren't part of this templating system, such as Graphviz dot. We'll need to make sure that it has no adverse effects when the repository contains such files. I don't think I'll have the time to investigate this week, however, so apologies in advance for leaving this PR hanging for a bit, as another issue is currently taking priority.

[asgerf]

A quick code search shows about 800k files with the .dot extension, and from a few other searches I'd estimate around 1k of those are DoT templates.

Based on that, I'd like to restrict the extraction a bit to avoid extracting so many unrelated files as HTML.

For .erb files we only actually extract .html.erb files (due to this code). Would a similar restriction work for your use case, i.e. restricting to just .html.dot files?

Otherwise we can sniff the file header for HTML tags but that's a bit more work.

[asgerf]

Looks like the two escaping mechanisms were flipped around, either in the comment or in the implementation.

• The comment says {{! }} is safe, but the implementation treats it as raw (i.e. unsafe)
• The comment says {{= }} is unsafe, but the implementation treats it as escaping (i.e. safe, in most contexts).

[jorgectf]

Oops, sorry, the model is right, {{! }} is for unsafe (raw) interpolation.

[jorgectf]

Based on that, I'd like to restrict the extraction a bit to avoid extracting so many unrelated files as HTML.

For .erb files we only actually extract .html.erb files (due to this code). Would a similar restriction work for your use case, i.e. restricting to just .html.dot files?

Otherwise we can sniff the file header for HTML tags but that's a bit more work.

Thanks for the analysis! I'm happy to restrict it to .html.dot to avoid extracting unrelated files. wdyt @Kwstubbs?

[asgerf]

@jorgectf any updates? Let me know if you need to help from my end. I'd prefer to get the PR merged rather than hanging.

[Kwstubbs]

@asgerf Hi Asger, Jorge is currently on a month long vacation. I will look into this PR very soon.

[Kwstubbs]

@asgerf What do you think about adding the .jst extension as well? Seems this is used by other template engines as well.

[asgerf]

@Kwstubbs I'd prefer to focus on getting the PR merged, without expanding its scope. JST can be added in a separate PR.

[Kwstubbs]

@asgerf made the same changes as the .html.erb files, let me know if that will do the trick.

[asgerf]

Unfortunately the test doesn't seem to react to the changes. There should be an update to the corresponding .expected file showing the new data flow into the template, and currently it doesn't find that flow.

I've pushed two commits that fix two of the underlying issues. For some reason the test still isn't finding the flow, so there's still some work to do here, though I need to commit my time elsewhere right now.

[jorgectf]

Thank you @asgerf for your dedication, I'll work with @Kwstubbs to try to figure things out.

[asgerf]

The issue might be that TEMPLATE_EXPR_OPENING_TAG needs to include {{! as the opening of an expression tag.

[jorgectf]

The issue might be that TEMPLATE_EXPR_OPENING_TAG needs to include {{! as the opening of an expression tag.

The tests still don't find the new results :/

[erik-krogh]

The tests still don't find the new results :/

I found the reason. I've pinged you in an internal PR.
There is also a backref to the internal PR.

Your tests work after that PR has been merged.

Also, could you add .html.dot to this list:

codeql/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java

Line 156 in 68e7f84

* FileType#HTML} (currently ".htm", ".html", ".xhtm", ".xhtml", ".vue", ".html.erb", ".jsp").

Edit: The internal PR has been merged.
If you do a merge with main, and build your CodeQL CLI from the latest main (including the above PR), then everything should work.
Alternatively you can wait for the next CodeQL CLI release.

[erik-krogh]

Looks good, can you add a change-note?

[erik-krogh]

Hmm.
The javascript/ql/test/library-tests/frameworks/Templating/Xss.qlref and javascript/ql/test/library-tests/frameworks/Templating/test.ql tests are failing in the CI.
Can you look into that?

I did some evaluations. They look fine and were very uneventful.

[jorgectf]

Hmm. The javascript/ql/test/library-tests/frameworks/Templating/Xss.qlref and javascript/ql/test/library-tests/frameworks/Templating/test.ql tests are failing in the CI. Can you look into that?

I did some evaluations. They look fine and were very uneventful.

I haven't been able to figure out a fix for the error. I think it has to be with the internal test, which I'm not familiar with.

[erik-krogh]

I haven't been able to figure out a fix for the error. I think it has to be with the internal test, which I'm not familiar with.

It's because you have to compile the extractor with your changes to see the changes to the tests.
I've done that and pushed the updated test outputs here.

I think those outputs are what we should expect from your changes?

[jorgectf]

I haven't been able to figure out a fix for the error. I think it has to be with the internal test, which I'm not familiar with.

It's because you have to compile the extractor with your changes to see the changes to the tests. I've done that and pushed the updated test outputs here.

I think those outputs are what we should expect from your changes?

Yes, the expected results LGTM, thank you!

[erik-krogh]

Great.
LGTM 👍

@asgerf: Do you have any comments?

[asgerf]

Perhaps a final evaluation, otherwise LGTM, and thanks for driving this to completion!

[erik-krogh]

Perhaps a final evaluation, otherwise LGTM, and thanks for driving this to completion!

Final evaluations (see backrefs) were very uneventful.
They were on nightly and default, and with building the extractor from github/codeql.

I'm merging.