Update codeql.yml to exclude YAML front-matter and Liquid code

[gaylem]

Overview

Many of our Javascript and HTML code files cannot be scanned by CodeQL as-is because they contain non-JS Liquid code {% ... %} or YAML front matter --- ... ---, which cause syntax errors. We need to try and resolve these errors without removing all non-JS code.

Details

The error message "Could not process some files due to syntax errors" indicates that these "syntax errors" may prevent CodeQL from scanning the files below (see issue #5234 for details).

• hamburger-nav.js: YAML front-matter with a title
• toolkit.js: 1 line of Liquid, empty YAML front-matter
• wins.js : 2 lines (Liquid), empty YAML front-matter
• project.js : 2 lines (Liquid), empty YAML front-matter
• about.js: for loop (Liquid), empty YAML front-matter
• current-project.js: 2 lines + for loop (Liquid), empty YAML front-matter
• Separately, we have observed problems with CodeQL scanning of HTML with embedded liquid statements - see #6485

Screenshot: CodeQL error message

Simply deleting the Liquid lines would break the site (and CodeQL raised those errors accordingly in testing), so an alternative, holistic solution is required.

Action Items

• [x] Review the Possible Solutions content under Resources
• [x] Implement a solution that will exclude YAML front-matter and Liquid code from CodeQL scans on .js and .html files.
• [x] Thoroughly test your changes and ensure the codeql.yml file runs as expected. If it does not run as expected, detail your testing in a comment.

Testing

• [x] Test your solution by running the .codeql-scan-job.yml workflow.
• [x] You'll have to figure out a way to confirm that CodeQL was able to scan the files listed above without scanning the YAML front-matter or Liquid code and without throwing the error.

Resources/Instructions

• This issue resulted from #6378
• Hack for LA's GitHub Actions Wiki
• https://github.com/hackforla/website/issues/5234#issuecomment-1949927685

Possible Solutions

Here are two possible solutions (in order of preference) to this problem. Please use your best judgment, these are only recommendations.

Option 1

This approach is preferred because it is

• more generic and reusable
• compatible with using a CodeQL for VS Code extension (See Setting up CodeQL in Visual Studio)

Define a new CodeQL query file that excludes Liquid and YAML patterns within JavaScript files.

Create a file named exclude-patterns.ql

import javascript

from File file
where (file.getExtension() = "js" or file.getExtension() = "html")
  and not file.getCode().matches(".*\\{%.*%\\}.*") // Exclude Liquid code
  and not file.getCode().matches(".*---.*")        // Exclude YAML front matter
select file

Then modify codeql-scan-job.yml file to use the new query file for analysis. Update the queries section in the Initialize CodeQL step to include the new query file:

# On codeql-scan-job.yml file:

- name: Initialize CodeQL
     uses: github/codeql-action/init@v3
     with:
       languages: ${{ matrix.language }}
       queries: path/to/exclude-patterns.ql, security-and-quality

Option 2

Exclude liquid code and YAML front matter patterns from the CodeQL analysis within `codeql-scan-job.yml`

    # On codeql-scan-job.yml file:
    
        - name: Perform CodeQL Analysis
          uses: github/codeql-action/analyze@v3
          with:
            languages: javascript
            queries: security-and-quality
            # Exclude Liquid and YAML code within JavaScript files
            exclude: |
              path: "**/*.{js,html}"
              patterns:
                - pattern: |
                    // Start of Liquid code
                    {% if variable %}
                    // Liquid code here
                    {% endif %}
                    // End of Liquid code
                - pattern: |
                    // Start of YAML front matter
                    ---
                    # YAML front matter here
                    ---
                    // End of YAML front matter

[github-actions[bot]]

Hi @gaylem.

Please don't forget to add the proper labels to this issue. Currently, the labels for the following are missing:

• Complexity, Role, Feature, Size

NOTE: Please ignore this comment if you do not have 'write' access to this directory.

To add a label, take a look at Github's documentation here.

Also, don't forget to remove the "missing labels" afterwards. To remove a label, the process is similar to adding a label, but you select a currently added label to remove it.

After the proper labels are added, the merge team will review the issue and add a "Ready for Prioritization" label once it is ready for prioritization.

Additional Resources:

• Wiki: How to create issues
• Wiki: How to read and interpret labels

[gaylem]

FYI, I'm going to chat with @roslynwythe about this soon to confirm if I did it correctly.

[roslynwythe]

@gaylem Thank you for writing this issue and for proposing some promising solutions to enable us to exclude liquid and yaml code from the CodeQL scan.

@gaylem @ExperimentsInHonesty There is another factor we need to consider when selecting an approach for this ER; that is, we are planning to ask developers to run CodeQL locally using the "CodeQL for VS Code" extension. I have been holding off on writing an issue to configure and document use of "CodeQL for VS Code" extension, until we had a solution to the problem of excluding liquid and yaml code. Ideally in this ER we will select an approach that can be utilized in the extension as well as in the GitHub workflow. According to the documentation Setting up CodeQL in Visual Studio there are options for such customization, but the setup is a bit complex.

I would like to hear your thoughts on this idea:

1. Let's remove mention of the third option ("adding a build step to the codeql.yml file to run the Jekyll build before the analyze step is triggered") since it would not be compatible with local testing, and it sounds like it won't be effective anyway.
2. Let's add an Action item to setup "CodeQL for VS Code" and explore both the remaining options you describe for excluding liquid and yaml code, with the requirement that they can be utilized in the extension as well as in the GitHub workflow

[gaylem]

Hey @roslynwythe, I'm good with removing the third option. I don't know much about CodeQL for VS Code but if it's agreed that we want that in here as well, I'll be happy to investigate and add it.

[ExperimentsInHonesty]

@gaylem thank you for putting so much work into this issue. It is written very clearly! Here are a few notes for you and @roslynwythe

• It sounds like we are still discussing possible solutions and that should be happening over in the Epic https://github.com/hackforla/website/issues/6378, which this issue seems to be replacing.
• The point of the Epics is to break down the work into smaller issues. This issue has three issues in it, each of which should be done serially until we find a solution we are satisfied with (that is done by creating the issues and then adding the ones that are not ready to be done yet, to the icebox with a dependency on the prior one being unsuccessful in achieving our goal).
  1. Define a new CodeQL query file that excludes Liquid and YAML patterns within JavaScript files.
  2. Exclude liquid code and YAML front matter patterns from the CodeQL analysis within codeq.yml
  3. Add a build step to the codeql.yml file to run the Jekyll build before the analyze step is triggered. (NOTE: I don't think this will work because CodeQL will still analyze the JavaScript files, including any Liquid code or YAML front matter within them, unless explicitly excluded.)

I am open to being wrong about the need to break these into smaller issues, and if that is the case, please explain in a comment.

[gaylem]

Hi @ExperimentsInHonesty, thank you for your feedback!

@roslynwythe and I met to discuss this issue today. Here's where we landed:

• We removed option 3 (run Jekyll build before analyze step) because it likely won't work.
• The other two items are just suggestions for how a developer might solve the problem, and the only way to find out if they will work is to actually implement a solution. So I think they should just be in this story. I updated the description to make that more clear.

As for the epic (#6378), Roslyn wants to discuss it in a meeting to see what you think.

• My suggestion was that we repurpose that epic as a place to consolidate all of the CodeQL recommendations that seem to be floating around.
• For example, Will made some suggestions on #6503, and Roslyn as a few items that need to be addressed, and we can raise additional issues for those and add them as dependencies.
• Roslyn also has a couple of additional CodeQL epics (#5005 and #5159). However, they seem to be somewhat separate as they are tracking the individual alerts, rather than making higher-level improvements to the workflow.

At any rate, this seems like this is a good chance to consolidate things better. I'm still getting a feel for how you organize things so I'm open to guidance and would love to discuss more in a meeting as well if there's time.

Thanks again! 😄

[roslynwythe]

• @gaylem @ExperimentsInHonesty I reworked #5005 so it addresses CodeQL deployment in general and mention of specific issues only appears in #5159. So I could use #5005 to address all the other issues, and this one can be closed.

[ExperimentsInHonesty]

@roslynwythe

• There is a comment in https://github.com/hackforla/website/issues/5159#issuecomment-1996856071 that suggests we close #5005.

  At this point, I think that https://github.com/hackforla/website/issues/5005 can be closed, do you agree?

• Then there is the comment you left above that seems like you are saying that you are now tracking everything in 5005 and that you want to close it. But that interpretation depends on what you meant by "this one can be closed". Perhaps you didn't realize you were in 5005 when posted the comment.

  I reworked https://github.com/hackforla/website/issues/5005 so it addresses CodeQL deployment in general and mention of specific issues only appears in https://github.com/hackforla/website/issues/5159. So I could use https://github.com/hackforla/website/issues/5005 to address all the other issues, and this one can be closed.

There seems like there is content in this issue now that does not exist in the other issues. So I think this just has to wait until we can have a three-way meeting with @gaylem you and me.

[github-actions[bot]]

Hi @aidanwsimmons, thank you for taking up this issue! Hfla appreciates you :)

Do let fellow developers know about your:- i. Availability: (When are you available to work on the issue/answer questions other programmers might have about your issue?) ii. ETA: (When do you expect this issue to be completed?)

You're awesome!

P.S. - You may not take up another issue until this issue gets merged (or closed). Thanks again :)

[aidanwsimmons]

Availability: weekday afternoons ETA: 5/9/24

[ExperimentsInHonesty]

@aidanwsimmons I noticed the closed PR. How are things going on this issue. Do you need some help...

Please provide update

1. Progress: "What is the current status of your project? What have you completed and what is left to do?"
2. Blockers: "Difficulties or errors encountered."
3. Availability: "How much time will you have this week to work on this issue?"
4. ETA: "When do you expect this issue to be completed?"
5. Pictures or links* (if necessary): "Add any pictures or links that will help illustrate what you are working on."

• remember to add links to the top of the issue if they are going to be needed again.

p.s., I added a ready for product label to this issue, so I can check back in with you.

[aidanwsimmons]

1. Progress: I created a new query file to exclude YAML front matter and Liquid code when scanning js and html files. I added the new query file to the queries list in the Initialize CodeQL step in codeql-scan-job-yml.
2. Blockers: Running the codeql.yml workflow produces the error message: "Could not process some files due to syntax errors" for the same files mentioned in the issue body. See screenshot of logs below. Upon inspecting logs- it seems as though the query file is not correctly filtering YAML front matter or Liquid code. Ive tried adjusting my query file to account for additional white space / new lines, but this has produced the same error message for the same files.
3. Availability: all day 5/10, afternoons 5/11 onwards
4. ETA: I hope to find a solution by the end of Sunday, but if I cant make further progress- im happy to pass the issue off to someone else
5. Pictures:

[aidanwsimmons]

Update- I have been unable to implement a query file for the codeql scanning process that can exclude YAML front matter and Liquid code blocks. Unassigning myself from the issue as I am unfortunately too busy this week to dedicate the proper time to this issue. Im attaching a list of other possible methods to resolve the issue(though i have not assessed each of their viability.

Selective Scanning: Instead of trying to scan the entire file as JavaScript, you can selectively scan only the JavaScript portions. You can achieve this by programmatically separating JavaScript code blocks from Liquid or YAML blocks.

Escaping: You can use escaping mechanisms to prevent the Liquid or YAML code from being interpreted as part of the JavaScript syntax. For example, you might wrap Liquid or YAML code blocks within comments or strings to ensure they are ignored by the JavaScript parser.

Preprocessing: Before scanning the files with CodeQL, you can preprocess them to temporarily remove or comment out the Liquid or YAML code. Once the scanning is complete, you can restore the removed sections.

Custom Parsing: Develop a custom parser that can handle both JavaScript and Liquid/YAML syntax within the same file. This approach might be more complex but allows for a more seamless integration of different code types.

Plugin/Extension: Check if there are any existing plugins or extensions for CodeQL or other static analysis tools that support mixed-language parsing or can be extended to support it.

[roslynwythe]

@aidanwsimmons thank you for providing a list of possible solutions. I did explore the "Escaping" solution, and I saw that some tools such as ESLint do support excluding portions of code via specailly formatted JS comments, but I couldn't find anything in CodeQL documentation to indicate support for such a solution. Are you suggesting that in codeql-scan-job.yml, we could define such a specially formatted comment? Regarding the "Selective Scanning" solution you listed, are you suggesting that we break up the javascript into modules and exclude the modules that contain YAML/liquid code from scanning in codeql-scan-job.yml?

[gaylem]

@roslynwythe if a programmatic solution seems elusive, could we just dismiss the alerts manually?

https://docs.github.com/en/enterprise-cloud@latest/code-security/code-scanning/managing-code-scanning-alerts/managing-code-scanning-alerts-for-your-repository#dismissing-alerts

[roslynwythe]

@roslynwythe if a programmatic solution seems elusive, could we just dismiss the alerts manually?

https://docs.github.com/en/enterprise-cloud@latest/code-security/code-scanning/managing-code-scanning-alerts/managing-code-scanning-alerts-for-your-repository#dismissing-alerts

@gaylem It is possible to dismiss the alerts as false positives, however based on the error message "Could not process some files due to syntax errors", I'm concerned that CodeQL might stop scanning the file when it encounters non-JS code, and so we could be missing some test coverage. We've seen in some HTML files, that CodeQL will create an alert only on one instance of problematic markup and then seem to ignore a later instance of similar markup. But this is just my supposition, not the result of testing. We could create an issue for someone to test this out.

[gaylem]

@roslynwythe I did a little testing out of curiosity:

Liquid Code:

1. I dismissed alert 63 (Liquid code): https://github.com/gaylem/hackforla-website/security/code-scanning/63

2. Then added more liquid code to the file: https://github.com/gaylem/hackforla-website/blob/codeql-test/assets/js/utility/vrms-events.js

3. I ran the scan, and it didn’t throw an alert for the new liquid code

4. Then I added an unused global variable and it threw this error and didn’t highlight the liquid code it caught before: https://github.com/gaylem/hackforla-website/security/code-scanning/65

YAML Front Matter:

1. I dismissed alert 13 (YAML front matter): https://github.com/gaylem/hackforla-website/security/code-scanning/13

2. Then I added an unused global variable to this file: https://github.com/gaylem/hackforla-website/edit/codeql-test/assets/js/hamburger-nav.js

3. I ran the codeql workflow a couple of times and it threw these alerts, but they weren’t for the variable I added. They were the same YAML front matter code.:

   • https://github.com/gaylem/hackforla-website/security/code-scanning/66
   • https://github.com/gaylem/hackforla-website/security/code-scanning/67

Conclusion: So it seems like Liquid code will be ignored and the file will still be scanned for other kinds of errors, but YAML front matter alerts will be recreated if the original alert is dismissed. In both cases, it does appear that after dismissal, the files are still being scanned.

It would probably be a good idea if one or two people tested this out, too.

[roslynwythe]

Thank you @gaylem for running these tests. This is very useful! I'm not able to view the links into your repository (404 error), but based on your explanation I have a question on point 3 under YAML Front Matter": If CodeQL failed to throw alerts for the unused variable in hamburger-nav.js, doesn't that indicate that it is not scanning the full code?

[gaylem]

@roslynwythe Step three of the YAML Front Matter section did throw errors, but only on the front matter alert code that I had previously dismissed. So it did scan the file, though I'm not sure why it didn't pick up the variable. Maybe the scan stopped at the front-matter when it was detected? Here's what I added (test2):

---
title: hamburger nav file
---
 
const test2 = "test" 

// Retrieve the SVG element
const svgElement = document.querySelector('#burgerImage svg');

[gaylem]

@roslynwythe Here are images of all the alerts since you can't see them:

[roslynwythe]

@gaylem I repeated the YAML test in my repo and my results were the same. After dismissing the initial CodeQL alert on the frontmatter in hamburger-nav.js, I found:

• CodeQL did not raise an alert on the unused variable, and
• In the workflow log, there was still an "Extraction error" displayed for hamburger-nav.js and hamburger-nav.js was listed under "Could not process some files due to syntax errors"

so to me it sounds like CodeQL is not able to successfully scan code past the "error" that occurs with the frontmatter, and we shouldn't dismiss those alerts.

[roslynwythe]

I did have a suggestion based on a ChatGPT conversation. ChatGPT suggests that when listing queries, a "filter" type query should be listed first, before the query that scans for security and quality issues. So I would suggest switching the order in Option 1:

- name: Initialize CodeQL
     uses: github/codeql-action/init@v3
     with:
       languages: ${{ matrix.language }}
       queries: path/to/exclude-patterns.ql, security-and-quality

ChatGPT4o response

When specifying queries in a CodeQL workflow using the syntax queries: [security-and-quality, ignoreYamlAndLiquid.ql], the order in which you list the queries does matter.

Here's the reason:

Execution Order

• Order of Queries: CodeQL processes the queries in the order they are listed. This means that if you have a query that modifies or filters out certain results (like ignoreYamlAndLiquid.ql), it should generally be listed first. This ensures that subsequent queries (like those from security-and-quality) operate on the already filtered set of code.
• Chaining Effects: If ignoreYamlAndLiquid.ql is designed to filter out certain lines of code before running standard security and quality checks, it should be executed before security-and-quality. This ensures the filtering takes place first, and the standard checks are performed only on the relevant code.

Correct Order

Given this, the correct order would be to list ignoreYamlAndLiquid.ql first, followed by security-and-quality:

queries:
  - ignoreYamlAndLiquid.ql
  - security-and-quality

Example Workflow Configuration

Here’s an example of how you might specify this in a CodeQL workflow file (e.g., .github/workflows/codeql-analysis.yml):

name: "CodeQL"

on:
  push:
    branches: [ main ]
  pull_request:
    # The branches below must be a subset of the branches above
    branches: [ main ]
  schedule:
    - cron: '0 0 * * 0'

jobs:
  analyze:
    name: Analyze
    runs-on: ubuntu-latest
    steps:
      - name: Checkout repository
        uses: actions/checkout@v2

      - name: Initialize CodeQL
        uses: github/codeql-action/init@v1
        with:
          languages: javascript
          queries: |
            ./path/to/ignoreYamlAndLiquid.ql
            security-and-quality

      - name: Autobuild
        uses: github/codeql-action/autobuild@v1

      - name: Perform CodeQL Analysis
        uses: github/codeql-action/analyze@v1

In this configuration:

1. Checkout repository: Checks out the code repository.
2. Initialize CodeQL: Initializes the CodeQL analysis with the custom query ignoreYamlAndLiquid.ql followed by the standard security-and-quality queries.
3. Autobuild: Builds the project (necessary for CodeQL to understand the code structure).
4. Perform CodeQL Analysis: Runs the CodeQL analysis using the specified queries.

By ensuring that ignoreYamlAndLiquid.ql runs before security-and-quality, you ensure that your custom filtering logic is applied before the standard security and quality checks are performed.

[roslynwythe]

To summarize the activity so far on this issue:

• The first attempt to implement Option 1 or 2 above was not successful, but suggestions for alternative solutions were provided in https://github.com/hackforla/website/issues/6548#issuecomment-2106501961
• A change to Option1 has been made as described in https://github.com/hackforla/website/issues/6548#issuecomment-2121930484
• We determined that if we dismiss alerts caused by YAML front matter, CodeQL will not scan the remainder of the file

Due to the recent refactoring of CodeQL.yml, references to "CodeQL.yml" in this issue have been changed to "codeql-scan-job.yml"

[github-actions[bot]]

Hi @luisitocanlas, thank you for taking up this issue! Hfla appreciates you :)

Do let fellow developers know about your:- i. Availability: (When are you available to work on the issue/answer questions other programmers might have about your issue?) ii. ETA: (When do you expect this issue to be completed?)

You're awesome!

P.S. - You may not take up another issue until this issue gets merged (or closed). Thanks again :)

[luisitocanlas]

Updated Availability: 6/11 ~ 6/28 New ETA: EOD 6/28

Current state:

• added ./codeql-queries/exclude-patterns.ql following option 1 above.
• added the path of exclude-patterns.ql to the queries at line 48 in ./.github/workflows/codeql-scan-job.yml
• when manually running the workflow using my branch (update-codeql-6548), there are no errors and CodeQL scan is successful

Blocker: Figuring out a way to confirm that CodeQL was able to scan the files listed above without scanning the YAML front-matter or Liquid code and without throwing the error.

What I've done so far for the blocker:

• created local codeql database
• figuring out what files/tools are needed to successfully analyze the codeql database
• currently trying to have a successful codeql database analyze

[luisitocanlas]

Progress update: Below are the solutions that I've tried and the errors that kept flagging. I would have to say that I did not find a solution and would be moving this issue back to the Prioritized Backlog.

Solution # 1:

Option 1 under the possible solutions that is slightly modified.

import javascript

from File file
where (file.getExtension() = "js" or file.getExtension() = "html")
  and not file.getCode().matches(".*\\{%-?\\s*[a-zA-Z]+.*%\\}.*") // Exclude Liquid code
  and not file.getCode().matches("(?s).*---.*---.*") // Exclude YAML front matter
select file

Solution # 2:

Using predicates.

/**
 * @name Exclude YAML and Liquid Front Matter
 * @description Excludes YAML front matter and Liquid template sections from the analysis
 * @kind problem
 * @problem.severity warning
 */

 import javascript

 /** Predicate to identify YAML front matter lines */
 predicate isYamlFrontMatterLine(File f, int line) {
   exists (
     int start, int end |
     start = f.getLine(1).getLineNumber() and
     (end = f.getLine(2).getLineNumber() or end = f.getLine(3).getLineNumber()) and
     line >= start and
     line <= end and
     f.getLine(start).getText().matches("---") and
     f.getLine(end).getText().matches("---")
   )
 }
 
 /** Predicate to identify Liquid template sections */
 predicate isLiquidTemplateLine(File f, int line) {
   exists (
     string content |
     f.getLine(line).getText() = content and
     (
       content.matches("{%.*%}") or
       content.matches("{{.*}}")
     )
   )
 }
 
 /** Class to represent code excluding YAML front matter and Liquid templates */
 class CodeExcludingFrontMatter extends Expr {
   CodeExcludingFrontMatter() {
     this.getFile().getExtension() = "js" and
     not isYamlFrontMatterLine(this.getFile(), this.getLocation().getStartLine()) and
     not isLiquidTemplateLine(this.getFile(), this.getLocation().getStartLine())
   }
 }
 
 from CodeExcludingFrontMatter c
 select c, "Code excluding YAML front matter and Liquid templates"

Error 1: When the first solution was implemented, this error keeps popping up. When the second solution was implemented this error didn't pop up.

Inside Perform CodeQL Analysis Log

Error 2: For both solutions, the alerts are persistent.

Under Security Tab inside Code scanning alerts

[roslynwythe]

@ExperimentsInHonesty @t-will-gillis Here is a summary of our options:

• re-prioritize the issue as-is. I still think it makes sense and another developer may have success with it
• we can experiment with moving liquid statements from JS into HTML
• we can deploy another code security/quality linter such as ESLint which features the ability to exclude particular lines of "code"
• we can try to modify the workflow such that CodeQL analyzes code after the Jekyll build. This article discusses such a pattern https://some-natalie.dev/blog/codeql-container-builds/ but it involves using a container job, and I'm not sure if we are comfortable with those.

[HackforLABot]

Hi @duojet2ez, thank you for taking up this issue! Hfla appreciates you :)

Do let fellow developers know about your:- i. Availability: (When are you available to work on the issue/answer questions other programmers might have about your issue?) ii. ETA: (When do you expect this issue to be completed?)

You're awesome!

P.S. - You may not take up another issue until this issue gets merged (or closed). Thanks again :)

[duojet2ez]

I look forward to tackling this issue! It seems to be a challenge but also presents a good learning opportunity.

This week 8/26 I have time Mon - Thurs and then will be away labor day weekend and back the following week. My eta on this is not known at this point. I plan on reviewing the previous work on this/looking at comments.. after that I may have a better estimate.

[t-will-gillis]

Hey @duojet2ez We talked about this issue in our Monday meeting... If you look at @roslynwythe 's note above, we would like to focus most of your efforts on the last one:

we can try to modify the workflow such that CodeQL analyzes code after the Jekyll build. This article discusses such a pattern https://some-natalie.dev/blog/codeql-container-builds/ but it involves using a container job, and I'm not sure if we are comfortable with those.