From 8feb77b20aeb64366d106b08f2941287755c173b Mon Sep 17 00:00:00 2001 From: Bill Rich Date: Fri, 10 Jul 2026 15:17:52 -0700 Subject: [PATCH] [INT-718] Add TargetNotFoundError for targeted scan targets missing from the source (#5123) * Add TargetNotFoundError for targeted scan targets missing from the source * Check download response status in exact-path retry --- pkg/sources/errors.go | 19 ++++ pkg/sources/errors_test.go | 16 +++ pkg/sources/github/github.go | 63 ++++++++++- pkg/sources/github/target_gone_test.go | 150 +++++++++++++++++++++++++ 4 files changed, 247 insertions(+), 1 deletion(-) create mode 100644 pkg/sources/github/target_gone_test.go diff --git a/pkg/sources/errors.go b/pkg/sources/errors.go index 3933a7f0b..fc52a4b69 100644 --- a/pkg/sources/errors.go +++ b/pkg/sources/errors.go @@ -74,3 +74,22 @@ func (t TargetedScanError) Error() string { func (t TargetedScanError) Unwrap() error { return t.Err } + +// TargetNotFoundError indicates a targeted scan failed because the requested +// target is definitively absent from the source (e.g. the file, commit, or +// message it pointed to was deleted), as opposed to inaccessible due to +// authorization or transient failures. Consumers can treat this as terminal: +// retrying cannot succeed until a fresh scan updates the stored location. +type TargetNotFoundError struct { + Err error +} + +var _ error = (*TargetNotFoundError)(nil) + +func (t TargetNotFoundError) Error() string { + return t.Err.Error() +} + +func (t TargetNotFoundError) Unwrap() error { + return t.Err +} diff --git a/pkg/sources/errors_test.go b/pkg/sources/errors_test.go index a01ccaaa6..ee122d74e 100644 --- a/pkg/sources/errors_test.go +++ b/pkg/sources/errors_test.go @@ -159,3 +159,19 @@ func TestScanErrorsString(t *testing.T) { t.Errorf("got %q, want %q", got, want) } } + +func TestTargetNotFoundErrorUnwrapsThroughJobChain(t *testing.T) { + inner := &TargetNotFoundError{Err: fmt.Errorf("could not download file for scan: 404")} + // The chain a targeted scan failure travels before a consumer sees it: + // scanTargets wraps in TargetedScanError, the source manager wraps in + // Fatal, and JobProgressMetrics.FatalErrors joins the results. + chain := errors.Join(Fatal{&TargetedScanError{Err: inner, SecretID: 42}}) + + var tnf *TargetNotFoundError + if !errors.As(chain, &tnf) { + t.Fatal("TargetNotFoundError not found in wrapped chain") + } + if tnf.Error() != inner.Error() { + t.Fatalf("unexpected message: %q", tnf.Error()) + } +} diff --git a/pkg/sources/github/github.go b/pkg/sources/github/github.go index 8253981cd..6068435e9 100644 --- a/pkg/sources/github/github.go +++ b/pkg/sources/github/github.go @@ -2214,7 +2214,30 @@ func (s *Source) scanTarget(ctx context.Context, target sources.ChunkingTarget, defer func() { _ = resp.Body.Close() }() } if err != nil { - return fmt.Errorf("could not download file for scan: %w", err) + // DownloadContents locates the file by listing its parent directory, + // and the contents API caps listings at 1000 entries, so it can fail + // for files that exist. Retry with an exact-path lookup, which also + // serves as the existence probe when it fails. + retryCloser, retryResp, retryErr := s.downloadContentsByPath( + ctx, apiClient, segments[1], segments[2], meta.GetFile(), meta.GetCommit()) + if retryResp != nil && retryResp.Response != nil && retryResp.Body != nil { + defer func() { _ = retryResp.Body.Close() }() + } + if retryErr != nil { + wrapped := fmt.Errorf("could not download file for scan: %w", retryErr) + // The exact-path lookup 404ing is authoritative for the path, but + // GitHub also returns 404 for existing resources the credentials + // cannot see, so only classify the target as gone when the + // repository itself is still reachable with the same client. + if retryResp != nil && retryResp.Response != nil && + retryResp.StatusCode == http.StatusNotFound && + s.repoReachable(ctx, apiClient, segments[1], segments[2]) { + return &sources.TargetNotFoundError{Err: wrapped} + } + return wrapped + } + fileCtx := context.WithValues(ctx, "path", meta.GetFile()) + return handlers.HandleFile(fileCtx, retryCloser, &chunkSkel, reporter) } if resp.StatusCode != http.StatusOK { return fmt.Errorf("unexpected HTTP response status when trying to download file for scan: %v", resp.Status) @@ -2224,6 +2247,44 @@ func (s *Source) scanTarget(ctx context.Context, target sources.ChunkingTarget, return handlers.HandleFile(fileCtx, readCloser, &chunkSkel, reporter) } +// downloadContentsByPath fetches a file the same way DownloadContents does, +// but resolves it with an exact-path contents lookup instead of searching a +// directory listing, so it is immune to the 1000-entry listing cap. On +// failure the returned response is always the exact-path lookup's, whose 404 +// is an authoritative statement about the path at that ref; a failure of the +// download itself must not be mistaken for the target being gone, because +// the lookup just proved it exists. +func (s *Source) downloadContentsByPath(ctx context.Context, apiClient *github.Client, owner, repo, filePath, ref string) (io.ReadCloser, *github.Response, error) { + fileContent, _, resp, err := apiClient.Repositories.GetContents( + ctx, owner, repo, filePath, &github.RepositoryContentGetOptions{Ref: ref}) + if err != nil { + return nil, resp, err + } + if fileContent == nil || fileContent.GetDownloadURL() == "" { + return nil, resp, fmt.Errorf("no download link found for %s", filePath) + } + dlReq, err := http.NewRequestWithContext(ctx, http.MethodGet, fileContent.GetDownloadURL(), nil) + if err != nil { + return nil, resp, err + } + dlResp, err := apiClient.Client().Do(dlReq) + if err != nil { + return nil, resp, err + } + if dlResp.StatusCode != http.StatusOK { + _ = dlResp.Body.Close() + return nil, resp, fmt.Errorf("unexpected HTTP response status when trying to download file for scan: %v", dlResp.Status) + } + return dlResp.Body, &github.Response{Response: dlResp}, nil +} + +// repoReachable reports whether the repository is still visible with the +// same client. +func (s *Source) repoReachable(ctx context.Context, apiClient *github.Client, owner, repo string) bool { + _, resp, err := apiClient.Repositories.Get(ctx, owner, repo) + return err == nil && resp != nil && resp.StatusCode == http.StatusOK +} + func (s *Source) scanWikiTarget(ctx context.Context, wikiURL string, meta *source_metadatapb.Github, chunkSkel *sources.Chunk, reporter sources.ChunkReporter) error { if meta.GetCommit() == "" { return fmt.Errorf("wiki target has no commit") diff --git a/pkg/sources/github/target_gone_test.go b/pkg/sources/github/target_gone_test.go new file mode 100644 index 000000000..082c680a5 --- /dev/null +++ b/pkg/sources/github/target_gone_test.go @@ -0,0 +1,150 @@ +package github + +import ( + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/google/go-github/v67/github" + "github.com/stretchr/testify/assert" + + "github.com/trufflesecurity/trufflehog/v3/pkg/context" +) + +func testClientForServer(t *testing.T, serverURL string) *github.Client { + t.Helper() + client := github.NewClient(nil) + base, err := url.Parse(serverURL + "/") + assert.NoError(t, err) + client.BaseURL = base + return client +} + +// downloadByPathServer answers the exact-path contents lookup, the raw +// download it points at, and the repository lookup repoReachable makes. +func downloadByPathServer(t *testing.T, fileStatus, rawStatus, repoStatus int) *httptest.Server { + t.Helper() + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/repos/o/r/contents/dir/f.txt": + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(fileStatus) + if fileStatus == http.StatusOK { + _, _ = fmt.Fprintf(w, `{"type":"file","name":"f.txt","path":"dir/f.txt","download_url":"%s/raw/dir/f.txt"}`, server.URL) + return + } + _, _ = w.Write([]byte(`{"message":"Not Found"}`)) + case "/raw/dir/f.txt": + w.WriteHeader(rawStatus) + if rawStatus == http.StatusOK { + _, _ = w.Write([]byte("file body")) + return + } + _, _ = w.Write([]byte("raw host error page")) + case "/repos/o/r": + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(repoStatus) + if repoStatus == http.StatusOK { + _, _ = w.Write([]byte(`{"id":1,"name":"r","full_name":"o/r"}`)) + return + } + _, _ = w.Write([]byte(`{"message":"Not Found"}`)) + default: + t.Errorf("unexpected request: %s", r.URL.Path) + w.WriteHeader(http.StatusInternalServerError) + } + })) + return server +} + +func TestDownloadContentsByPathFetchesFileBody(t *testing.T) { + server := downloadByPathServer(t, http.StatusOK, http.StatusOK, http.StatusOK) + defer server.Close() + + s := &Source{} + client := testClientForServer(t, server.URL) + rc, resp, err := s.downloadContentsByPath(context.Background(), client, "o", "r", "dir/f.txt", "abc123") + assert.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + body, err := io.ReadAll(rc) + assert.NoError(t, err) + assert.NoError(t, rc.Close()) + assert.Equal(t, "file body", string(body)) +} + +func TestDownloadContentsByPathReturns404Response(t *testing.T) { + server := downloadByPathServer(t, http.StatusNotFound, http.StatusOK, http.StatusOK) + defer server.Close() + + s := &Source{} + client := testClientForServer(t, server.URL) + rc, resp, err := s.downloadContentsByPath(context.Background(), client, "o", "r", "dir/f.txt", "abc123") + assert.Error(t, err) + assert.Nil(t, rc) + // The 404 is the exact-path lookup's, which the caller combines with + // repoReachable to classify the target as gone. + assert.NotNil(t, resp) + assert.Equal(t, http.StatusNotFound, resp.StatusCode) +} + +func TestDownloadContentsByPathNon404Failure(t *testing.T) { + server := downloadByPathServer(t, http.StatusForbidden, http.StatusOK, http.StatusOK) + defer server.Close() + + s := &Source{} + client := testClientForServer(t, server.URL) + _, resp, err := s.downloadContentsByPath(context.Background(), client, "o", "r", "dir/f.txt", "abc123") + assert.Error(t, err) + assert.NotNil(t, resp) + assert.Equal(t, http.StatusForbidden, resp.StatusCode) +} + +func TestRepoReachable(t *testing.T) { + server := downloadByPathServer(t, http.StatusNotFound, http.StatusOK, http.StatusOK) + defer server.Close() + + s := &Source{} + client := testClientForServer(t, server.URL) + assert.True(t, s.repoReachable(context.Background(), client, "o", "r")) +} + +func TestRepoNotReachable(t *testing.T) { + server := downloadByPathServer(t, http.StatusNotFound, http.StatusOK, http.StatusNotFound) + defer server.Close() + + s := &Source{} + client := testClientForServer(t, server.URL) + // The repo 404s too: could be lost access rather than deletion, so the + // caller must not classify the target as gone. + assert.False(t, s.repoReachable(context.Background(), client, "o", "r")) +} + +func TestRepoReachableServerDown(t *testing.T) { + server := downloadByPathServer(t, http.StatusNotFound, http.StatusOK, http.StatusOK) + server.Close() + + s := &Source{} + client := testClientForServer(t, server.URL) + assert.False(t, s.repoReachable(context.Background(), client, "o", "r")) +} + +func TestDownloadContentsByPathNon200Download(t *testing.T) { + server := downloadByPathServer(t, http.StatusOK, http.StatusInternalServerError, http.StatusOK) + defer server.Close() + + s := &Source{} + client := testClientForServer(t, server.URL) + rc, resp, err := s.downloadContentsByPath(context.Background(), client, "o", "r", "dir/f.txt", "abc123") + assert.Error(t, err) + assert.ErrorContains(t, err, "unexpected HTTP response status") + assert.Nil(t, rc) + // The returned response is the exact-path lookup's (200), not the failed + // download's, so the caller cannot mistake a download failure for the + // target being gone. + assert.NotNil(t, resp) + assert.Equal(t, http.StatusOK, resp.StatusCode) +}