Skip to content

fix: address Ralph Loop adversarial review findings - #67

Merged
danielewood merged 1 commit into
mainfrom
fix/ralph-adversarial-fixes
Feb 25, 2026
Merged

fix: address Ralph Loop adversarial review findings#67
danielewood merged 1 commit into
mainfrom
fix/ralph-adversarial-fixes

Conversation

@danielewood

Copy link
Copy Markdown
Collaborator

Summary

  • Fix WASM certkitInspect missing 30s context timeout and recover() panic guard
  • Fix showStatus inline style leak — error-red color persisted through subsequent processing updates
  • Fix ekuOIDNames missing Microsoft SGC and Netscape SGC OIDs (CSR EKU display parity with cert display)
  • Add 31 test cases for dn.go exported functions (FormatEKUs, FormatEKUOIDs, FormatKeyUsage, FormatKeyUsageBitString, ParseOtherNameSANs, FormatDN)
  • Add 5 tests for ResolveInspectAIA (no-certs passthrough, all-resolved, intermediate fetching, fetcher errors, deduplication)
  • Add 2 tests for CSR extension parsing (Key Usage and EKU from raw ASN.1)

Test plan

  • go test -race ./... passes
  • go vet ./... passes
  • golangci-lint run passes
  • GOOS=js GOARCH=wasm go build ./cmd/wasm/ passes
  • cd web && npm test passes (78 tests)
  • All pre-commit hooks pass

🤖 Generated with Claude Code

Fix WASM certkitInspect missing timeout/recovery, showStatus style leak,
and ekuOIDNames map gaps. Add comprehensive tests for dn.go exported
functions, ResolveInspectAIA, and CSR extension parsing.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 25, 2026 00:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This pull request addresses findings from a Ralph Loop adversarial code review, focusing on robustness improvements to WASM inspect functionality, UI style fixes, EKU OID coverage, and comprehensive test coverage for DN formatting functions.

Changes:

  • Fix WASM certkitInspect missing 30s context timeout and panic recovery guard
  • Fix UI style leak where error-red color persisted after status updates
  • Add missing Microsoft SGC and Netscape SGC OIDs to ekuOIDNames for CSR EKU display parity
  • Add 38 new test cases covering dn.go exported functions, AIA resolution, and CSR extension parsing

Reviewed changes

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

Show a summary per file
File Description
web/public/app.js Fix CSS color reset logic in showStatus to prevent error-red color leaking to subsequent processing updates
cmd/wasm/inspect.go Add 30-second context timeout and panic recovery to inspectFiles goroutine; improve AIA warning logging
dn.go Add Microsoft Server Gated Crypto and Netscape Server Gated Crypto OIDs to ekuOIDNames map
dn_test.go Add comprehensive tests for FormatEKUs, FormatEKUOIDs, FormatKeyUsage, FormatKeyUsageBitString, ParseOtherNameSANs, and FormatDN functions
internal/inspect_test.go Add tests for ResolveInspectAIA edge cases and CSR extension parsing (Key Usage and EKU)
CHANGELOG.md Document all fixes and new tests under Unreleased section

@danielewood
danielewood merged commit f7b0643 into main Feb 25, 2026
19 checks passed
@danielewood
danielewood deleted the fix/ralph-adversarial-fixes branch February 25, 2026 00:29
@claude

claude Bot commented Feb 25, 2026

Copy link
Copy Markdown

Code review

Issues found

CL-3 / CL-4 — Wrong commit SHA in CHANGELOG.md

All six new CHANGELOG entries and the link definition at the bottom reference commit 2b8cb8c, which does not exist in this repository. The actual commit SHA for this PR's changes is 712c8a1 (full: 712c8a16c0709d3e5121773f423f7f1bb1e2a642).

Relevant lines:

  • certkit/CHANGELOG.md

    Lines 31 to 42 in 712c8a1

    - Fix WASM `certkitInspect` missing timeout and panic recovery — add 30s context timeout and `recover()` to prevent unhandled goroutine panics ([`2b8cb8c`])
    - Fix `showStatus` style leak in web UI — error-red text color persisted after a subsequent processing status update ([`2b8cb8c`])
    - Fix `ekuOIDNames` missing Microsoft Server Gated Crypto and Netscape Server Gated Crypto OIDs — CSR EKU display now matches certificate EKU display ([`2b8cb8c`])
    - Fix AIA `progressTotal` double-counting certs whose issuer fetch fails — the same cert appeared in both `processed` and `queue` sets, inflating the progress bar total ([#64])
    ### Tests
    - Add tests for all `dn.go` exported functions: `FormatEKUs`, `FormatEKUOIDs`, `FormatKeyUsage`, `FormatKeyUsageBitString`, `ParseOtherNameSANs`, and `FormatDN` certificate round-trip (31 test cases) ([`2b8cb8c`])
    - Add tests for `ResolveInspectAIA` — no-certs passthrough, all-resolved passthrough, intermediate fetching, fetcher errors, and deduplication ([`2b8cb8c`])
    - Add tests for CSR extension parsing — Key Usage and Extended Key Usage extraction from raw ASN.1 extensions ([`2b8cb8c`])
    (six entries referencing 2b8cb8c)
  • certkit/CHANGELOG.md

    Lines 596 to 599 in 712c8a1

    [0.1.0]: https://github.com/sensiblebit/certkit/releases/tag/v0.1.0
    [`2b8cb8c`]: https://github.com/sensiblebit/certkit/commit/2b8cb8c
    [`392878a`]: https://github.com/sensiblebit/certkit/commit/392878a
    (link definition pointing to a non-existent commit)

Per CL-3 and CL-4:

CL-3 (MUST): Each entry ends with a commit ref: ([abc1234]). Use the short (7-char) commit SHA.
CL-4 (MUST): Add the corresponding link definition at the bottom of the file.

The link definition at line 598 should be:

[`712c8a1`]: https://github.com/sensiblebit/certkit/commit/712c8a1

And all six entries should reference 712c8a1 instead of 2b8cb8c.

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