fix: address post-merge review findings from PR #62

[danielewood]

Summary

Fixes 4 issues flagged by claude-review on PR #62 after it was merged:

• Bug: progressTotal overflow — AIA resolution recomputes progressTotal each depth round to include newly-discovered intermediates, preventing completed > total (which caused progress bar >100% mid-loop)
• CTX-2: context propagated — RunValidation now checks ctx.Err() between root pool load and validation checks, honoring the 30s WASM timeout
• CL-3/CL-4: stale changelog refs — Pre-rebase SHAs d8b9759/837e5e8 replaced with squash merge SHA 392878a
• T-9: validation logic testable — Extracted CheckExpiration, CheckKeyStrength, CheckSignature, CheckTrustChain from cmd/wasm/validate.go (behind //go:build js && wasm) into internal/certstore/validate.go (reachable by go test). WASM file is now a thin JS interop wrapper. Added table-driven tests covering policy thresholds.

Test plan

• go test -race -count=1 ./... passes (includes new validate_test.go)
• GOOS=js GOARCH=wasm go vet ./cmd/wasm/ && go build -o /dev/null ./cmd/wasm/
• All pre-commit hooks pass

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR addresses four post-merge review findings from PR #62 by fixing a progress bar overflow bug, adding context cancellation checks, updating stale changelog commit references, and extracting WASM validation logic into testable internal packages.

Changes:

• Fixed AIA resolution progress tracking by recalculating progressTotal each depth round to include newly-discovered intermediates
• Added context cancellation check in RunValidation between Mozilla root pool load and validation checks to honor WASM timeout constraints
• Updated CHANGELOG.md to replace pre-rebase commit SHAs with the squash merge SHA 392878a
• Extracted certificate validation logic from cmd/wasm/validate.go into internal/certstore/validate.go with comprehensive table-driven tests

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
internal/certstore/validate.go	New file containing extracted validation logic (CheckExpiration, CheckKeyStrength, CheckSignature, CheckTrustChain) now testable outside WASM build constraints
internal/certstore/validate_test.go	New comprehensive table-driven tests for all validation functions covering policy thresholds and edge cases
internal/certstore/aia.go	Fixed progress bar overflow by recalculating progressTotal each round and moving processed map before loop
cmd/wasm/validate.go	Reduced to thin JS interop wrapper by delegating to internal/certstore.RunValidation
CHANGELOG.md	Updated commit references from pre-rebase SHAs to squash merge SHA 392878a

Comments suppressed due to low confidence (3)

internal/certstore/validate_test.go:126

• The substring matching logic is unnecessarily complex. Lines 121-122 attempt to check if check.Detail starts with tt.detail, but if that fails, it falls back to containsSubstring which does a full substring search. This creates redundant logic - if you want substring matching, use strings.Contains directly. If you want prefix matching, use strings.HasPrefix. The current implementation is confusing and harder to maintain.

			if check.Detail != "" && tt.detail != "" {
				if len(check.Detail) < len(tt.detail) || check.Detail[:len(tt.detail)] != tt.detail {
					if !containsSubstring(check.Detail, tt.detail) {
						t.Errorf("CheckKeyStrength() detail = %q, want substring %q", check.Detail, tt.detail)
					}
				}
			}

internal/certstore/validate_test.go:235

• The custom containsSubstring and findSubstring helper functions unnecessarily reimplement strings.Contains from the standard library. The logic in containsSubstring is overly complex and the implementation of findSubstring duplicates what strings.Contains already does. Replace both with direct calls to strings.Contains(s, substr), which is the established pattern throughout the codebase.

func containsSubstring(s, substr string) bool {
	return len(s) >= len(substr) && (s == substr || len(s) > 0 && findSubstring(s, substr))
}

func findSubstring(s, substr string) bool {
	for i := 0; i <= len(s)-len(substr); i++ {
		if s[i:i+len(substr)] == substr {
			return true
		}
	}
	return false
}

internal/certstore/validate_test.go:111

• Test setup code silently ignores errors from rsa.GenerateKey, ecdsa.GenerateKey, and ed25519.GenerateKey. While these functions rarely fail in practice, the coding guidelines (ERR-5) require that errors never be silently ignored. In test setup code, errors should be handled with t.Fatal() or similar to fail the test early if key generation fails.

		{
			name: "RSA 2048 passes",
			cert: func() *x509.Certificate {
				key, _ := rsa.GenerateKey(rand.Reader, 2048)
				return &x509.Certificate{PublicKey: &key.PublicKey}
			}(),
			status: "pass",
			detail: "RSA 2048-bit",
		},
		{
			name: "RSA 1024 fails",
			cert: func() *x509.Certificate {
				key, _ := rsa.GenerateKey(rand.Reader, 1024) //nolint:gosec // intentionally weak for test
				return &x509.Certificate{PublicKey: &key.PublicKey}
			}(),
			status: "fail",
			detail: "RSA 1024-bit",
		},
		{
			name: "ECDSA P-256 passes",
			cert: func() *x509.Certificate {
				key, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
				return &x509.Certificate{PublicKey: &key.PublicKey}
			}(),
			status: "pass",
			detail: "ECDSA",
		},
		{
			name: "Ed25519 passes",
			cert: func() *x509.Certificate {
				pub, _, _ := ed25519.GenerateKey(rand.Reader)
				return &x509.Certificate{PublicKey: pub}
			}(),
			status: "pass",
			detail: "Ed25519",
		},
	}

[claude]

Code review

Bugs

progressTotal double-counts persistently-unresolved certs — internal/certstore/aia.go:119

certkit/internal/certstore/aia.go

Lines 117 to 121 in e011c27

	// Recount progressTotal each round to include newly-discovered
	// intermediates, preventing completed from exceeding total.
	progressTotal = len(processed) + len(queue)
	// Phase 1: Collect unique work items and pre-validate URLs.

A cert whose AIA issuer fetch fails is still added to processed unconditionally at the end of each round (every cert in queue is marked processed regardless of fetch success). On the next depth round, needsResolution still returns true for it (its issuer is still missing), so it re-enters queue. The formula progressTotal = len(processed) + len(queue) then double-counts it — the same cert appears in both sets. The if !processed[ski] guard prevents map corruption, but progressTotal is inflated by 1 per persistently-unresolved cert. The progress bar stalls mid-run and only reaches 100% via the forced final OnProgress(progressTotal, progressTotal) call.

One fix: maintain a separate totalSeen set that accumulates certs the first time they enter the queue, and use len(totalSeen) as progressTotal instead of recomputing each round.

---

CLAUDE.md violations

CL-1 (MUST): Missing changelog entries for this PR's production changes — CHANGELOG.md

certkit/CHANGELOG.md

Lines 9 to 20 in e011c27

	### Added
	- Add `certkitValidateCert` WASM function for browser-based certificate validation ([`392878a`])
	- Add concurrent AIA resolution — fetches up to `Concurrency` URLs in parallel per depth round (default 20, WASM uses 50) ([`392878a`])
	- Add `serial` field to WASM `getState()` certificate data — hex-encoded serial number ([`392878a`])
	- Add paste support to web UI drop zone — Ctrl+V / Cmd+V pastes PEM or certificate text directly without needing a file ([`392878a`])
	### Changed
	- Replace Inspect/Verify tab navigation with unified category tabs (Leaf, Intermediate, Root, Keys) — certificates are now organized by type with click-to-expand detail rows showing validation checks and metadata ([`392878a`])

The PR updates existing SHA references in ## [Unreleased] but adds no new entries for the production code changes it introduces:

• Fixed: progressTotal overflow in AIA resolution (internal/certstore/aia.go)
• Added: RunValidation, CheckExpiration, CheckKeyStrength, CheckSignature, CheckTrustChain exported to internal/certstore package
• Changed: RunValidation propagates context cancellation via ctx.Err() check

Per CL-1: "a commit that touches production code (not just tests) always needs one."

---

ERR-5 (MUST): Silently ignored errors in TestCheckKeyStrength table closures — internal/certstore/validate_test.go:79,88,97,106

certkit/internal/certstore/validate_test.go

Lines 76 to 112 in e011c27

	{
	name: "RSA 2048 passes",
	cert: func() *x509.Certificate {
	key, _ := rsa.GenerateKey(rand.Reader, 2048)
	return &x509.Certificate{PublicKey: &key.PublicKey}
	}(),
	status: "pass",
	detail: "RSA 2048-bit",
	},
	{
	name: "RSA 1024 fails",
	cert: func() *x509.Certificate {
	key, _ := rsa.GenerateKey(rand.Reader, 1024) //nolint:gosec // intentionally weak for test
	return &x509.Certificate{PublicKey: &key.PublicKey}
	}(),
	status: "fail",
	detail: "RSA 1024-bit",
	},
	{
	name: "ECDSA P-256 passes",
	cert: func() *x509.Certificate {
	key, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
	return &x509.Certificate{PublicKey: &key.PublicKey}
	}(),
	status: "pass",
	detail: "ECDSA",
	},
	{
	name: "Ed25519 passes",
	cert: func() *x509.Certificate {
	pub, _, _ := ed25519.GenerateKey(rand.Reader)
	return &x509.Certificate{PublicKey: pub}
	}(),
	status: "pass",
	detail: "Ed25519",
	},
	}

The anonymous closures that initialize cert fields in the table all discard errors with _:

key, _ := rsa.GenerateKey(rand.Reader, 2048)
key, _ := rsa.GenerateKey(rand.Reader, 1024)
key, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
pub, _, _ := ed25519.GenerateKey(rand.Reader)

If any key generation fails, the closure returns &x509.Certificate{PublicKey: nil} and the test proceeds silently with a nil public key, producing a misleading result. The closures cannot call t.Fatal (not in test goroutine scope). The established pattern in this codebase (see testhelpers_test.go) is to use named helper functions with t.Helper() and proper t.Fatal error handling.

Per ERR-5: "Never silently ignore errors."

---

T-12 (MUST): Two separate TestCheckTrustChain functions instead of a single table-driven test — internal/certstore/validate_test.go:157-219

certkit/internal/certstore/validate_test.go

Lines 157 to 219 in e011c27

	func TestCheckTrustChain_NilRoots(t *testing.T) {
	t.Parallel()
	cert := &x509.Certificate{
	Subject: pkix.Name{CommonName: "test.example.com"},
	}
	checks := CheckTrustChain(CheckTrustChainInput{
	Leaf: cert,
	Roots: nil,
	Now: time.Now(),
	})
	if len(checks) != 2 {
	t.Fatalf("expected 2 checks, got %d", len(checks))
	}
	for _, c := range checks {
	if c.Status != "fail" {
	t.Errorf("check %q: status = %q, want fail", c.Name, c.Status)
	}
	}
	}
	func TestCheckTrustChain_SelfSigned(t *testing.T) {
	// WHY: A self-signed leaf that is not a Mozilla root should fail trust
	// chain verification — it cannot build a chain to a trusted root.
	t.Parallel()
	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
	if err != nil {
	t.Fatal(err)
	}
	template := &x509.Certificate{
	SerialNumber: randomSerial(t),
	Subject: pkix.Name{CommonName: "self-signed.example.com"},
	NotBefore: time.Now().Add(-time.Hour),
	NotAfter: time.Now().Add(24 * time.Hour),
	KeyUsage: x509.KeyUsageDigitalSignature,
	BasicConstraintsValid: true,
	}
	certBytes, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key)
	if err != nil {
	t.Fatal(err)
	}
	cert, err := x509.ParseCertificate(certBytes)
	if err != nil {
	t.Fatal(err)
	}
	roots := x509.NewCertPool()
	checks := CheckTrustChain(CheckTrustChainInput{
	Leaf: cert,
	Roots: roots,
	Now: time.Now(),
	})
	if len(checks) != 2 {
	t.Fatalf("expected 2 checks, got %d", len(checks))
	}
	if checks[0].Status != "fail" {
	t.Errorf("Trust Chain: status = %q, want fail", checks[0].Status)
	}
	if checks[1].Status != "fail" {
	t.Errorf("Trusted Root: status = %q, want fail", checks[1].Status)
	}
	}

CheckTrustChain is covered by two top-level functions (TestCheckTrustChain_NilRoots, TestCheckTrustChain_SelfSigned) rather than a single table-driven test. Both assert the same property (exactly 2 fail checks returned) under different inputs. These are directly consolidatable into a single TestCheckTrustChain with named subtests.

Per T-12: "One parametric test over N inputs, not N copy-paste tests."