Skip to content

chore: merge develop into main#110

Merged
danielewood merged 8 commits intomainfrom
develop
Mar 2, 2026
Merged

chore: merge develop into main#110
danielewood merged 8 commits intomainfrom
develop

Conversation

@danielewood
Copy link
Copy Markdown
Collaborator

)

* fix(wasm): harden ingest and make unverified export explicit

* fix: address PR 105 reliability review feedback

* fix(web): enforce upstream timeout through body reads

* fix: address latest PR 105 review feedback

* fix(crl): clarify read errors and harden size-limit coverage
* fix: harden parsing and issuer/key selection correctness

* docs(changelog): reference PR for unreleased parsing fixes

* fix: prevent parser fallback and JKS identity regressions

* fix(inspect): preserve valid keys when PEM bundle has malformed blocks

* fix: address remaining PR 107 review feedback

* fix(jks): surface skipped-entry reasons in debug logs
* fix(scan): keep traversal bounded and restore export summaries

* fix(scan): fail fast on walker processing errors

* fix(scan): use typed max-size errors in read paths

* fix(scan): reject invalid export formats consistently

* fix(scan): keep export destination off stdout
…timeout (#108)

* fix(network): harden revocation fetch SSRF checks and connect timeout defaults

* fix(network): propagate SSRF validation deadlines and unblock inspect AIA opt-in

* fix(bundle): restore private-network opt-in for AIA chain fetches

* fix(network): address remaining PR feedback for inspect AIA handling

* fix(wasm): keep AIA resolution working without DNS lookups
#109)

* fix(cli): align validation exits and secure export defaults

* fix(cli): address PR #109 review feedback

* fix(tests): align PR #109 follow-up feedback
Copilot AI review requested due to automatic review settings March 2, 2026 01:09
@danielewood
Copy link
Copy Markdown
Collaborator Author

Review loop pass started for this PR. I’m actively monitoring and will address incoming review feedback quickly, including code updates and follow-up replies in-thread.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Merges develop into main, bringing in the latest hardened parsing/ingestion, SSRF protections, CLI/JSON normalization, and related test/Docs/Changelog updates across the Go library, CLI, WASM build, and web proxy/UI.

Changes:

  • Harden network fetch + SSRF validation (AIA/OCSP/CRL) with explicit --allow-private-network opt-in across CLI/WASM/web.
  • Improve container/parsing behavior (malformed PEM tolerance, DER private key handling, JKS key-entry alias/chain pairing) and add targeted regression tests.
  • Normalize CLI outputs/semantics (JSON schema keys, export password requirements, scan text summaries) and update docs/changelog accordingly.

Reviewed changes

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

Show a summary per file
File Description
web/public/index.html Add private-network opt-in checkboxes in scan/inspect UI
web/public/app.js Add AIA fetch timeout handling + export retry UX + pass allow-private flag into WASM
web/functions/api/fetch.ts Add upstream timeout/abort handling and cleanup in AIA proxy
web/functions/api/fetch.test.ts Add tests for upstream abort/timeout behaviors
sign.go Add CA cert/key mismatch validation during CSR signing
sign_test.go Add test coverage for CA cert/key mismatch behavior
ocsp.go Thread allow-private-network option through OCSP validation/redirect handling
ocsp_test.go Update tests for new allow-private behavior + blocked-by-default case
jks.go Add DecodeJKSKeyEntries preserving alias/key/chain pairing + debug logs
jks_test.go Add tests asserting alias/chain pairing preservation
internal/verify.go Add allow-private flag; rename diagnosesdiagnostics; adjust status semantics
internal/verify_test.go Update diagnoses expectations + add JSON schema regression test
internal/testhelpers_test.go Add symlink creation helper for portable tests
internal/scanwalk.go Introduce bounded scan walker with symlink/root boundary enforcement
internal/scanwalk_test.go Add walker tests (symlink boundary, size checks, error propagation)
internal/io.go Introduce sentinel size-limit errors + exported ReadFileLimited
internal/io_test.go Update tests to assert stable wrapped size-limit errors
internal/inspect.go Harden PEM/DER inspection (malformed blocks, DER key parsing) + AIA options
internal/inspect_test.go Add tests for malformed+valid PEM mixes and DER key inputs
internal/format.go Add shared scan text summary formatter
internal/format_test.go Add tests for scan text summary formatting
internal/exporter_test.go Ensure YAML bundle outputs are treated as sensitive
internal/certstore/memstore.go Adjust missing-AKI identity dedup to include issuer+serial behavior
internal/certstore/memstore_test.go Add tests for missing-AKI issuer identity dedup behavior
internal/certstore/sqlite.go Persist missing-AKI authority identity to avoid cross-issuer collisions
internal/certstore/sqlite_test.go Add SQLite round-trip test for missing-AKI identity preservation
internal/certstore/export.go Require explicit PKCS#12 password; mark .yaml outputs sensitive
internal/certstore/export_test.go Add tests for required P12 password + YAML sensitivity
internal/certstore/container.go Improve container parsing (JKS pairing, DER key-only handling)
internal/certstore/container_test.go Add tests for DER key-only + JKS pairing selection
internal/certstore/aia.go Thread allow-private-network into AIA URL validation
internal/certstore/aia_test.go Update AIA resolution tests for allow-private-network option
crl.go Add CRL size cap enforcement + ReadCRLFile helper
crl_test.go Add/adjust tests for SSRF + size limit behavior
connect.go Add default connect timeout when ctx has no deadline + allow-private plumbing
connect_test.go Add test for connect-timeout fallback + allow-private updates
cmd/wasm/main.go Add bounded WASM ingestion limits + allow-private + export retry signaling
cmd/wasm/inspect.go Add bounded WASM inspect ingestion + allow-private AIA resolution
cmd/wasm/export.go Require explicit verified export; support explicit unverified retry
cmd/wasm/aia.go Propagate timeouts/cancellation to JS fetch + release callbacks reliably
cmd/certkit/root.go Add --insecure-default-password global flag
cmd/certkit/verify.go Add --allow-private-network + rename diagnoses→diagnostics
cmd/certkit/connect.go Add --allow-private-network and plumb to library
cmd/certkit/scan.go Use bounded walker; add allow-private flag; improve export text summary
cmd/certkit/inspect.go Add allow-private flag and normalize expired filtering to validation error
cmd/certkit/ocsp.go Add allow-private flag; issuer auto-selection improvements; JSON key normalization
cmd/certkit/crl.go Use certkit.ReadCRLFile for local CRLs
cmd/certkit/bundle.go Normalize JSON payload schema + explicit export password requirement
cmd/certkit/convert.go Normalize JSON payload schema + explicit export password requirement
cmd/certkit/convert_test.go Add CLI-level regression for PKCS#12 multi-match error semantics
cmd/certkit/payload_json.go Introduce shared payload JSON struct for bundle/convert
cmd/certkit/cli_semantics_test.go Add JSON schema consistency regression tests
certkit.go Harden PEM parsing (skip malformed blocks; scan multiple blocks) + issuer selection helper
certkit_test.go Add tests for malformed PEM tolerance + issuer selection + CSR parsing improvements
bundle.go Add DNS-resolution SSRF validation + allow-private option plumbing
bundle_test.go Update AIA fetch test for allow-private behavior
bundle_lookup_default.go Provide default DNS lookup for SSRF validation (non-js builds)
bundle_lookup_js.go Disable DNS resolution on js builds safely
README.md Update flags/docs for SSRF opt-in, password requirements, formats, outputs
EXAMPLES.md Update examples for explicit export password requirement
CHANGELOG.md Document security/behavior changes and breaking JSON schema changes

Comment thread cmd/wasm/main.go
Comment thread cmd/certkit/cli_semantics_test.go
Comment thread cmd/certkit/convert_test.go
Comment thread cmd/certkit/scan.go
Comment thread web/functions/api/fetch.ts Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b64a1db0ae

ℹ️ 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".

Comment thread cmd/certkit/scan.go Outdated
Copilot AI review requested due to automatic review settings March 2, 2026 01:20
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated 1 comment.

Comment thread jks.go
@claude

This comment has been minimized.

@danielewood
Copy link
Copy Markdown
Collaborator Author

Addressed review feedback in on :\n\n- fixed WASM marshal-error rejection to use a real object ()\n- updated test guidance to explicitly allow scoped CLI command-surface tests under \n- restored AIA HTTP client reuse in scan path by caching one client per private-network mode ()\n- renamed TS type alias to PascalCase () in web proxy fetch code\n- stopped logging trusted JKS entries as non-private-key by continuing after trusted-cert handling ()\n- corrected changelog reference for explicit PKCS#12/JKS password requirement from to ()\n\nAlso reran 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 locally; all hooks are passing.

@danielewood
Copy link
Copy Markdown
Collaborator Author

Addressed review feedback in f1974c7 on develop:

  • fixed WASM addFiles marshal-error rejection to use a real Error object (cmd/wasm/main.go)
  • updated CLAUDE.md test guidance to explicitly allow scoped CLI command-surface tests under cmd/certkit/*_test.go
  • restored AIA HTTP client reuse in scan path by caching one client per private-network mode (cmd/certkit/scan.go)
  • renamed TS type alias to PascalCase (SafeFetchResult) in web proxy fetch code
  • stopped logging trusted JKS entries as "non-private-key" by continuing after trusted-cert handling (jks.go)
  • corrected changelog reference for explicit PKCS#12/JKS password requirement from [#87] to [#89] (CHANGELOG.md)

Also reran pre-commit run --all-files locally; all hooks are passing.

@danielewood
Copy link
Copy Markdown
Collaborator Author

Review loop pass complete: there are no unresolved actionable reviewer/bot threads or comments at this time. Waiting for new feedback.

@claude
Copy link
Copy Markdown

claude bot commented Mar 2, 2026

test

@claude
Copy link
Copy Markdown

claude bot commented Mar 2, 2026

Removing test comment

@claude
Copy link
Copy Markdown

claude bot commented Mar 2, 2026

test2

@claude
Copy link
Copy Markdown

claude bot commented Mar 2, 2026

removing placeholder

@claude
Copy link
Copy Markdown

claude bot commented Mar 2, 2026

Code review

5 issues found (all MUST-severity CLAUDE.md violations).


1. ERR-5: continue without slog.Debug in ParsePEMPrivateKey and ParsePEMPrivateKeyWithPasswords

certkit.go#L115-L116 · certkit.go#L171-L172

ERR-5 (MUST): "Never silently ignore errors. Use continue in loops only with a slog.Debug explaining why."

The new PEM-parsing loops skip non-key PEM blocks without any debug log. All other continue paths in these same functions (malformed blocks, decryption failures, OpenSSH) correctly pair with slog.Debug.

Fix (apply to both line 116 and line 172):

if !keyBlockTypes[block.Type] {
    slog.Debug("skipping non-key PEM block", "type", block.Type)
    continue
}

2. ERR-5: continue without slog.Debug in ParsePEMCertificateRequest

certkit.go#L311-L312

ERR-5 (MUST): non-CSR PEM blocks are skipped silently. The very next continue in this function (line 320, for malformed CSR DER) correctly includes slog.Debug.

Fix:

if block.Type != "CERTIFICATE REQUEST" && block.Type != "NEW CERTIFICATE REQUEST" {
    slog.Debug("skipping non-CSR PEM block", "type", block.Type)
    continue
}

3. ERR-5: two continue statements without slog.Debug in SelectIssuerCertificate

certkit.go#L635-L640

ERR-5 (MUST): both the nil-candidate guard (line 635) and issuer-DN mismatch check (line 638) lack slog.Debug. The signature-mismatch continue at line 642 correctly pairs with slog.Debug, making this inconsistency visible.

Fix:

if candidate == nil {
    slog.Debug("skipping nil candidate issuer")
    continue
}
if !bytes.Equal(cert.RawIssuer, candidate.RawSubject) {
    slog.Debug("skipping candidate with mismatched issuer DN", "candidate_subject", candidate.Subject.String())
    continue
}

4. ERR-5: continue without slog.Debug in PEM key inspection loop

internal/inspect.go#L105-L106

ERR-5 (MUST): non-key PEM blocks are silently skipped. The next continue in this loop (line 112, for malformed key parsing) correctly pairs with slog.Debug.

Fix:

if !isPrivateKeyPEMBlockType(block.Type) {
    slog.Debug("skipping non-key PEM block during inspect", "type", block.Type)
    continue
}

5. CS-5: resolveAIA takes 3 arguments without an input struct

cmd/wasm/aia.go#L21

CS-5 (MUST): "Use input structs for functions receiving more than 2 arguments. Input contexts must not go in the input struct."

Adding allowPrivateNetworks bool brings the total parameter count to 3. Context counts toward the threshold; the carve-out only governs placement. This PR correctly applies the pattern for exportBundles (exportBundlesInput) and readWASMFileData (readWASMFileDataInput) in the same file — resolveAIA should follow suit.

Fix:

type resolveAIAInput struct {
    Store                *certstore.MemStore
    AllowPrivateNetworks bool
}

func resolveAIA(ctx context.Context, in resolveAIAInput) []string {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants