Repository: moov-io/ach
Pull request #1306 (2023-10-17 21:04:52)

fix: code review cleanup
- fix: skip directories in ReadDir
- fix: avoid nil panic with malformed readers
- feat: check for valid JSON before parsing 


Patch:
diff --git a/.github/workflows/scorecards.yml b/.github/workflows/scorecards.yml
index d3018812..85f464c2 100644
--- a/.github/workflows/scorecards.yml
+++ b/.github/workflows/scorecards.yml
@@ -36,7 +36,7 @@ jobs:
           persist-credentials: false
 
       - name: "Run analysis"
-        uses: ossf/scorecard-action@483ef80eb98fb506c348f7d62e28055e49fe2398 # v2.3.0
+        uses: ossf/scorecard-action@08b4669551908b1024bb425080c797723083c031 # v2.2.0
         with:
           results_file: results.sarif
           results_format: sarif
diff --git a/.gitignore b/.gitignore
index fbebf870..2a2f7cec 100644
--- a/.gitignore
+++ b/.gitignore
@@ -61,6 +61,5 @@ go-licenser
 
 openapi-generator*.jar
 /cmd/webui/assets/ach.wasm
-/client/
-.idea/
-.vscode/
+/.idea/*
+/client/
\ No newline at end of file
diff --git a/dir.go b/dir.go
index 747987a9..83ceecf8 100644
--- a/dir.go
+++ b/dir.go
@@ -57,6 +57,14 @@ func ReadDir(dir string) ([]*File, error) {
 	for i := range infos {
 		path := filepath.Join(dir, infos[i].Name())
 
+		stat, err := os.Stat(path)
+		if err != nil {
+			return nil, fmt.Errorf("stat of %s failed: %v", path, err)
+		}
+		if stat.IsDir() {
+			continue
+		}
+
 		f, err1 := readACH(path)
 		if f != nil {
 			out = append(out, f)
diff --git a/docs/Gemfile.lock b/docs/Gemfile.lock
index 8256ebfc..fa30d919 100644
--- a/docs/Gemfile.lock
+++ b/docs/Gemfile.lock
@@ -35,7 +35,7 @@ GEM
       faraday-net_http (>= 2.0, < 3.1)
       ruby2_keywords (>= 0.0.4)
     faraday-net_http (3.0.2)
-    ffi (1.16.3)
+    ffi (1.15.5)
     forwardable-extended (2.6.0)
     gemoji (3.0.1)
     github-pages (228)
diff --git a/file.go b/file.go
index 1b949983..a3963434 100644
--- a/file.go
+++ b/file.go
@@ -141,6 +141,9 @@ func FileFromJSONWith(bs []byte, opts *ValidateOpts) (*File, error) {
 	if len(bs) == 0 {
 		return nil, errors.New("no JSON data provided")
 	}
+	if !json.Valid(bs) {
+		return nil, fmt.Errorf("problem reading File: %w", ErrInvalidJSON)
+	}
 
 	// read file root level
 	var f file
diff --git a/fileErrors.go b/fileErrors.go
index 2714c73d..995a13ff 100644
--- a/fileErrors.go
+++ b/fileErrors.go
@@ -45,6 +45,8 @@ var (
 	ErrFileIATSEC = errors.New("IAT Standard Entry Class Code should use iatBatch")
 	// ErrFileNoBatches is the error given if a file has no batches
 	ErrFileNoBatches = errors.New("must have []*Batches or []*IATBatches to be built")
+
+	ErrInvalidJSON = errors.New("invalid JSON")
 )
 
 // RecordWrongLengthErr is the error given when a record is the wrong length
diff --git a/go.mod b/go.mod
index 1ff3123e..13d83df2 100644
--- a/go.mod
+++ b/go.mod
@@ -9,11 +9,11 @@ require (
 	github.com/go-kit/log v0.2.1
 	github.com/gorilla/mux v1.8.0
 	github.com/juju/ansiterm v1.0.0
-	github.com/moov-io/base v0.47.0
+	github.com/moov-io/base v0.46.0
 	github.com/moov-io/iso4217 v0.3.0
-	github.com/prometheus/client_golang v1.17.0
+	github.com/prometheus/client_golang v1.16.0
 	github.com/stretchr/testify v1.8.4
-	golang.org/x/net v0.17.0
+	golang.org/x/net v0.15.0
 	golang.org/x/text v0.13.0
 )
 
@@ -28,11 +28,11 @@ require (
 	github.com/mattn/go-isatty v0.0.19 // indirect
 	github.com/matttproud/golang_protobuf_extensions v1.0.4 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
-	github.com/prometheus/client_model v0.4.1-0.20230718164431-9a2bf3000d16 // indirect
+	github.com/prometheus/client_model v0.4.0 // indirect
 	github.com/prometheus/common v0.44.0 // indirect
 	github.com/prometheus/procfs v0.11.1 // indirect
 	github.com/rickar/cal/v2 v2.1.13 // indirect
-	golang.org/x/sys v0.13.0 // indirect
+	golang.org/x/sys v0.12.0 // indirect
 	google.golang.org/protobuf v1.31.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )
diff --git a/go.sum b/go.sum
index 24a092b0..3842b616 100644
--- a/go.sum
+++ b/go.sum
@@ -45,20 +45,14 @@ github.com/matttproud/golang_protobuf_extensions v1.0.4 h1:mmDVorXM7PCGKw94cs5zk
 github.com/matttproud/golang_protobuf_extensions v1.0.4/go.mod h1:BSXmuO+STAnVfrANrmjBb36TMTDstsz7MSK+HVaYKv4=
 github.com/moov-io/base v0.46.0 h1:4sgFjfD9EELJ+i6cGfPtezoPAAxwE3imLJ64bbHtBwM=
 github.com/moov-io/base v0.46.0/go.mod h1:AFoLPJA1uuylLp33g1nGzTnGk8zyqYH7KfBS7flc0P8=
-github.com/moov-io/base v0.47.0 h1:r4PALYPSMwCB/smDUcMUk3a2JW6TA4mT17oOBx9ZpEE=
-github.com/moov-io/base v0.47.0/go.mod h1:jz9m/WyTr6RTbm1DP0V5j4RVUuXqmq3bUnzJ6NwCPo8=
 github.com/moov-io/iso4217 v0.3.0 h1:xAxeJEn7QIpEy971eElEGiaLNdj81UwOcCcvE6j2ZAo=
 github.com/moov-io/iso4217 v0.3.0/go.mod h1:/pF1mBInDSJtwyWkLNr8EMdIn1zIl2cyjaVi4Xhpd44=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/prometheus/client_golang v1.16.0 h1:yk/hx9hDbrGHovbci4BY+pRMfSuuat626eFsHb7tmT8=
 github.com/prometheus/client_golang v1.16.0/go.mod h1:Zsulrv/L9oM40tJ7T815tM89lFEugiJ9HzIqaAx4LKc=
-github.com/prometheus/client_golang v1.17.0 h1:rl2sfwZMtSthVU752MqfjQozy7blglC+1SOtjMAMh+Q=
-github.com/prometheus/client_golang v1.17.0/go.mod h1:VeL+gMmOAxkS2IqfCq0ZmHSL+LjWfWDUmp1mBz9JgUY=
 github.com/prometheus/client_model v0.4.0 h1:5lQXD3cAg1OXBf4Wq03gTrXHeaV0TQvGfUooCfx1yqY=
 github.com/prometheus/client_model v0.4.0/go.mod h1:oMQmHW1/JoDwqLtg57MGgP/Fb1CJEYF2imWWhWtMkYU=
-github.com/prometheus/client_model v0.4.1-0.20230718164431-9a2bf3000d16 h1:v7DLqVdK4VrYkVD5diGdl4sxJurKJEMnODWRJlxV9oM=
-github.com/prometheus/client_model v0.4.1-0.20230718164431-9a2bf3000d16/go.mod h1:oMQmHW1/JoDwqLtg57MGgP/Fb1CJEYF2imWWhWtMkYU=
 github.com/prometheus/common v0.44.0 h1:+5BrQJwiBB9xsMygAB3TNvpQKOwlkc25LbISbrdOOfY=
 github.com/prometheus/common v0.44.0/go.mod h1:ofAIvZbQ1e/nugmZGz4/qCb9Ap1VoSTIO7x0VV9VvuY=
 github.com/prometheus/procfs v0.11.1 h1:xRC8Iq1yyca5ypa9n1EZnWZkt7dwcoRPQwX/5gwaUuI=
@@ -70,10 +64,6 @@ github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcU
 github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
 golang.org/x/net v0.15.0 h1:ugBLEUaxABaB5AJqW9enI0ACdci2RUd4eP51NTBvuJ8=
 golang.org/x/net v0.15.0/go.mod h1:idbUs1IY1+zTqbi8yxTbhexhEEk5ur9LInksu6HrEpk=
-golang.org/x/net v0.16.0 h1:7eBu7KsSvFDtSXUIDbh3aqlK4DPsZ1rByC8PFfBThos=
-golang.org/x/net v0.16.0/go.mod h1:NxSsAGuq816PNPmqtQdLE42eU2Fs7NoRIZrHJAlaCOE=
-golang.org/x/net v0.17.0 h1:pVaXccu2ozPjCXewfr1S7xza/zcXTity9cCdXQYSjIM=
-golang.org/x/net v0.17.0/go.mod h1:NxSsAGuq816PNPmqtQdLE42eU2Fs7NoRIZrHJAlaCOE=
 golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sys v0.0.0-20200116001909-b77594299b42/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20200223170610-d5e6a3e2c0ae/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
@@ -82,8 +72,6 @@ golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.12.0 h1:CM0HF96J0hcLAwsHPJZjfdNzs0gftsLfgKt57wWHJ0o=
 golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.13.0 h1:Af8nKPmuFypiUBjVoU9V20FiaFXOcuZI21p0ycVYYGE=
-golang.org/x/sys v0.13.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/text v0.13.0 h1:ablQoSUd0tRdKxZewP80B+BaqeKJuVhuRxj/dkrun3k=
 golang.org/x/text v0.13.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
diff --git a/reader.go b/reader.go
index ac829b54..9ac05077 100644
--- a/reader.go
+++ b/reader.go
@@ -19,6 +19,7 @@ package ach
 
 import (
 	"bufio"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -172,6 +173,9 @@ func (r *Reader) SetMaxLines(max int) {
 func (r *Reader) Read() (File, error) {
 	r.lineNum = 0
 	// read through the entire file
+	if r.scanner == nil {
+		return r.File, errors.New("nil scanner")
+	}
 	for r.scanner.Scan() {
 		line := r.scanner.Text()
 		r.lineNum++


Test patch:
diff --git a/dir_test.go b/dir_test.go
index 5f925cc0..791d4684 100644
--- a/dir_test.go
+++ b/dir_test.go
@@ -22,6 +22,8 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestReadDir(t *testing.T) {
@@ -37,12 +39,16 @@ func TestReadDir(t *testing.T) {
 	defer os.RemoveAll(dir)
 
 	files, err := ReadDir(dir)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if len(files) != 5 {
-		t.Errorf("found %d files", len(files))
-	}
+	require.NoError(t, err)
+	require.Len(t, files, 5)
+
+	// Make a directory which is skipped
+	err = os.Mkdir(filepath.Join(dir, "other"), 0777)
+	require.NoError(t, err)
+
+	files, err = ReadDir(dir)
+	require.NoError(t, err)
+	require.Len(t, files, 5)
 }
 
 func TestReadDirErr(t *testing.T) {
@@ -55,17 +61,12 @@ func TestReadDirErr(t *testing.T) {
 	defer os.RemoveAll(dir)
 
 	// zzz- is a prefix as os.ReadDir seems to return file descriptors ordered alphabetically by filename
-	if err := os.WriteFile(filepath.Join(dir, "zzz-bad.ach"), []byte("bad data"), 0600); err != nil {
-		t.Fatal(err)
-	}
+	err := os.WriteFile(filepath.Join(dir, "zzz-bad.ach"), []byte("bad data"), 0600)
+	require.NoError(t, err)
 
 	files, err := ReadDir(dir)
-	if len(files) != 2 {
-		t.Errorf("found %d files", len(files))
-	}
-	if err == nil {
-		t.Error("expected error")
-	}
+	require.Error(t, err)
+	require.Len(t, files, 2)
 
 	files, err = ReadDir("/not/exist/")
 	if n := len(files); n != 0 || err == nil {
@@ -75,30 +76,23 @@ func TestReadDirErr(t *testing.T) {
 
 func TestReadDirSymlinkErr(t *testing.T) {
 	dir, err := os.MkdirTemp("", "readdir-symlink")
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 	defer os.RemoveAll(dir)
 
 	// write an invalid symlink
-	if err := os.Symlink(filepath.Join("missing", "directory"), filepath.Join(dir, "foo.ach")); err != nil {
-		t.Fatal(err)
-	}
+	err = os.Symlink(filepath.Join("missing", "directory"), filepath.Join(dir, "foo.ach"))
+	require.NoError(t, err)
 
 	files, err := ReadDir(dir)
-	if len(files) != 0 {
-		t.Errorf("got %d files", len(files))
-	}
-	if err == nil {
-		t.Error("expected error")
-	}
+	require.Error(t, err)
+	require.Len(t, files, 0)
 }
 
 func copyFilesToTempDir(t *testing.T, filenames []string) string {
+	t.Helper()
+
 	dir, err := os.MkdirTemp("", "ach-readdir")
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 
 	for i := range filenames {
 		in, err := os.Open(filepath.Join("test", "testdata", filenames[i]))
@@ -114,9 +108,7 @@ func copyFilesToTempDir(t *testing.T, filenames []string) string {
 		in.Close()
 		out.Close()
 
-		if err != nil {
-			t.Fatalf("copy: %v", err)
-		}
+		require.NoError(t, err)
 	}
 
 	return dir