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

SARIF report upload fails because of deleted file #1702

Closed
wesley-dean-flexion opened this issue Aug 3, 2022 · 5 comments · Fixed by #1698
Closed

SARIF report upload fails because of deleted file #1702

wesley-dean-flexion opened this issue Aug 3, 2022 · 5 comments · Fixed by #1698
Labels
bug Something isn't working

Comments

@wesley-dean-flexion
Copy link
Contributor

Describe the bug

TL;DR:
Megalinter's SARIF reporter was failing due to matching a file that no longer existed.

Longer version

 - Image Creation Date: 2022-08-01T06:04:12Z
 - Image Revision: b1e3b0eee
 - Image Version: v6

The SARIF reporter was failing saying:

Unable to upload "megalinter-reports/megalinter-report.sarif" as it is not valid SARIF: - instance.runs[2].results[3].locations[0].physicalLocation.region.endColumn must have a minimum value of 1

Exploring the SARIF file from the build artifact revealed:

{
  "locations": [
    {
      "physicalLocation": {
        "artifactLocation": {
          "uri": "path/to/filename.ext"
        },
        "region": {
          "endColumn": 0,
          "endLine": 6,
          "snippet": {
            "text": "[...snip...]"
          },
          "startColumn": 11,
          "startLine": 6
        }
      }
    }
  ]

As reported, endColumn was less than 1. (tangentially, it was also less than startColumn)

Digging deeper, the thing it found was in a file committed some time ago and subsequently removed. This made finding the issue more fun because the file structure had been changed such that the physicalLocation.artifactLocation.url no longer pointed to anything.

I was able to work around the issue by adding the following to .megalinter.yml configuration file:

REPOSITORY_GITLEAKS_ARGUMENTS: --no-git

That is, only scan the files and directories that are present in the local repository and not scan the git history..

This is less good. However, once a given commit is scanned and found to be ok, the only time that would change is it the heuristics / patterns in the tool were updated. So, while it's less good, the degree is mitigated to some degree. Let's hope our developers don't get more creative in embedding secrets into their code.

To Reproduce
Steps to reproduce the behavior:

  1. merge a commit with a file with a secret in it
  2. rename that file (git mv), push, and merge that commit
  3. come to one's senses and setup Megalinter
  4. enable SARIF_REPORTER
  5. allow Megalinter to run repository/gitleaks

Expected behavior

If a file is not present in the local repository, so not include a reference to that file in a SARIF report.

Screenshots

https://github.com/IMLS/estimating-wifi/actions/runs/2791588499

Additional context

A workaround is to inhibit gitleaks from looking through the repository's history and only focus on the files present.

Alternatively, it may be diagnostically interesting to note that in this instance, endColumn was 0; perhaps one can filter out entries where .runs[].results[].locations[].physicalLocation.region.endColumn == 0 either when writing the report or using jq as a postprocessor..?

I've worked around this so our builds are now...building, so there's no immediate action requested.

@wesley-dean-flexion wesley-dean-flexion added the bug Something isn't working label Aug 3, 2022
@wesley-dean-flexion
Copy link
Contributor Author

Also, the fetch-depth for the checkout action (for running via GitHub) doesn't affect this -- even setting to 1 doesn't affect whether or not gitleaks scans the git history or not. I've seen FAQ entries and issues about how configuration in .megalinter.yml doesn't affect the tooling. This is that. (included so folks coming afterwards can, hopefully, avoid going down that path like I did)

@nvuillam
Copy link
Member

nvuillam commented Aug 3, 2022

Some linters produce invalid SARIF, so I already added some refactoring of SARIF generated file so Github accepts it, it seems I forgot one !

It's done here ->

def fix_sarif_physical_location(self, physical_location):

I just add to add also such checks for region property... will do that someday, or you can do it if you have some time :)

@wesley-dean-flexion
Copy link
Contributor Author

I'll be happy to help! However, it will be a while before I get there. I would support putting this in the backest of backlogs, the coldest of iceboxes..

Also, I'm not surprised in the slightest that you've a brilliant response.

@nvuillam
Copy link
Member

nvuillam commented Aug 3, 2022

Haha you flatterer ^^
Ok it works, and thanks to your nice sponsoring I have 2 free beers by months... the fix is on its way :)

@nvuillam
Copy link
Member

nvuillam commented Aug 3, 2022

@wesley-dean-flexion please can you try again with oxsecurity/megalinter:beta ? ( in about 30 mn when beta release job will be completed) -> https://github.com/oxsecurity/megalinter/actions/runs/2792887110

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants