[go: nahoru, domu]

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

codeql-action/upload-sarif@v1 doesn't accept empty sarif #390

Open
ghost opened this issue Feb 12, 2021 · 4 comments
Open

codeql-action/upload-sarif@v1 doesn't accept empty sarif #390

ghost opened this issue Feb 12, 2021 · 4 comments

Comments

@ghost
Copy link
ghost commented Feb 12, 2021

Expected behaviour:
No error

Actual behaviour:
codeql-action/upload-sarif@v1 doesn't accept empty sarif

Exemple:

{
  "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json",
  "version": "2.1.0",
  "runs": []
}

Result:

Uploading sarif files: ["final.sarif"]
Uploading results
Error: Invalid request.

1 item required; only 0 were supplied.
RequestError [HttpError]: Invalid request.

1 item required; only 0 were supplied.
    at /home/runner/work/_actions/github/codeql-action/v1/node_modules/@octokit/request/dist-node/index.js:66:23
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async uploadPayload (/home/runner/work/_actions/github/codeql-action/v1/lib/upload-lib.js:60:22)
    at async uploadFiles (/home/runner/work/_actions/github/codeql-action/v1/lib/upload-lib.js:217:5)
    at async Object.uploadFromActions (/home/runner/work/_actions/github/codeql-action/v1/lib/upload-lib.js:91:12)
    at async run (/home/runner/work/_actions/github/codeql-action/v1/lib/upload-sarif-action.js:34:29)
    at async runWrapper (/home/runner/work/_actions/github/codeql-action/v1/lib/upload-sarif-action.js:46:9) {
  name: 'HttpError',
  status: 422,
  headers: {
    'access-control-allow-origin': '*',
    'access-control-expose-headers': 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, Deprecation, Sunset',
    connection: 'close',
    'content-length': '123',
    'content-security-policy': "default-src 'none'",
    'content-type': 'application/json; charset=utf-8',
    date: 'Fri, 12 Feb 2021 20:35:44 GMT',
    'referrer-policy': 'origin-when-cross-origin, strict-origin-when-cross-origin',
    server: 'GitHub.com',
    'strict-transport-security': 'max-age=31536000; includeSubdomains; preload',
    vary: 'Accept-Encoding, Accept, X-Requested-With',
    'x-content-type-options': 'nosniff',
    'x-frame-options': 'deny',
    'x-github-media-type': 'github.v3; format=json',
    'x-github-request-id': '07C1:040D:2E46AC:90C8F9:6026E6A0',
    'x-ratelimit-limit': '500',
    'x-ratelimit-remaining': '496',
    'x-ratelimit-reset': '1613162249',
    'x-ratelimit-used': '4',
    'x-xss-protection': '1; mode=block'
  },
  request: {
    method: 'PUT',
    url: 'https://api.github.com/repos/rizinorg/rizin/code-scanning/analysis',
    headers: {
      accept: 'application/vnd.github.v3+json',
      'user-agent': 'CodeQL Action octokit-core.js/3.1.2 Node.js/12.13.1 (linux; x64)',
      authorization: 'token [REDACTED]',
      'content-type': 'application/json; charset=utf-8'
    },
    body: '{"commit_oid":"820c45f5933c343c34a94e1d2382e91980359a8f","ref":"refs/pull/610/merge","analysis_key":".github/workflows/code-analysis.yml:build","analysis_name":"Code scanning","sarif":"H4sIAAAAAAAAA6tWKkstKs7Mz1OyUjLSM9QzUNJRKirNK1ayio6tBQBSlZKzHQAAAA==","workflow_run_id":562142767,"checkout_uri":"file:///home/runner/work/rizin/rizin","environment":"{\\n  \\"name\\": \\"CodeQL-javascript\\"\\n}","started_at":"2021-02-12T20:31:48.765Z","tool_names":[],"base_ref":"refs/heads/dev","base_sha":"af5ebbb92533d1015336f2257bfe5e7dc67c2494"}',
    request: { agent: [Agent], hook: [Function: bound bound register] }
  },
  documentation_url: 'https://docs.github.com/rest'
}

The sarif file was generated by github/codeql-action/analyze@v1 and sarif-multitool.

npx @microsoft/sarif-multitool merge reports/*.sarif
hefloryd added a commit to hefloryd/osal that referenced this issue Feb 25, 2021
Detect if issues were found and upload SARIF report only in that case,
because reports without entries are not accepted. See
github/codeql-action#390.

The analyzer is run twice in order to get a SARIF report as well as an
exit status when issues are detected, as the scan-build --status-bugs
parameter doesn't work when the output format is SARIF.
hefloryd added a commit to hefloryd/osal that referenced this issue Feb 25, 2021
Detect if issues were found and upload SARIF report only in that case,
because reports without entries are not accepted. See
github/codeql-action#390.

The analyzer is run twice in order to get a SARIF report as well as an
exit status when issues are detected, as the scan-build --status-bugs
parameter doesn't work when the output format is SARIF.
hefloryd added a commit to hefloryd/osal that referenced this issue Feb 25, 2021
Detect if issues were found and upload SARIF report only in that case,
because reports without entries are not accepted. See
github/codeql-action#390.

The analyzer is run twice in order to get a SARIF report as well as an
exit status when issues are detected, as the scan-build --status-bugs
parameter doesn't work when the output format is SARIF.
hefloryd added a commit to hefloryd/osal that referenced this issue Feb 25, 2021
Detect if issues were found and upload SARIF report only in that case,
because reports without entries are not accepted. See
github/codeql-action#390.

The analyzer is run twice in order to get a SARIF report as well as an
exit status when issues are detected, as the scan-build --status-bugs
parameter doesn't work when the output format is SARIF.
hefloryd added a commit to rtlabs-com/osal that referenced this issue Feb 25, 2021
Detect if issues were found and upload SARIF report only in that case,
because reports without entries are not accepted. See
github/codeql-action#390.

The analyzer is run twice in order to get a SARIF report as well as an
exit status when issues are detected, as the scan-build --status-bugs
parameter doesn't work when the output format is SARIF.
@hefloryd
Copy link

I'm uploading results from clang scan-build and seeing this also when no issues are detected. As a workaround the report is only uploaded if issues were found however I suspect this would mean that fixed issues may never get flagged as closed if there are no more issues.

@robertbrignull
Copy link
Contributor

You're right that not uploading when there are no alerts will lead to alerts being left open erroneously. Code scanning decides that an alert is closed when a SARIF file is uploaded for that tool and ref that doesn't contain the alert.

I would consider what does it mean to upload an empty SARIF file. It means that you ran zero code scanning tools. If you ran a tool and it found zero alerts then there should be an entry in the runs list for that tool and then that run has an empty list of alerts. So even if uploading an empty SARIF file was allowed, then code scanning wouldn't know that clang was run but found no issues. Essentially it would tell you zero information.

I think the real problem here lies with clang or the other tools that's generating a SARIF file with an empty runs list. In the case of @microsoft/sarif-multitool I expect the reports directory doesn't contain any SARIF files so it would be good to investigate why that is.

@hefloryd
Copy link

Thanks, that makes sense. Note that I am also using sarif-multitool to convert clang individual files to a merged report. Clang produces one report per analyzed file that looks like this if there are no alerts:

{
  "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
  "runs": [
    {
      "artifacts": [],
      "columnKind": "unicodeCodePoints",
      "results": [],
      "tool": {
        "driver": {
          "fullName": "clang static analyzer",
          "language": "en-US",
          "name": "clang",
          "rules": [],
          "version": "clang version 10.0.0-4ubuntu1 "
        }
      }
    }
  ],
  "version": "2.1.0"
}

Running sarif-multitool merge on those files produces:

{
  "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json",
  "version": "2.1.0",
  "runs": []
}

so it appears information is lost. Perhaps this is an issue with the sarif-mutiltool, there are some bug reports that could be related.

The reason I am merging clang reports is that when these were uploaded using codeql-action/upload-sarif@v1, unfixed alerts would get randomly closed and reappear on the Github Security tab between runs. Merging the reports made them appear consistently. Is there a problem with the sarif output from clang? Here's an example where an alert was found:


{
  "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
  "runs": [
    {
      "artifacts": [
        {
          "length": 35831,
          "location": {
            "uri": "file:///home/runner/work/p-net/p-net/src/device/pf_cmrdr.c"
          },
          "mimeType": "text/plain",
          "roles": [
            "resultFile"
          ]
        }
      ],
      "columnKind": "unicodeCodePoints",
      "results": [
        {
          "codeFlows": [
            {
              "threadFlows": [
                {
                  "locations": [
                    {
                      "importance": "essential",
                      "location": {
                        "message": {
                          "text": "Value stored to 'ret' is never read"
                        },
                        "physicalLocation": {
                          "artifactLocation": {
                            "index": 0,
                            "uri": "file:///home/runner/work/p-net/p-net/src/device/pf_cmrdr.c"
                          },
                          "region": {
                            "endColumn": 7,
                            "startColumn": 7,
                            "startLine": 108
                          }
                        }
                      }
                    }
                  ]
                }
              ]
            }
          ],
          "locations": [
            {
              "physicalLocation": {
                "artifactLocation": {
                  "index": 0,
                  "uri": "file:///home/runner/work/p-net/p-net/src/device/pf_cmrdr.c"
                },
                "region": {
                  "endColumn": 7,
                  "startColumn": 7,
                  "startLine": 108
                }
              }
            }
          ],
          "message": {
            "text": "Value stored to 'ret' is never read"
          },
          "ruleId": "deadcode.DeadStores",
          "ruleIndex": 0
        }
      ],
      "tool": {
        "driver": {
          "fullName": "clang static analyzer",
          "language": "en-US",
          "name": "clang",
          "rules": [
            {
              "fullDescription": {
                "text": "Check for values stored to variables that are never read afterwards"
              },
              "helpUri": "https://clang-analyzer.llvm.org/available_checks.html#deadcode.DeadStores",
              "id": "deadcode.DeadStores",
              "name": "deadcode.DeadStores"
            }
          ],
          "version": "clang version 10.0.0-4ubuntu1 "
        }
      }
    }
  ],
  "version": "2.1.0"
}

I'll open a new issue for this once it's clear where the problem is.

@jsoref
Copy link
Contributor
jsoref commented Oct 9, 2022

Bugs

@microsoft/sarif-multitool

I've opened the above bug. It's very clear that the merge agent is buggy. Whether there are other bugs is subject to discussion.

github/codeql

From my perspective, there's a bug in github/codeql as well.

1 item required; only 0 were supplied.

Is not remotely human friendly. If you (=codeql team) require that there be at least one runs item, then the error message should clearly mention runs. That it doesn't is a bug.

github/codeql-action

Also note that while the codeql backend should provide a friendly error akin to the above, this codeql-action should be able to preprocess, detect the same error and report it without hitting the codeql endpoint (saving everyone X api calls from their github api call quota).

It's ok for rare edge cases not to be handled in wrappers like this, but this is not going to be a rare edge case. Especially when a major vendor (👋 Microsoft) is generating bogus outputs of this form and you're actively encouraging people to use your tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants