mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 16:11:29 +00:00
fix: collapse identical if/else branches in Account handler (CodeQL #25)
CodeQL alert #25 (go/duplicate-branches) on internal/api/handler/
acme.go::ACMEHandler.Account flagged that 'if readOnly { ... } else
{ ... }' had byte-identical bodies — both setting the same
Content-Type: application/json header. The 'readOnly' bool was
threaded through the function as a placeholder for differentiated
headers (Cache-Control etc. on the POST-as-GET path) that never
landed; both branches collapsed to the same value with no
follow-through.
Audit + fix:
- The alert is real (verified by re-reading the source); not a
false positive.
- The Copilot Autofix Anthropic surfaced was correct in spirit but
incomplete: it collapsed the if/else but left 'readOnly' as
dead code (declared at line 395, assigned at lines 400 and 436,
only read at the now-removed if). golangci-lint's 'unused'
linter would flag 'readOnly' next.
- Complete fix: collapse the if/else AND remove the now-unused
'readOnly' variable + its 2 assignments. Single unconditional
'w.Header().Set("Content-Type", "application/json")' covers
both paths (RFC 8555 §6.3 POST-as-GET + §7.3.2 / §7.3.6 update
+ deactivation all return the same account JSON shape — no spec
rationale for differentiating headers).
Verified locally: 'gofmt -l .' clean; 'go vet ./...' clean;
'go test -short -count=1 ./internal/api/handler/' green; 'grep
readOnly' on the file returns only the new explanatory comment
(no live references).
The alert was first detected in commit 27bd660 (Phase 1b) — the
duplicate has been sitting in the codebase since the Account
handler shipped. No functional regression for any RFC 8555 client
(cert-manager, lego, Posh-ACME): same status code, same headers,
same body.
This commit is contained in:
@@ -390,14 +390,10 @@ func (h ACMEHandler) Account(w http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
var (
|
||||
updated *domain.ACMEAccount
|
||||
readOnly bool
|
||||
)
|
||||
var updated *domain.ACMEAccount
|
||||
// Empty body or empty JSON object → POST-as-GET (§6.3).
|
||||
trimmed := trimBody(verified.Payload)
|
||||
if len(trimmed) == 0 || string(trimmed) == "{}" {
|
||||
readOnly = true
|
||||
updated = verified.Account
|
||||
} else {
|
||||
var req acme.AccountUpdateRequest
|
||||
@@ -433,18 +429,19 @@ func (h ACMEHandler) Account(w http.ResponseWriter, r *http.Request) {
|
||||
if updated == nil {
|
||||
// Empty status + nil contact → no-op; treat as POST-as-GET.
|
||||
updated = verified.Account
|
||||
readOnly = true
|
||||
}
|
||||
}
|
||||
|
||||
if nonce, err := h.svc.IssueNonce(r.Context()); err == nil {
|
||||
w.Header().Set("Replay-Nonce", nonce)
|
||||
}
|
||||
if readOnly {
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
} else {
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
}
|
||||
// RFC 8555 §6.3 (POST-as-GET) and §7.3.2 / §7.3.6 (account update +
|
||||
// deactivation) both return the same account JSON shape, so a single
|
||||
// unconditional Content-Type header covers both paths. Earlier code
|
||||
// kept this behind an if/readOnly switch as a placeholder for
|
||||
// differentiated headers (Cache-Control etc.) that never landed;
|
||||
// CodeQL flagged the duplicate branches as quality issue #25.
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.WriteHeader(http.StatusOK)
|
||||
_ = json.NewEncoder(w).Encode(
|
||||
acme.MarshalAccount(updated, h.accountOrdersURL(r, profileID, updated.AccountID)),
|
||||
|
||||
Reference in New Issue
Block a user