octokit / request-action

A GitHub Action to send arbitrary requests to GitHub's REST API
https://github.com/marketplace/actions/GitHub-API-Request
MIT License
369 stars 48 forks source link

ci: adds codeql-analysis.yml for codeql execution #154

Closed nickfloyd closed 2 years ago

nickfloyd commented 2 years ago

This enables codeql for this repo to meet the security and stability requirements of the octokit org.

timrogers commented 2 years ago

@nickfloyd I'm not too sure what the CodeQL alert here means in practice. Can you provide some context on that and your thinking on dismissing it?

nickfloyd commented 2 years ago

Hey @timrogers I'll break them down and give a reason for the ignore. Let me know your thoughts on if we should then try to track them down or if the ignores seem reasonable... I am up for either!

So the first one is complaining about incomplete escaping

This is typically flagged when in a regex the /g qualifier is not used - it's complaining about the code below which clearly uses the global search. Also, it's a private API being pulled in via webpack and is encapsulated in an unused module export.

/**
 * Converts a "shell expression" to a JavaScript RegExp.
 *
 * @api private
 */
function toRegExp(str) {
    str = String(str)
        .replace(/\./g, '\\.')
        .replace(/\?/g, '.')
        .replace(/\*/g, '.*');
    return new RegExp('^' + str + '$');
}

The second one was flagging on incorrect suffix check (from the code below). This one is also a result of the output from the packaging done. You'll notice the -1 at the end which does what the warning asks for - it wants to check for the index to make sure that it's !== -1. So in this case, if the methodName exists and the function name contains ".methodName" and is not equal to the index of the delta of the length of those - 1 then don't append the method name.

On further inspection, this code is only used to generate call stacks for logging and is not critical path or user accessible.

      if (methodName && functionName.lastIndexOf('.' + methodName) !== functionName.length - methodName.length - 1) {
        line += ' [as ' + methodName + ']'
      }

Those were the reasons I chose to ignore the violations - let me know if you think we should still track them down though - they are in disparate modules not present in the source itself and only referenced in and packed - so it will take some hunting.

timrogers commented 2 years ago

Sounds good to me 🏁 I just wanted to make sure we understood the alerts and had a rationale for skipping them.