test(dang-workflows-remediation): create initial tests

[diogoteles08]

Tests are covering the overall run of the dangerous workflow script injection fix -- inputs as the whole workflow and outputs as the fixed workflow.

[pnacht]

Move inputs and outputs to dedicated files, and only have the filepaths here. This is already done for the DangerousWorkflow check, for example:

https://github.com/joycebrum/scorecard/blob/main/checks/raw/dangerous_workflow_test.go#L110

I believe this:

• makes it easier to visually inspect the "diffs" between the files (using VSCode or git diff, for example)
• makes it easier to visually "parse" the individual files, since we can use VSCode's syntax highlighting
• makes the test logic more legible, since impl_test goes from "90% examples" to "90% logic".

You can also store only the input filename, and the expected output filename is simply "fixed_{input}" or something.

[diogoteles08]

Move inputs and outputs to dedicated files, and only have the filepaths here. This is already done for the DangerousWorkflow check, for example:

https://github.com/joycebrum/scorecard/blob/main/checks/raw/dangerous_workflow_test.go#L110

I believe this:

• makes it easier to visually inspect the "diffs" between the files (using VSCode or git diff, for example)
• makes it easier to visually "parse" the individual files, since we can use VSCode's syntax highlighting
• makes the test logic more legible, since impl_test goes from "90% examples" to "90% logic".

You can also store only the input filename, and the expected output filename is simply "fixed_{input}" or something.

I have mixed feelings about this, because I agree with the point of splitting the test logic from the examples themselves, and I like to have the examples outside the actual code, but the current version is way easier to me to read. My arguments:

1. The examples on this file are small and very straightforward -- I think it'd be valuable to have extensive and real workflows as a different test as well, and those definitely should be in different files.
2. The logic of this test is barely dumb: I'm only making the diff and checking if it's empty. So I could argue that a spending a whole file for this logic could be unnecessary.
3. It's so easier to me to read the test that is self-contained on a single file. E.g., when I followed your link to understand the test you mentioned, I had to spend some time reading the test file to find the actual path for the files (bc the path on the test was a relative path), and had to be changing from file to file to actually understand the test, and that really annoys me (too many tabs! aaaa).

That said, one purpose that I have is to just update this file to

1. make the examples even more straightforward -- remove the workflow names, triggers, etc; keep only the actual job that we're evaluating.
2. Create a second test function (or test file, if you prefer), to run tests over whole real workflows, extracted from real repositories -- and them put the workflows as separated files and refer to them by filepath.

WDYT?

[joycebrum]

Humm this is a good point. I'd say for us to follow the "dedicated file" option to be in accordance to how scorecard is doing it now.

Besides, I'd also like to see the tests running on very complex cases -- with lots of different identations and comments, multiple script injections on the same file, etc. We could even use the existing ones they use for testing script injection identification and just create the "expected" for it.

[pnacht]

+1 to what Joyce said.

[diogoteles08]

Ok, I'm working on it. But a question that came up:
For the workflow examples that I'm making up, I'm adding the header of the Scorecard LICENSE (as it was added on the example you sent me), but does it make sense to also add it on the workflow that I'm just copying from a real scenario? For example, I'm including as a test example a workflow that SACI has fixed for Angular.

Additional tricky question: is it OK to mention that the workflow was extracted from Angular?

[pnacht]

I don't think we need to add licenses to the test files. There are already some test cases which don't have licenses:

https://github.com/joycebrum/scorecard/blob/main/checks/raw/testdata/.github/workflows/github-pysa-workflow.yaml

As for mentioning the workflow came from Angular... I think it'd be fine, but I wouldn't bother: it doesn't add any relevant information about the test; we care about the behavior expressed by the test case, not its origins.

[diogoteles08]

Hey @joycebrum and @pnacht, I've made the changes you requested, added some additional tests, and merged the code on main into my branch. PTAL =)

[pnacht]

Additional tests required:

• Workflow already has an env
  • and a variable must be appended to the env; OR
  • the env is "irrelevant", variables must be added at a different scope
• Workflow ending with a newline (all the current examples don't)
• Results when the script is run to add all environment variables at the workflow level, not at the smallest scope.

[joycebrum]

LGTM
We can decide to remove the commented tests later, let's keep it there by now.

[gabibguti]

What is the goal of "ignorePatternInsideComments.yaml"?

[joycebrum]

Just noticed that the tests not passing could be a problem in the future PRs.

[diogoteles08]

What is the goal of "ignorePatternInsideComments.yaml"?

It just evaluates if the script isn't changing code that is part of a comment, as it wouldn't be unsafe.