Fix: Forgeable discount coupons: unsigned z85-encoded coupon codes accepted by PUT /rest/basket/:id/coupon/:coupon allow arbitrary discounts up to 99%

[WesternConcrete]

Description

Finding: Forgeable discount coupons: unsigned z85-encoded coupon codes accepted by PUT /rest/basket/:id/coupon/:coupon allow arbitrary discounts up to 99%
Repo: COG-GTM/juice-shop
Severity: HIGH

Fix approach: Coupon codes now carry a truncated HMAC-SHA256 signature (72 bits) over the <MMMYY>-<discount> payload (lib/insecurity.ts), and discountFromCoupon rejects any code whose signature does not verify (constant-time compare), so an attacker can no longer mint a discount by z85-encoding a plaintext payload.

Supersedes #360 (closed by the contribution bot: wrong base branch / missing DCO sign-off); this PR targets develop, is signed off, and addresses the Devin Review findings from #360.

sig(payload, nonce)      = nonce + hex(hmac_sha256(key, payload + nonce))[0:18]      // nonce ∈ 0..f, 19 hex chars total
generateCoupon(d, date)  -> for nonce in 0..f: c = z85.encode(`${MMMYY}-${d}-${sig}`); return first c without '%' or '{…}'
discountFromCoupon(c)    -> decode; verify format; recompute sig from embedded nonce; timingSafeEqual; verify month; return d

• Why the nonce: z85 output can contain % (breaks the route's decodeURIComponent → HTTP 500) and {…} (Cypress type() treats it as a key sequence). The unsigned 10-char codes had the same latent problem, but the longer signed codes hit it deterministically this month (SEP26-99 → …{HeO…%vR…), which is what failed api-test and both e2e-test jobs on the first revision. The generator now tries 16 nonces and picks the first code free of those sequences; routes/coupon.ts additionally falls back to the raw param if decodeURIComponent throws, so a % inside a code can no longer 500.
• Signing key is COUPON_SIGNING_KEY env var, falling back to the app's existing JWT private key so cypress.config.ts (which generates coupons out-of-process) stays consistent with the server.
• Two-digit-discount codes are always 35 chars; payment.component coupon validator changed from exactly 10 to 10–35 so both signed codes and the 10-char client-side campaign codes (WMNSDY2019, …) remain enterable. COUPON_CODE_HINT (en) updated accordingly.
• insecuritySpec.ts: the test pinning the old unsigned literal (n<MiifFb4l) is updated to assert on the payload prefix; forgery/tamper/nonce-swap tests and a URL/typing-safety test are added.

Verified: mocha -r tsx test/server/insecuritySpec.ts (37 passing), backend eslint + tsc --noEmit, frontend eslint on changed files (only pre-existing label-has-associated-control errors remain, present on base).

Resolved or fixed issue: none

AI Tool Disclosure

• My contribution does not include any AI-generated content
• My contribution includes AI-generated content, as disclosed below:
  • AI Tools: Devin
  • LLMs and versions: Devin (Cognition)
  • Prompts: Automated security sweep: fix forgeable unsigned coupon codes by signing them

Affirmation

• My code follows the CONTRIBUTING.md guidelines

Link to Devin session: https://app.devin.ai/sessions/b243e9dbc6d0422fb8bc01103a1ae3ae
Open in Devin Desktop: https://app.devin.ai/desktop/session/b243e9dbc6d0422fb8bc01103a1ae3ae?variant=devin
Requested by: @WesternConcrete

---

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[devin-ai-integration]

Devin Review found 3 new potential issues.

[devin-ai-integration]

🔍 Unsafe fallback coupon escapes filtering

generateCoupon returns the last candidate when every nonce matches UNSAFE_COUPON_CHARS. Some future payload prefixes contain % across all candidates.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Correct — when the % sits in the z85 blocks that encode the fixed <MMMYY>-<dd> prefix, no nonce can remove it (≈4.7% of month/discount combos; this was equally true of the old unsigned 10-char codes). That case is covered by the second half of this commit: routes/coupon.ts now falls back to the raw param when decodeURIComponent throws, so such a code redeems normally instead of returning HTTP 500. The nonce loop is best-effort for the % case and effectively complete for the {…} case (the closing } needs to land in the signature blocks, which the nonce does vary).

[devin-ai-integration]

🟥 Default coupon signatures remain forgeable

Without COUPON_SIGNING_KEY, couponSigningKey uses a public repository key. Anyone can mint accepted current-month coupons with arbitrary two-digit discounts.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Agreed that the fallback key is public, so a deployment that sets neither COUPON_SIGNING_KEY nor a private JWT key does not get authenticity from this change. This is deliberate scoping: the hard-coded JWT/HMAC key in lib/insecurity.ts is a separate, already-tracked finding for this repo, and it is the same key that already protects every session token — a deployment that has rotated it (as it must for JWTs) gets unforgeable coupons for free. A per-process random default was rejected because cypress.config.ts generates coupons out-of-process and multi-instance deployments would disagree on signatures. Operators wanting an independent secret set COUPON_SIGNING_KEY. Happy to switch to a random default if maintainers prefer that trade-off.

[devin-ai-integration]

🟨 Signed coupons accept unchecked discounts

The chatbot can pass any number to generateCoupon, which signs it without range or integer validation. Oversized discounts can produce negative order totals.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Out-of-range values are rejected at verification time rather than generation time: hasValidFormat only matches -[0-9]{2}-, so a coupon signed over 100, 5, -5 or 12.5 fails the format check in discountFromCoupon and returns undefined (404 at the route). Discounts >99% or negative totals via a coupon are therefore not reachable; this is unchanged from the base branch. The chatbot's generateCoupon tool is pre-existing and its input handling is out of scope here.