Skip to content

chore(api): dead duplicate check, skipped revoke placeholders, triple lookup, weak email #311

Closed

Description

@cristim

Summary

Four low-severity findings in internal/api that clear in one sitting. findDuplicatePendingExecution has no production caller and is kept alive only by its test, while the live duplicate check is matchDuplicateInList; revokeViaSession discards a CAS result with a "kept for future use" binding. Two four-eyes revocation tests are permanent t.Skip placeholders "until LeanerCloud/cloud-commitments-cli#1005 lands", but LeanerCloud/cloud-commitments-cli#1005 has landed for approvals and neither revoke path applies dual control, so the placeholders document an unenforced expectation and inflate the apparent coverage of the revoke money path. The Azure exchange execute path resolves the same CloudAccount three times per request with three different "no account" outcomes. The public registration endpoint accepts any 5-character string containing @ as a contact email while validateEmailFormat exists in the same package for exactly this purpose.

Location

All at 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd.

  • A01-015 findDuplicatePendingExecution at internal/api/handler_purchases.go:2513-2547, called only from handler_purchases_guards_test.go:509-564; live check is matchDuplicateInList (:2419) via persistExecutionAndSuppressions (:2461). _ = updated // kept for future use at :1471. Delete the function and its test (or point the test at matchDuplicateInList) and drop the updated binding.
  • A01-022 TestRevoke_4EyesMode_TODO at internal/api/handler_purchases_test.go:5304-5306 and TestRevokePurchase_FourEyesApproval at handler_purchases_revoke_test.go:1311-1313 are bare t.Skip. enforceFourEyesPolicy (internal/purchase/approvals.go:216-230) is live for approvals, but neither revokeViaSession (handler_purchases.go:1446-1478) nor the revokePurchase/authorizeSessionRevoke* chain (handler_purchases_revoke.go:139-330) references it. Decide whether revocation is subject to dual control; implement and test it, or delete the placeholders.
  • A01-020 authorizeAzureExchangeExecution (internal/api/handler_ri_exchange.go:982-1022) calls GetCloudAccountByExternalID in requireAzureSubscriptionScope (:805, scoped sessions), buildAzureExchangeClient (:214) and resolveAzureExchangeAccountID (:1029), with three no-account outcomes (errNotFound, 404, unattributedAccountConstraint). Resolve once at the top and pass the account down.
  • A02-025 validateRegistrationRequest at internal/api/handler_registrations.go:429-431 accepts any string of length 5 containing @; validateEmailFormat (validation.go:134-146) already exists. notifyRegistrant (:240-247) hands the stored value to the notifier as a Destination field, so header injection is not reachable, but "a@b\r\nBcc: x@y" is stored. Replace the check with validateEmailFormat and store mail.ParseAddress(...).Address. Related: chore(server/scheduler): medium/low findings from the 2026-07-28 full review #111's federation-tfvars bullet cites this same weak check as an input to its HCL-injection scenario; fixing it here narrows that too.

Failure scenario

No live defect. The t.Skip placeholders are the one item with a policy question attached: as long as they exist, a reader assumes revoke is dual-controlled when it is not.

Evidence

func (h *Handler) findDuplicatePendingExecution(ctx context.Context, creatorID, key string, now time.Time) (*config.PurchaseExecution, error) {
	pending, err := h.config.GetPendingExecutions(ctx)
func TestRevoke_4EyesMode_TODO(t *testing.T) {
	t.Skip("placeholder until LeanerCloud/cloud-commitments-cli#1005 lands: verify that 4-eyes-mode approval flows interact correctly with the revocation window")
}
if !strings.Contains(req.ContactEmail, "@") || len(req.ContactEmail) < 5 {
    return NewClientError(400, "contact_email must be a valid email address")
}

Suggested fix

Per checkbox above.


Found by the 2026-09-02 codebase audit, findings A01-015, A01-022, A01-020, A02-025, each reported by one reviewer and independently confirmed by a second. Full report: docs/audits/codebase-audit-2026-09-02.md.

Activity

  1. IAmJSD commented on Oct 23, 2026

    @IAmJSD
    Collaborator

    Claiming: working on this

  2. self-assigned this
    on Oct 23, 2026
  3. IAmJSD commented on Oct 26, 2026

    @IAmJSD
    Collaborator

    Closing as fixed on main. #1285 fixed A01-015(b) (dead updated binding), A01-022 (deleted the t.Skip placeholders), A01-020 (single Azure account lookup) and A02-025 (registration email validation). A01-015(a) (findDuplicatePendingExecution) was deleted by #1217 (merged 2026-10-25), and the function is gone from main. The PR said all items are done once #1217 merges.

    The fix PRs said Refs, so GitHub did not close this automatically. Checked against main on 2026-10-26 by IAmJSD's agent (IAmJSD is acting owner while the maintainers are away). Reopen if anything here is still wrong.

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

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions