fix(store,logger,scan): concurrency-adjacent races and correctness fixes (#343)

* fix(logger): create log dirs and flatten url log filenames

CreateFile/Write kept the '/' from a target's url path when building the
log filename, so a target like http://host:port/ tried to open
<dir>/host:port/.log whose parent directory never existed. os.OpenFile
failed and scanTarget aborted the whole target before any scanning ran.

fold every '/' and '\\' run in the sanitized url into a single '_' via a
shared logPath helper so CreateFile and Write always resolve to the same
flat file, and mkdir the parent defensively in getWriter as a second line
of defense.

* fix(scan): route module status lines through the locking sink

subdomaintakeover, cloudstorage, and the next.js framework detector
printed their status/error lines with a bare fmt.Println straight to
os.Stdout, bypassing the output package's sink. under -concurrency>1
only the sink is lock-wrapped, so these lines could interleave or tear
against lines other targets write concurrently.

swap them for output.ScanStart / output.Error, matching every other
scanner in the package.

* fix(store): make snapshot filenames injective and writes atomic

sanitize() folds every separator run (and a literal '_') to one '_', so
distinct targets like https://a.com/x and https://a.com//x, or
host:8443/path and host_8443_path, sanitized to the identical string and
shared one snapshot file - the second target's Save silently clobbered
the first's baseline. Save's os.WriteFile also wasn't atomic, so two
targets racing on a collided path (or -concurrency>1 in general) could
interleave partial writes.

pathFor now appends 16 hex chars of the target's sha256 to the sanitized
name, so distinct targets never collide, and Save writes through a temp
file in the same dir followed by os.Rename so a reader always sees a
complete snapshot or the previous one, never a partial write.

this changes the on-disk snapshot filename scheme: existing users' -diff
baselines under the old sanitize()-only names won't be found and the
next run re-baselines from scratch (every finding reports as newly
added once). noting this as a deliberate tradeoff for a local hardening
pass rather than silently orphaning them.

* fix(store): check the deferred temp-file cleanup error

golangci-lint (errcheck) flagged the bare defer os.Remove(tmpPath) in
writeFileAtomic; wrap it so the discard is explicit.

internal/logger/logger.go

@@ -62,6 +62,13 @@ func (l *Logger) getWriter(path string) (*bufio.Writer, error) {
 		return w, nil
 	}
 
+	// belt-and-suspenders: logPath already flattens '/' out of the filename
+	// so path's parent is always dir itself, but a future caller could still
+	// hand getWriter a nested path directly, so make sure it exists too.
+	if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil {
+		return nil, err
+	}
+
 	f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o600)
 	if err != nil {
 		return nil, err
@@ -122,13 +129,43 @@ func (l *Logger) Close() error {
 	return firstErr
 }
 
-// CreateFile initializes a log file for the given URL and writes the header.
-func CreateFile(logFiles *[]string, url string, dir string) error {
+// flattenPath folds every '/' and '\\' run in s into a single '_' and trims
+// them from the ends. a target's path segments (or a stray backslash) would
+// otherwise be read as an implied (and almost always missing) subdirectory
+// tree; a URL maps to exactly one flat log file, never a directory tree.
+func flattenPath(s string) string {
+	var b strings.Builder
+	b.Grow(len(s))
+	prevSep := false
+	for i := 0; i < len(s); i++ {
+		c := s[i]
+		if c == '/' || c == '\\' {
+			if !prevSep {
+				b.WriteByte('_')
+				prevSep = true
+			}
+			continue
+		}
+		b.WriteByte(c)
+		prevSep = false
+	}
+	return strings.Trim(b.String(), "_")
+}
+
+// logPath builds the flattened log-file path for a URL under dir. CreateFile
+// and Write both go through it so the header and every later line for a
+// target always land in the same file.
+func logPath(dir, url string) string {
 	sanitizedURL := url
 	if _, after, ok := strings.Cut(url, "://"); ok {
 		sanitizedURL = after
 	}
-	path := filepath.Join(dir, sanitizedURL+".log")
+	return filepath.Join(dir, flattenPath(sanitizedURL)+".log")
+}
+
+// CreateFile initializes a log file for the given URL and writes the header.
+func CreateFile(logFiles *[]string, url string, dir string) error {
+	path := logPath(dir, url)
 
 	header := fmt.Sprintf("       _____________\n__________(_)__  __/\n__  ___/_  /__  /_  \n_(__  )_  / _  __/  \n/____/ /_/  /_/    \n\nsif log file for %s\nhttps://sif.sh\n\n", url)
 
@@ -142,7 +179,7 @@ func CreateFile(logFiles *[]string, url string, dir string) error {
 
 // Write appends text to the log file for the given URL.
 func Write(url string, dir string, text string) error {
-	path := filepath.Join(dir, url+".log")
+	path := logPath(dir, url)
 	return defaultLogger.write(path, text)
 }
 

internal/logger/logger_test.go

@@ -116,6 +116,112 @@ func TestCreateFile(t *testing.T) {
 	Close()
 }
 
+// TestCreateFileURLWithPathDoesNotFail proves a target with a URL path (e.g.
+// http://127.0.0.1:8931/) gets a writable log file. Before the fix, the
+// scheme-stripped sanitized URL kept the '/' verbatim, so CreateFile tried to
+// open "<dir>/127.0.0.1:8931/.log", a path whose parent directory was never
+// created, and OpenFile failed, aborting the whole target's scan.
+func TestCreateFileURLWithPathDoesNotFail(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	var logFiles []string
+	url := "http://127.0.0.1:8931/"
+	if err := CreateFile(&logFiles, url, tmpDir); err != nil {
+		t.Fatalf("CreateFile with path-bearing url: %v", err)
+	}
+
+	if err := Flush(); err != nil {
+		t.Fatalf("Flush failed: %v", err)
+	}
+
+	if len(logFiles) != 1 {
+		t.Fatalf("expected 1 log file, got %d", len(logFiles))
+	}
+
+	if filepath.Dir(logFiles[0]) != tmpDir {
+		t.Fatalf("expected log file directly under %q, got %q", tmpDir, logFiles[0])
+	}
+
+	content, err := os.ReadFile(logFiles[0])
+	if err != nil {
+		t.Fatalf("log file not readable: %v", err)
+	}
+	if !strings.Contains(string(content), "sif log file for "+url) {
+		t.Errorf("expected header content, got %q", content)
+	}
+
+	Close()
+}
+
+// TestCreateFileURLWithMultiSlashAndQueryDoesNotFail covers a messier target:
+// multiple path segments (and a doubled slash) plus query characters. None of
+// it should ever produce a nested directory or a failed OpenFile.
+func TestCreateFileURLWithMultiSlashAndQueryDoesNotFail(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	var logFiles []string
+	url := "https://example.com:8443//a/b/c?x=1&y=2"
+	if err := CreateFile(&logFiles, url, tmpDir); err != nil {
+		t.Fatalf("CreateFile with multi-slash/query url: %v", err)
+	}
+
+	if err := Flush(); err != nil {
+		t.Fatalf("Flush failed: %v", err)
+	}
+
+	if len(logFiles) != 1 {
+		t.Fatalf("expected 1 log file, got %d", len(logFiles))
+	}
+	if filepath.Dir(logFiles[0]) != tmpDir {
+		t.Fatalf("expected log file directly under %q, got %q", tmpDir, logFiles[0])
+	}
+	if _, err := os.Stat(logFiles[0]); err != nil {
+		t.Fatalf("log file not created: %v", err)
+	}
+
+	Close()
+}
+
+// TestWriteURLWithPathUsesSameFlatFile proves Write (used for every line after
+// the header) resolves to the exact same path CreateFile wrote the header to,
+// for a URL with a path component. Before the fix, both functions kept the raw
+// '/' verbatim and would have hit the same missing-subdirectory error.
+func TestWriteURLWithPathUsesSameFlatFile(t *testing.T) {
+	tmpDir := t.TempDir()
+	sanitizedURL := "127.0.0.1:8931/some/path"
+
+	if err := WriteHeader(sanitizedURL, tmpDir, "test"); err != nil {
+		t.Fatalf("WriteHeader with path-bearing url: %v", err)
+	}
+	if err := Write(sanitizedURL, tmpDir, "line two\n"); err != nil {
+		t.Fatalf("Write with path-bearing url: %v", err)
+	}
+	if err := Flush(); err != nil {
+		t.Fatalf("Flush failed: %v", err)
+	}
+
+	entries, err := os.ReadDir(tmpDir)
+	if err != nil {
+		t.Fatalf("ReadDir: %v", err)
+	}
+	if len(entries) != 1 {
+		t.Fatalf("expected exactly 1 flat log file under %q, got %d entries", tmpDir, len(entries))
+	}
+	if entries[0].IsDir() {
+		t.Fatalf("expected a flat file, got a directory: %s", entries[0].Name())
+	}
+
+	content, err := os.ReadFile(filepath.Join(tmpDir, entries[0].Name()))
+	if err != nil {
+		t.Fatalf("reading log file: %v", err)
+	}
+	if !strings.Contains(string(content), "line two") {
+		t.Errorf("Write did not land in the same file WriteHeader created: %q", content)
+	}
+
+	Close()
+}
+
 func TestConcurrentWrites(t *testing.T) {
 	tmpDir := t.TempDir()
 

internal/scan/cloudstorage.go

@@ -24,6 +24,7 @@ import (
 	"github.com/charmbracelet/log"
 	"github.com/vmfunc/sif/internal/httpx"
 	"github.com/vmfunc/sif/internal/logger"
+	"github.com/vmfunc/sif/internal/output"
 	"github.com/vmfunc/sif/internal/styles"
 )
 
@@ -57,7 +58,7 @@ type CloudStorageResult struct {
 }
 
 func CloudStorage(url string, timeout time.Duration, logdir string) ([]CloudStorageResult, error) {
-	fmt.Println(styles.Separator.Render("Starting " + styles.Status.Render("Cloud Storage Misconfiguration Scan") + "..."))
+	output.ScanStart("Cloud Storage Misconfiguration Scan")
 
 	sanitizedURL := stripScheme(url)
 

internal/scan/js/frameworks/next.go

@@ -24,7 +24,6 @@ package frameworks
 
 import (
 	"context"
-	"fmt"
 	"io"
 	"net/http"
 	"regexp"
@@ -33,6 +32,7 @@ import (
 
 	urlutil "github.com/projectdiscovery/utils/url"
 	"github.com/vmfunc/sif/internal/httpx"
+	"github.com/vmfunc/sif/internal/output"
 )
 
 // nextPagesRegex matches JavaScript file references in Next.js build manifest.
@@ -50,7 +50,7 @@ func GetPagesRouterScripts(scriptUrl string, timeout time.Duration) ([]string, e
 
 	req, err := http.NewRequestWithContext(context.TODO(), http.MethodGet, scriptUrl, http.NoBody)
 	if err != nil {
-		fmt.Println(err)
+		output.Error("%v", err)
 		return nil, err
 	}
 
@@ -58,14 +58,14 @@ func GetPagesRouterScripts(scriptUrl string, timeout time.Duration) ([]string, e
 	// hang the whole scan; a zero timeout would read with no deadline.
 	resp, err := httpx.Client(timeout).Do(req)
 	if err != nil {
-		fmt.Println(err)
+		output.Error("%v", err)
 		return nil, err
 	}
 	defer resp.Body.Close()
 
 	body, err := io.ReadAll(io.LimitReader(resp.Body, maxManifestSize))
 	if err != nil {
-		fmt.Println(err)
+		output.Error("%v", err)
 		return nil, err
 	}
 	// the manifest ships minified on one line; strip line breaks so the regex

internal/scan/js/frameworks/stdout_bypass_test.go

@@ -0,0 +1,36 @@
+/*
+·━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━·
+:                                                                               :
+:   █▀ █ █▀▀   ·   Blazing-fast pentesting suite                                :
+:   ▄█ █ █▀    ·   BSD 3-Clause License                                         :
+:                                                                               :
+:   (c) 2022-2026 vmfunc, xyzeva,                                               :
+:                 lunchcat alumni & contributors                                :
+:                                                                               :
+·━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━·
+*/
+
+package frameworks
+
+import (
+	"os"
+	"strings"
+	"testing"
+)
+
+// TestNextDoesNotPrintDirectlyToStdout mirrors internal/scan's guard: next.go
+// used to fmt.Println(err) straight to os.Stdout, which can interleave/tear
+// under -concurrency>1 since only the output sink is lock-wrapped there.
+func TestNextDoesNotPrintDirectlyToStdout(t *testing.T) {
+	data, err := os.ReadFile("next.go")
+	if err != nil {
+		t.Fatalf("reading next.go: %v", err)
+	}
+	src := string(data)
+	if strings.Contains(src, "fmt.Println(") {
+		t.Error("next.go still calls fmt.Println directly, want output.Error instead")
+	}
+	if strings.Contains(src, "os.Stdout") {
+		t.Error("next.go references os.Stdout directly, want the output sink instead")
+	}
+}

internal/scan/stdout_bypass_test.go

@@ -0,0 +1,44 @@
+/*
+·━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━·
+:                                                                               :
+:   █▀ █ █▀▀   ·   Blazing-fast pentesting suite                                :
+:   ▄█ █ █▀    ·   BSD 3-Clause License                                         :
+:                                                                               :
+:   (c) 2022-2026 vmfunc, xyzeva,                                               :
+:                 lunchcat alumni & contributors                                :
+:                                                                               :
+·━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━·
+*/
+
+package scan
+
+import (
+	"os"
+	"strings"
+	"testing"
+)
+
+// TestModulesDoNotPrintDirectlyToStdout guards against status/error lines that
+// bypass the output sink via a raw fmt.Println(os.Stdout, ...): under
+// -concurrency>1 those interleave/tear with lines the rest of the run writes
+// through output.Info/Error, since only the sink is wrapped with a lock. every
+// module should route its chrome through the output package instead.
+func TestModulesDoNotPrintDirectlyToStdout(t *testing.T) {
+	files := []string{
+		"cloudstorage.go",
+		"subdomaintakeover.go",
+	}
+	for _, f := range files {
+		data, err := os.ReadFile(f)
+		if err != nil {
+			t.Fatalf("reading %s: %v", f, err)
+		}
+		src := string(data)
+		if strings.Contains(src, "fmt.Println(") {
+			t.Errorf("%s still calls fmt.Println directly, want output.Info/Error/ScanStart instead", f)
+		}
+		if strings.Contains(src, "os.Stdout") {
+			t.Errorf("%s references os.Stdout directly, want the output sink instead", f)
+		}
+	}
+}

internal/scan/subdomaintakeover.go

@@ -25,6 +25,7 @@ import (
 	"github.com/charmbracelet/log"
 	"github.com/vmfunc/sif/internal/httpx"
 	"github.com/vmfunc/sif/internal/logger"
+	"github.com/vmfunc/sif/internal/output"
 	"github.com/vmfunc/sif/internal/pool"
 	"github.com/vmfunc/sif/internal/styles"
 )
@@ -74,7 +75,7 @@ var takeoverProviders = map[string]string{
 // SubdomainTakeover checks dnsResults for dangling subdomains pointing at
 // unclaimed third-party services.
 func SubdomainTakeover(url string, dnsResults []string, timeout time.Duration, threads int, logdir string) ([]SubdomainTakeoverResult, error) {
-	fmt.Println(styles.Separator.Render("Starting " + styles.Status.Render("Subdomain Takeover Vulnerability Check") + "..."))
+	output.ScanStart("Subdomain Takeover Vulnerability Check")
 
 	sanitizedURL := stripScheme(url)
 

internal/store/store.go

@@ -18,6 +18,8 @@
 package store
 
 import (
+	"crypto/sha256"
+	"encoding/hex"
 	"encoding/json"
 	"fmt"
 	"os"
@@ -95,11 +97,26 @@ func sanitize(target string) string {
 	return name
 }
 
+// targetHashLen is how many hex characters of the target's sha256 are kept in
+// the snapshot filename. 16 hex chars is 64 bits of the hash, astronomically
+// collision-free for the number of targets a single run ever has, while
+// keeping the filename short and stable.
+const targetHashLen = 16
+
 // pathFor builds the absolute snapshot path for a target under dir. kept private
-// so the sanitized-filename invariant lives in one place; Save and Load both go
-// through it so a target always maps to the same file.
+// so the filename invariant lives in one place; Save and Load both go through
+// it so a target always maps to the same file.
+//
+// sanitize alone is lossy: it folds every separator run (and a literal '_')
+// to one '_', so distinct targets like "https://a.com/x" and "https://a.com//x"
+// (or "host:8443/path" and "host_8443_path") produce the identical string and
+// would silently share, and clobber, one snapshot. appending a hash of the
+// full, un-sanitized target makes the path injective for distinct targets
+// while keeping the sanitized prefix for a human skimming the state dir.
 func pathFor(dir, target string) string {
-	return filepath.Join(dir, sanitize(target)+snapshotExt)
+	sum := sha256.Sum256([]byte(target))
+	suffix := hex.EncodeToString(sum[:])[:targetHashLen]
+	return filepath.Join(dir, sanitize(target)+"-"+suffix+snapshotExt)
 }
 
 // Save writes the run's findings for target as a json snapshot under dir,
@@ -126,12 +143,44 @@ func Save(dir, target string, findings []finding.Finding) error {
 	}
 
 	path := pathFor(dir, target)
-	if err := os.WriteFile(path, data, snapshotFileMode); err != nil {
+	if err := writeFileAtomic(dir, path, data); err != nil {
 		return fmt.Errorf("writing snapshot %q: %w", path, err)
 	}
 	return nil
 }
 
+// writeFileAtomic writes data to path without ever exposing a reader to a
+// partially-written file: it writes to a temp file in the same directory
+// (so the later rename is same-filesystem and atomic) and only renames it
+// onto path once the write and close both succeed. under -concurrency>1, two
+// targets whose sanitized names previously collided could otherwise race two
+// os.WriteFile calls on the same path and interleave their writes; a rename
+// is atomic, so a concurrent reader always sees one complete snapshot or the
+// other, never a mix.
+func writeFileAtomic(dir, path string, data []byte) error {
+	tmp, err := os.CreateTemp(dir, ".snapshot-*.tmp")
+	if err != nil {
+		return err
+	}
+	tmpPath := tmp.Name()
+	// on any early return the temp file must not linger; once the rename
+	// below succeeds this is a no-op (the path is already gone) and its
+	// error is deliberately discarded.
+	defer func() { _ = os.Remove(tmpPath) }()
+
+	if _, err := tmp.Write(data); err != nil {
+		tmp.Close()
+		return err
+	}
+	if err := tmp.Close(); err != nil {
+		return err
+	}
+	if err := os.Chmod(tmpPath, snapshotFileMode); err != nil {
+		return err
+	}
+	return os.Rename(tmpPath, path)
+}
+
 // Load reads the previously saved snapshot for target under dir. a missing
 // snapshot is not an error - it's the first run for that target, so an empty
 // slice comes back and the caller treats every current finding as new. a present

internal/store/store_test.go

@@ -127,7 +127,7 @@ func TestLoadCorruptSnapshotErrors(t *testing.T) {
 	// would silently re-flag every finding as new on every run.
 	dir := t.TempDir()
 	const target = "https://corrupt.test"
-	path := filepath.Join(dir, sanitize(target)+snapshotExt)
+	path := pathFor(dir, target)
 	if err := os.WriteFile(path, []byte("{not json"), snapshotFileMode); err != nil {
 		t.Fatalf("seeding corrupt snapshot: %v", err)
 	}
@@ -136,6 +136,96 @@ func TestLoadCorruptSnapshotErrors(t *testing.T) {
 	}
 }
 
+func TestPathForDistinctTargetsNeverCollide(t *testing.T) {
+	// see pathFor's doc comment (store.go) for why this used to collide.
+	dir := t.TempDir()
+	tests := [][2]string{
+		{"https://a.com/x", "https://a.com//x"},
+		{"https://a.com/x", "https://a_com/x"},
+		{"host:8443/path", "host_8443_path"},
+	}
+	for _, tt := range tests {
+		a, b := tt[0], tt[1]
+		if sanitize(a) != sanitize(b) {
+			t.Fatalf("test premise broken: sanitize(%q)=%q != sanitize(%q)=%q, pick a colliding pair", a, sanitize(a), b, sanitize(b))
+		}
+		pa, pb := pathFor(dir, a), pathFor(dir, b)
+		if pa == pb {
+			t.Errorf("pathFor collided for distinct targets %q and %q: both %q", a, b, pa)
+		}
+	}
+}
+
+func TestSaveDistinctCollidingTargetsRoundTripIndependently(t *testing.T) {
+	dir := t.TempDir()
+	a, b := "https://a.com/x", "https://a.com//x"
+	if sanitize(a) != sanitize(b) {
+		t.Fatalf("test premise broken: sanitize no longer collides for %q and %q", a, b)
+	}
+
+	findingsA := []finding.Finding{{Target: a, Module: "headers", Severity: finding.SeverityInfo, Key: "a", Title: "a"}}
+	findingsB := []finding.Finding{{Target: b, Module: "headers", Severity: finding.SeverityInfo, Key: "b", Title: "b"}}
+
+	if err := Save(dir, a, findingsA); err != nil {
+		t.Fatalf("Save a: %v", err)
+	}
+	if err := Save(dir, b, findingsB); err != nil {
+		t.Fatalf("Save b: %v", err)
+	}
+
+	gotA, err := Load(dir, a)
+	if err != nil {
+		t.Fatalf("Load a: %v", err)
+	}
+	gotB, err := Load(dir, b)
+	if err != nil {
+		t.Fatalf("Load b: %v", err)
+	}
+	if !reflect.DeepEqual(gotA, findingsA) {
+		t.Errorf("Load(a) = %#v, want %#v (b's save must not have clobbered a)", gotA, findingsA)
+	}
+	if !reflect.DeepEqual(gotB, findingsB) {
+		t.Errorf("Load(b) = %#v, want %#v", gotB, findingsB)
+	}
+}
+
+func TestSaveWritesAtomicallyViaTempAndRename(t *testing.T) {
+	// Save must never leave a stray temp file behind, and a reader must never
+	// observe a partially-written snapshot: it either sees the old file (not
+	// yet renamed) or the fully-written new one, never a half-written one at
+	// the final path.
+	dir := t.TempDir()
+	const target = "https://atomic.test"
+
+	if err := Save(dir, target, sampleFindings()); err != nil {
+		t.Fatalf("Save: %v", err)
+	}
+
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatalf("ReadDir: %v", err)
+	}
+	if len(entries) != 1 {
+		t.Fatalf("dir has %d entries after Save, want exactly 1 (no leftover temp file): %v", len(entries), entries)
+	}
+	if entries[0].Name() != filepath.Base(pathFor(dir, target)) {
+		t.Fatalf("unexpected file left in state dir: %q", entries[0].Name())
+	}
+
+	// a second Save (overwrite path) must also leave exactly one file, proving
+	// the temp file it wrote for the update got renamed/cleaned up too.
+	if err := Save(dir, target, nil); err != nil {
+		t.Fatalf("second Save: %v", err)
+	}
+	entries, err = os.ReadDir(dir)
+	if err != nil {
+		t.Fatalf("ReadDir after second Save: %v", err)
+	}
+	if len(entries) != 1 {
+		t.Fatalf("dir has %d entries after overwrite, want exactly 1: %v", len(entries), entries)
+	}
+}
+
 func TestDiffAddedAndRemoved(t *testing.T) {
 	base := sampleFindings()