Skip to content

Commit e7d7f6f

Browse files
committed
fix nested archive handler error wraps to preserve errors.Is identity
The non-archive content paths in pkg/handlers/ar.go:109 and pkg/handlers/rpm.go:120 wrapped the inner error returned from handleNonArchiveContent with %v, dropping its identity from the errors.Is chain. Because handleNonArchiveContent can return ctx.Err() (context.Canceled or context.DeadlineExceeded) when its CancellableWrite fails, isFatal at handlers.go:425 then misclassified those cancelled or deadline-exceeded conditions as non-fatal warnings and continued processing. Switch both wraps to %w so the inner cause is preserved alongside ErrProcessingWarning, matching the same-shape fix already applied to the canonical wrap at default.go:137. Add regression tests for both handlers that exercise the production wrap path via an injected chunk reader that cancels the parent context before its chunk is written, which forces handleNonArchiveContent to return context.Canceled and processARFiles / processRPMFiles to wrap it. The tests assert the outer ErrProcessingWarning wrap is preserved, the inner cancellation cause is inspectable via errors.Is, and isFatal classifies the result as fatal.
1 parent cf31c26 commit e7d7f6f

4 files changed

Lines changed: 124 additions & 2 deletions

File tree

pkg/handlers/ar.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ func (h *arHandler) processARFiles(ctx logContext.Context, reader *deb.Ar, dataO
106106

107107
if err := h.handleNonArchiveContent(fileCtx, rdr, dataOrErrChan); err != nil {
108108
dataOrErrChan <- DataOrErr{
109-
Err: fmt.Errorf("%w: error handling archive content in AR: %v", ErrProcessingWarning, err),
109+
Err: fmt.Errorf("%w: error handling archive content in AR: %w", ErrProcessingWarning, err),
110110
}
111111
h.metrics.incErrors()
112112
continue

pkg/handlers/ar_test.go

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,18 @@
11
package handlers
22

33
import (
4+
stdctx "context"
5+
"errors"
6+
"io"
47
"os"
58
"testing"
69
"time"
710

811
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
913

1014
"github.com/trufflesecurity/trufflehog/v3/pkg/context"
15+
"github.com/trufflesecurity/trufflehog/v3/pkg/sources"
1116
)
1217

1318
func TestHandleARFile(t *testing.T) {
@@ -34,3 +39,63 @@ func TestHandleARFile(t *testing.T) {
3439

3540
assert.Equal(t, wantChunkCount, count)
3641
}
42+
43+
// TestARHandler_NonArchiveErrPreservesIdentity is a regression test for the
44+
// %v wrap at ar.go:109. Before the fix, wrapping handleNonArchiveContent's
45+
// return value with %v dropped the inner error's identity from the errors.Is
46+
// chain, causing isFatal in handleChunksWithError to misclassify cancellation
47+
// (and other fatal causes) as a non-fatal warning. The test injects a
48+
// cancelling chunk reader so handleNonArchiveContent's CancellableWrite
49+
// returns context.Canceled, which is then wrapped by processARFiles. It
50+
// asserts both that the outer ErrProcessingWarning wrap is preserved and
51+
// that the inner cancellation cause remains inspectable.
52+
func TestARHandler_NonArchiveErrPreservesIdentity(t *testing.T) {
53+
file, err := os.Open("testdata/test.deb")
54+
require.NoError(t, err)
55+
defer file.Close()
56+
57+
ctx, cancel := context.WithCancel(context.Background())
58+
defer cancel()
59+
60+
rdr, err := newFileReader(ctx, file)
61+
require.NoError(t, err)
62+
defer rdr.Close()
63+
64+
// Cancel the parent context the moment handleNonArchiveContent asks for
65+
// chunks, then deliver a single non-error chunk. CancellableWrite of that
66+
// chunk's data sees the cancelled context and returns context.Canceled,
67+
// which handleNonArchiveContent returns and processARFiles wraps. This
68+
// is the only path that exercises the ar.go:107-110 wrap.
69+
cancellingChunkReader := sources.ChunkReader(func(_ context.Context, _ io.Reader) <-chan sources.ChunkResult {
70+
ch := make(chan sources.ChunkResult, 1)
71+
cancel()
72+
ch <- sources.NewChunkResult([]byte("data"), 4)
73+
close(ch)
74+
return ch
75+
})
76+
77+
handler := &arHandler{
78+
defaultHandler: newDefaultHandler(arHandlerType, withChunkReader(cancellingChunkReader)),
79+
}
80+
81+
var got []DataOrErr
82+
for d := range handler.HandleFile(context.AddLogger(ctx), rdr) {
83+
got = append(got, d)
84+
}
85+
86+
var warnErr error
87+
for _, d := range got {
88+
if d.Err != nil && errors.Is(d.Err, ErrProcessingWarning) {
89+
warnErr = d.Err
90+
break
91+
}
92+
}
93+
require.Error(t, warnErr, "expected wrapped warning from ar.go non-archive error path")
94+
95+
assert.True(t, errors.Is(warnErr, ErrProcessingWarning),
96+
"outer ErrProcessingWarning wrap should be preserved")
97+
assert.True(t, errors.Is(warnErr, stdctx.Canceled),
98+
"inner cancellation cause should be inspectable via errors.Is")
99+
assert.True(t, isFatal(warnErr),
100+
"isFatal should classify the wrapped error based on the inner cause")
101+
}

pkg/handlers/rpm.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ func (h *rpmHandler) processRPMFiles(
117117

118118
if err := h.handleNonArchiveContent(fileCtx, rdr, dataOrErrChan); err != nil {
119119
dataOrErrChan <- DataOrErr{
120-
Err: fmt.Errorf("%w: error processing RPM archive: %v", ErrProcessingWarning, err),
120+
Err: fmt.Errorf("%w: error processing RPM archive: %w", ErrProcessingWarning, err),
121121
}
122122
h.metrics.incErrors()
123123
}

pkg/handlers/rpm_test.go

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,18 @@
11
package handlers
22

33
import (
4+
stdctx "context"
5+
"errors"
6+
"io"
47
"os"
58
"testing"
69
"time"
710

811
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
913

1014
"github.com/trufflesecurity/trufflehog/v3/pkg/context"
15+
"github.com/trufflesecurity/trufflehog/v3/pkg/sources"
1116
)
1217

1318
func TestHandleRPMFile(t *testing.T) {
@@ -34,3 +39,55 @@ func TestHandleRPMFile(t *testing.T) {
3439

3540
assert.Equal(t, wantChunkCount, count)
3641
}
42+
43+
// TestRPMHandler_NonArchiveErrPreservesIdentity is a regression test for the
44+
// %v wrap at rpm.go:120. Same shape as TestARHandler_NonArchiveErrPreservesIdentity:
45+
// inject a cancelling chunk reader so handleNonArchiveContent's CancellableWrite
46+
// returns context.Canceled, which processRPMFiles wraps with ErrProcessingWarning.
47+
// Asserts the outer warning wrap and inner cancellation cause are both
48+
// observable via errors.Is so isFatal can classify correctly.
49+
func TestRPMHandler_NonArchiveErrPreservesIdentity(t *testing.T) {
50+
file, err := os.Open("testdata/test.rpm")
51+
require.NoError(t, err)
52+
defer file.Close()
53+
54+
ctx, cancel := context.WithCancel(context.Background())
55+
defer cancel()
56+
57+
rdr, err := newFileReader(ctx, file)
58+
require.NoError(t, err)
59+
defer rdr.Close()
60+
61+
cancellingChunkReader := sources.ChunkReader(func(_ context.Context, _ io.Reader) <-chan sources.ChunkResult {
62+
ch := make(chan sources.ChunkResult, 1)
63+
cancel()
64+
ch <- sources.NewChunkResult([]byte("data"), 4)
65+
close(ch)
66+
return ch
67+
})
68+
69+
handler := &rpmHandler{
70+
defaultHandler: newDefaultHandler(rpmHandlerType, withChunkReader(cancellingChunkReader)),
71+
}
72+
73+
var got []DataOrErr
74+
for d := range handler.HandleFile(context.AddLogger(ctx), rdr) {
75+
got = append(got, d)
76+
}
77+
78+
var warnErr error
79+
for _, d := range got {
80+
if d.Err != nil && errors.Is(d.Err, ErrProcessingWarning) {
81+
warnErr = d.Err
82+
break
83+
}
84+
}
85+
require.Error(t, warnErr, "expected wrapped warning from rpm.go non-archive error path")
86+
87+
assert.True(t, errors.Is(warnErr, ErrProcessingWarning),
88+
"outer ErrProcessingWarning wrap should be preserved")
89+
assert.True(t, errors.Is(warnErr, stdctx.Canceled),
90+
"inner cancellation cause should be inspectable via errors.Is")
91+
assert.True(t, isFatal(warnErr),
92+
"isFatal should classify the wrapped error based on the inner cause")
93+
}

0 commit comments

Comments
 (0)