fix: address final review comments

Author: danielewood

CHANGELOG.md

@@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Consolidate shared `connect` status-line formatting between standard and verbose text output paths. ([#121])
 - Centralize pre-commit hooks under the shared `sensiblebit/.github` hook set (including shared `markdownlint`) and run dependency update hooks first; refresh resulting indirect Go and web lockfile dependencies. ([#128])
 - Remove the arbitrary 200-file cap from WASM upload and inspect flows; rely on byte limits instead. ([#129])
+- Enforce a stricter repo-local `golangci-lint` policy and refactor error handling, protocol encoding helpers, file-permission behavior, and tests to satisfy the higher lint bar. ([#130])
 
 ### Added
 

bundle.go

@@ -30,20 +30,21 @@ var (
 	mozillaRootKeys     map[string][]byte // RawSubject → marshaled PKIX public key
 
 	// ErrChainVerificationFailed indicates that certificate path validation failed.
-	ErrChainVerificationFailed = errors.New("chain verification failed")
-	errMozillaRootParse        = errors.New("parsing embedded Mozilla root certificates")
-	errAIAAddressBlocked       = errors.New("blocked address for AIA fetch")
-	errAIAPrivateAddress       = errors.New("blocked private address for AIA fetch")
-	errAIAUnsupportedScheme    = errors.New("unsupported scheme")
-	errAIAMissingHostname      = errors.New("missing hostname in URL")
-	errAIAResolveNoIPs         = errors.New("no IP addresses returned")
-	errFetchLeafHTTPSRequired  = errors.New("invalid URL scheme")
-	errFetchLeafNotTLS         = errors.New("TLS dial did not return TLS connection")
-	errFetchLeafNoCerts        = errors.New("no certificates returned by TLS server")
-	errAIAFetchRedirects       = errors.New("AIA redirect limit exceeded")
-	errAIAHTTPStatus           = errors.New("AIA server returned non-200 status")
-	errBundleLeafNil           = errors.New("leaf certificate is nil")
-	errBundleUnknownTrustStore = errors.New("unknown trust_store")
+	ErrChainVerificationFailed  = errors.New("chain verification failed")
+	errMozillaRootParse         = errors.New("parsing embedded Mozilla root certificates")
+	errAIAAddressBlocked        = errors.New("blocked address for AIA fetch")
+	errAIAPrivateAddress        = errors.New("blocked private address for AIA fetch")
+	errAIAUnsupportedScheme     = errors.New("unsupported scheme")
+	errAIAMissingHostname       = errors.New("missing hostname in URL")
+	errAIAResolveNoIPs          = errors.New("no IP addresses returned")
+	errFetchLeafHTTPSRequired   = errors.New("invalid URL scheme")
+	errFetchLeafMissingHostname = errors.New("fetch leaf URL is missing hostname")
+	errFetchLeafNotTLS          = errors.New("TLS dial did not return TLS connection")
+	errFetchLeafNoCerts         = errors.New("no certificates returned by TLS server")
+	errAIAFetchRedirects        = errors.New("AIA redirect limit exceeded")
+	errAIAHTTPStatus            = errors.New("AIA server returned non-200 status")
+	errBundleLeafNil            = errors.New("leaf certificate is nil")
+	errBundleUnknownTrustStore  = errors.New("unknown trust_store")
 )
 
 // privateNetworks contains CIDR ranges for private, reserved, and shared
@@ -384,7 +385,7 @@ func FetchLeafFromURL(ctx context.Context, input FetchLeafFromURLInput) (*x509.C
 
 	host := parsed.Hostname()
 	if host == "" {
-		return nil, errAIAMissingHostname
+		return nil, errFetchLeafMissingHostname
 	}
 	port := parsed.Port()
 	if port == "" {

cmd/certkit/readonly_commands_test.go

@@ -289,44 +289,6 @@ func TestRunScan_CommandSurface(t *testing.T) {
 	}
 }
 
-func TestRunScan_DumpCertsPermissions(t *testing.T) {
-	snap := snapshotReadonlyGlobals()
-	t.Cleanup(func() { restoreReadonlyGlobals(snap) })
-
-	dir := t.TempDir()
-	_, cert := generateKeyAndCert(t, "scan.example.com", false)
-	writeCertPEM(t, dir, "leaf.pem", cert)
-
-	passwordList = nil
-	passwordFile = ""
-	verbose = false
-	jsonOutput = false
-	scanBundlePath = ""
-	scanConfigPath = filepath.Join(dir, "missing-config.yaml")
-	scanForceExport = true
-	scanDuplicates = false
-	scanDumpKeys = ""
-	scanDumpCerts = filepath.Join(dir, "dumped-certs.pem")
-	scanMaxFileSize = 10 * 1024 * 1024
-	scanFormat = "text"
-	scanAllowPrivateNetwork = false
-	scanSaveDB = ""
-	scanLoadDB = ""
-
-	_, _, err := captureOutput(t, func() error { return runScan(newCommandWithContext(), []string{dir}) })
-	if err != nil {
-		t.Fatalf("runScan with --dump-certs failed: %v", err)
-	}
-
-	info, err := os.Stat(scanDumpCerts)
-	if err != nil {
-		t.Fatalf("stat dumped certs: %v", err)
-	}
-	if perm := info.Mode().Perm(); perm != 0o644 {
-		t.Fatalf("dumped certs permissions = %04o, want 0644", perm)
-	}
-}
-
 func TestRunInspect_CommandSurface(t *testing.T) {
 	snap := snapshotReadonlyGlobals()
 	t.Cleanup(func() { restoreReadonlyGlobals(snap) })

internal/csr_test.go

@@ -206,13 +206,6 @@ func TestGenerateCSRFiles_WithExistingKey(t *testing.T) {
 	if perm := csrInfo.Mode().Perm(); perm != 0o644 {
 		t.Errorf("csr file permissions = %04o, want 0644", perm)
 	}
-	outDirInfo, err := os.Stat(outDir)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if perm := outDirInfo.Mode().Perm(); perm != 0o755 {
-		t.Errorf("output directory permissions = %04o, want 0755", perm)
-	}
 	if !key.PublicKey.Equal(csr.PublicKey) {
 		t.Error("CSR public key does not match existing key — CSR was signed by a different key")
 	}

internal/keygen_test.go

@@ -137,14 +137,6 @@ func TestGenerateKeyFiles(t *testing.T) {
 		t.Errorf("key file permissions = %04o, want 0600", perm)
 	}
 
-	dirInfo, err := os.Stat(dir)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if perm := dirInfo.Mode().Perm(); perm != 0o755 {
-		t.Errorf("output directory permissions = %04o, want 0755", perm)
-	}
-
 	// Verify pub.pem exists, is parseable, and has standard permissions
 	pubPath := filepath.Join(dir, "pub.pem")
 	pubData := mustReadTestFile(t, pubPath)

legacyprobe_test.go

@@ -27,20 +27,21 @@ func mustWrapTLSRecord(t *testing.T, handshakeMsg []byte) []byte {
 
 // buildCertificateMessageBody constructs a TLS Certificate message body
 // from DER-encoded certificates.
-func buildCertificateMessageBody(certs ...[]byte) []byte {
+func buildCertificateMessageBody(t *testing.T, certs ...[]byte) []byte {
+	t.Helper()
 	var entries []byte
 	for _, cert := range certs {
 		certLen, err := checkedUint24Len(len(cert), "certificate entry")
 		if err != nil {
-			panic(err)
+			t.Fatalf("certificate entry length: %v", err)
 		}
 		entries = appendUint24(entries, certLen)
 		entries = append(entries, cert...)
 	}
 	var body []byte
 	entriesLen, err := checkedUint24Len(len(entries), "certificate message entries")
 	if err != nil {
-		panic(err)
+		t.Fatalf("certificate message entries length: %v", err)
 	}
 	body = appendUint24(body, entriesLen)
 	body = append(body, entries...)
@@ -69,10 +70,10 @@ func TestReadServerCertificates(t *testing.T) {
 	}
 
 	// Build a ServerHello handshake message (cipher 0x0033, TLS 1.2).
-	serverHello := buildMockServerHello(0x0303, 0x0033)
+	serverHello := buildMockServerHello(t, 0x0303, 0x0033)
 
 	// Build a Certificate handshake message.
-	certMsg := buildCertificateHandshakeMessage(certDER)
+	certMsg := buildCertificateHandshakeMessage(t, certDER)
 
 	// Build a Certificate handshake message that spans two records by splitting
 	// mid-message. This exercises the "incomplete message, need more records"
@@ -117,7 +118,7 @@ func TestReadServerCertificates(t *testing.T) {
 		},
 		{
 			name:    "alert record",
-			records: buildAlertRecord(),
+			records: buildAlertRecord(t),
 			wantErr: errAlertReceived,
 		},
 		{
@@ -129,12 +130,12 @@ func TestReadServerCertificates(t *testing.T) {
 		},
 		{
 			name:            "oversized TLS record",
-			records:         buildRawTLSRecord(0x16, 16641),
+			records:         buildRawTLSRecord(t, 0x16, 16641),
 			wantErrContains: "tls record too large",
 		},
 		{
 			name:            "unexpected content type",
-			records:         buildRawTLSRecord(0x17, 1), // ApplicationData
+			records:         buildRawTLSRecord(t, 0x17, 1), // ApplicationData
 			wantErrContains: "unexpected tls content type",
 		},
 	}
@@ -190,8 +191,8 @@ func TestReadServerCertificates_AlertAfterServerHello(t *testing.T) {
 	// WHY: Legacy parser must return parsed ServerHello context even when a
 	// fatal alert follows, preserving diagnostic signal for callers.
 	t.Parallel()
-	serverHello := buildMockServerHello(0x0303, 0x0033)
-	records := append(mustWrapTLSRecord(t, serverHello), buildAlertRecord()...)
+	serverHello := buildMockServerHello(t, 0x0303, 0x0033)
+	records := append(mustWrapTLSRecord(t, serverHello), buildAlertRecord(t)...)
 
 	sh, certs, err := readServerCertificates(bytes.NewReader(records))
 	if !errors.Is(err, errAlertReceived) {
@@ -236,7 +237,8 @@ func TestReadServerCertificates_PayloadLimit(t *testing.T) {
 }
 
 // buildMockServerHello builds a minimal ServerHello handshake message.
-func buildMockServerHello(version, cipherSuite uint16) []byte {
+func buildMockServerHello(t *testing.T, version, cipherSuite uint16) []byte {
+	t.Helper()
 	var body []byte
 	// Version (2 bytes).
 	body = appendUint16(body, version)
@@ -253,20 +255,21 @@ func buildMockServerHello(version, cipherSuite uint16) []byte {
 	msg := []byte{0x02}
 	bodyLen, err := checkedUint24Len(len(body), "server hello body")
 	if err != nil {
-		panic(err)
+		t.Fatalf("server hello body length: %v", err)
 	}
 	msg = appendUint24(msg, bodyLen)
 	msg = append(msg, body...)
 	return msg
 }
 
 // buildCertificateHandshakeMessage builds a TLS Certificate handshake message.
-func buildCertificateHandshakeMessage(certs ...[]byte) []byte {
-	body := buildCertificateMessageBody(certs...)
+func buildCertificateHandshakeMessage(t *testing.T, certs ...[]byte) []byte {
+	t.Helper()
+	body := buildCertificateMessageBody(t, certs...)
 	msg := []byte{0x0B} // Certificate
 	bodyLen, err := checkedUint24Len(len(body), "certificate handshake body")
 	if err != nil {
-		panic(err)
+		t.Fatalf("certificate handshake body length: %v", err)
 	}
 	msg = appendUint24(msg, bodyLen)
 	msg = append(msg, body...)
@@ -277,26 +280,28 @@ func buildCertificateHandshakeMessage(certs ...[]byte) []byte {
 // payload of payloadLen zero bytes. Unlike wrapTLSRecord it does NOT cap the
 // payload size, so it can be used to construct intentionally oversized records
 // for negative test cases.
-func buildRawTLSRecord(contentType byte, payloadLen int) []byte {
+func buildRawTLSRecord(t *testing.T, contentType byte, payloadLen int) []byte {
+	t.Helper()
 	record := []byte{contentType, 0x03, 0x03}
 	payloadSize, err := checkedUint16Len(payloadLen, "raw TLS record payload")
 	if err != nil {
-		panic(err)
+		t.Fatalf("raw TLS record payload length: %v", err)
 	}
 	record = appendUint16(record, payloadSize)
 	record = append(record, make([]byte, payloadLen)...)
 	return record
 }
 
 // buildAlertRecord builds a TLS Alert record.
-func buildAlertRecord() []byte {
+func buildAlertRecord(t *testing.T) []byte {
+	t.Helper()
 	// Alert: handshake_failure (40), fatal (2).
 	payload := []byte{0x02, 0x28}
 	record := []byte{0x15} // ContentType: Alert
 	record = append(record, 0x03, 0x01)
 	payloadLen, err := checkedUint16Len(len(payload), "alert payload")
 	if err != nil {
-		panic(err)
+		t.Fatalf("alert payload length: %v", err)
 	}
 	record = appendUint16(record, payloadLen)
 	record = append(record, payload...)
@@ -339,8 +344,8 @@ func TestLegacyFallbackConnect(t *testing.T) {
 			}
 
 			// Send ServerHello + Certificate as raw TLS records.
-			serverHello := buildMockServerHello(0x0303, 0x0033)
-			certMsg := buildCertificateHandshakeMessage(leaf.DER)
+			serverHello := buildMockServerHello(t, 0x0303, 0x0033)
+			certMsg := buildCertificateHandshakeMessage(t, leaf.DER)
 			helloDone := []byte{0x0E, 0x00, 0x00, 0x00} // ServerHelloDone
 
 			// Pack all handshake messages into a single TLS record.