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

fix: [M3-8424] - Fix CodeQL alerts for DOM text reinterpreted as HTML #11008

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

carrillo-erik
Copy link
Contributor

@carrillo-erik carrillo-erik commented Sep 25, 2024

Description 📝

This PR attempts to reproduce and resolve the CodeQL code scan alerts for DOM text reinterpreted as HTML. Alternatively, these alerts might be determined to be false positives and will be dismissed as so.

Changes 🔄

List any change relevant to the reviewer.

  • The changes proposed by this PR were generated by GitHub's Copilot Autofix feature.

Target release date 🗓️

10/16/2024

How to test 🧪

Prerequisites

(How to setup test environment)

  • Install the CodeQL extension on VSCode
  • Install the CodeQL CLI tool using this guide
    • Once you reached the Create a database of your code with CodeQL step, cd into the src/ directory of the manager package and execute the codeql database create codeqldb --language=javascript command.
    • This is the last step you'll follow on this guide.
  • In a separate VSCode window, open the codeql-repo (this is one of the directories created using the linked guide).
  • Click on the CodeQL icon found in the VSCode sidebar.
  • Select the DataBase created from executing the codeql database create codeqldb --language=javascript command. (This DataBase will be located in the src/ directory of the manager package.

Verification steps

(How to verify changes)

  • In the Queries tab, navigate to the javascript/ql/src/Security/ directory.
  • Under CWE-079 locate the XssThroughDom.ql query and run it by clicking the play button.
    • Verify that running the query returns 0 results.
  • Under CWE-116 run all the queries by clicking the play button at the directory level.
    • Verify that running the query returns 0 results.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@carrillo-erik carrillo-erik added the Security Pull requests that address a security vulnerability label Sep 25, 2024
@carrillo-erik carrillo-erik self-assigned this Sep 25, 2024
@carrillo-erik carrillo-erik requested a review from a team as a code owner September 25, 2024 20:55
@carrillo-erik carrillo-erik requested review from hkhalil-akamai and coliu-akamai and removed request for a team September 25, 2024 20:55
Copy link

github-actions bot commented Sep 25, 2024

Coverage Report:
Base Coverage: 86.98%
Current Coverage: 86.98%

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

I may have my local CodeQL configured incorrectly but I'm not seeing any results when scanning the selected vulnerabilities either before or after this change. Still, reviewing the changes, I believe they are sensical and will cause no harm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was the vulnerability detected in this file a false alarm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes didn't really make sense to include as part of the PR because they didn't see to accomplish anything different than the existing code, so I removed them. The change made in the sessions.ts file actually made sense and I decided to leave it.

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

In the same boat as Hussain - encodeURIComponent makes sense to me. Will the other alerts be dismissed as false positives?

@carrillo-erik
Copy link
Contributor Author

In the same boat as Hussain - encodeURIComponent makes sense to me. Will the other alerts be dismissed as false positives?

My hope is that by merging this small change, we should see if the alert goes away or at least trigger a new alert, which may provide more context. I will check with John and Jaalah before I take an action with respect to handling the alert on the GitHub page.

@carrillo-erik carrillo-erik merged commit 8a771ca into linode:develop Sep 27, 2024
19 of 20 checks passed
Copy link

cypress bot commented Sep 27, 2024

Cloud Manager E2E    Run #6582

Run Properties:  status check failed Failed #6582  •  git commit 8a771ca548: fix: [M3-8424] - Fix CodeQL alerts for `DOM text reinterpreted as HTML` (#11008)
Project Cloud Manager E2E
Branch Review develop
Run status status check failed Failed #6582
Run duration 27m 17s
Commit git commit 8a771ca548: fix: [M3-8424] - Fix CodeQL alerts for `DOM text reinterpreted as HTML` (#11008)
Committer carrillo-erik
View all properties for this run ↗︎

Test results
Tests that failed  Failures 1
Tests that were flaky  Flaky 2
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 407
View all changes introduced in this branch ↗︎

Tests for review

Failed  cypress/e2e/core/linodes/clone-linode.spec.ts • 1 failed test

View Output Video

Test Artifacts
clone linode > can clone a Linode from Linode details page Screenshots Video
Flakiness  linodes/rebuild-linode.spec.ts • 1 flaky test

View Output Video

Test Artifacts
rebuild linode > cannot rebuild a provisioning linode Screenshots Video
Flakiness  placementGroups/delete-placement-groups.spec.ts • 1 flaky test

View Output Video

Test Artifacts
Placement Group deletion > can unassign Linode when unexpected error show up and reopen the dialog Screenshots Video

@abailly-akamai
Copy link
Contributor

@carrillo-erik This PR is being reverted. Not only did it not fix the dependabot warnings but it introduced a bad bug - see #11017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants