[engine] Fix gosec
The gosec check was previously failing silently (producing a retcode of
zero) because it was run in a sandbox without the necessary environment
variables set (specifically `GOPACKAGESDRIVER=off` needed to be set for
it to work inside the sandbox).
After fixing the check there were a bunch of fixes required, mostly
related to error propagation.
Change-Id: Ib6ff1ae370d3e07fb9c63cb2bfddf907a526955f
Reviewed-on: https://fuchsia-review.googlesource.com/c/shac-project/shac/+/922774
Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com>
Fuchsia-Auto-Submit: Oliver Newman <olivernewman@google.com>
Reviewed-by: Anthony Fandrianto <atyfto@google.com>
diff --git a/checks/go.star b/checks/go.star
index 517bf01..5cf216b 100644
--- a/checks/go.star
+++ b/checks/go.star
@@ -41,7 +41,14 @@
gofmt = shac.check(_gofmt, formatter = True)
-def _gosec(ctx, version = "v2.15.0", level = "error"):
+def _gosec(ctx, version = "v2.15.0", level = "error", exclude = [
+ # shac checks are allowed to run arbitrary subprocesses, so it's common for
+ # shac's source code to run non-constant subcommands.
+ "G204",
+ # shac checks are allowed to read arbitrary files, so it's common for shac's
+ # source code to read non-constant files.
+ "G304",
+]):
"""Runs gosec on a Go code base.
See https://github.com/securego/gosec for more details.
@@ -52,29 +59,43 @@
be rolled from time to time.
level: level at which issues should be emitted.
"""
+ affected_files = set(ctx.scm.affected_files())
exe = go_install(ctx, "github.com/securego/gosec/v2/cmd/gosec", version)
- res = ctx.os.exec([exe, "-fmt=json", "-quiet", "-exclude=G304", "-exclude-dir=.tools", "./..."], raise_on_failure = False).wait()
- if res.retcode:
- # Schema is https://pkg.go.dev/github.com/securego/gosec/v2#ReportInfo
- d = json.decode(res.stdout)
- o = len(ctx.scm.root) + 1
- for file, data in d["Golang errors"]:
+ res = ctx.os.exec(
+ [exe, "-fmt=json", "-quiet", "-exclude=%s" % ",".join(exclude), "-exclude-dir=.tools", "./..."],
+ ok_retcodes = (0, 1),
+ env = go_env(ctx, "gosec"),
+ ).wait()
+ if not res.retcode:
+ return
+
+ # Schema is https://pkg.go.dev/github.com/securego/gosec/v2#ReportInfo
+ d = json.decode(res.stdout)
+ o = len(ctx.scm.root) + 1
+ for file, errs in d["Golang errors"].items():
+ filepath = file[o:]
+ if filepath not in affected_files:
+ continue
+ for e in errs:
ctx.emit.finding(
level = "error",
- message = i["error"],
- filepath = file[o:],
- line = int(i["line"]),
- col = int(i["column"]),
+ message = e["error"],
+ filepath = filepath,
+ line = int(e["line"]),
+ col = int(e["column"]),
)
- for i in d["Issues"]:
- line = i["line"].split("-")[0]
- ctx.emit.finding(
- level = level,
- message = i["rule_id"] + ": " + i["details"],
- filepath = i["file"][o:],
- line = int(line),
- col = int(i["column"]),
- )
+ for i in d["Issues"]:
+ line = i["line"].split("-")[0]
+ filepath = i["file"][o:]
+ if filepath not in affected_files:
+ continue
+ ctx.emit.finding(
+ level = level,
+ message = i["rule_id"] + ": " + i["details"],
+ filepath = filepath,
+ line = int(line),
+ col = int(i["column"]),
+ )
gosec = shac.check(_gosec)
diff --git a/internal/cli/check.go b/internal/cli/check.go
index b88e5c7..34d81f6 100644
--- a/internal/cli/check.go
+++ b/internal/cli/check.go
@@ -63,7 +63,7 @@
}
if c.jsonOutput != "" {
- if err2 := os.WriteFile(c.jsonOutput, buf.Bytes(), 0o666); err == nil {
+ if err2 := os.WriteFile(c.jsonOutput, buf.Bytes(), 0o600); err == nil {
err = err2
}
}
diff --git a/internal/engine/pkg.go b/internal/engine/pkg.go
index 82ddaa0..e770b05 100644
--- a/internal/engine/pkg.go
+++ b/internal/engine/pkg.go
@@ -193,7 +193,7 @@
}
parentdir := filepath.Dir(depdir)
- if err = os.MkdirAll(parentdir, 0o777); err != nil {
+ if err = os.MkdirAll(parentdir, 0o700); err != nil {
return nil, err
}
if err = p.gitCommand(ctx, parentdir, "clone", fullURL, filepath.Base(depdir)); err != nil {
diff --git a/internal/engine/run.go b/internal/engine/run.go
index 705c07c..fa73f5b 100644
--- a/internal/engine/run.go
+++ b/internal/engine/run.go
@@ -576,8 +576,7 @@
return errors.New("did you forget to call shac.register_check?")
}
// Last phase where checks are called.
- s.bufferAllChecks(ctx, ch)
- return nil
+ return s.bufferAllChecks(ctx, ch)
}
// parse parses a single shac.star file.
@@ -604,8 +603,12 @@
}
// bufferAllChecks adds all the checks to the channel for execution.
-func (s *shacState) bufferAllChecks(ctx context.Context, ch chan<- func() error) {
- args := starlark.Tuple{getCtx(path.Join(s.root, s.subdir), s.vars)}
+func (s *shacState) bufferAllChecks(ctx context.Context, ch chan<- func() error) error {
+ shacCtx, err := getCtx(path.Join(s.root, s.subdir), s.vars)
+ if err != nil {
+ return err
+ }
+ args := starlark.Tuple{shacCtx}
args.Freeze()
for i := range s.checks {
if s.filter != nil && !s.filter(s.checks[i]) {
@@ -631,6 +634,7 @@
return err
}
}
+ return nil
}
func (s *shacState) newTempDir() (string, error) {
@@ -699,10 +703,10 @@
var err error
for _, proc := range c.subprocesses {
if !proc.waitCalled {
- proc.cleanup()
if err == nil {
err = fmt.Errorf("wait() was not called on %s", proc.String())
}
+ _ = proc.cleanup()
}
}
return err
diff --git a/internal/engine/runtime_ctx.go b/internal/engine/runtime_ctx.go
index 0cbf6ca..d9f48b6 100644
--- a/internal/engine/runtime_ctx.go
+++ b/internal/engine/runtime_ctx.go
@@ -23,10 +23,12 @@
// getCtx returns the ctx object to pass to a registered check callback.
//
// Make sure to update //doc/stdlib.star whenever this function is modified.
-func getCtx(root string, vars map[string]string) starlark.Value {
+func getCtx(root string, vars map[string]string) (starlark.Value, error) {
starlarkVars := starlark.NewDict(len(vars))
for k, v := range vars {
- starlarkVars.SetKey(starlark.String(k), starlark.String(v))
+ if err := starlarkVars.SetKey(starlark.String(k), starlark.String(v)); err != nil {
+ return nil, err
+ }
}
return toValue("ctx", starlark.StringDict{
// Implemented in runtime_ctx_emit.go
@@ -61,5 +63,5 @@
"vars": toValue("ctx.vars", starlark.StringDict{
"get": newBuiltin("ctx.vars.get", ctxVarsGet),
}),
- })
+ }), nil
}
diff --git a/internal/engine/runtime_ctx_os.go b/internal/engine/runtime_ctx_os.go
index 8189bb6..115335e 100644
--- a/internal/engine/runtime_ctx_os.go
+++ b/internal/engine/runtime_ctx_os.go
@@ -199,7 +199,7 @@
// Ignore errors during cleanup because cleanupFuncs will only be
// populated if another error occurred prior to starting the
// subprocess.
- f()
+ _ = f()
}
}()
diff --git a/internal/reporting/multi.go b/internal/reporting/multi.go
index d8fd3af..c7ef18b 100644
--- a/internal/reporting/multi.go
+++ b/internal/reporting/multi.go
@@ -43,14 +43,14 @@
}
func (t *MultiReport) CheckCompleted(ctx context.Context, check string, start time.Time, d time.Duration, level engine.Level, err error) {
- t.do(func(r Report) error {
+ _ = t.do(func(r Report) error {
r.CheckCompleted(ctx, check, start, d, level, err)
return nil
})
}
func (t *MultiReport) Print(ctx context.Context, check, file string, line int, message string) {
- t.do(func(r Report) error {
+ _ = t.do(func(r Report) error {
r.Print(ctx, check, file, line, message)
return nil
})
diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go
index 2db044c..6041e65 100644
--- a/internal/sandbox/sandbox.go
+++ b/internal/sandbox/sandbox.go
@@ -138,7 +138,6 @@
}
args = append(args, "--")
args = append(args, config.Cmd...)
- //#nosec G204
return exec.CommandContext(ctx, s.nsjailPath, args...)
}
