🐛 Only validate shell scripts supported by our parser #862
Conversation
checks/pinned_dependencies_test.go
Outdated
| Score: checker.MaxResultScore, | ||
| NumberOfWarn: 0, | ||
| NumberOfInfo: 1, |
There was a problem hiding this comment.
This is because we are no longer parsing ksh:
There was a problem hiding this comment.
instead of ignoring the file, change the shell name in the workflow. We want to make sure this test passes with a supported shell.
e2e/pinned_dependencies_test.go
Outdated
| @@ -49,7 +49,7 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { | |||
| expected := scut.TestReturn{ | |||
| Errors: nil, | |||
| Score: checker.MinResultScore, | |||
| NumberOfWarn: 154, | |||
| NumberOfWarn: 152, | |||
There was a problem hiding this comment.
This is because we are no longer parsing ksh:
https://github.com/ossf-tests/scorecard-check-pinned-dependencies-e2e/blob/91af031ed4e8f5f70d0db2bfd8c126cc27332e1d/.github/workflows/github-workflow-wget-across-steps#L43
There was a problem hiding this comment.
I've updated the file in https://github.com/ossf-tests/scorecard-check-pinned-dependencies-e2e/blob/main/.github/workflows/github-workflow-wget-across-steps to use bash so you need not update this test.
ristomcgehee
force-pushed
the
shell-parse-errors
branch
from
f27fca1
to
6dcf32f
Compare
|
Is this PR ready for review? |
Yes. Am I supposed to request reviewers or tag it with something? |
You can request reviewers to let them know PR is ready for review. Have added myself and @laurentsimon for now. |
| t.Parallel() | ||
|
|
||
| tests := []struct { | ||
| filename string |
There was a problem hiding this comment.
Nit: add a name string field here which can be used to identify the test. Currently we use filename to identify the test (line 66).
| t.Parallel() | ||
| var content []byte | ||
| var err error | ||
| if tt.filename == "" { |
There was a problem hiding this comment.
Why do we need to test for empty filename here?
There was a problem hiding this comment.
Ok, yeah, we don't need that here. I copy-pasted and didn't look closely at what I copied.
| } else { | ||
| content, err = ioutil.ReadFile(tt.filename) | ||
| if err != nil { | ||
| panic(fmt.Errorf("cannot read file: %w", err)) |
There was a problem hiding this comment.
Instead of panic, use t.Failf
| } | ||
| result := isSupportedShellScriptFile(tt.filename, content) | ||
| if result != tt.expected { | ||
| t.Fail() |
There was a problem hiding this comment.
Add a failure message, showing the expected and got values. Something like: https://github.com/ossf/scorecard/blob/main/utests/utlib.go#L129
There was a problem hiding this comment.
Yeah, I was unsure whether I should do that here since the only possibilities are true and false, but I do think it would be helpful.
| @@ -35,6 +35,10 @@ var ( | |||
| shellNames = []string{ | |||
| "sh", "bash", "dash", "ksh", "mksh", | |||
| } | |||
| // supportedShells is the list of shells that are supported by mvdan.cc/sh/v3/syntax. | |||
There was a problem hiding this comment.
you can declare supportedShells , and build shellNames from it. Take shellInterpreters as an example.
checks/pinned_dependencies_test.go
Outdated
| Score: checker.MaxResultScore, | ||
| NumberOfWarn: 0, | ||
| NumberOfInfo: 1, |
There was a problem hiding this comment.
instead of ignoring the file, change the shell name in the workflow. We want to make sure this test passes with a supported shell.
e2e/pinned_dependencies_test.go
Outdated
| @@ -49,7 +49,7 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { | |||
| expected := scut.TestReturn{ | |||
| Errors: nil, | |||
| Score: checker.MinResultScore, | |||
| NumberOfWarn: 154, | |||
| NumberOfWarn: 152, | |||
There was a problem hiding this comment.
I've updated the file in https://github.com/ossf-tests/scorecard-check-pinned-dependencies-e2e/blob/main/.github/workflows/github-workflow-wget-across-steps to use bash so you need not update this test.
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix
What is the current behavior? (You can also link to an open issue here)
The pinned dependency check is failing on some repos because they contain shell scripts that our not supported by the parser we are using. For example, https://github.com/u-boot/u-boot/blob/master/scripts/ld-version.sh is an awk file that we are trying to parse as a shell script.
Part of BUG: Parsing errors #839
What is the new behavior (if this is a feature change)?
The parser we are using, https://github.com/mvdan/sh, supports parsing the following scripts: bash, sh, and mksh. For this PR, I am only parsing a file if it is using of those 3 shells. If a shebang is present in the file, I base the decision to parse entirely on it, otherwise I use the file extension.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No
Other information:
This PR makes it so that some shell scripts (dash, ksh, etc.) will no longer be checked for pinned dependencies. I think this is the most correct thing to do since our parser does not support these shells. If we don't want to lose the checking of these shells, what we could do is attempt to parse these shells, and if parsing fails, swallow the error and continue with the check. This should work ~99% of the time, and in the 1% of times where it doesn't work, the check will run without giving any errors. Let me know if you'd prefer this approach instead.