Skip to content

chore(pkg/cmd): medium/low findings from the 2026-07-28 full review [platform part] #357

Open

Description

@cristim

Split from LeanerCloud/cloud-commitments-cli#1613 (cross-component issue, monorepo split)

This issue carries the two findings whose files are in cmd/ subdirectories that belong to this repo post-split (cmd/cleanup-lambda, cmd/lambda), per the monorepo split's path mapping: root-level cmd/*.go stays with cloud-commitments-cli, while cmd/ subdirectories such as cleanup-lambda, lambda, server, rekey, and gen-permissions belong to this repo. Confirmed both files still exist here (2026-09-28).


MEDIUM

  • cmd/cleanup-lambda can never delete a purchase execution, and has no status guard if it ever does
    • Where: cmd/cleanup-lambda/main.go:63 (dry-run count) and :91 (delete), both DELETE FROM purchase_executions WHERE expires_at < $1.
    • What: PurchaseExecution.ExpiresAt is *time.Time (internal/config/types.go:888), and no non-test writer sets it (the only .ExpiresAt writers found are ApprovalTokenExpiresAt, API-key expiry, and PurchaseSuppression.ExpiresAt, a different struct). The column is therefore always NULL, and NULL < $1 is NULL, so the predicate never matches. Separately, the delete has no status filter, so if ExpiresAt is ever populated it would hard-delete pending / scheduled / running rows alongside terminal ones.
    • Failure scenario (today): the scheduled cleanup Lambda runs on its cron, logs "Deleted 0 expired executions" forever, and reports executionsDeleted: 0 in its response. Operators reading that see a healthy job while purchase_executions grows without bound. Nothing alerts, because 0-deleted is indistinguishable from "nothing was due".
    • Failure scenario (once wired): an execution stuck in running past its grace window is deleted. The per-recommendation idempotency token is derived from the execution ID (pkg/common/tokens.go:64-67, cloud-commitments-go repo), so a re-drive after the row is gone necessarily uses a new execution ID and a new token. The AWS-side dedupe (ClientToken / IdempotencyTagKey) no longer recognises the earlier attempt, and the purchase can be made twice.
    • Fix direction: decide the intended retention rule and implement it explicitly, either by setting ExpiresAt at execution creation or by changing the predicate to a completed_at < $1 AND status IN (<terminal statuses>) form. Either way, add a terminal-status guard before any DELETE.

LOW

  • RATE_LIMIT_DOMAIN is interpolated into a SQL LIKE pattern without escaping wildcards
    • Where: cmd/lambda/clear-rate-limit/main.go:40-42, with the value from getDomain() (:15-20).
    • What: the query is properly parameterised, so this is not injection, but % and _ inside the env var are LIKE metacharacters, so the deletion scope is wider than the variable name implies.
    • Failure scenario: RATE_LIMIT_DOMAIN=% makes the pattern EMAIL#%@%#ENDPOINT#forgot_password, which deletes the forgot-password rate-limit rows for every domain rather than one, removing brute-force protection across all tenants. A typo containing _, for example leaner_cloud.com, silently widens the match by one character position.
    • Fix direction: reject or escape % and _ in getDomain(), or match on an exact suffix column instead of a constructed LIKE pattern.

Acceptance criteria

  • Each checklist item above is fixed with a regression test covering the described failure scenario.

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 27, 2026

    @IAmJSD
    Collaborator

    Status update (2026-10-27, checked against main at 7b86e38): Waiting on an owner decision.

    The LIKE-wildcard finding was fixed in #1272, and cmd/cleanup-lambda gained its terminal-status guard in #1173. Finding 1b is still open: nothing writes purchase_executions.expires_at, so the cleanup Lambda can never delete anything.

    Left / blocked on: Owner question: should cmd/cleanup-lambda be kept and made to work, or removed? Recommended answer: delete cmd/cleanup-lambda and its unused deploy modules (option b). The in-app scheduled cleanup task already calls CleanupOldExecutions, and once #120 item 9 lands that function has a live-status guard. Option a (have the Lambda call the shared CleanupOldExecutions) duplicates it, and option c (set a TTL at creation) is not recommended.

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