fix: harden export paths and outputs

[danielewood]

Summary

• Harden bundle export safety by sanitizing folder names, enforcing safe path joins, and detecting sanitized-folder collisions before writing.
• Improve robustness with size-limited input reads, fail-fast export error handling, and wrapped IO errors for clearer diagnostics.
• Normalize output contracts by standardizing serial formatting (0x hex) and verify-chain JSON (not_after) for consistent machine parsing.
• Keep PKCS#12/JKS interoperability defaults (changeit) while preferring user-provided passwords and processing decode/export password sets in a single pass.
• Expand test coverage for IO limit enforcement, password-set behavior, fail-fast export paths, and folder-collision handling.

[Copilot]

Pull request overview

This PR hardens certificate/key ingestion and bundle export paths, tightens PKCS#12/JKS password handling, and standardizes serial / JSON output formatting across the CLI, internal packages, and WASM entrypoints.

Changes:

• Add size-limited input readers and use them for file/stdin ingestion to avoid unbounded reads.
• Harden bundle export directory handling (sanitize folder names, safe join) and require explicit passwords for PKCS#12/JKS outputs.
• Standardize serial formatting (0x-prefixed hex) and align verify/scan JSON field naming (not_after, scan export JSON).

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
ocsp_test.go	Update serial formatting expectations
ocsp.go	Use shared serial formatter
internal/verify_test.go	Update chain JSON fields + serial expectations
internal/verify.go	Align chain JSON (not_after), bundle API update
internal/passwords.go	Wrap file/scan errors with context
internal/keygen_test.go	Update GenerateKey call sites to input struct
internal/keygen.go	Introduce GenerateKeyInput + wrap keygen errors
internal/io.go	Add size-limited read helpers
internal/inspect_test.go	Update serial expectations + ResolveInspectAIA signature
internal/inspect.go	Size-limited reads + ResolveInspectAIAInput; handle ingest errors
internal/exporter_test.go	Update ExportBundles signature and P12 password
internal/exporter.go	Safe join for folders; sanitize bundle folders; pass P12 password
internal/csr.go	Update GenerateKey usage + wrap CSR parse errors
internal/crypto.go	Add size limits + input structs for ProcessFile/ProcessData
internal/container.go	Use size-limited file reads
internal/certstore/process_test.go	Provide P12 password in round-trip export test
internal/certstore/helpers_test.go	Add tests for bundle folder sanitization
internal/certstore/helpers.go	Add SanitizeBundleFolder helper
internal/certstore/export_test.go	Add P12Password to export inputs; update writer-error assertion; PKCS#12 round-trip password
internal/certstore/export.go	Skip .p12 without password; serial formatting in JSON; pass P12Password through export
internal/bundleconfig.go	Wrap config read/parse errors
internal/archive.go	Pass per-entry size limits into ProcessData
connect_test.go	Update OCSP/CRL serial expectations
connect.go	Use input struct for AIA fetch; serial formatting in CRL detail
cmd/wasm/main.go	Add timeout for addFiles; use shared serial formatter
cmd/wasm/inspect.go	Add timeout for inspect; update ResolveInspectAIA signature
cmd/certkit/verify.go	Quiet validation errors when emitting JSON
cmd/certkit/sign.go	Handle MarkFlagRequired errors explicitly (panic on programmer error)
cmd/certkit/scan.go	Use ProcessFileInput; pass P12Password to export; add scan export JSON output
cmd/certkit/ocsp.go	Quiet validation errors when emitting JSON
cmd/certkit/inspect.go	Update ResolveInspectAIA signature
cmd/certkit/errors.go	Add doc comment for Error()
cmd/certkit/crl.go	Serial formatting + quiet validation for JSON
cmd/certkit/convert.go	Require explicit password for PKCS#12/JKS outputs
cmd/certkit/connect.go	Quiet validation errors when emitting JSON; serial formatting via helper
cmd/certkit/bundle.go	Bundle API update; require explicit password for p12/jks outputs
certkit.go	Add FormatSerialNumber; wrap previously-unwrapped errors
bundle_test.go	Update Bundle/FetchLeafFromURL/FetchAIACertificates APIs; add non-https URL error test
bundle.go	Convert to input structs; validate URL scheme/hostname; nil guard for AIA fetch; bundle input struct
README.md	Update docs for password-required PKCS#12; Bundle API example update
EXAMPLES.md	Update examples to require explicit p12/jks password
CHANGELOG.md	Document breaking password change, serial formatting, scan export JSON; add PR reference placeholder
.claude/rules/adversarial-review-cycle.md	Add automation rule doc for adversarial review cycle

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d3298725ab

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[danielewood]

Addressed the current review feedback in 32aa489:\n\n- Enforced explicit export passwords using user-provided values only (no implicit defaults from decode password list) in bundle/convert/scan.\n- Preserved WASM export behavior by allowing an optional export password arg and defaulting to when omitted.\n- Updated and to wrap read/stat errors with context.\n- Switched export flows to fail fast on hard errors (ERR-6) and kept debug-only continues for intentional skips.\n- Updated export tests for fail-fast behavior.\n\nAll checks pass via no commit to main........................................................Passed
branch name..............................................................Passed
goimports................................................................Passed
go fix...................................................................Passed
go vet...................................................................Passed
golangci-lint............................................................Passed
wasm.....................................................................Passed
go build.................................................................Passed
go test..................................................................Passed
govulncheck..............................................................Passed
gendocs..................................................................Passed
prettier.................................................................Passed
vitest...................................................................Passed
wrangler build...........................................................Passed
markdownlint.............................................................Passed.

[danielewood]

Follow-up with formatting fixed: addressed in commit 32aa489. Implemented explicit export-password selection from user-provided inputs only; preserved WASM PKCS#12 export behavior with optional export password argument; wrapped read/stat errors with context in internal IO helpers; changed export paths to fail fast on hard errors and kept debug-only continues for intentional skips; updated export tests accordingly. pre-commit run all-files passes.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 32aa489d9e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[danielewood]

Addressed latest review comments in 3b43763 and pushed:\n- switched to single-pass password loading (ProcessPasswordSets) to avoid double-reading password files\n- kept intentional changeit PKCS#12/JKS fallback with explicit in-code comments\n- added path-sanitization collision detection for export folders\n- added docs/comments + tests for size-limited IO helpers\n- updated changelog wording for the password behavior and marked serial format as breaking\n\nAll checks pass via pre-commit run --all-files.

[Copilot]

Pull request overview

Copilot reviewed 45 out of 45 changed files in this pull request and generated 2 comments.

[Copilot]

README says .p12 export "requires --passwords/--password-file", but the current export implementation still falls back to the default password ("changeit") when no explicit password is provided. Please align documentation with actual behavior, or change the export path to error/skip .p12 when no user password is given (per PR description).

[Copilot]

GenerateBundleFiles defaults an empty P12Password to "changeit", which conflicts with the PR description/README language about requiring an explicit password (or skipping .p12 output when missing). Please pick one behavior and make code + docs consistent (either remove the fallback and conditionally omit/error, or document the fallback clearly).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b4a5e68e14

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Reserve sanitized folders only for writable bundles

exportBundleCerts records input.UsedFolders[folder] before it checks whether the certificate has a matching private key, so a bundle that is ultimately skipped (keyRec == nil) can still claim a sanitized folder name and cause a later, valid bundle to fail with a collision error. This creates false-positive export failures when two bundle names sanitize to the same path but only one is actually exportable.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve user-supplied whitespace in export passwords

deduplicateNonEmptyPasswords calls strings.TrimSpace, which changes explicit passwords before they are used for PKCS#12/JKS export. If a user intentionally provides a password with leading/trailing spaces (via --password or password file), the generated artifact is encrypted with a different password than the user supplied, making later imports fail unless the trimmed variant is used.

Useful? React with 👍 / 👎.

[claude]

Code review

Issues found

T-11 + T-2 (MUST): internal/io_test.go

• TestReadAllLimited and TestReadFileLimited directly test unexported helpers instead of testing through public callers (ProcessFile, InspectFile, LoadContainerFile). Tests should verify size-limit behavior via the exported API.
• TestReadFileLimited uses sequential inline assertions instead of a table-driven structure, violating T-2.

T-8 (MUST): internal/exporter.go — safeJoin traversal inputs untested

• The path traversal guard (../ escapes, absolute paths) has no test coverage. Add a behavioral ExportBundles test case with traversal-containing folder names to exercise this security boundary end-to-end.

See inline comments for details.

[claude]

[Inline comment on internal/io_test.go lines 11-71]

T-11 + T-2: Tests of unexported functions should be behavioral

T-11 (MUST): CLAUDE.md states: "If an unexported helper is already exercised through its public caller, do not add a direct test for the helper. Remove direct tests of unexported functions when behavioral coverage exists."

Both TestReadAllLimited and TestReadFileLimited test the unexported helpers directly. The size-limiting behavior should instead be tested through the public callers that use these helpers: ProcessFile, InspectFile, and LoadContainerFile. For example, TestProcessFile_OversizedInput that passes a file exceeding defaultMaxInputBytes would cover the behavior end-to-end.

T-2 (MUST): TestReadFileLimited is not table-driven — it uses sequential inline assertions for two distinct scenarios (over-limit rejection and within-limit success). Per T-2, use a table-driven pattern with t.Run named subtests, consistent with TestReadAllLimited in the same file.

The fix is to:

1. Replace these tests with behavioral tests against ProcessFile/InspectFile/LoadContainerFile that verify size-limit errors are returned for oversized inputs.
2. If TestReadFileLimited is kept (pending the T-11 fix), convert it to a table-driven test.

[claude]

[Inline comment on internal/exporter.go lines 43-66]

T-8: Path traversal test coverage missing for safeJoin

T-8 (MUST): CLAUDE.md requires tests to cover edge cases including "corrupted or invalid input data." This function is the core path-traversal guard, and its rejection paths (absolute paths like /etc/passwd and ../-escape sequences like ../../../../etc/shadow) are not exercised anywhere in the test suite.

Note: per T-11, don't add a direct TestSafeJoin — instead add a behavioral TestExportBundles table entry that passes a bundle with a traversal-containing folder name (e.g., "../escape" or "/absolute/path") and asserts that ExportBundles returns an error. This verifies the security boundary end-to-end through the public API.

[claude]

Code review

Two issues found.

CS-5 (MUST): exportBundles needs an input struct — cmd/wasm/export.go:19

The PR adds a fourth parameter p12Password string to exportBundles, bringing the non-context parameter count to 3 (s, filterSKIs, p12Password). Per CLAUDE.md CS-5, any function with more than 2 non-context arguments must use an input struct.

The rest of the PR correctly introduces input structs for the same pattern (ExportBundlesInput, ProcessDataInput, ProcessFileInput), but the WASM exportBundles was missed. It should follow the same pattern:

type exportBundlesInput struct {
    Store       *certstore.MemStore
    FilterSKIs  []string
    P12Password string
}

func exportBundles(ctx context.Context, input exportBundlesInput) ([]byte, error) { ... }

---

ERR-2 (MUST): Bare fmt.Errorf forces string-matching in tests — internal/exporter.go:166

exportBundleCerts returns:

return fmt.Errorf("sanitized bundle folder collision: %q and %q both map to %q", ...)

Because there is no sentinel error, the test in internal/exporter_test.go:225 is forced to check:

if !strings.Contains(err.Error(), "sanitized bundle folder collision") {

This is exactly the pattern CLAUDE.md ERR-2 prohibits: "Use errors.Is/errors.As for control flow; no string matching."

The fix is to define a package-level sentinel and wrap it with %w:

var errBundleFolderCollision = errors.New("sanitized bundle folder collision")

// in exportBundleCerts:
return fmt.Errorf("%w: %q and %q both map to %q", errBundleFolderCollision, previousBundle, input.BundleName, folder)

Then the test becomes:

if !errors.Is(err, errBundleFolderCollision) {

The same pattern applies to the size-limit errors in internal/io.go ("input exceeds max size", "file exceeds max size") which are also matched by string in internal/io_test.go.