Skip to content

hardening(auth): negative MaxPurchaseAmount constraint is treated as 'no constraint' instead of deny-all #83

Description

@cristim

Context

Surfaced during the Fable adversarial review of LeanerCloud/cloud-commitments-cli#1515 (Azure RI-exchange execute). Pre-existing, affects the AWS execute path identically -- NOT introduced by LeanerCloud/cloud-commitments-cli#1515.

Issue

matchPurchaseAmountConstraint (internal/auth/service_group.go ~:269) gates on permMax > 0: a permission whose stored MaxPurchaseAmount is negative is treated as no amount constraint (allow), rather than deny-all. If admin input validation does not reject negative MaxPurchaseAmount values at write time, a negative value silently disables the spend cap on that permission.

Fix

Treat permMax < 0 as deny-all in the matcher, rather than as 'no constraint'. One condition, fails closed, and it covers permission rows that are already stored negative, which a write-time validation cannot.

Rejecting negatives at the write boundary is worth adding as well if it is cheap, but it is not the fix on its own: it leaves existing malformed rows fail-open.

Leave permMax == 0 alone. Zero is the unset zero-value here, and folding it into the same condition would turn every permission with no configured cap into deny-all.

Notes

  • Non-exploitable via the API today (JSON request bodies can't inject a negative stored permission cap; this is about stored permission data), but it's a real fail-open on malformed/negative admin config.
  • There is a cosmetic asymmetry on the currency compare (EqualFold on the constraint path vs case-sensitive in the money guardrail; both fail closed). Left alone deliberately: harmonising it by uppercase-normalising body.Currency adds a normaliser on a spend-cap path, whose whole purpose is to make two different strings compare equal. Neither compare is wrong today. If the asymmetry is worth removing, make the constraint path case-sensitive to match the guardrail, in its own change.

Remedy simplified (2026-08-03): the "and/or" became one fail-closed condition in the matcher, with the write-boundary check noted as insufficient alone; the currency normaliser was dropped from this issue. Bundling a normaliser onto a money path into an unrelated fail-open fix is how the repo's substring-for-equality defects have arrived before, and PR LeanerCloud/cloud-commitments-cli#1515 already settled that a non-USD money path must fail closed rather than be normalised into comparability.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions