Skip to content
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

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

Closed

Conversation

luisitocanlas
Copy link
Member

Fixes #6548

What changes did you make?

  • added ./codeql-queries/exclude-patterns.ql following the suggestion solution option 1, slightly modifying it.
  • added the path of exclude-patterns.ql to the queries at line 48 in ./.github/workflows/codeql-scan-job.yml

Why did you make the changes (we will use this info to test)?

Additional info:

  • I tried to figure out a way to see if the CodeQL was able to scan. Below is what I ended up doing:
    • On my forked repository, I run the CodeQL Scan workflow from my branch update-codeql-6548 with the exclude-pattern.ql inside codeql-scan-job.yml. The workflow was able to run without throwing any errors.
    • As for verifying if CodeQL was able to scan the files, I did the following steps:
      • Go to the Actions tab and click on the CodeQL Scan workflow.
      • Click on the completed job.
      • Open the Perform CodeQL Analysis log.
      • Look for the search logs and type in the js files that has either Liquid code or YAML front matter (ex. hamburger-nav.js)
      • Confirm if there are no errors associated with the files.
      • Additional step would be to look at the Security tab of the repository and clicking on the view alerts on the Code scanning alerts and see if there are any alerts thrown by the scan.

Copy link

Want to review this pull request? Take a look at this documentation for a step by step guide!


From your project repository, check out a new branch and test the changes.

git checkout -b luisitocanlas-update-codeql-6548 gh-pages
git pull https://github.com/luisitocanlas/website.git update-codeql-6548

@dcotelessa
Copy link
Member

Great work!

  • Verified the CodeQL scan results to not include the files mentioned (hamburger-nav.js)
  • No Code scanning alerts related to these changes.
  • Addition Info was helpful in testing.

dcotelessa
dcotelessa previously approved these changes Jul 1, 2024
@t-will-gillis t-will-gillis self-requested a review July 8, 2024 03:51
@t-will-gillis
Copy link
Member

available 7/7
eta: 7/7 eod

Copy link
Member

@t-will-gillis t-will-gillis left a comment

Choose a reason for hiding this comment

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

Hey @luisitocanlas Thanks for taking on this issue, and good work so far.

  • I checked out your branch locally, pushed it to my desktop, and triggered "CodeQL Scan" through a workflow_dispatch. Unfortunately, when I ran CodeQL on your branch, it generated nearly the same "Security Alerts" as currently on the HfLA main branch. That is, the code edits do not appear to change the "Security alert" results or prevent the "Syntax error(s)" due to YAML front matter or Liquid code. Example alert Let me know if you think I am missing something here.

    Screenshot 2024-07-07 215331
    Screenshot 2024-07-07 215452
  • Side note: if this route works, I would like to discuss with the team about where the exclude-patterns.ql file is located. I believe it should be in a sub-directory within github-actions, possibly github-actions/code-ql/ (or similar) rather than as current.

Thank you again for working on this, and again if you think I am missing something let me know!

@luisitocanlas
Copy link
Member Author

Hi @t-will-gillis,

I rerun the CodeQL workflow on my branch, this time the errors are back under the Perform CodeQL Analysis compared to when I first run the workflow.

Screenshot of errors

image

I'll redo the exclude-patterns.ql and try to make it work. If you have any suggestions for this problem, please let me know.

I also agree that the file should be in a sub-directory in github-actions instead of its current location. Just let me know where and what should I name the sub-directory.

Thank you!

@t-will-gillis
Copy link
Member

Hi @luisitocanlas thanks for slogging through this.

To answer the second question, github-actions/code-ql/ or similar works for me. To answer your first question, I would say to try the approaches mentioned in the original issue and anything else you might think of, and keep notes. If you do not find a solution, I think it is reasonable to say this and to give a brief summary of what you tried and what results you saw.

@luisitocanlas
Copy link
Member Author

Hi @t-will-gillis,

I moved the exclude-patterns.ql to ./github-actions/code-ql/.

I also tried something different using predicates inside exclude-patterns.ql. I waited a day before running the CodeQL workflow since last time I ran it right after pushing my updates there were no errors but after a while there were errors again.

So far, I don't see any errors under Perform CodeQL Analysis.

For the Syntax error under Security Alerts, does it take time for it to be removed or appear there?

The screenshot below says that the error appeared on the branch on Jun 13.

Screenshot

image

Thank you again!

@t-will-gillis t-will-gillis self-requested a review July 22, 2024 22:44
Copy link
Member

@t-will-gillis t-will-gillis left a comment

Choose a reason for hiding this comment

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

Hi @luisitocanlas - I copied your branch and made sure that no "Security" alerts were showing, and then I ran "codeql-scan-job" manually. The result is that CodeQL found 16 alerts, including the same 7 "Could not process some files due to syntax errors".

Screenshot 2024-07-22 154757

I could be wrong in interpreting this but I believe that CodeQL is still flagging the Liquid syntax as new errors, that is not just re-stating the errors it found on previous scans.

Comparing your scan job with the one I ran on my repo, I can see that your job did not find errors but that mine did. I don't know why that would be the case. Have you set up "Security" on your repo? Just curious....

@t-will-gillis
Copy link
Member

@luisitocanlas Also want to reiterate:

I think it is reasonable to say that you did not find a solution and to give a brief summary of what you tried and what results you saw.

@t-will-gillis
Copy link
Member

Hey @luisitocanlas Checking in with you to see your thoughts about this issue- are you still working on it? Last messaging I said that I believe CodeQL is still flagging the Liquid syntax, however if you believe your changes are correct we/you can ask others to review this PR as well.

Please let us know how you would like to proceed-

@t-will-gillis
Copy link
Member

Hi @luisitocanlas I just noticed that you unassigned from #6548 and left some notes there- thank you. I will discuss this issue with Bonnie and Roslyn to see how we want to proceed, so we may ask for your thoughts too.

But again, thanks for taking on this likely frustrating issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complexity: Large Feature: Code Alerts role: back end/devOps Tasks for back-end developers size: 3pt Can be done in 13-18 hours
Projects
Development

Successfully merging this pull request may close these issues.

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