Skip to content

fix(security): RQ-3893 stop reading and writing appSumoCodes from the browser - #115

Open
rohitneharabrowserstack wants to merge 1 commit into
masterfrom
rq-3893-appsumo-server-side-validation
Open

fix(security): RQ-3893 stop reading and writing appSumoCodes from the browser#115
rohitneharabrowserstack wants to merge 1 commit into
masterfrom
rq-3893-appsumo-server-side-validation

Conversation

@rohitneharabrowserstack

@rohitneharabrowserstack rohitneharabrowserstack commented Aug 25, 2026

Copy link
Copy Markdown

Client half of RQ-3893. Prerequisite for requestly/requestly-cloud#880 — those Firestore rules cannot deploy until this ships.

Why this is needed

requestly-cloud#880 closes an escalation path: the appSumoCodes Firestore rules granted every authenticated user read, update, so any account could enumerate licence codes and clear redeemed on a paying customer's. The fix locks the collection to if false.

This component is the reason those permissions existed. It performed both halves of redemption in the browser:

// Appsumo.tsx:99  — validate
const docSnap = await getDoc(doc(db, "appSumoCodes", enteredCode));
// Appsumo.tsx:122 — consume
batch.update(docRef, { redeemed: true });

Deploying the rules against this code would have broken redemption outright:

  1. The read fails, so verified is never set, so isAllCodeCheckPassed stays false and onSubmit throws "Please fill all the fields correctly"nobody could redeem at all.
  2. The write fails too, and because redeemSubmittedCodes() was called un-awaited inside .then() with no catch, it failed silently — the user would see the success toast while the code stayed unredeemed and infinitely reusable.

Changes

Validation moves to a callable

verifyCode now calls subscription-validateAppSumoCodes (added in requestly-cloud#880) instead of reading Firestore. The callable returns a verdict only for codes the caller supplied, so it is not an enumeration oracle — the previous wildcard allow read also granted list, which is what made the whole table readable.

The user-facing messages are unchanged: "Invalid code" for a code that does not exist, "Code already redeemed" for one already used, plus a new "Could not verify this code, please try again" when the call itself fails (previously an exception here left the input silently unverified).

Client-side redemption removed

redeemSubmittedCodes() is deleted. The server now marks codes redeemed inside the same transaction that grants the tier, which also closes the race the client version had between reading redeemed and setting it.

Failed redemptions no longer report success

  if (!response?.data?.success && response?.data?.error === "max_limit_reached") {
    setShowMaxCodesExeceededError(true);
+ } else if (!response?.data?.success) {
+   toast.error(response?.data?.message || "Could not redeem these codes, please try again", 10);
  } else {

Any unsuccessful response that was not max_limit_reached previously fell into the success branch and showed "Lifetime access … unlocked" for a redemption that never happened. The outer catch now surfaces a toast as well, instead of only console.error.

The component no longer imports firebase/firestore at all.

Not changed

The workspace picker already restricts to workspaces where the caller is an admin (AppSumoWorkspaceDropdown, members[uid].role === "admin"), so it is consistent with the new admin check on the callable. No change needed there.

Deploy order

Corrected 2026-08-26. An earlier revision of this section said to deploy this PR first. That is wrong: this PR calls subscription-validateAppSumoCodes, so shipping it before that callable exists makes httpsCallable reject with functions/not-found, the input never verifies, and the submit button never enables — the same breakage, just from the other side.

The safe order is:

  1. Deploy the callables from requestly/requestly-cloud#880 (subscription-validateAppSumoCodes and the reworked subscription-updateTeamSubscriptionForAppSumo). Additive — the currently-deployed client does not call them.
  2. Deploy this PR. The callable it depends on now exists.
  3. Deploy the Firestore rules from requestly/requestly-cloud#880, last — once no browser touches appSumoCodes.

Deploying this PR before step 1, or the rules before step 2, breaks AppSumo redemption.

… browser

This component performed both halves of AppSumo redemption client-side: it read
`appSumoCodes/{code}` to validate a typed code, then wrote `redeemed: true` in a
batch after the callable returned. That is the only reason the Firestore rules
granted every authenticated user read+update on the collection — which let any
account enumerate licence codes and clear `redeemed` on a paying customer's.

requestly/requestly-cloud#880 closes the collection and moves both operations to
the server. Those rules cannot deploy until this ships: with the collection
locked, the read fails so no code ever validates and the submit button never
enables, and the redeem write fails silently (it was un-awaited with no catch),
leaving codes reusable while the user sees a success toast.

- verifyCode calls the new `subscription-validateAppSumoCodes` callable, which
  returns a verdict only for codes the caller supplied rather than exposing the
  collection to a wildcard read (which also grants `list`).
- redeemSubmittedCodes is deleted; the callable now marks codes redeemed in the
  same transaction that grants the tier.
- An unsuccessful response that is not `max_limit_reached` no longer falls into
  the success branch and reports "unlocked" for a redemption that never happened.

The workspace picker already restricted itself to workspaces where the caller is
an admin, so it needs no change to match the callable's new admin check.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@rohitneharabrowserstack

Copy link
Copy Markdown
Author

Type-check verified

tsc --noEmit over the whole app/ project, branch vs master, same toolchain both runs:

errors
master 1398
this branch 1398
delta 0

A set diff of the two error lists is empty — this change introduces no new type errors. Nothing in the output mentions Appsumo.tsx. The one diagnostic in the Appsumo directory (AppSumoWorkspaceDropdown.tsx(16,26) TS2532) is present on master too and is in a file this PR does not touch.

Two caveats, so the numbers are not read as more than they are:

  • That 1398 is a pre-existing baseline, inflated further because only app/ dependencies are installed here — the @requestly/* workspace packages do not resolve, so many errors are TS2307. It is a valid A/B comparison, not a claim that the project type-checks clean.
  • The repo's lint script is eslint --fix --ext .js,.jsx, which does not cover .tsx, so lint gives no signal on this file either way.

Getting this to run at all needed npm install --ignore-scripts plus dropping the root prepare hook locally (husky: command not found → exit 127 rolls the whole install back). That local edit is not part of the commit — this PR is one file.

Behaviour notes for review

  • User-facing validation messages are unchanged ("Invalid code", "Code already redeemed"). New: "Could not verify this code, please try again" when the callable itself fails — previously an exception there left the input silently unverified.
  • verifyCode sends one code per call. The callable accepts up to 10, so this could be batched later; kept 1:1 to preserve the existing per-input debounced UX exactly.
  • Requires subscription-validateAppSumoCodes to be deployed first — see requestly/requestly-cloud#880. Deploy order: this PR → the callables → the Firestore rules last.

@rohitneharabrowserstack

Copy link
Copy Markdown
Author

⚠️ Correction to the deploy ordering I gave earlier. I previously wrote that the interceptor client change must deploy first. That is wrong and would itself break AppSumo redemption.

The coupling runs in both directions:

  • Rules before client → the client's getDoc on appSumoCodes is denied, no code validates, the submit button never enables.
  • Client before callables → the client calls subscription-validateAppSumoCodes, which does not exist yet, so httpsCallable rejects with functions/not-found — same dead submit button, opposite cause.

Safe order is functions → client → rules:

  1. Callables from requestly-cloud#880 — additive, the deployed client never calls them, rules untouched, redemption unaffected.
  2. fix(security): RQ-3893 stop reading and writing appSumoCodes from the browser #115 — the callable it needs now exists.
  3. firestore.rules from requestly-cloud#880, last — by then no browser touches the collection.

Both PR descriptions have been updated. Since steps 1 and 3 live in the same PR yet must not deploy together, #880 is probably worth splitting (functions in one PR, firestore.rules in another) so the ordering is structural rather than a paragraph someone has to read.

@rohitneharabrowserstack

Copy link
Copy Markdown
Author

Superseded by #116. AppSumo is being retired, so the redemption screen is deleted rather than rewired through server-side callables — see #116 and requestly/requestly-cloud#882.

This PR is still correct as written, but it is now unnecessary work: it exists to keep a screen functioning that is being removed, and it carries a deploy-ordering dependency on the new callables that #116 does not have.

Recommend closing.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant