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 suppressions lack justification #4361

Open
2 tasks done
TheJCAB opened this issue Apr 29, 2024 · 2 comments · May be fixed by #4362
Open
2 tasks done

CodeQL suppressions lack justification #4361

TheJCAB opened this issue Apr 29, 2024 · 2 comments · May be fixed by #4362
Assignees

Comments

@TheJCAB
Copy link
Contributor

TheJCAB commented Apr 29, 2024

Description

In this repo there are a handful of old-style "lgtm" suppressions for issues raised by CodeQL. As we use it at Microsoft in internal tooling for Time Travel Debugging, this code goes through internal security scanning, and this issue was raised up with these suppressions.

The policy that was violated requires having a justification on all such suppressions. This justification needs to use the new format \\ CodeQL[id] Justification goes here which needs to be in its own line just above the line that had the issue. And the justification needs to be at least 25 characters long.

The issue we're having is specifically with lexer.hpp:

    token_type scan_number()  // lgtm [cpp/use-of-goto]

A possible solution (untested as I write this - I can't run the policy scan locally):

    // CodeQL [cpp/use-of-goto] `goto` is used here safely to implement a simple lexer's winding control flow
    token_type scan_number()

That might not be the best justification. Ideally this would be provided by the person who wrote the scan_number function.

Note that I failed to find any actual documentation for the format of CodeQL suppressions. I opened an issue there too: microsoft/codeql#63

Reproduction steps

This seems to be a new internal security policy over here, so I don't particularly have repro steps to give.

Expected vs. actual results

Expected: we can use this wonderful library without triggering security policies.
Actual: we trigger a security policy.

Minimal code example

No response

Error messages

No response

Compiler and operating system

Microsoft Visual C++, Visual Studio 2022 17.10, C++20

Library version

Used via vcpkg, with commit 0ca0fe433eb70cea0d5761079c0c5b47b736565b

Validation

@nlohmann
Copy link
Owner

I searched the repo for lgtm and only found the one instance you mentioned. You mention "a handful" - what did I miss?

I'll create a PR for this. Is there anything I can add to the CI to avoid regressions?

@nlohmann nlohmann self-assigned this Apr 29, 2024
@nlohmann nlohmann linked a pull request Apr 29, 2024 that will close this issue
@TheJCAB
Copy link
Contributor Author

TheJCAB commented May 1, 2024

"You mention "a handful" -- what did I miss?" I just did a search in VSCode:

json\include\nlohmann\detail\input\lexer.hpp:
  969      */
  970:     token_type scan_number()  // lgtm [cpp/use-of-goto]
  971      {

json\single_include\nlohmann\json.hpp:
  8340      */
  8341:     token_type scan_number()  // lgtm [cpp/use-of-goto]
  8342      {

json\tests\abi\include\nlohmann\json_v3_10_5.hpp:
  7370      */
  7371:     token_type scan_number()  // lgtm [cpp/use-of-goto]
  7372      {

json\tests\thirdparty\doctest\doctest.h:
     1: // ====================================================================== lgtm [cpp/missing-header-guard]
     2  // == DO NOT MODIFY THIS FILE BY HAND - IT IS AUTO GENERATED BY CMAKE! ==

  1810              try {
  1811:                 throw; // lgtm [cpp/rethrow-no-exception]
  1812                  // cppcheck-suppress catchExceptionByValue

  3473  
  3474:         T operator=(T desired) DOCTEST_NOEXCEPT { // lgtm [cpp/assignment-does-not-return-this]
  3475              store(desired);

  6336              }
  6337:             s << Color::None; // lgtm [cpp/useless-expression]
  6338          }

json\tools\serve_header\serve_header.py:
  242  
  243: class HeaderRequestHandler(SimpleHTTPRequestHandler): # lgtm[py/missing-call-to-init]
  244      def __init__(self, request, client_address, server):

I guess it's three copies of the same thing, and then two in doctest (not yours) and Python. I guess I didn't look very closely, sorry.

"anything I can add to the CI to avoid regressions?" -- I honestly don't know. I sent a support request internally to the team handling our CodeQL scanning (it's all automated and enforced 😁), see what they suggest.

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

Successfully merging a pull request may close this issue.

2 participants