fix(handlers): apk handler now doesnt check for apk extension since json-enumerator and other byte stream methods wouldnt have it (#5151)

* fix(handlers): apk handler now doesnt check for apk extension since json-enumerator and other byte stream methods wouldnt have it * fix(handlers): don't fail zip/jar processing when APK check errors Content-based APK detection now runs isAPKFile on every zip/jar (not just .apk-named files). Previously a zip.NewReader failure (truncated zip, corrupted central directory, or a polyglot mimetype still reports as zip) was returned as a fatal error from newFileReader, causing HandleFile to skip the file entirely. Treat the APK check error as non-fatal: log at V(3) and fall through to the archive/default handler. Also remove the now-dead readerConfig/readerOption/withFileExtension and getFileExtension infrastructure left unused after shouldHandleAsAPK stopped consuming the file extension. Co-authored-by: Cursor <[email protected]> --------- Co-authored-by: Cursor <[email protected]>
2026-07-24 10:38:46 -07:00
parent 05a583290b
commit 6f3c981e7b
3 changed files with 229 additions and 108 deletions
@@ -0,0 +1,116 @@
+package handlers
+
+import (
+	"archive/zip"
+	"bytes"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/trufflesecurity/trufflehog/v3/pkg/context"
+	"github.com/trufflesecurity/trufflehog/v3/pkg/feature"
+)
+
+// makeAPKZip builds an in-memory zip that satisfies isAPKFile (contains both
+// AndroidManifest.xml and classes.dex). A large stored padding entry is written
+// first so the manifest/dex local headers fall outside mimetype's detection
+// window, keeping mimetype's verdict "application/zip" rather than APK. This
+// forces the content-based isAPKFile deep scan (rather than mimetype's own APK
+// detection) to be what promotes the file to the APK handler.
+func makeAPKZip(t *testing.T, extraFiles map[string][]byte) []byte {
+	t.Helper()
+
+	var buf bytes.Buffer
+	zw := zip.NewWriter(&buf)
+
+	padding, err := zw.CreateHeader(&zip.FileHeader{Name: "padding.txt", Method: zip.Store})
+	require.NoError(t, err)
+	_, err = padding.Write(bytes.Repeat([]byte("A"), 5000))
+	require.NoError(t, err)
+
+	for _, name := range []string{"AndroidManifest.xml", "classes.dex"} {
+		w, err := zw.Create(name)
+		require.NoError(t, err)
+		_, err = w.Write([]byte("placeholder"))
+		require.NoError(t, err)
+	}
+
+	for name, content := range extraFiles {
+		w, err := zw.Create(name)
+		require.NoError(t, err)
+		_, err = w.Write(content)
+		require.NoError(t, err)
+	}
+
+	require.NoError(t, zw.Close())
+	return buf.Bytes()
+}
+
+// makeNonAPKZip builds a plain in-memory zip without any APK markers.
+func makeNonAPKZip(t *testing.T) []byte {
+	t.Helper()
+
+	var buf bytes.Buffer
+	zw := zip.NewWriter(&buf)
+	w, err := zw.Create("hello.txt")
+	require.NoError(t, err)
+	_, err = w.Write([]byte("just some content"))
+	require.NoError(t, err)
+	require.NoError(t, zw.Close())
+	return buf.Bytes()
+}
+
+// TestNewFileReaderAPKContentDetection verifies APK detection is driven by file
+// content (the presence of AndroidManifest.xml + classes.dex) and not by any
+// filename/extension: the reader is promoted to the APK mime type with no
+// extension supplied, while a zip lacking those markers stays a generic zip.
+func TestNewFileReaderAPKContentDetection(t *testing.T) {
+	feature.EnableAPKHandler.Store(true)
+	t.Cleanup(func() { feature.EnableAPKHandler.Store(false) })
+
+	ctx := context.Background()
+
+	rdr, err := newFileReader(ctx, bytes.NewReader(makeAPKZip(t, nil)))
+	require.NoError(t, err)
+	assert.Equal(t, string(apkMime), rdr.mime.String(), "APK content should route to the APK handler without an extension")
+	require.NoError(t, rdr.Close())
+
+	rdrPlain, err := newFileReader(ctx, bytes.NewReader(makeNonAPKZip(t)))
+	require.NoError(t, err)
+	assert.Equal(t, string(zipMime), rdrPlain.mime.String(), "a zip without APK markers should remain a generic zip")
+	require.NoError(t, rdrPlain.Close())
+}
+
+// TestNewFileReaderAPKDetectionDisabled ensures APK content detection does not
+// run when the feature flag is off, leaving the file as a generic zip.
+func TestNewFileReaderAPKDetectionDisabled(t *testing.T) {
+	feature.EnableAPKHandler.Store(false)
+
+	rdr, err := newFileReader(context.Background(), bytes.NewReader(makeAPKZip(t, nil)))
+	require.NoError(t, err)
+	assert.Equal(t, string(zipMime), rdr.mime.String())
+	require.NoError(t, rdr.Close())
+}
+
+// TestNewFileReaderAPKCheckErrorFallsThrough ensures that when the APK feature
+// flag is on and mimetype identifies a file as a zip, but the content-based APK
+// check fails to parse it (e.g. truncated/corrupted zip, or a polyglot that
+// mimetype still reports as zip), newFileReader does NOT return a fatal error.
+// Instead it must fall through to normal handling so the file is still processed
+// rather than skipped entirely. This guards the regression where dropping the
+// .apk extension guard made every unparseable zip/jar unprocessable.
+func TestNewFileReaderAPKCheckErrorFallsThrough(t *testing.T) {
+	feature.EnableAPKHandler.Store(true)
+	t.Cleanup(func() { feature.EnableAPKHandler.Store(false) })
+
+	// Start from a valid zip (so mimetype detects "application/zip") then truncate
+	// the tail, removing the end-of-central-directory record so zip.NewReader fails.
+	valid := makeNonAPKZip(t)
+	truncated := valid[:len(valid)-10]
+
+	rdr, err := newFileReader(context.Background(), bytes.NewReader(truncated))
+	require.NoError(t, err, "APK check failure must not be fatal; file should fall through to normal handling")
+	assert.Equal(t, string(zipMime), rdr.mime.String(), "file should remain a generic zip, not be skipped")
+	require.NoError(t, rdr.Close())
+}
@@ -7,7 +7,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"path/filepath"

 	"github.com/gabriel-vasile/mimetype"
 	"github.com/mholt/archives"
@@ -54,14 +53,6 @@ var (
 	ErrProcessingWarning = errors.New("error processing file")
 )

-type readerConfig struct{ fileExtension string }
-
-type readerOption func(*readerConfig)
-
-func withFileExtension(ext string) readerOption {
-	return func(c *readerConfig) { c.fileExtension = ext }
-}
-
 // mimeTypeReader wraps an io.Reader with MIME type information.
 // This type is used to pass content through the processing pipeline
 // while carrying its detected MIME type, avoiding redundant type detection.
@@ -103,12 +94,7 @@ func newMimeTypeReader(r io.Reader) (mimeTypeReader, error) {

 // newFileReader creates a fileReader from an io.Reader, optionally using BufferedFileWriter for certain formats.
 // The caller is responsible for closing the reader when it is no longer needed.
-func newFileReader(ctx context.Context, r io.Reader, options ...readerOption) (fReader fileReader, err error) {
-	var cfg readerConfig
-
-	for _, opt := range options {
-		opt(&cfg)
-	}
+func newFileReader(ctx context.Context, r io.Reader) (fReader fileReader, err error) {
 	// To detect the MIME type of the input data, we need a reader that supports seeking.
 	// This allows us to read the data multiple times if necessary without losing the original position.
 	// We use a BufferedReaderSeeker to wrap the original reader, enabling this functionality.
@@ -136,15 +122,26 @@ func newFileReader(ctx context.Context, r io.Reader, options ...readerOption) (f
 		return fReader, fmt.Errorf("error resetting reader after MIME detection: %w", err)
 	}

-	// Check for APK files
-	if shouldHandleAsAPK(cfg, fReader) {
-		isAPK, err := isAPKFile(&fReader)
-		if err != nil {
-			return fReader, fmt.Errorf("error checking for APK: %w", err)
-		}
-		if isAPK {
+	// Detection is content-based, so this now runs for every zip/jar rather than only for files with an apk extension.
+	if shouldHandleAsAPK(fReader) {
+		// A failure here (e.g. truncated zip, corrupted central directory, or a polyglot that mimetype still
+		// reports as zip/jar) only means the file isn't a parseable APK. Since this check now runs for every
+		// zip/jar rather than only files with an .apk extension, we must not treat the error as fatal; otherwise
+		// any such file would be skipped entirely instead of falling through to the archive or default handler.
+		isAPK, apkErr := isAPKFile(&fReader)
+		switch {
+		case apkErr != nil:
+			logContext.AddLogger(ctx).Logger().V(3).Info(
+				"error checking for APK, falling back to normal handling", "error", apkErr)
+		case isAPK:
 			return handleAPKFile(&fReader)
 		}
+
+		// isAPKFile inspects the archive via zip.NewReader, which leaves the reader positioned mid-stream.
+		// Reset to the start so archive identification and any downstream handler read from the beginning.
+		if _, seekErr := fReader.Seek(0, io.SeekStart); seekErr != nil {
+			return fReader, fmt.Errorf("error resetting reader after APK check: %w", seekErr)
+		}
 	}

 	// If a MIME type is known to not be an archive type, we might as well return here rather than
@@ -224,7 +221,6 @@ const (
 	rpmHandlerType     handlerType = "rpm"
 	apkHandlerType     handlerType = "apk"
 	defaultHandlerType handlerType = "default"
-	apkExt                         = ".apk"
 )

 type mimeType string
@@ -359,8 +355,7 @@ func HandleFile(
 		return errors.New("reader is nil")
 	}

-	readerOption := withFileExtension(getFileExtension(chunkSkel))
-	rdr, err := newFileReader(ctx, reader, readerOption)
+	rdr, err := newFileReader(ctx, reader)
 	if err != nil {
 		if errors.Is(err, ErrEmptyReader) {
 			ctx.Logger().V(5).Info("empty reader, skipping file")
@@ -492,91 +487,14 @@ func isFatal(err error) bool {
 	}
 }

-// getFileExtension extracts the file extension from the chunk's SourceMetadata.
-// It considers all sources defined in the MetaData message.
-// Note: Probably should add this as a method to the source_metadatapb object.
-// then it'd just be chunkSkel.SourceMetadata.GetFileExtension()
-func getFileExtension(chunkSkel *sources.Chunk) string {
-	if chunkSkel == nil || chunkSkel.SourceMetadata == nil {
-		return ""
-	}
-
-	var fileName string
-
-	// Inspect the SourceMetadata to determine the source type
-	switch metadata := chunkSkel.SourceMetadata.Data.(type) {
-	case *source_metadatapb.MetaData_Artifactory:
-		fileName = metadata.Artifactory.Path
-	case *source_metadatapb.MetaData_Azure:
-		fileName = metadata.Azure.File
-	case *source_metadatapb.MetaData_AzureRepos:
-		fileName = metadata.AzureRepos.File
-	case *source_metadatapb.MetaData_Bitbucket:
-		fileName = metadata.Bitbucket.File
-	case *source_metadatapb.MetaData_Buildkite:
-		fileName = metadata.Buildkite.Link
-	case *source_metadatapb.MetaData_Circleci:
-		fileName = metadata.Circleci.Link
-	case *source_metadatapb.MetaData_Confluence:
-		fileName = metadata.Confluence.File
-	case *source_metadatapb.MetaData_Docker:
-		fileName = metadata.Docker.File
-	case *source_metadatapb.MetaData_Ecr:
-		fileName = metadata.Ecr.File
-	case *source_metadatapb.MetaData_Filesystem:
-		fileName = metadata.Filesystem.File
-	case *source_metadatapb.MetaData_Git:
-		fileName = metadata.Git.File
-	case *source_metadatapb.MetaData_Github:
-		fileName = metadata.Github.File
-	case *source_metadatapb.MetaData_Gitlab:
-		fileName = metadata.Gitlab.File
-	case *source_metadatapb.MetaData_Gcs:
-		fileName = metadata.Gcs.Filename
-	case *source_metadatapb.MetaData_GoogleDrive:
-		fileName = metadata.GoogleDrive.File
-	case *source_metadatapb.MetaData_Huggingface:
-		fileName = metadata.Huggingface.File
-	case *source_metadatapb.MetaData_Jira:
-		fileName = metadata.Jira.Link
-	case *source_metadatapb.MetaData_Jenkins:
-		fileName = metadata.Jenkins.Link
-	case *source_metadatapb.MetaData_Npm:
-		fileName = metadata.Npm.File
-	case *source_metadatapb.MetaData_Pypi:
-		fileName = metadata.Pypi.File
-	case *source_metadatapb.MetaData_S3:
-		fileName = metadata.S3.File
-	case *source_metadatapb.MetaData_Slack:
-		fileName = metadata.Slack.File
-	case *source_metadatapb.MetaData_Sharepoint:
-		fileName = metadata.Sharepoint.Link
-	case *source_metadatapb.MetaData_Gerrit:
-		fileName = metadata.Gerrit.File
-	case *source_metadatapb.MetaData_Test:
-		fileName = metadata.Test.File
-	case *source_metadatapb.MetaData_Teams:
-		fileName = metadata.Teams.File
-	case *source_metadatapb.MetaData_TravisCI:
-		fileName = metadata.TravisCI.Link
-	// Add other sources if they have a file or equivalent field
-	// Skipping Syslog, Forager, Postman, Vector, Webhook and Elasticsearch
-	default:
-		return ""
-	}
-
-	// Use filepath.Ext to extract the file extension from the file name
-	ext := filepath.Ext(fileName)
-	return ext
-}
-
-// shouldHandleAsAPK checks if the file should be handled as an APK based on config and MIME type.
-// Note: We can't extend the mimetype package with an APK detection function b/c it would require adjusting settings
+// shouldHandleAsAPK reports whether the file should be inspected as a potential APK.
+// Detection is content-based (file magic): after confirming the file is a zip/jar, the caller runs isAPKFile, which
+// inspects the archive entries for APK markers such as AndroidManifest.xml and classes.dex.
+// It deliberately does NOT rely on the file extension so APKs are detected regardless of the source or filename.
+// Note: Don't extend the mimetype package with APK detection function. It would require adjusting settings
 // so that all files are fully read into a byte slice for detection (mimetype.SetLimit(0)), which would bloat memory.
-// Instead we call the isAPKFile function in here after ensuring it's a zip/jar file and has an .apk extension.
-func shouldHandleAsAPK(cfg readerConfig, fReader fileReader) bool {
+func shouldHandleAsAPK(fReader fileReader) bool {
 	return feature.EnableAPKHandler.Load() &&
-		cfg.fileExtension == apkExt &&
 		(fReader.mime.String() == string(zipMime) || fReader.mime.String() == string(jarMime))
 }

@@ -1,6 +1,8 @@
 package json_enumerator

 import (
+	"archive/zip"
+	"bytes"
 	"encoding/json"
 	"io"
 	"strings"
@@ -12,6 +14,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/trufflesecurity/trufflehog/v3/pkg/context"
+	"github.com/trufflesecurity/trufflehog/v3/pkg/feature"
 	"github.com/trufflesecurity/trufflehog/v3/pkg/sources"
 )

@@ -127,3 +130,87 @@ func TestScanEnumerator(t *testing.T) {
 		})
 	}
 }
+
+// makeZip builds an in-memory zip from the given entries. A large stored padding
+// entry is written first so mimetype classifies the bytes as a generic zip,
+// leaving APK identification to the content-based deep scan.
+func makeZip(t *testing.T, files map[string]string) []byte {
+	t.Helper()
+
+	var buf bytes.Buffer
+	zw := zip.NewWriter(&buf)
+
+	padding, err := zw.CreateHeader(&zip.FileHeader{Name: "padding.txt", Method: zip.Store})
+	require.NoError(t, err)
+	_, err = padding.Write(bytes.Repeat([]byte("A"), 5000))
+	require.NoError(t, err)
+
+	for name, content := range files {
+		w, err := zw.Create(name)
+		require.NoError(t, err)
+		_, err = w.Write([]byte(content))
+		require.NoError(t, err)
+	}
+
+	require.NoError(t, zw.Close())
+	return buf.Bytes()
+}
+
+// TestScanEnumeratorAPKContentRouting proves json-enumerator input is routed by
+// file content, not by any filename/extension. The records carry no filename.
+// A zip whose contents mark it as an APK (AndroidManifest.xml + classes.dex) is
+// routed to the APK handler; because this synthetic archive lacks resources.arsc
+// the APK handler yields no chunks, so the plaintext secret is not surfaced.
+// The identical secret in a plain zip (no APK markers) is handled generically
+// and surfaces the secret.
+func TestScanEnumeratorAPKContentRouting(t *testing.T) {
+	feature.EnableAPKHandler.Store(true)
+	t.Cleanup(func() { feature.EnableAPKHandler.Store(false) })
+
+	secret := secretPart1 + secretPart2
+
+	apkZip := makeZip(t, map[string]string{
+		"AndroidManifest.xml": "placeholder",
+		"classes.dex":         "placeholder",
+		"assets/secret.txt":   secret,
+	})
+	plainZip := makeZip(t, map[string]string{
+		"assets/secret.txt": secret,
+	})
+
+	run := func(data []byte) (string, error) {
+		readJSON, writeJSON := io.Pipe()
+		chunksChan := make(chan *sources.Chunk, 16)
+
+		var workerError error
+		var wg sync.WaitGroup
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			defer close(chunksChan)
+			ctx := context.WithLogger(t.Context(), logr.Discard())
+			source := &Source{}
+			workerError = source.chunkJSONEnumeratorReader(ctx, readJSON, chunksChan)
+		}()
+
+		// No filename is set on the record; routing is content-based only.
+		enc := json.NewEncoder(writeJSON)
+		require.NoError(t, enc.Encode(&jsonEntry{Metadata: makeRawMessage(t, "{}"), Data: data}))
+		require.NoError(t, writeJSON.Close())
+
+		found := ""
+		for chunk := range chunksChan {
+			found += string(chunk.Data)
+		}
+		wg.Wait()
+		return found, workerError
+	}
+
+	foundAPK, err := run(apkZip)
+	require.NoError(t, err)
+	assert.NotContains(t, foundAPK, secret, "APK-content archive should be routed to the APK handler")
+
+	foundZip, err := run(plainZip)
+	require.NoError(t, err)
+	assert.Contains(t, foundZip, secret, "non-APK zip should be handled generically and surface the secret")
+}