fix(dashmate): use header authentication for ZeroSSL - #4419
Conversation
Move ZeroSSL credentials out of request URLs and into the recommended ApiKey Authorization header. Centralize header validation and sanitize reflected credential material before constructing errors. Test would have caught this in CI: ✖ 17 focused failures before fix, ✔ 20 focused tests after fix; ✔ 178 Dashmate unit tests.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
🕓 Ready for review — 1 ahead in queue (commit 3a0d8eb) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head cde8874, the production diff removes the ZeroSSL API key from all seven request URLs, adds one canonical case-insensitive Authorization header without mutating caller options, and sanitizes application-controlled response errors. The endpoint behavior and focused tests align with the PR goal, and no in-scope correctness or security findings were identified.
Source: Codex general reviewer backend gpt-5.6-sol; Codex security-auditor reviewer backend gpt-5.6-sol; final Codex verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The ZeroSSL authentication changes correctly remove credentials from request URLs, centralize header authentication, preserve endpoint behavior, and sanitize response-derived errors. One in-scope documentation suggestion remains: delete the accidentally committed internal migration progress log, which contains stale and contradictory implementation states.
Source: Codex general reviewer backend gpt-5.6-sol; Codex security-auditor reviewer backend gpt-5.6-sol; final Codex verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `docs/ZEROSSL_HEADER_AUTH_STATUS.md`:
- [SUGGESTION] docs/ZEROSSL_HEADER_AUTH_STATUS.md:1-84: Remove the internal migration progress log
This file is a chronological implementation log rather than durable project documentation. It preserves superseded states such as `BLOCKED`, says that source and tests were untouched, claims no commit or PR was made, and refers to a migration specification that the final commit removes. Those statements contradict the completed implementation and can mislead readers; delete this status artifact together with the removed specification.
| STATUS: PROGRESS | ||
| WHAT: Verified the requested branch and base commit; beginning official-doc and repository-flow research. | ||
| NEED: nothing | ||
| NEXT: Verify ZeroSSL's current authentication contract and enumerate every Dashmate call, caller, helper, and test. | ||
|
|
||
| STATUS: FINDING | ||
| WHAT: ZeroSSL's current public REST overview and endpoint pages still require access_key query parameters; no public official page documenting Authorization: ApiKey was discoverable. | ||
| NEED: nothing | ||
| NEXT: Record the evidence conflict explicitly and design a conditional implementation gate around the newer announcement. | ||
|
|
||
| STATUS: FINDING | ||
| WHAT: Confirmed seven production wrappers and no existing wrapper/requestApi unit coverage; revokeCertificate currently has no repository caller. | ||
| NEED: nothing | ||
| NEXT: Compare authentication-injection seams and specify parameterized red-to-green request-contract coverage. | ||
|
|
||
| STATUS: PROGRESS | ||
| WHAT: Chose centralized header injection in the ZeroSSL-only requestApi helper; endpoint signatures and upstream callers remain unchanged. | ||
| NEED: nothing | ||
| NEXT: Write the evidence-gated design, exact request matrix, failure analysis, and TDD contract into the spec. | ||
|
|
||
| STATUS: DONE | ||
| WHAT: Completed the repository-grounded ZeroSSL header-auth spec with official-source conflict, centralized design, forward-port notes, and red-to-green request matrix. | ||
| NEED: Canonical ZeroSSL announcement or written support confirmation before implementation. | ||
| NEXT: Waiting for parent-orchestrated independent spec review and Ivan's alignment; no code work is authorized. | ||
|
|
||
| STATUS: PROGRESS | ||
| WHAT: Started independent coherence, feasibility, security, adversarial, and product reviews of the spec in two waves. | ||
| NEED: Nothing. | ||
| NEXT: Verify and fold review findings, then explain the reviewed plan and request Ivan's alignment before coding. | ||
|
|
||
| STATUS: FINDING | ||
| WHAT: Review confirmed invalid header errors can echo the key, case-variant Authorization values can combine, and reflected response errors can reach logs. | ||
| NEED: Nothing. | ||
| NEXT: Specify case-insensitive header ownership, generic local failures, exact-key response redaction, and adversarial tests. | ||
|
|
||
| STATUS: FINDING | ||
| WHAT: Public ZeroSSL docs still conflict with the supplied announcement; reviewers required a named evidence owner, terminal deferral path, and rollout validation. | ||
| NEED: Canonical ZeroSSL announcement or written ZeroSSL support confirmation. | ||
| NEXT: Fold the gate and operational decision path into the reviewed spec, then wait for Ivan's alignment and evidence. | ||
|
|
||
| STATUS: FINDING | ||
| WHAT: Second-pass review found that redaction must precede Error construction and malformed JSON parse errors must be replaced before they can quote reflected key material. | ||
| NEED: Nothing. | ||
| NEXT: Tighten the response-boundary contract and adversarial tests, then complete the reviewed-plan sync. | ||
|
|
||
| STATUS: BLOCKED | ||
| WHAT: Completed two-pass multi-agent review and folded verified findings; no production or test code was changed. | ||
| NEED: Canonical ZeroSSL announcement or written support confirmation, plus Ivan's explicit alignment with the reviewed plan. | ||
| NEXT: After both gates clear, write and run the failing request-contract tests, implement the centralized migration, verify green, and run independent code review. | ||
|
|
||
| STATUS: BLOCKED | ||
| WHAT: Ivan requested implementation and aligned on proceeding, but the mandatory ZeroSSL contract evidence gate remains unsatisfied; source and tests are untouched. | ||
| NEED: Canonical ZeroSSL announcement/account notice showing the exact header, or written ZeroSSL support confirmation for the scoped endpoints. | ||
| NEXT: On receipt, start with the failing request-contract tests, then implement, verify, and independently review the code. | ||
|
|
||
| STATUS: DONE | ||
| WHAT: Verified ZeroSSL's official 2026-08-17 update: header auth is live, URL access_key is deprecated, and the exact exclusive prefix is Authorization: ApiKey. | ||
| NEED: Nothing. | ||
| NEXT: Begin TDD implementation with the focused request-contract test failing against untouched production code. | ||
|
|
||
| STATUS: PROGRESS | ||
| WHAT: Added the focused request contract and confirmed 17 failures against untouched production code, including all seven credential-bearing URLs and error-boundary leaks. | ||
| NEED: Nothing. | ||
| NEXT: Implement centralized header injection and remove access_key from the seven endpoint URLs, then rerun the identical test. | ||
|
|
||
| STATUS: FINDING | ||
| WHAT: Independent code review found reflected error member names and leading key whitespace were not covered; both new regression cases failed before the follow-up fix. | ||
| NEED: Nothing. | ||
| NEXT: Redact member names, reject leading/trailing key whitespace, and rerun focused tests and lint. | ||
|
|
||
| STATUS: DONE | ||
| WHAT: Implemented header-only auth for all seven wrappers; 20 focused and 32 ZeroSSL unit tests pass, Dashmate lint has zero errors, and production access_key search is empty. | ||
| NEED: Full Dashmate unit discovery still requires the missing wasm-dpp build, which is blocked by a confirmed sccache Operation not permitted failure. | ||
| NEXT: Complete final post-fix review and hand off the uncommitted implementation; deployment still requires the spec's header-only read-only smoke check. | ||
|
|
||
| STATUS: DONE | ||
| WHAT: Independent post-fix security and test review is clean after strengthening Certificate conversion and sanitized-error shape assertions. | ||
| NEED: Nothing for the scoped implementation; the full-suite WASM/sccache prerequisite and deployment smoke check remain explicit handoff items. | ||
| NEXT: Waiting for commit/PR instructions; no commit, push, PR, or v4.2-dev change was made. | ||
|
|
||
| STATUS: DONE | ||
| WHAT: Rebuilt wasm-dpp with Homebrew LLVM and ran the complete Dashmate unit suite outside the sandbox; all 178 tests pass. | ||
| NEED: Nothing for repository verification; the deployment smoke check remains an operational handoff. | ||
| NEXT: Waiting for commit/PR instructions; no commit, push, PR, or v4.2-dev change was made. |
There was a problem hiding this comment.
🟡 Suggestion: Remove the internal migration progress log
This file is a chronological implementation log rather than durable project documentation. It preserves superseded states such as BLOCKED, says that source and tests were untouched, claims no commit or PR was made, and refers to a migration specification that the final commit removes. Those statements contradict the completed implementation and can mislead readers; delete this status artifact together with the removed specification.
source: ['codex']
2 participants
Issue being fixed or feature implemented
ZeroSSL now recommends authenticating API requests with
Authorization: ApiKey <access-key>because query-string credentials can leak through URL logs. Dashmate still placed its ZeroSSL key in every REST request URL.Official announcement: https://zerossl.com/updates#publications/api-users-access-key-via-header-is-now-the-recommended-approach-for-authentication
What was done?
This change is independent of #4415 and does not depend on its nullable-expiry behavior.
How Has This Been Tested?
yarn workspace dashmate exec mocha test/unit/ssl/zerossl/apiRequests.spec.js— 20 passing.yarn workspace dashmate test:unit— 178 passing.yarn workspace dashmate lint— 0 errors (43 pre-existing warnings).access_keyremains underpackages/dashmate/src/ssl/zerossl.Breaking Changes
None. Exported endpoint signatures, configuration, CLI behavior, and certificate workflow remain unchanged.
Checklist:
For repository code-owners and collaborators only