From 4a5b6e71bf36ffe9ca9582912215266decfeb93a Mon Sep 17 00:00:00 2001
From: Drumee Dev <drumee@debian.local.drumee>
Date: Thu, 27 Aug 2026 14:49:10 -0700
Subject: [PATCH 1/2] feat(oauth): carry the visitor's destination across the
 provider bounce
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

initiate parks `dest` on oauth_state beside ref and utm_*; the callback reads it
back and appends it to the `home` URL it already builds. A signed-out visitor
who clicks a campaign CTA and signs in with Google or Apple now lands where the
link named instead of on a bare desk.

THE DESTINATION GOES ON THE URL, not into storage. The visitor may land on a
different deploy slot from the one they clicked (measured on stage: clicked
/-/huan/, landed /-/), and a fragment could never have reached this server
anyway. ui-team's billing-deep-link consume() already reads a destination off
the URL when storage has none — so nothing changes over there, which was worth
verifying rather than assuming.

ALL THREE SUCCESSFUL EXITS carry it: existing sign-in, new account (the welcome
card's CTA) and 2FA (through the OTP screen). Missing one would make the feature
work for some users and silently not for others.

_sanitiseDest IS A SHAPE CHECK THAT REBUILDS, not an escaping pass: one path,
four params, each matched against its own anchored regex, and the string
assembled again from what survived — so a value can only ever be one this
function could have written. An unknown param is REFUSED rather than dropped,
because honouring half a link written against a contract we do not have is how a
destination becomes a wrong one rather than an absent one. Run at both ends: the
row is data, and the code that builds a template's input is the code that has to
have checked it.

AND THE TEMPLATES NO LONGER INTERPOLATE THAT URL RAW. lib/loby.js renders with
lodash, whose equals-delimiter is the RAW one — the reverse of EJS — so
location.replace('<url>') put request-derived text straight into a JS string
literal on the page that runs immediately after authentication. Both landing
templates now emit it through JSON.stringify, and the new-account CTA through
the escaping delimiter. The sanitiser refusing quotes and the template escaping
are independent; either alone is one edit away from an XSS.

The initiate INSERT now degrades one column-group at a time (dest → utm → ref →
bare) rather than falling straight to the bare row: a database without `dest`
must still keep the campaign. Signing in never depends on any of it.

Two guards in the sanitiser — the character check and the 255 cap — cannot
currently decide anything, verified by mutation: with the path fixed and every
param bounded, the longest value it can emit is 125 chars. They are kept as
defence in depth, and the suite pins the PREMISE instead (every param regex is
anchored and quote-free), so adding a free-text param fails loudly at the moment
those guards start doing real work.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 offline/test/oauth-dest.test.js        | 232 +++++++++++++++++++++++++
 service/apple.js                       |  63 +++++--
 service/google.js                      |  67 +++++--
 service/lib/loby.js                    | 109 +++++++++++-
 service/templates/account-created.html |  22 ++-
 service/templates/otp-challenge.html   |   5 +-
 6 files changed, 463 insertions(+), 35 deletions(-)
 create mode 100644 offline/test/oauth-dest.test.js

diff --git a/offline/test/oauth-dest.test.js b/offline/test/oauth-dest.test.js
new file mode 100644
index 0000000..9047af5
--- /dev/null
+++ b/offline/test/oauth-dest.test.js
@@ -0,0 +1,232 @@
+#!/usr/bin/env node
+
+/**
+ * The OAuth destination — the value that survives the bounce to the provider.
+ *
+ * WHY THIS EXISTS. A campaign CTA names where the visitor is going:
+ *
+ *   #/desk/billing?plan=team&cycle=monthly&tab=checkout&promo=EMAILMKT270826_2
+ *
+ * ui-team parks that in sessionStorage before the signin plugin rewrites the
+ * hash, which carries an email/password sign-in and not an OAuth one: this
+ * callback is server-side, it rebuilds the landing URL from scratch, and a URL
+ * fragment is never sent to a server in the first place. So the destination
+ * rides on `oauth_state` beside `ref` and utm_*, and comes back out on `home`.
+ *
+ * TWO THINGS ARE PINNED HERE, and the second is why this file is not optional:
+ *
+ *   1. _sanitiseDest accepts exactly the destinations the campaign can name and
+ *      refuses everything else. It REBUILDS rather than passes through, so a
+ *      value can only ever be one this function could have written.
+ *
+ *   2. The landing templates do not interpolate that URL raw. lib/loby.js uses
+ *      LODASH, whose equals-delimiter is the RAW one — the reverse of EJS — so
+ *      `location.replace('<the url>')` put request-derived text straight into a
+ *      JS string literal on the page that runs immediately after
+ *      authentication. Both halves are asserted: the sanitiser refuses quotes,
+ *      AND the template escapes. Either alone is one edit away from an XSS.
+ *
+ * Standalone runner (no test framework in this repo): `node <thisfile>`.
+ */
+
+const assert = require("assert");
+const { readFileSync } = require("fs");
+const { join } = require("path");
+const { template } = require("lodash");
+
+const ROOT = join(__dirname, "../..");
+const read = (p) => readFileSync(join(ROOT, p), "utf8");
+const LOBY = read("service/lib/loby.js");
+
+let failures = 0;
+function test(name, fn) {
+  try { fn(); console.log("  ok   " + name); }
+  catch (e) { failures++; console.log("  FAIL " + name + "\n         " + e.message); }
+}
+
+/**
+ * The real _sanitiseDest, lifted and compiled.
+ *
+ * Out of the source rather than restated here, so these cases cannot pass
+ * against a rule the service does not actually apply.
+ */
+const sanitiseSrc = /_sanitiseDest\(raw\) \{([\s\S]*?)\n  \}/.exec(LOBY);
+assert(sanitiseSrc, "_sanitiseDest not found in service/lib/loby.js");
+const sanitise = new Function("raw", sanitiseSrc[1]);
+
+const CAMPAIGN_DEST =
+  "/desk/billing?plan=team&cycle=monthly&tab=checkout&promo=EMAILMKT270826_2";
+
+// ── what it accepts ────────────────────────────────────────────────────
+test("the campaign's own destination survives byte for byte", () => {
+  assert.strictEqual(sanitise(CAMPAIGN_DEST), CAMPAIGN_DEST);
+});
+
+test("the bare billing path is a destination", () => {
+  assert.strictEqual(sanitise("/desk/billing"), "/desk/billing");
+});
+
+test("params are rebuilt in a fixed order", () => {
+  // Two links meaning the same thing must produce the same string: the value is
+  // compared and stored, and an order-dependent one would look like two.
+  assert.strictEqual(
+    sanitise("/desk/billing?tab=checkout&plan=team"),
+    "/desk/billing?plan=team&tab=checkout");
+});
+
+test("whitespace around the value is tolerated", () => {
+  assert.strictEqual(sanitise(`  ${CAMPAIGN_DEST}  `), CAMPAIGN_DEST);
+});
+
+// ── what it refuses ────────────────────────────────────────────────────
+const REFUSED = {
+  "a foreign path": "/desk/wm/open/123",
+  "a path outside the desk": "/welcome/signin",
+  "a protocol-relative url": "//evil.example/",
+  "an absolute url": "https://evil.example/desk/billing",
+  "a quote (the XSS vector)": "/desk/billing?plan=team');alert(1);//",
+  "a double quote": '/desk/billing?plan=team"x',
+  "a backslash": "/desk/billing?plan=team\\x",
+  "an angle bracket": "/desk/billing?plan=<script>",
+  "whitespace inside": "/desk/billing?plan=te am",
+  "an unknown param": "/desk/billing?plan=team&evil=1",
+  "an unknown plan": "/desk/billing?plan=enterprise",
+  "an unknown cycle": "/desk/billing?cycle=weekly",
+  "a tab that is not checkout": "/desk/billing?tab=admin",
+  "a promo with punctuation": "/desk/billing?promo=A.B",
+  "an over-long promo": "/desk/billing?promo=" + "A".repeat(80),
+  "an over-long value": "/desk/billing?promo=" + "A".repeat(400),
+  "the empty string": "",
+  "null": null,
+  "undefined": undefined,
+};
+for (const [why, value] of Object.entries(REFUSED)) {
+  test(`refuses ${why}`, () => {
+    assert.strictEqual(sanitise(value), null);
+  });
+}
+
+test("an unknown param is refused, not silently dropped", () => {
+  // Dropping it would honour half a link written against a contract this code
+  // does not have — and guessing which half is how a destination becomes a
+  // wrong one rather than an absent one.
+  assert.strictEqual(sanitise("/desk/billing?plan=team&seats=5"), null);
+});
+
+// ── the outer guards are redundant, and that is the point ──────────────
+// Two guards in _sanitiseDest cannot currently decide anything: the
+// character check (no quote/backslash/angle/whitespace) and the 255 cap.
+// With the path fixed to one literal and every param matched against a
+// bounded, alphanumeric regex, nothing that reaches them can fail them —
+// removing either changes no verdict, verified by mutation.
+//
+// They are kept because that redundancy is a property of the CURRENT param
+// list, not of the function. The day someone adds a free-text param they
+// become the only thing standing between a request and the landing page's
+// location.replace(). So what is pinned here is the premise rather than the
+// guards: every accepted value is quote-free and bounded by its own regex.
+// Add a looser one and this fails, which is the moment to notice the outer
+// guards just started doing real work.
+test("every accepted param is bounded and quote-free by its own regex", () => {
+  const body = /_sanitiseDest\(raw\) \{([\s\S]*?)\n  \}/.exec(LOBY)[1];
+  const allow = /const ALLOWED = \{([\s\S]*?)\n    \};/.exec(body);
+  assert.ok(allow, "the ALLOWED map is gone — the destination is no longer an allowlist");
+  const regexes = [...allow[1].matchAll(/^\s*(\w+):\s*(\/.*\/),?\s*$/gm)];
+  assert.ok(regexes.length >= 4, `only ${regexes.length} params are shape-checked`);
+  for (const [, key, src] of regexes) {
+    assert.ok(/^\/\^/.test(src) && /\$\/$/.test(src),
+      `${key} is not anchored — it would match a substring of a hostile value`);
+    assert.ok(!/[.*+]\)?\$?\/$|\\w|\\S|\[\^/.test(src.replace(/\{1,\d+\}/, "")),
+      `${key} accepts an open-ended or negated class — the outer character and `
+      + "length guards in _sanitiseDest are now load-bearing, and untested");
+  }
+  // And the longest thing the function can emit is far under the column width.
+  const longest = sanitise(
+    "/desk/billing?plan=business&cycle=monthly&tab=checkout&promo=" + "A".repeat(64));
+  assert.ok(longest && longest.length < 255,
+    "an accepted destination can now approach the column width — the cap has "
+    + "stopped being belt-and-braces and needs its own case");
+});
+
+// ── the ladder that keeps sign-in working ──────────────────────────────
+test("initiate degrades one column-group at a time", () => {
+  // A database without `dest` must still keep the campaign, and one without
+  // utm_* must still keep the referral. Falling straight to the bare row would
+  // discard attribution the instance can perfectly well store.
+  for (const f of ["service/google.js", "service/apple.js"]) {
+    const src = read(f);
+    const tiers = (src.match(/INSERT IGNORE INTO oauth_state/g) || []).length;
+    assert.strictEqual(tiers, 4,
+      `${f} no longer has four INSERT tiers — a missing column would cost more `
+      + "than the column, or would stop a visitor signing in");
+    assert.ok(/oauth_state \(state, session_id, ref, utm_source, utm_medium, utm_campaign, utm_content, dest, ctime\)/.test(src),
+      `${f} does not write dest on its top tier`);
+    assert.ok(src.includes("this._destFromInput()"),
+      `${f} never reads the destination from the request`);
+  }
+});
+
+// ── both ends validate ─────────────────────────────────────────────────
+test("the destination is validated on the way in AND on the way out", () => {
+  // The row is data. The code that builds the template's input is the code that
+  // has to have checked it.
+  assert.ok(/_destFromInput\(\) \{\s*return this\._sanitiseDest\(this\.input\.get\("dest"\)\);/.test(LOBY),
+    "_destFromInput no longer sanitises at capture");
+  assert.ok(/dest = this\._sanitiseDest\(dest\)/.test(LOBY),
+    "handleOAuthCallback returns the row's dest unvalidated");
+  for (const f of ["service/google.js", "service/apple.js"]) {
+    assert.ok(/const dest = this\._sanitiseDest\(res\.dest\)/.test(read(f)),
+      `${f} puts the destination on the landing URL without re-validating it`);
+  }
+});
+
+test("all three successful callback exits carry it", () => {
+  // signin, signup and 2FA. Missing one means the feature works for some users
+  // and silently not for others — the hardest kind of bug to be told about.
+  const exits = (LOBY.match(/dest: this\._sanitiseDest\(dest\)|\.dest = this\._sanitiseDest\(dest\)/g) || []);
+  assert.strictEqual(exits.length, 3,
+    `${exits.length} of 3 callback exits carry a destination`);
+  for (const f of ["service/google.js", "service/apple.js"]) {
+    assert.ok(read(f).includes("&dest=${encodeURIComponent(res.dest)}"),
+      `${f} drops the destination on the 2FA path`);
+  }
+});
+
+test("home gains a fragment only when a destination survived", () => {
+  for (const f of ["service/google.js", "service/apple.js"]) {
+    assert.ok(/home = `https:\/\/\$\{res\.domain\}\$\{endpoint_path\}\/\$\{dest \? `#\$\{dest\}` : ''\}`/.test(read(f)),
+      `${f}'s landing URL is not conditional on a surviving destination`);
+  }
+});
+
+// ── the templates cannot be broken out of ──────────────────────────────
+const HOSTILE = "https://x/-/#/desk/billing?promo=A');alert(1);//";
+
+test("account-created does not interpolate the landing URL raw", () => {
+  const html = template(read("service/templates/account-created.html"))(
+    { home: HOSTILE, auto_redirect: 1, is_new: 0, email: "a@b.c" });
+  const line = html.split("\n").find((l) => l.includes("location.replace"));
+  assert.ok(line, "the redirect is gone");
+  assert.ok(!line.includes("replace('"),
+    "the URL is back inside a single-quoted literal — one apostrophe reaches script context");
+  assert.ok(line.includes('replace("https://x/-/#/desk/billing?promo=A\');alert(1);//")'),
+    "the hostile URL is not contained by the quoting");
+});
+
+test("the new-account CTA escapes its href", () => {
+  const html = template(read("service/templates/account-created.html"))(
+    { home: HOSTILE, auto_redirect: 0, is_new: 1, email: "a@b.c" });
+  const line = html.split("\n").find((l) => l.includes("goto-desk"));
+  assert.ok(line.includes("&#39;") || !line.includes("');"),
+    "the CTA href is unescaped — an attribute is as escapable as a string literal");
+});
+
+test("otp-challenge does not interpolate its redirect raw", () => {
+  const html = template(read("service/templates/otp-challenge.html"))({ redirect: HOSTILE });
+  const line = html.split("\n").find((l) => l.includes("location.replace"));
+  assert.ok(!line.includes("replace('"),
+    "the 2FA redirect is back inside a single-quoted literal");
+});
+
+console.log(failures ? `\n${failures} failed\n` : "\nall passed\n");
+process.exit(failures ? 1 : 0);
diff --git a/service/apple.js b/service/apple.js
index a1ac401..8738590 100644
--- a/service/apple.js
+++ b/service/apple.js
@@ -203,27 +203,46 @@ class Register extends Loby {
       // Campaign attribution, parked for the same reason as `ref` — see
       // google.js, which does this identically.
       const utm = this._utmFromInput();
+      // Where the visitor was heading, parked for the third time and the same
+      // reason as `ref` and `utm` above — see _sanitiseDest. Validated HERE, on
+      // the way in, as well as on the way back out at callback.
+      const dest = this._destFromInput();
       try {
         await this.yp.await_query(
-          'INSERT IGNORE INTO oauth_state (state, session_id, ref, utm_source, utm_medium, utm_campaign, utm_content, ctime)'
-          + ' VALUES (?, ?, ?, ?, ?, ?, ?, UNIX_TIMESTAMP())',
+          'INSERT IGNORE INTO oauth_state (state, session_id, ref, utm_source, utm_medium, utm_campaign, utm_content, dest, ctime)'
+          + ' VALUES (?, ?, ?, ?, ?, ?, ?, ?, UNIX_TIMESTAMP())',
           state, this.input.sid(), ref || null,
           utm.utm_source || null, utm.utm_medium || null,
-          utm.utm_campaign || null, utm.utm_content || null
+          utm.utm_campaign || null, utm.utm_content || null,
+          dest || null
         );
       } catch (e) {
-        // Attribution is best-effort; signing in is not. Fall back to the
-        // ref-only shape, then to the bare row.
+        // One tier per column-group, newest first: a database that has utm_*
+        // but not `dest` still keeps the campaign. Falling straight to the bare
+        // row would throw away attribution that the instance can perfectly well
+        // store, for the sake of a column it cannot.
         try {
           await this.yp.await_query(
-            'INSERT IGNORE INTO oauth_state (state, session_id, ref, ctime) VALUES (?, ?, ?, UNIX_TIMESTAMP())',
-            state, this.input.sid(), ref || null
-          );
-        } catch (e2) {
-          await this.yp.await_query(
-            'INSERT IGNORE INTO oauth_state (state, session_id, ctime) VALUES (?, ?, UNIX_TIMESTAMP())',
-            state, this.input.sid()
+            'INSERT IGNORE INTO oauth_state (state, session_id, ref, utm_source, utm_medium, utm_campaign, utm_content, ctime)'
+            + ' VALUES (?, ?, ?, ?, ?, ?, ?, UNIX_TIMESTAMP())',
+            state, this.input.sid(), ref || null,
+            utm.utm_source || null, utm.utm_medium || null,
+            utm.utm_campaign || null, utm.utm_content || null
           );
+        } catch (eUtm) {
+          // Attribution is best-effort; signing in is not. Fall back to the
+          // ref-only shape, then to the bare row.
+          try {
+            await this.yp.await_query(
+              'INSERT IGNORE INTO oauth_state (state, session_id, ref, ctime) VALUES (?, ?, ?, UNIX_TIMESTAMP())',
+              state, this.input.sid(), ref || null
+            );
+          } catch (e2) {
+            await this.yp.await_query(
+              'INSERT IGNORE INTO oauth_state (state, session_id, ctime) VALUES (?, ?, UNIX_TIMESTAMP())',
+              state, this.input.sid()
+            );
+          }
         }
       }
       const authUrl = `https://appleid.apple.com/auth/authorize?` +
@@ -263,7 +282,11 @@ class Register extends Loby {
       // session cookie (sendHtml/setAuthorization) and bounce the browser to the
       // signin app's OTP screen, which finalizes via oauth.verify_otp.
       if (res.status === 'otp_required') {
-        const redirect = `https://${main_domain}${endpoint_path}/#/welcome/signin?oauth_mfa=1&email=${encodeURIComponent(res.email || '')}`;
+        // `dest` rides through the OTP screen so a 2FA account arrives where the
+        // link named. Appended as an ordinary param on a hash that already has
+        // a query; the signin app hands it back once verify_otp finalises.
+        const destParam = res.dest ? `&dest=${encodeURIComponent(res.dest)}` : '';
+        const redirect = `https://${main_domain}${endpoint_path}/#/welcome/signin?oauth_mfa=1&email=${encodeURIComponent(res.email || '')}${destParam}`;
         const tpl = resolve(__dirname, './templates/otp-challenge.html');
         // res.session_id is the pending cookie's id (original signin session) —
         // sendHtml binds the browser's authorization to it.
@@ -275,7 +298,19 @@ class Register extends Loby {
         // previously fell through and answered the browser with nothing at all.
         return this.sendOauthError(res.error);
       }
-      const home = `https://${res.domain}${endpoint_path}/`;
+      // THE DESTINATION GOES ON THE URL, not into storage. The visitor may land
+      // on a different deploy slot from the one they clicked on, and a fragment
+      // could never have reached this server in the first place — so the
+      // landing URL is the only carrier that works from here. ui-team's
+      // billing-deep-link consume() already reads a destination off the URL
+      // when storage has none, which is why nothing has to change over there.
+      //
+      // Re-validated at the point of use (handleOAuthCallback already ran it):
+      // this string is about to be interpolated into the landing page's
+      // location.replace(), and the code that builds a template's input is the
+      // code that has to have checked it.
+      const dest = this._sanitiseDest(res.dest);
+      const home = `https://${res.domain}${endpoint_path}/${dest ? `#${dest}` : ''}`;
       const tpl = resolve(__dirname, './templates/account-created.html');
       // New OAuth account: show the welcome card (auto_redirect off) so the
       // user lands on it; the CTA continues to the desk, where the onboarding
diff --git a/service/google.js b/service/google.js
index 3ef031f..91b245c 100644
--- a/service/google.js
+++ b/service/google.js
@@ -112,29 +112,48 @@ class Goggle extends Loby {
       // by the same insert. Without it an OAuth signup that arrived on a
       // campaign link is recorded as organic — not wrong, invisible.
       const utm = this._utmFromInput();
+      // Where the visitor was heading, parked for the third time and the same
+      // reason as `ref` and `utm` above — see _sanitiseDest. Validated HERE, on
+      // the way in, as well as on the way back out at callback.
+      const dest = this._destFromInput();
       try {
         await this.yp.await_query(
-          'INSERT IGNORE INTO oauth_state (state, session_id, ref, utm_source, utm_medium, utm_campaign, utm_content, ctime)'
-          + ' VALUES (?, ?, ?, ?, ?, ?, ?, UNIX_TIMESTAMP())',
+          'INSERT IGNORE INTO oauth_state (state, session_id, ref, utm_source, utm_medium, utm_campaign, utm_content, dest, ctime)'
+          + ' VALUES (?, ?, ?, ?, ?, ?, ?, ?, UNIX_TIMESTAMP())',
           state, this.input.sid(), ref || null,
           utm.utm_source || null, utm.utm_medium || null,
-          utm.utm_campaign || null, utm.utm_content || null
+          utm.utm_campaign || null, utm.utm_content || null,
+          dest || null
         );
       } catch (e) {
-        // DB without the optional utm columns: fall back to the shape that has
-        // only `ref`, then to the bare row. ATTRIBUTION IS BEST-EFFORT AND
-        // SIGNING IN IS NOT — a visitor must never be unable to sign in
-        // because a column for reporting is missing.
+        // One tier per column-group, newest first: a database that has utm_*
+        // but not `dest` still keeps the campaign. Falling straight to the bare
+        // row would throw away attribution that the instance can perfectly well
+        // store, for the sake of a column it cannot.
         try {
           await this.yp.await_query(
-            'INSERT IGNORE INTO oauth_state (state, session_id, ref, ctime) VALUES (?, ?, ?, UNIX_TIMESTAMP())',
-            state, this.input.sid(), ref || null
-          );
-        } catch (e2) {
-          await this.yp.await_query(
-            'INSERT IGNORE INTO oauth_state (state, session_id, ctime) VALUES (?, ?, UNIX_TIMESTAMP())',
-            state, this.input.sid()
+            'INSERT IGNORE INTO oauth_state (state, session_id, ref, utm_source, utm_medium, utm_campaign, utm_content, ctime)'
+            + ' VALUES (?, ?, ?, ?, ?, ?, ?, UNIX_TIMESTAMP())',
+            state, this.input.sid(), ref || null,
+            utm.utm_source || null, utm.utm_medium || null,
+            utm.utm_campaign || null, utm.utm_content || null
           );
+        } catch (eUtm) {
+          // DB without the optional utm columns: fall back to the shape that
+          // has only `ref`, then to the bare row. ATTRIBUTION IS BEST-EFFORT
+          // AND SIGNING IN IS NOT — a visitor must never be unable to sign in
+          // because a column for reporting is missing.
+          try {
+            await this.yp.await_query(
+              'INSERT IGNORE INTO oauth_state (state, session_id, ref, ctime) VALUES (?, ?, ?, UNIX_TIMESTAMP())',
+              state, this.input.sid(), ref || null
+            );
+          } catch (e2) {
+            await this.yp.await_query(
+              'INSERT IGNORE INTO oauth_state (state, session_id, ctime) VALUES (?, ?, UNIX_TIMESTAMP())',
+              state, this.input.sid()
+            );
+          }
         }
       }
 
@@ -174,7 +193,11 @@ class Goggle extends Loby {
       // session cookie (sendHtml/setAuthorization) and bounce the browser to the
       // signin app's OTP screen, which finalizes via oauth.verify_otp.
       if (res.status === 'otp_required') {
-        const redirect = `https://${main_domain}${endpoint_path}/#/welcome/signin?oauth_mfa=1&email=${encodeURIComponent(res.email || '')}`;
+        // `dest` rides through the OTP screen so a 2FA account arrives where the
+        // link named. Appended as an ordinary param on a hash that already has
+        // a query; the signin app hands it back once verify_otp finalises.
+        const destParam = res.dest ? `&dest=${encodeURIComponent(res.dest)}` : '';
+        const redirect = `https://${main_domain}${endpoint_path}/#/welcome/signin?oauth_mfa=1&email=${encodeURIComponent(res.email || '')}${destParam}`;
         const tpl = resolve(__dirname, './templates/otp-challenge.html');
         // res.session_id is the pending cookie's id (original signin session) —
         // sendHtml binds the browser's authorization to it.
@@ -186,7 +209,19 @@ class Goggle extends Loby {
         // previously this fell through without ever answering the browser.
         return this.sendOauthError(res.error);
       }
-      const home = `https://${res.domain}${endpoint_path}/`;
+      // THE DESTINATION GOES ON THE URL, not into storage. The visitor may land
+      // on a different deploy slot from the one they clicked on, and a fragment
+      // could never have reached this server in the first place — so the
+      // landing URL is the only carrier that works from here. ui-team's
+      // billing-deep-link consume() already reads a destination off the URL
+      // when storage has none, which is why nothing has to change over there.
+      //
+      // Re-validated at the point of use (handleOAuthCallback already ran it):
+      // this string is about to be interpolated into the landing page's
+      // location.replace(), and the code that builds a template's input is the
+      // code that has to have checked it.
+      const dest = this._sanitiseDest(res.dest);
+      const home = `https://${res.domain}${endpoint_path}/${dest ? `#${dest}` : ''}`;
       const tpl = resolve(__dirname, './templates/account-created.html');
       // New OAuth account: show the welcome card (auto_redirect off) so the
       // user lands on it; the CTA continues to the desk, where the onboarding
diff --git a/service/lib/loby.js b/service/lib/loby.js
index bd477fa..f22c89c 100644
--- a/service/lib/loby.js
+++ b/service/lib/loby.js
@@ -139,6 +139,100 @@ class Account extends Entity {
     return utm;
   }
 
+  /**
+   * The destination this visit named, as a flat request param.
+   *
+   * Read at initiate and parked on oauth_state beside `ref` and utm_*, for the
+   * same reason those are: the callback runs server-side, the visitor is
+   * bounced out to the provider in between, and browser storage is unreachable
+   * from there. A URL fragment is worse off still — it is never sent to a
+   * server at all.
+   *
+   * @returns {String|null} a validated fragment, or null
+   */
+  _destFromInput() {
+    return this._sanitiseDest(this.input.get("dest"));
+  }
+
+  /**
+   * Validate a destination, or refuse it.
+   *
+   * A SHAPE CHECK THAT REBUILDS, not an escaping pass. Every part is parsed,
+   * matched against an allowlist, and the string is assembled again from what
+   * survived — so a value can only ever be one this function could have
+   * written. Escaping alone would let an unrecognised-but-quoted destination
+   * through, and "somewhere the campaign never named" is the failure that
+   * matters here, not "a broken quote".
+   *
+   * WHY IT MATTERS MORE THAN IT LOOKS. The result ends up interpolated into
+   * the landing page's location.replace(), and lib/loby.js renders those
+   * templates with LODASH — whose <%= %> is the RAW delimiter, the reverse of
+   * EJS. An unvalidated apostrophe would therefore break out of that JS string
+   * literal into script context, on the page that runs immediately after
+   * authentication. The template is being escaped too (both are cheap); this
+   * is the guard that does not depend on remembering which delimiter is which.
+   *
+   * RUN AT BOTH ENDS — on the way into oauth_state and on the way back out of
+   * it. The row is data, and the code that builds the template's input is the
+   * code that has to have checked it.
+   *
+   * @param {*} raw
+   * @returns {String|null} "/desk/billing?…" or null
+   */
+  _sanitiseDest(raw) {
+    const s = (raw == null ? "" : String(raw)).trim();
+    // Length first, and REFUSED rather than truncated: half a destination is a
+    // wrong one, and the column is 255.
+    if (!s || s.length > 255) return null;
+    // No character that could leave the fragment, the attribute or the string
+    // literal it will sit in. Checked before parsing so nothing exotic reaches
+    // URLSearchParams.
+    if (/[\s"'`<>\\]/.test(s)) return null;
+
+    const q = s.indexOf("?");
+    const path = q === -1 ? s : s.slice(0, q);
+    // ONE path. Widening this list is the only way this function should ever
+    // learn a new destination — never by relaxing the parse.
+    if (path !== "/desk/billing") return null;
+    if (q === -1) return path;
+
+    const ALLOWED = {
+      plan: /^(free|pro|team|business)$/,
+      cycle: /^(monthly|yearly)$/,
+      tab: /^checkout$/,
+      // The shape yp.mkt_coupon.code stores (ascii, and the dashboard's own
+      // field). Not a lookup: whether the code exists is the checkout's
+      // question, and answering it here would refuse a valid link because a
+      // coupon was created a minute later.
+      promo: /^[A-Za-z0-9_-]{1,64}$/,
+    };
+    let usp;
+    try {
+      usp = new URLSearchParams(s.slice(q + 1));
+    } catch (e) {
+      return null;
+    }
+    // Rebuilt in a FIXED order from a fixed key list, so two links that mean
+    // the same thing produce the same string and an unknown key cannot ride
+    // along by being ignored.
+    const out = [];
+    for (const k of ["plan", "cycle", "tab", "promo"]) {
+      const v = usp.get(k);
+      if (v == null || v === "") continue;
+      if (!ALLOWED[k].test(v)) return null;
+      out.push(`${k}=${v}`);
+    }
+    // An unknown param is a refusal, not something to drop: it means this link
+    // was written against a contract this code does not have, and guessing
+    // which half of it to honour is how a destination becomes a wrong one.
+    for (const k of usp.keys()) {
+      if (!Object.prototype.hasOwnProperty.call(ALLOWED, k)) return null;
+    }
+    if (!out.length) return path;
+    const rebuilt = `${path}?${out.join("&")}`;
+    return rebuilt.length > 255 ? null : rebuilt;
+  }
+
   /**
    * Record one signup in yp.signup_track.
    *
@@ -462,7 +556,7 @@ class Account extends Entity {
       // databases that don't have the optional oauth_state.ref / utm_* columns
       // yet — those simply come back undefined there.
       const {
-        validState, session_id, ref,
+        validState, session_id, ref, dest,
         utm_source, utm_medium, utm_campaign, utm_content,
       } = await this.yp.await_query(
         'SELECT 1 validState, s.* FROM oauth_state s WHERE state = ? AND ctime > UNIX_TIMESTAMP() - 600 LIMIT 1',
@@ -528,6 +622,11 @@ class Account extends Entity {
         // oauth.verify_otp and logged there.
         await this._logConnection(sessionData.id);
         sessionData.method = 'signin';
+        // Where this visit was heading, recovered from the state row and
+        // re-validated on the way out — the callers append it to the landing
+        // URL. Carried on all three successful exits (signin, signup, 2FA), or
+        // it would work for some users and silently not for others.
+        sessionData.dest = this._sanitiseDest(dest);
         return sessionData;
       }
 
@@ -553,6 +652,7 @@ class Account extends Entity {
         if (Object.keys(utm).length) profile.utm = utm;
         let res = await this.addUser(profile);
         res.method = 'signup';
+        res.dest = this._sanitiseDest(dest);
         return res;
       }
 
@@ -572,7 +672,12 @@ class Account extends Entity {
           // redirect must bind the browser to THIS session so the SPA's later
           // oauth.verify_otp call resolves the right pending cookie.
           session_id,
-          provider
+          provider,
+          // Carried through the OTP screen: a 2FA account that arrived on a
+          // campaign link must land where the link named, like everyone else.
+          // The signin app hands it back on the redirect it builds after
+          // oauth.verify_otp finalises the session.
+          dest: this._sanitiseDest(dest),
         };
       }
 
diff --git a/service/templates/account-created.html b/service/templates/account-created.html
index 6683561..f9a27c0 100644
--- a/service/templates/account-created.html
+++ b/service/templates/account-created.html
@@ -7,7 +7,22 @@
   <% if (typeof auto_redirect !== 'undefined' && auto_redirect) { %>
   <!-- Existing OAuth sign-in: session is already established, go straight to
        the desk without showing the welcome card. -->
-  <script>location.replace('<%= home %>');</script>
+  <!-- JSON.stringify, not a URL pasted between quotes.
+       lib/loby.js renders these templates with LODASH, whose equals-delimiter
+       is the RAW one — the reverse of EJS, where it escapes. `home` now carries
+       a destination derived from the request (see _sanitiseDest in
+       lib/loby.js), so an apostrophe in it would break out of the string
+       literal into script context, on the page that runs immediately after
+       authentication. Stringify emits its own quotes and escapes theirs, so the
+       quoting stops being a hand-maintained property. The sanitiser refuses
+       quotes as well; this is the half that does not depend on anyone
+       remembering which delimiter is which.
+
+       EDITORS: do not write either lodash delimiter literally in a comment in
+       this file. lodash scans the whole template, comments included, so a
+       delimiter written to explain itself gets compiled — which is exactly how
+       this comment broke the build once. -->
+  <script>location.replace(<%= JSON.stringify(home) %>);</script>
   <% } %>
   <% if (typeof is_new !== 'undefined' && is_new) { %>
   <!-- New OAuth account: report the Google Ads sign-up conversion
@@ -127,7 +142,10 @@ <h1 class="card__title">Your account is all set!<br>Thanks for joining us.</h1>
       <p class="card__text">
         Your Drumee account has been successfully created. We're glad to have you &mdash; let's get your workspace ready.
       </p>
-      <a id="goto-desk" class="card__cta" href="<%= home %>">Discover your Drumee desk</a>
+      <!-- The new-account exit, on lodash's ESCAPING delimiter (the dash one).
+           Same request-derived URL as the redirect above, and an attribute is
+           as escapable as a string literal. -->
+      <a id="goto-desk" class="card__cta" href="<%- home %>">Discover your Drumee desk</a>
       <p class="card__footer">The Drumee team</p>
     </div>
   </div>
diff --git a/service/templates/otp-challenge.html b/service/templates/otp-challenge.html
index 8a61f4a..b8db0d1 100644
--- a/service/templates/otp-challenge.html
+++ b/service/templates/otp-challenge.html
@@ -4,7 +4,10 @@
   <meta charset="UTF-8">
   <meta name="viewport" content="width=device-width, initial-scale=1.0">
   <title>Verifying…</title>
-  <script>location.replace('<%= redirect %>');</script>
+  <!-- See the note in account-created.html: lodash's equals-delimiter is the
+       RAW one, and `redirect` now carries a dest param derived from the
+       request. -->
+  <script>location.replace(<%= JSON.stringify(redirect) %>);</script>
   <style>
     html, body { height: 100%; margin: 0; }
     body {

From 33a03dab3c85921f67ab2dbda994c9df4a308780 Mon Sep 17 00:00:00 2001
From: Drumee Dev <drumee@debian.local.drumee>
Date: Thu, 27 Aug 2026 16:32:57 -0700
Subject: [PATCH 2/2] fix(oauth): allow the recipient marker through the
 destination allowlist
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

_sanitiseDest refuses any param it does not recognise — deliberately, so a link
written against a contract this code does not have cannot be half-honoured. That
makes an omission here worse than elsewhere: a destination carrying the campaign
CTA's `for=<tag>` was rejected WHOLE, so the OAuth deep link stopped working
entirely rather than merely losing its marker.

Case-tolerant and normalised to lowercase on rebuild. The server only ever emits
lowercase and the dashboard compares case-insensitively, so refusing an
uppercased tag would kill an entire destination over something neither end cares
about.

An EMPTY `for=` reads as absent rather than malformed, like every other param:
stripping a value is no easier than stripping the whole param, and the marker is
a UX guard rather than a control either way. Stated as its own case because the
first version of that test asserted the opposite — my expectation was wrong, not
the code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 offline/test/oauth-dest.test.js | 41 +++++++++++++++++++++++++++++++++
 service/lib/loby.js             | 16 +++++++++++--
 2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/offline/test/oauth-dest.test.js b/offline/test/oauth-dest.test.js
index 9047af5..a58c0df 100644
--- a/offline/test/oauth-dest.test.js
+++ b/offline/test/oauth-dest.test.js
@@ -148,6 +148,47 @@ test("every accepted param is bounded and quote-free by its own regex", () => {
     + "stopped being belt-and-braces and needs its own case");
 });
 
+// ── the recipient marker must be allowed through ───────────────────────
+// The CTA carries `for=<tag>` naming who the mail was written for. An unknown
+// param is REFUSED outright by design, so forgetting this one here does not
+// merely lose the marker — it rejects the whole destination and the OAuth deep
+// link stops working entirely. That asymmetry is why it gets its own cases.
+test("an addressed destination survives intact", () => {
+  const d = "/desk/billing?plan=team&cycle=monthly&tab=checkout"
+    + "&promo=EMAILMKT270826_2&for=cd8f5912";
+  assert.strictEqual(sanitise(d), d,
+    "the addressed campaign destination is refused — the whole OAuth deep link dies");
+});
+
+test("the tag is case-tolerant and normalised", () => {
+  // The server only ever emits lowercase and the dashboard compares
+  // case-insensitively, so refusing an uppercased tag would kill an entire
+  // destination over something neither end cares about.
+  assert.strictEqual(sanitise("/desk/billing?for=CD8F5912"),
+    "/desk/billing?for=cd8f5912");
+});
+
+test("a malformed tag is refused", () => {
+  for (const bad of ["zzzzzzzz", "cd8f591", "cd8f59123", "cd8f5912x"]) {
+    assert.strictEqual(sanitise(`/desk/billing?for=${bad}`), null, `accepted "${bad}"`);
+  }
+});
+
+test("an EMPTY tag reads as absent, not as malformed", () => {
+  // `for=` with no value is dropped like any other empty param, leaving an
+  // unaddressed destination rather than a refusal. Consistent with plan=, tab=
+  // and the rest, and it costs nothing: stripping the value is no easier than
+  // stripping the whole param, and the marker is a UX guard rather than a
+  // control either way.
+  assert.strictEqual(sanitise("/desk/billing?plan=team&for="),
+    "/desk/billing?plan=team");
+});
+
+test("an unaddressed destination is still valid", () => {
+  // Links written before the marker existed carry none.
+  assert.strictEqual(sanitise("/desk/billing?plan=team"), "/desk/billing?plan=team");
+});
+
 // ── the ladder that keeps sign-in working ──────────────────────────────
 test("initiate degrades one column-group at a time", () => {
   // A database without `dest` must still keep the campaign, and one without
diff --git a/service/lib/loby.js b/service/lib/loby.js
index f22c89c..faa1baf 100644
--- a/service/lib/loby.js
+++ b/service/lib/loby.js
@@ -205,6 +205,17 @@ class Account extends Entity {
       // question, and answering it here would refuse a valid link because a
       // coupon was created a minute later.
       promo: /^[A-Za-z0-9_-]{1,64}$/,
+      // The marker naming who the campaign CTA was written for
+      // (analytics-server _recipientTag): 8 hex. It MUST be here — an unknown
+      // param is refused outright, so a destination carrying `for` would be
+      // rejected whole and the OAuth deep link would stop working entirely
+      // rather than merely losing its marker.
+      //
+      // Case-insensitive, and lowercased on the way out below. The server only
+      // ever emits lowercase and the dashboard compares case-insensitively, so
+      // refusing an uppercased tag would kill an entire destination over
+      // something neither end cares about.
+      for: /^[0-9a-fA-F]{8}$/,
     };
     let usp;
     try {
@@ -216,11 +227,12 @@ class Account extends Entity {
     // the same thing produce the same string and an unknown key cannot ride
     // along by being ignored.
     const out = [];
-    for (const k of ["plan", "cycle", "tab", "promo"]) {
+    for (const k of ["plan", "cycle", "tab", "promo", "for"]) {
       const v = usp.get(k);
       if (v == null || v === "") continue;
       if (!ALLOWED[k].test(v)) return null;
-      out.push(`${k}=${v}`);
+      // Normalised so the two ends compare equal whatever case arrived.
+      out.push(`${k}=${k === "for" ? v.toLowerCase() : v}`);
     }
     // An unknown param is a refusal, not something to drop: it means this link
     // was written against a contract this code does not have, and guessing