From 787ddd23c2917a070293ee7595146ae829e6a868 Mon Sep 17 00:00:00 2001 From: Aalv3 Date: Sun, 30 Aug 2026 16:36:04 -0400 Subject: [PATCH 01/20] Separate security-configuration auth failures from network failures A missing keychain entitlement (OSStatus -34018) surfaced to members as "Unable to connect. Please try again in a moment.", which pointed members and operators at connectivity instead of the real signing defect. Unknown programming faults fell into the same bucket. classifyAuthFailure now inspects code, message and name together, routes security-configuration faults to KEYCHAIN, matches genuine transport faults explicitly, and returns a new bounded UNKNOWN category instead of defaulting to NETWORK. Network copy is unchanged. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013ZGE4hGQn7cJiRBAP1o44B --- js/__tests__/authFailure.test.js | 91 ++++++++++++++++++++++++++++++++ js/authFailure.js | 88 ++++++++++++++++++++++++------ 2 files changed, 162 insertions(+), 17 deletions(-) diff --git a/js/__tests__/authFailure.test.js b/js/__tests__/authFailure.test.js index 1abb8ad4d..0f7c8b232 100644 --- a/js/__tests__/authFailure.test.js +++ b/js/__tests__/authFailure.test.js @@ -33,4 +33,95 @@ describe('authentication failure categories', () => { /token|payload|nonce|url/i, ); }); + + // Regression: the staging certification lane surfaced OSStatus -34018 + // ("Client has neither application-identifier nor keychain-access-groups + // entitlements") as "Unable to connect", which pointed operators at + // connectivity instead of the real signing/entitlement defect. + describe('security configuration failures are never network failures', () => { + const securityFailures = [ + 'The operation couldn’t be completed. (OSStatus error -34018.)', + 'Client has neither application-identifier nor keychain-access-groups entitlements', + 'errSecMissingEntitlement', + 'SecItemCopyMatching failed', + 'secure_storage_unavailable', + ]; + + test.each(securityFailures)('classifies %s as keychain', message => { + expect(classifyAuthFailure(new Error(message))).toBe( + AUTH_FAILURE.KEYCHAIN, + ); + }); + + test.each(securityFailures)('never shows network copy for %s', message => { + const alert = authFailureAlert(classifyAuthFailure(new Error(message))); + expect(alert.title).not.toBe('Unable to connect'); + expect(alert).toEqual({ + title: 'Secure sign-in unavailable', + message: 'Secure sign-in could not be prepared. Please try again.', + }); + }); + + test('classifies a numeric-code keychain rejection by its message', () => { + const error = Object.assign(new Error('OSStatus error -34018'), { + code: '-34018', + }); + expect(classifyAuthFailure(error)).toBe(AUTH_FAILURE.KEYCHAIN); + }); + }); + + describe('unknown and programming faults do not borrow network copy', () => { + const unknownFailures = [ + new TypeError('undefined is not a function'), + new TypeError("Cannot read property 'requestAuth' of undefined"), + new Error('something entirely unexpected'), + new Error(''), + undefined, + null, + ]; + + test.each(unknownFailures.map((e, i) => [i, e]))( + 'classifies unknown fault %i as UNKNOWN', + (_index, error) => { + expect(classifyAuthFailure(error)).toBe(AUTH_FAILURE.UNKNOWN); + }, + ); + + test('unknown copy is bounded and not connectivity-flavoured', () => { + const alert = authFailureAlert(AUTH_FAILURE.UNKNOWN); + expect(alert.title).not.toBe('Unable to connect'); + expect(alert.message).not.toMatch(/connect|network|offline|internet/i); + expect(alert.title).toBe('Sign-in could not be completed'); + }); + }); + + describe('genuine transport failures keep existing behaviour', () => { + test.each([ + 'Network request failed', + 'The request timed out', + 'The Internet connection appears to be offline.', + 'Could not connect to the server', + 'getaddrinfo ENOTFOUND staging.adjusternetwork.org', + ])('classifies %s as network', message => { + expect(classifyAuthFailure(new Error(message))).toBe( + AUTH_FAILURE.NETWORK, + ); + }); + + test('network copy is unchanged from the shipped build', () => { + expect(authFailureAlert(AUTH_FAILURE.NETWORK)).toEqual({ + title: 'Unable to connect', + message: 'Please try again in a moment.', + }); + }); + }); + + test('every category maps to bounded non-empty copy', () => { + Object.values(AUTH_FAILURE).forEach(category => { + const alert = authFailureAlert(category); + expect(alert.title.length).toBeGreaterThan(0); + expect(alert.message.length).toBeGreaterThan(0); + expect(/^[a-z0-9_.-]{1,64}$/.test(category)).toBe(true); + }); + }); }); diff --git a/js/authFailure.js b/js/authFailure.js index fddfd65e1..c269f6ee7 100644 --- a/js/authFailure.js +++ b/js/authFailure.js @@ -8,37 +8,86 @@ export const AUTH_FAILURE = Object.freeze({ PRESENTATION: 'presentation_failure', USER_CANCEL: 'user_cancel', CALLBACK: 'callback_failure', + UNKNOWN: 'unknown_failure', }); +// Keychain access can fail for reasons that never mention "keychain": iOS +// reports a missing entitlement as OSStatus -34018, and react-native-keychain +// surfaces it as a bare "OSStatus error" string. Those are security +// configuration faults, not connectivity faults, and must never be presented +// to a member as a network problem. +const SECURITY_PATTERNS = [ + 'keychain', + 'credential', + 'rsa_key', + 'secure_storage', + 'entitlement', + 'osstatus', + 'errsec', + '-34018', + 'secitem', +]; + +// Only a genuine transport fault may use the connectivity copy. Anything that +// is not recognised here stays UNKNOWN rather than borrowing that message. +const NETWORK_PATTERNS = [ + 'network request failed', + 'network error', + 'timeout', + 'timed out', + 'offline', + 'connection', + 'unable to connect', + 'could not connect', + 'host', + 'dns', + 'econn', + 'enotfound', + 'internet', +]; + +function haystack(error) { + // `code` alone is not enough: a native rejection can carry a numeric code + // while the diagnostic detail lives only in the message. + return [error?.code, error?.message, error?.name] + .filter(value => value !== undefined && value !== null) + .map(value => String(value)) + .join(' ') + .toLowerCase(); +} + export function classifyAuthFailure(error) { - const code = String(error?.code || error?.message || '').toLowerCase(); - if (code.includes('auth_user_cancelled')) return AUTH_FAILURE.USER_CANCEL; + const text = haystack(error); + + if (text.includes('auth_user_cancelled')) return AUTH_FAILURE.USER_CANCEL; if ( - code.includes('auth_presentation') || - code.includes('auth_start_failed') || - code.includes('auth_session_failed') + text.includes('auth_presentation') || + text.includes('auth_start_failed') || + text.includes('auth_session_failed') ) { return AUTH_FAILURE.PRESENTATION; } - if ( - code.includes('keychain') || - code.includes('credential') || - code.includes('rsa_key') || - code.includes('secure_storage') - ) { + if (SECURITY_PATTERNS.some(pattern => text.includes(pattern))) { return AUTH_FAILURE.KEYCHAIN; } - if (code.includes('auth_invalid_url') || code.includes('auth_origin')) { + if (text.includes('auth_invalid_url') || text.includes('auth_origin')) { return AUTH_FAILURE.AUTH_URL; } if ( - code.includes('auth_callback') || - code.includes('auth_payload') || - code.includes('auth_nonce') + text.includes('auth_callback') || + text.includes('auth_payload') || + text.includes('auth_nonce') ) { return AUTH_FAILURE.CALLBACK; } - return AUTH_FAILURE.NETWORK; + if (NETWORK_PATTERNS.some(pattern => text.includes(pattern))) { + return AUTH_FAILURE.NETWORK; + } + + // A programming fault (TypeError, undefined native module) reaching this + // point previously rendered as "Unable to connect", which sent members and + // operators to look at connectivity instead of the real defect. + return AUTH_FAILURE.UNKNOWN; } export function authFailureAlert(category) { @@ -65,10 +114,15 @@ export function authFailureAlert(category) { message: 'The secure sign-in response could not be verified. Please try again.', }; - default: + case AUTH_FAILURE.NETWORK: return { title: 'Unable to connect', message: 'Please try again in a moment.', }; + default: + return { + title: 'Sign-in could not be completed', + message: 'Sign-in could not be completed. Please try again.', + }; } } From b2f7a401969dab74f239d6c27ba20b9f386231e8 Mon Sep 17 00:00:00 2001 From: Aalv3 Date: Sun, 30 Aug 2026 19:17:11 -0400 Subject: [PATCH 02/20] Return to sign-in when a User API credential is authoritatively retired A revoked credential cleared the in-memory token but never told SiteManager, so the persisted site record kept the dead token and the root navigator stayed AUTHENTICATED. The member was stranded on "Your saved profile could not be loaded" with a Try again button that reissued the same doomed request, and a relaunch rehydrated the dead token and repeated it. Only deleting the app recovered. Site.retireCredential now latches the retirement and notifies the manager, which persists the cleared record, drops the keychain token, authorization profile, avatar authority and cached authenticated GETs, and notifies subscribers so the root falls back to the signed-out welcome screen. Retirement remains driven solely by classifyAuthResponse's authoritative 401. Ordinary 403 authorization limits, onboarding/policy gating, 429 cooldowns, offline failures and 5xx errors continue to preserve the session. A stale auth completion can no longer restore a token retired while it was in flight, and a verified fresh authorization clears the latch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013ZGE4hGQn7cJiRBAP1o44B --- js/__tests__/credentialRetirement.test.js | 118 ++++++++++++++++++++++ js/site.js | 17 +++- js/site_manager.js | 54 +++++++++- 3 files changed, 183 insertions(+), 6 deletions(-) create mode 100644 js/__tests__/credentialRetirement.test.js diff --git a/js/__tests__/credentialRetirement.test.js b/js/__tests__/credentialRetirement.test.js new file mode 100644 index 000000000..5d27243cf --- /dev/null +++ b/js/__tests__/credentialRetirement.test.js @@ -0,0 +1,118 @@ +/* @flow */ +'use strict'; + +import { classifyAuthResponse } from '../authResponsePolicy'; +import Site from '../site'; + +jest.mock('../secureCredentialStore', () => ({ + credentialStore: { + removeSiteToken: jest.fn().mockResolvedValue(undefined), + storeSiteToken: jest.fn().mockResolvedValue(undefined), + readSiteToken: jest.fn().mockResolvedValue(null), + }, +})); + +const makeSite = () => + new Site({ + url: 'https://adjusternetwork.org', + title: 'Adjuster Network', + apiVersion: 4, + authToken: 'live-token', + }); + +describe('authoritative credential retirement', () => { + test('only 401 is classified as revoked', () => { + expect(classifyAuthResponse(401)).toBe('revoked'); + [403, 429, 404, 500, 502, 503, 200, 204].forEach(status => { + expect(classifyAuthResponse(status)).not.toBe('revoked'); + }); + }); + + test('retireCredential clears the token and notifies the manager once', () => { + const site = makeSite(); + const seen = []; + site.onCredentialRetired = (retired, reason) => + seen.push([retired.url, reason]); + + site.retireCredential('revoked'); + expect(site.authToken).toBeNull(); + expect(site.credentialRetired).toBe(true); + expect(seen).toEqual([['https://adjusternetwork.org', 'revoked']]); + + // Idempotent: a second authoritative failure must not re-notify. + site.retireCredential('revoked'); + expect(seen).toHaveLength(1); + }); + + test('logoff alone does not mark the credential retired', () => { + const site = makeSite(); + const seen = []; + site.onCredentialRetired = () => seen.push('notified'); + site.logoff(); + expect(site.authToken).toBeNull(); + expect(site.credentialRetired).toBeFalsy(); + expect(seen).toHaveLength(0); + }); + + describe('non-authoritative failures never retire the credential', () => { + test.each([ + ['ordinary authorization 403', 403], + ['rate limit 429', 429], + ['not found 404', 404], + ['server failure 500', 500], + ['bad gateway 502', 502], + ['unavailable 503', 503], + ])('%s preserves the session', (_label, status) => { + const site = makeSite(); + let notified = false; + site.onCredentialRetired = () => { + notified = true; + }; + if (classifyAuthResponse(status) === 'revoked') { + site.retireCredential('revoked'); + } + expect(site.authToken).toBe('live-token'); + expect(site.credentialRetired).toBeFalsy(); + expect(notified).toBe(false); + }); + + test('offline/network rejection preserves the session', () => { + const site = makeSite(); + let notified = false; + site.onCredentialRetired = () => { + notified = true; + }; + // A transport rejection never reaches a status classification at all. + const error = new Error('Network request failed'); + expect(error.status).toBeUndefined(); + expect(site.authToken).toBe('live-token'); + expect(notified).toBe(false); + }); + }); + + test('a retired site reports as not connected so the root signs out', () => { + const site = makeSite(); + site.onCredentialRetired = () => {}; + const connected = sites => sites.filter(s => s.authToken).length; + expect(connected([site])).toBe(1); + site.retireCredential('revoked'); + // This is exactly the predicate the root navigator uses to choose between + // the authenticated shell and the signed-out welcome screen. + expect(connected([site])).toBe(0); + expect([site].find(s => s.authToken)).toBeUndefined(); + }); + + test('a fresh verified authorization clears the retirement latch', () => { + const site = makeSite(); + site.onCredentialRetired = () => {}; + site.retireCredential('revoked'); + expect(site.credentialRetired).toBe(true); + + // Mirrors handleAuthPayload's post-verification assignment. + site.authToken = 'new-token'; + site.credentialRetired = false; + site.credentialRetiredReason = null; + expect(site.authToken).toBe('new-token'); + expect(site.credentialRetired).toBe(false); + }); +}); diff --git a/js/site.js b/js/site.js index 396707bd2..34db5c44b 100644 --- a/js/site.js +++ b/js/site.js @@ -234,7 +234,7 @@ class Site { error.rateLimitCode = errorCode || null; throw error; } else if (classifyAuthResponse(r1.status) === 'revoked') { - this.logoff(); + this.retireCredential('revoked'); credentialStore.removeSiteToken(this.url).catch(() => {}); const error = new Error('auth_revoked'); error.status = r1.status; @@ -301,7 +301,7 @@ class Site { } const classification = classifyAuthResponse(response.status); if (classification === 'revoked') { - this.logoff(); + this.retireCredential('revoked'); credentialStore.removeSiteToken(this.url).catch(() => {}); } const error = new Error( @@ -336,6 +336,19 @@ class Site { this.isStaff = null; } + // An authoritative 401 means the stored User API credential no longer exists + // server-side. Clearing it in memory is not enough: without telling the + // manager, the persisted record keeps the dead token and the signed-in + // navigator never re-evaluates, stranding the member on an authenticated + // screen whose only action retries the same doomed request. + retireCredential(reason) { + if (this.credentialRetired) return; + this.credentialRetired = true; + this.credentialRetiredReason = reason || 'revoked'; + this.logoff(); + this.onCredentialRetired?.(this, this.credentialRetiredReason); + } + ensureLatestApi() { if (this.apiVersion < 2) { this.logoff(); diff --git a/js/site_manager.js b/js/site_manager.js index 951326086..82bdb6c99 100644 --- a/js/site_manager.js +++ b/js/site_manager.js @@ -28,13 +28,23 @@ import { } from './notificationState'; import { clearAvatarAuthorityForSite } from './product/avatarAuthority'; import { + clearAuthorizationProfile, markAuthorizationProfileCurrent, REQUIRED_AUTHORIZATION_SCOPES, validateAuthorizationProfile, } from './authorizationProfile'; +import { requestOrchestrator } from './requestOrchestrator'; const { DiscourseKeyboardShortcuts } = NativeModules; const REFRESH_THROTTLE_MS = 5000; +// Authenticated GETs the orchestrator may still be caching for a site whose +// credential has just been retired. They must not survive into a fresh +// authorization for the same client id and path. +const RETIRED_CREDENTIAL_CACHE_PATHS = Object.freeze([ + '/native/v1/profile', + '/native/v1/onboarding', + '/native/v1/authorization-profile', +]); class SiteManager { lastRefresh = null; @@ -78,7 +88,7 @@ class SiteManager { } site.createdAt = Date.now(); - this.sites.push(site); + this.sites.push(this._adoptSite(site)); this.save(); this._onChange(); this.updateNativeMenu(); @@ -231,7 +241,7 @@ class SiteManager { const clientId = await this.getClientId(); this.sites = await Promise.all( records.map(async obj => { - const site = new Site(obj); + const site = this._adoptSite(new Site(obj)); // Repair sites saved by older builds that retained the manager // client ID but did not serialize it with site metadata. site.clientId = site.clientId || clientId; @@ -450,13 +460,20 @@ class SiteManager { const previousToken = nonceSite.authToken; const previousHasPush = nonceSite.hasPush; const previousApiVersion = nonceSite.apiVersion; + const retiredBefore = nonceSite.credentialRetired === true; const restorePreviousAuthorization = async () => { - nonceSite.authToken = previousToken; + // A credential retired by an authoritative 401 must stay retired. Only + // restore the prior token when it was still live when this attempt + // started and nothing retired it while the attempt was in flight. + const stillRetired = + retiredBefore || nonceSite.credentialRetired === true; nonceSite.hasPush = previousHasPush; nonceSite.apiVersion = previousApiVersion; - if (previousToken) { + if (previousToken && !stillRetired) { + nonceSite.authToken = previousToken; await credentialStore.storeSiteToken(nonceSite.url, previousToken); } else { + nonceSite.authToken = null; await credentialStore.removeSiteToken(nonceSite.url); } }; @@ -485,6 +502,9 @@ class SiteManager { await restorePreviousAuthorization(); return false; } + // A verified fresh authorization supersedes any earlier retirement. + nonceSite.credentialRetired = false; + nonceSite.credentialRetiredReason = null; this.save(); // cause we want to stop rendering connect @@ -771,6 +791,32 @@ class SiteManager { this._subscribers.forEach(sub => sub({ event: 'change' })); } + // Only an authoritative 401 reaches this path. Ordinary 403 authorization + // limits, onboarding/policy gating, 429 cooldowns, offline failures and 5xx + // errors all preserve the session by design and must never retire a + // credential. + _adoptSite(site) { + site.onCredentialRetired = retiredSite => + this._handleCredentialRetired(retiredSite); + return site; + } + + async _handleCredentialRetired(site) { + if (!site) return; + // Persist first so a relaunch cannot rehydrate the dead token, then drop + // every authenticated artifact tied to it, then let the root navigator + // re-evaluate and fall back to the signed-out welcome screen. + this.save(); + await credentialStore.removeSiteToken(site.url).catch(() => {}); + await clearAuthorizationProfile(site.clientId).catch(() => {}); + clearAvatarAuthorityForSite(site); + requestOrchestrator.invalidate( + RETIRED_CREDENTIAL_CACHE_PATHS.map(path => site.apiRequestKey(path)), + ); + this.updateUnreadBadge(); + this._onChange(); + } + storeLastPath(navState) { let shouldSave = false; From a767ed51d12b547a408c19a44bcdc2989e4d7a0b Mon Sep 17 00:00:00 2001 From: Aalv3 Date: Sun, 30 Aug 2026 19:43:44 -0400 Subject: [PATCH 03/20] Retire a revoked credential on canonical evidence, not on 403 alone Discourse answers a revoked or deleted User API key with 403, the same status an ordinary permission denial uses, so the previous 401-only rule never fired and the member stayed stranded on the authenticated bootstrap error. A forbidden response now retires the credential only on canonical evidence: not_logged_in is unambiguous and retires immediately, while the shared invalid_access is resolved by asking /session/current.json - an endpoint every authorized client can reach - whether the credential still authenticates. Preserved by construction: onboarding and policy gating (they carry reason or continue_at and are never probed), ordinary permission denials with a live credential, 429 cooldowns, 5xx failures and offline transport errors, all of which leave the session intact. The probe is a bare fetch so it cannot recurse through this branch, and it is de-duplicated per site. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013ZGE4hGQn7cJiRBAP1o44B --- js/__tests__/credentialRetirement.test.js | 78 +++++++++++++++++++++++ js/site.js | 70 +++++++++++++++++++- 2 files changed, 145 insertions(+), 3 deletions(-) diff --git a/js/__tests__/credentialRetirement.test.js b/js/__tests__/credentialRetirement.test.js index 5d27243cf..50089d8c8 100644 --- a/js/__tests__/credentialRetirement.test.js +++ b/js/__tests__/credentialRetirement.test.js @@ -4,6 +4,7 @@ import { classifyAuthResponse } from '../authResponsePolicy'; import Site from '../site'; +jest.mock('../../lib/fetch', () => jest.fn()); jest.mock('../secureCredentialStore', () => ({ credentialStore: { removeSiteToken: jest.fn().mockResolvedValue(undefined), @@ -102,6 +103,83 @@ describe('authoritative credential retirement', () => { expect([site].find(s => s.authToken)).toBeUndefined(); }); + describe('403 differentiation via canonical liveness probe', () => { + // site.js uses the repo's XHR-based fetch wrapper, not global.fetch. + const fetchMock = require('../../lib/fetch'); + + const withProbe = (probeStatus, opts = {}) => { + const site = makeSite(); + site.onCredentialRetired = () => {}; + fetchMock.mockReset(); + if (opts.throws) { + fetchMock.mockRejectedValue(new Error('Network request failed')); + } else { + fetchMock.mockResolvedValue({ status: probeStatus }); + } + return site; + }; + + test('not_logged_in retires without any probe', async () => { + const site = withProbe(200); + await expect( + site.shouldRetireOnForbidden({ error_type: 'not_logged_in' }), + ).resolves.toBe(true); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + test('revoked key (invalid_access, probe 403) retires', async () => { + const site = withProbe(403); + await expect( + site.shouldRetireOnForbidden({ error_type: 'invalid_access' }), + ).resolves.toBe(true); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + test('ordinary permission 403 with a live credential preserves the session', async () => { + const site = withProbe(200); + await expect( + site.shouldRetireOnForbidden({ error_type: 'invalid_access' }), + ).resolves.toBe(false); + }); + + test('onboarding/policy gating never retires and never probes', async () => { + const site = withProbe(403); + await expect( + site.shouldRetireOnForbidden({ + error_type: 'invalid_access', + reason: 'onboarding_incomplete', + continue_at: '/renaissance/onboarding', + }), + ).resolves.toBe(false); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + test.each([429, 500, 502, 503])( + 'inconclusive probe status %i preserves the session', + async status => { + const site = withProbe(status); + await expect( + site.shouldRetireOnForbidden({ error_type: 'invalid_access' }), + ).resolves.toBe(false); + }, + ); + + test('offline probe failure preserves the session', async () => { + const site = withProbe(0, { throws: true }); + await expect( + site.shouldRetireOnForbidden({ error_type: 'invalid_access' }), + ).resolves.toBe(false); + }); + + test('a site with no token never retires again', async () => { + const site = makeSite(); + site.authToken = null; + await expect( + site.shouldRetireOnForbidden({ error_type: 'not_logged_in' }), + ).resolves.toBe(false); + }); + }); + test('a fresh verified authorization clears the retirement latch', () => { const site = makeSite(); site.onCredentialRetired = () => {}; diff --git a/js/site.js b/js/site.js index 34db5c44b..68e59c2bf 100644 --- a/js/site.js +++ b/js/site.js @@ -241,14 +241,21 @@ class Site { throw error; } else if (classifyAuthResponse(r1.status) === 'forbidden') { // A valid, narrowly scoped user API key can be forbidden from an - // endpoint without being revoked. Preserve the session and let the - // caller render an unavailable state. + // endpoint without being revoked. Discourse also answers a revoked + // or deleted key with 403, so status alone cannot tell the two + // apart. Preserve the session by default and only retire the + // credential on canonical evidence that it no longer authenticates. const error = new Error('auth_forbidden'); error.status = r1.status; + let payload = null; try { - const payload = await r1.json(); + payload = await r1.json(); error.code = typeof payload?.error === 'string' ? payload.error : null; + error.errorType = + typeof payload?.error_type === 'string' + ? payload.error_type + : null; error.reason = typeof payload?.reason === 'string' ? payload.reason : null; error.continueAt = @@ -261,6 +268,11 @@ class Site { } catch { error.userMessages = []; } + if (await this.shouldRetireOnForbidden(payload)) { + this.retireCredential('revoked'); + credentialStore.removeSiteToken(this.url).catch(() => {}); + error.credentialRetired = true; + } throw error; } else { const error = new Error('api_request_failed'); @@ -336,6 +348,58 @@ class Site { this.isStaff = null; } + // Onboarding and policy gating answer 403 with an explanatory reason and a + // continuation target. Those are live-credential product states and must + // never retire anything. + static forbiddenIsGating(payload) { + return ( + typeof payload?.reason === 'string' || + typeof payload?.continue_at === 'string' + ); + } + + async shouldRetireOnForbidden(payload) { + if (!this.authToken) return false; + if (Site.forbiddenIsGating(payload)) return false; + // Discourse reports an absent or unusable credential as not_logged_in. + // That is unambiguous and needs no second request. + if (payload?.error_type === 'not_logged_in') return true; + // invalid_access is shared by an ordinary permission denial and a revoked + // key, so ask an endpoint every authorized client can reach whether this + // credential still authenticates at all. + return !(await this.credentialStillAuthenticates()); + } + + // Deliberately a bare fetch: it must not re-enter jsonApi (which would + // recurse through this same branch), must not be cached, and must not be + // deferred behind the orchestrator's cooldowns. + async credentialStillAuthenticates() { + if (this._credentialProbe) return this._credentialProbe; + this._credentialProbe = (async () => { + try { + const probe = await fetch(`${this.url}/session/current.json`, { + method: 'GET', + headers: { + 'User-Api-Key': this.authToken, + 'User-Api-Client-Id': this.clientId || '', + 'Content-Type': 'application/json', + 'Dont-Chunk': 'true', + }, + }); + if (probe.status >= 200 && probe.status < 300) return true; + if (probe.status === 401 || probe.status === 403) return false; + // 429, 5xx and anything else are inconclusive; never retire on those. + return true; + } catch { + // Offline or transport failure proves nothing about the credential. + return true; + } finally { + this._credentialProbe = null; + } + })(); + return this._credentialProbe; + } + // An authoritative 401 means the stored User API credential no longer exists // server-side. Clearing it in memory is not enough: without telling the // manager, the persisted record keeps the dead token and the signed-in From 0565d50d380cdcaaf0f0a3ef8e5ee05145d3a41f Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Sun, 30 Aug 2026 19:58:43 -0400 Subject: [PATCH 04/20] Revert "Retire a revoked credential on canonical evidence, not on 403 alone" This reverts commit a767ed51d12b547a408c19a44bcdc2989e4d7a0b. --- js/__tests__/credentialRetirement.test.js | 78 ----------------------- js/site.js | 70 +------------------- 2 files changed, 3 insertions(+), 145 deletions(-) diff --git a/js/__tests__/credentialRetirement.test.js b/js/__tests__/credentialRetirement.test.js index 50089d8c8..5d27243cf 100644 --- a/js/__tests__/credentialRetirement.test.js +++ b/js/__tests__/credentialRetirement.test.js @@ -4,7 +4,6 @@ import { classifyAuthResponse } from '../authResponsePolicy'; import Site from '../site'; -jest.mock('../../lib/fetch', () => jest.fn()); jest.mock('../secureCredentialStore', () => ({ credentialStore: { removeSiteToken: jest.fn().mockResolvedValue(undefined), @@ -103,83 +102,6 @@ describe('authoritative credential retirement', () => { expect([site].find(s => s.authToken)).toBeUndefined(); }); - describe('403 differentiation via canonical liveness probe', () => { - // site.js uses the repo's XHR-based fetch wrapper, not global.fetch. - const fetchMock = require('../../lib/fetch'); - - const withProbe = (probeStatus, opts = {}) => { - const site = makeSite(); - site.onCredentialRetired = () => {}; - fetchMock.mockReset(); - if (opts.throws) { - fetchMock.mockRejectedValue(new Error('Network request failed')); - } else { - fetchMock.mockResolvedValue({ status: probeStatus }); - } - return site; - }; - - test('not_logged_in retires without any probe', async () => { - const site = withProbe(200); - await expect( - site.shouldRetireOnForbidden({ error_type: 'not_logged_in' }), - ).resolves.toBe(true); - expect(fetchMock).not.toHaveBeenCalled(); - }); - - test('revoked key (invalid_access, probe 403) retires', async () => { - const site = withProbe(403); - await expect( - site.shouldRetireOnForbidden({ error_type: 'invalid_access' }), - ).resolves.toBe(true); - expect(fetchMock).toHaveBeenCalledTimes(1); - }); - - test('ordinary permission 403 with a live credential preserves the session', async () => { - const site = withProbe(200); - await expect( - site.shouldRetireOnForbidden({ error_type: 'invalid_access' }), - ).resolves.toBe(false); - }); - - test('onboarding/policy gating never retires and never probes', async () => { - const site = withProbe(403); - await expect( - site.shouldRetireOnForbidden({ - error_type: 'invalid_access', - reason: 'onboarding_incomplete', - continue_at: '/renaissance/onboarding', - }), - ).resolves.toBe(false); - expect(fetchMock).not.toHaveBeenCalled(); - }); - - test.each([429, 500, 502, 503])( - 'inconclusive probe status %i preserves the session', - async status => { - const site = withProbe(status); - await expect( - site.shouldRetireOnForbidden({ error_type: 'invalid_access' }), - ).resolves.toBe(false); - }, - ); - - test('offline probe failure preserves the session', async () => { - const site = withProbe(0, { throws: true }); - await expect( - site.shouldRetireOnForbidden({ error_type: 'invalid_access' }), - ).resolves.toBe(false); - }); - - test('a site with no token never retires again', async () => { - const site = makeSite(); - site.authToken = null; - await expect( - site.shouldRetireOnForbidden({ error_type: 'not_logged_in' }), - ).resolves.toBe(false); - }); - }); - test('a fresh verified authorization clears the retirement latch', () => { const site = makeSite(); site.onCredentialRetired = () => {}; diff --git a/js/site.js b/js/site.js index 68e59c2bf..34db5c44b 100644 --- a/js/site.js +++ b/js/site.js @@ -241,21 +241,14 @@ class Site { throw error; } else if (classifyAuthResponse(r1.status) === 'forbidden') { // A valid, narrowly scoped user API key can be forbidden from an - // endpoint without being revoked. Discourse also answers a revoked - // or deleted key with 403, so status alone cannot tell the two - // apart. Preserve the session by default and only retire the - // credential on canonical evidence that it no longer authenticates. + // endpoint without being revoked. Preserve the session and let the + // caller render an unavailable state. const error = new Error('auth_forbidden'); error.status = r1.status; - let payload = null; try { - payload = await r1.json(); + const payload = await r1.json(); error.code = typeof payload?.error === 'string' ? payload.error : null; - error.errorType = - typeof payload?.error_type === 'string' - ? payload.error_type - : null; error.reason = typeof payload?.reason === 'string' ? payload.reason : null; error.continueAt = @@ -268,11 +261,6 @@ class Site { } catch { error.userMessages = []; } - if (await this.shouldRetireOnForbidden(payload)) { - this.retireCredential('revoked'); - credentialStore.removeSiteToken(this.url).catch(() => {}); - error.credentialRetired = true; - } throw error; } else { const error = new Error('api_request_failed'); @@ -348,58 +336,6 @@ class Site { this.isStaff = null; } - // Onboarding and policy gating answer 403 with an explanatory reason and a - // continuation target. Those are live-credential product states and must - // never retire anything. - static forbiddenIsGating(payload) { - return ( - typeof payload?.reason === 'string' || - typeof payload?.continue_at === 'string' - ); - } - - async shouldRetireOnForbidden(payload) { - if (!this.authToken) return false; - if (Site.forbiddenIsGating(payload)) return false; - // Discourse reports an absent or unusable credential as not_logged_in. - // That is unambiguous and needs no second request. - if (payload?.error_type === 'not_logged_in') return true; - // invalid_access is shared by an ordinary permission denial and a revoked - // key, so ask an endpoint every authorized client can reach whether this - // credential still authenticates at all. - return !(await this.credentialStillAuthenticates()); - } - - // Deliberately a bare fetch: it must not re-enter jsonApi (which would - // recurse through this same branch), must not be cached, and must not be - // deferred behind the orchestrator's cooldowns. - async credentialStillAuthenticates() { - if (this._credentialProbe) return this._credentialProbe; - this._credentialProbe = (async () => { - try { - const probe = await fetch(`${this.url}/session/current.json`, { - method: 'GET', - headers: { - 'User-Api-Key': this.authToken, - 'User-Api-Client-Id': this.clientId || '', - 'Content-Type': 'application/json', - 'Dont-Chunk': 'true', - }, - }); - if (probe.status >= 200 && probe.status < 300) return true; - if (probe.status === 401 || probe.status === 403) return false; - // 429, 5xx and anything else are inconclusive; never retire on those. - return true; - } catch { - // Offline or transport failure proves nothing about the credential. - return true; - } finally { - this._credentialProbe = null; - } - })(); - return this._credentialProbe; - } - // An authoritative 401 means the stored User API credential no longer exists // server-side. Clearing it in memory is not enough: without telling the // manager, the persisted record keeps the dead token and the signed-in From bf0ba815dc72bd2b0940d4626f7131e4f028d139 Mon Sep 17 00:00:00 2001 From: Alex Alvarez Date: Sun, 30 Aug 2026 21:21:54 -0400 Subject: [PATCH 05/20] Retire credentials only on canonical server signal --- js/__tests__/authResponsePolicy.test.js | 13 +++- js/__tests__/credentialRetirement.test.js | 22 +++++-- js/authResponsePolicy.js | 15 ++++- js/site.js | 79 ++++++++++++----------- 4 files changed, 84 insertions(+), 45 deletions(-) diff --git a/js/__tests__/authResponsePolicy.test.js b/js/__tests__/authResponsePolicy.test.js index c4fa91964..242717fd9 100644 --- a/js/__tests__/authResponsePolicy.test.js +++ b/js/__tests__/authResponsePolicy.test.js @@ -1,8 +1,17 @@ -import { classifyAuthResponse } from '../authResponsePolicy'; +import { + classifyAuthResponse, + INVALID_USER_API_CREDENTIAL, +} from '../authResponsePolicy'; describe('auth response policy', () => { test('revokes a session only when authentication is invalid', () => { - expect(classifyAuthResponse(401)).toBe('revoked'); + expect( + classifyAuthResponse(401, { + error_type: INVALID_USER_API_CREDENTIAL.errorType, + reason: INVALID_USER_API_CREDENTIAL.reason, + }), + ).toBe('revoked'); + expect(classifyAuthResponse(401, null)).toBe('other'); }); test('preserves narrowly scoped sessions when an endpoint is forbidden', () => { diff --git a/js/__tests__/credentialRetirement.test.js b/js/__tests__/credentialRetirement.test.js index 5d27243cf..08daab7e8 100644 --- a/js/__tests__/credentialRetirement.test.js +++ b/js/__tests__/credentialRetirement.test.js @@ -1,7 +1,10 @@ /* @flow */ 'use strict'; -import { classifyAuthResponse } from '../authResponsePolicy'; +import { + classifyAuthResponse, + INVALID_USER_API_CREDENTIAL, +} from '../authResponsePolicy'; import Site from '../site'; jest.mock('../secureCredentialStore', () => ({ @@ -21,10 +24,19 @@ const makeSite = () => }); describe('authoritative credential retirement', () => { - test('only 401 is classified as revoked', () => { - expect(classifyAuthResponse(401)).toBe('revoked'); + test('only the canonical server credential tuple is classified as revoked', () => { + const canonical = { + error_type: INVALID_USER_API_CREDENTIAL.errorType, + reason: INVALID_USER_API_CREDENTIAL.reason, + }; + expect(classifyAuthResponse(401, canonical)).toBe('revoked'); + expect(classifyAuthResponse(401, null)).not.toBe('revoked'); + expect( + classifyAuthResponse(401, { error_type: canonical.error_type }), + ).not.toBe('revoked'); + expect(classifyAuthResponse(403, canonical)).toBe('forbidden'); [403, 429, 404, 500, 502, 503, 200, 204].forEach(status => { - expect(classifyAuthResponse(status)).not.toBe('revoked'); + expect(classifyAuthResponse(status, canonical)).not.toBe('revoked'); }); }); @@ -68,7 +80,7 @@ describe('authoritative credential retirement', () => { site.onCredentialRetired = () => { notified = true; }; - if (classifyAuthResponse(status) === 'revoked') { + if (classifyAuthResponse(status, null) === 'revoked') { site.retireCredential('revoked'); } expect(site.authToken).toBe('live-token'); diff --git a/js/authResponsePolicy.js b/js/authResponsePolicy.js index def51aa0c..3869ab71b 100644 --- a/js/authResponsePolicy.js +++ b/js/authResponsePolicy.js @@ -1,8 +1,19 @@ /* @flow */ 'use strict'; -export const classifyAuthResponse = (status: number) => { - if (status === 401) return 'revoked'; +export const INVALID_USER_API_CREDENTIAL = Object.freeze({ + errorType: 'invalid_user_api_credential', + reason: 'invalid_or_revoked_or_expired', +}); + +export const classifyAuthResponse = (status: number, payload: ?Object) => { + if ( + status === 401 && + payload?.error_type === INVALID_USER_API_CREDENTIAL.errorType && + payload?.reason === INVALID_USER_API_CREDENTIAL.reason + ) { + return 'revoked'; + } if (status === 403) return 'forbidden'; return 'other'; }; diff --git a/js/site.js b/js/site.js index 34db5c44b..855e53235 100644 --- a/js/site.js +++ b/js/site.js @@ -233,20 +233,28 @@ class Site { error.retryAfterMs = retryAfterMs; error.rateLimitCode = errorCode || null; throw error; - } else if (classifyAuthResponse(r1.status) === 'revoked') { - this.retireCredential('revoked'); - credentialStore.removeSiteToken(this.url).catch(() => {}); - const error = new Error('auth_revoked'); - error.status = r1.status; - throw error; - } else if (classifyAuthResponse(r1.status) === 'forbidden') { - // A valid, narrowly scoped user API key can be forbidden from an - // endpoint without being revoked. Preserve the session and let the - // caller render an unavailable state. - const error = new Error('auth_forbidden'); - error.status = r1.status; + } else { + let payload = null; try { - const payload = await r1.json(); + payload = await r1.json(); + } catch { + // A non-JSON error body cannot carry the canonical credential signal. + } + const authClassification = classifyAuthResponse(r1.status, payload); + if (authClassification === 'revoked') { + this.retireCredential('revoked'); + credentialStore.removeSiteToken(this.url).catch(() => {}); + const error = new Error('auth_revoked'); + error.status = r1.status; + error.code = payload?.error_type || null; + error.reason = payload?.reason || null; + throw error; + } else if (authClassification === 'forbidden') { + // A valid, narrowly scoped user API key can be forbidden from an + // endpoint without being revoked. Preserve the session and let the + // caller render an unavailable state. + const error = new Error('auth_forbidden'); + error.status = r1.status; error.code = typeof payload?.error === 'string' ? payload.error : null; error.reason = @@ -258,22 +266,15 @@ class Site { error.userMessages = Array.isArray(payload?.errors) ? payload.errors.filter(message => typeof message === 'string') : []; - } catch { - error.userMessages = []; - } - throw error; - } else { - const error = new Error('api_request_failed'); - error.status = r1.status; - try { - const payload = await r1.json(); + throw error; + } else { + const error = new Error('api_request_failed'); + error.status = r1.status; error.userMessages = Array.isArray(payload?.errors) ? payload.errors.filter(message => typeof message === 'string') : []; - } catch { - error.userMessages = []; + throw error; } - throw error; } } finally { if (this._currentFetch === activeFetch) this._currentFetch = undefined; @@ -299,7 +300,13 @@ class Site { if (response.status >= 200 && response.status < 300) { return response.json(); } - const classification = classifyAuthResponse(response.status); + let payload = null; + try { + payload = await response.json(); + } catch { + // A non-JSON error body cannot carry the canonical credential signal. + } + const classification = classifyAuthResponse(response.status, payload); if (classification === 'revoked') { this.retireCredential('revoked'); credentialStore.removeSiteToken(this.url).catch(() => {}); @@ -312,14 +319,13 @@ class Site { : 'api_request_failed', ); error.status = response.status; - try { - const payload = await response.json(); - error.userMessages = Array.isArray(payload?.errors) - ? payload.errors.filter(message => typeof message === 'string') - : []; - } catch { - error.userMessages = []; - } + error.code = + typeof payload?.error_type === 'string' ? payload.error_type : null; + error.reason = + typeof payload?.reason === 'string' ? payload.reason : null; + error.userMessages = Array.isArray(payload?.errors) + ? payload.errors.filter(message => typeof message === 'string') + : []; throw error; }) .finally(() => { @@ -336,8 +342,9 @@ class Site { this.isStaff = null; } - // An authoritative 401 means the stored User API credential no longer exists - // server-side. Clearing it in memory is not enough: without telling the + // Only the canonical server 401 + machine-readable credential tuple means + // the stored User API credential no longer exists server-side. Clearing it + // in memory is not enough: without telling the // manager, the persisted record keeps the dead token and the signed-in // navigator never re-evaluates, stranding the member on an authenticated // screen whose only action retries the same doomed request. From 3ff24c1f4d2d25e463f4c7fde27841716d545984 Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Mon, 31 Aug 2026 13:36:06 -0400 Subject: [PATCH 06/20] Polish Floor greeting and conversation stats (#5) --- js/__tests__/communityData.test.js | 4 +++- js/__tests__/floorMicroPolish.test.js | 24 +++++++++++++++++++++++ js/product/ProductScreens.js | 28 +++++++++------------------ js/product/floorPresentation.js | 22 +++++++++++++++++++++ 4 files changed, 58 insertions(+), 20 deletions(-) create mode 100644 js/__tests__/floorMicroPolish.test.js create mode 100644 js/product/floorPresentation.js diff --git a/js/__tests__/communityData.test.js b/js/__tests__/communityData.test.js index 449d9320d..eb75835ba 100644 --- a/js/__tests__/communityData.test.js +++ b/js/__tests__/communityData.test.js @@ -159,7 +159,9 @@ describe('community startup recovery', () => { expect(source).toContain( "unavailableWithoutSnapshot ? '—' : data.topics.length", ); - expect(source).toContain("unavailableWithoutSnapshot ? '—' : unanswered"); + expect(source).toContain( + "unavailableWithoutSnapshot ? '—' : activeConversations", + ); expect(source).toContain( "unavailableWithoutSnapshot ? '—' : data.categories.length", ); diff --git a/js/__tests__/floorMicroPolish.test.js b/js/__tests__/floorMicroPolish.test.js new file mode 100644 index 000000000..c1dc95144 --- /dev/null +++ b/js/__tests__/floorMicroPolish.test.js @@ -0,0 +1,24 @@ +import { memberDisplayName } from '../product/floorPresentation'; + +describe('Floor launch micro-polish', () => { + test('prefers a known display name and uses a bounded username fallback', () => { + expect(memberDisplayName('Alex Rivera', 'alex_rivera')).toBe('Alex Rivera'); + expect(memberDisplayName('', 'alex_rivera')).toBe('Alex Rivera'); + }); + + test('never greets an unknown person as Member', () => { + expect(memberDisplayName('', '')).toBeNull(); + expect(memberDisplayName('Member', 'member')).toBeNull(); + }); + + test('counts replied conversations instead of calling every seed topic unanswered', () => { + const source = require('fs').readFileSync( + require.resolve('../product/ProductScreens'), + 'utf8', + ); + expect(source).toContain('label="Conversations"'); + expect(source).toContain('detail="With replies"'); + expect(source).toContain('topic => (topic.posts_count || 1) > 1'); + expect(source).not.toContain('label="Unanswered"'); + }); +}); diff --git a/js/product/ProductScreens.js b/js/product/ProductScreens.js index 9b38e7b7b..d3fd6bd76 100644 --- a/js/product/ProductScreens.js +++ b/js/product/ProductScreens.js @@ -44,6 +44,7 @@ import { optionLabel, stateLabel } from './adjusterCardPresentation'; import AttachmentComposer, { useAttachmentQueue } from './AttachmentComposer'; import { reconcileAskSubmission, submitAskQuestion } from './AskSubmission'; import NotificationEducation from './NotificationEducation'; +import { memberDisplayName } from './floorPresentation'; import { captureAvatarAuthorityVersion, reconcileAvatarAuthority, @@ -280,17 +281,6 @@ function useCommunity(siteManager, contentVersion) { return { ...state, refresh }; } -const memberDisplayName = username => - String(username || 'member') - .split(/[_-]+/) - .filter(Boolean) - .map(part => - part.length <= 2 - ? part.toUpperCase() - : `${part.slice(0, 1).toUpperCase()}${part.slice(1)}`, - ) - .join(' '); - const topicActivityDate = topic => topic.last_posted_at ? new Date(topic.last_posted_at).toLocaleDateString(undefined, { @@ -487,12 +477,12 @@ export function FloorScreen({ navigation, screenProps }) { screenProps.memberContentVersion, ); const rateLimited = classifyCommunityLoadError(data.error) === 'rate_limited'; - const memberName = memberDisplayName(site?.username); + const memberName = memberDisplayName(site?.name, site?.username); const greeting = new Date().getHours() < 12 ? 'Good morning' : 'Welcome back'; const attentionTopics = data.topics.slice(0, 5); const attentionCardWidth = Math.min(Math.max(width - 64, 280), 340); - const unanswered = data.topics.filter( - topic => (topic.posts_count || 1) <= 1, + const activeConversations = data.topics.filter( + topic => (topic.posts_count || 1) > 1, ).length; const unavailableWithoutSnapshot = Boolean(data.error) && @@ -506,7 +496,7 @@ export function FloorScreen({ navigation, screenProps }) { maxFontSizeMultiplier={1.5} style={[styles.floorGreetingTitle, { color: colors.text }]} > - {greeting}, {memberName} + {memberName ? `${greeting}, ${memberName}` : greeting} navigation.navigate('Discussions')} tone="red" accessibleLayout={fontScale >= 1.6} diff --git a/js/product/floorPresentation.js b/js/product/floorPresentation.js new file mode 100644 index 000000000..d93ee0202 --- /dev/null +++ b/js/product/floorPresentation.js @@ -0,0 +1,22 @@ +/* @flow */ +'use strict'; + +export const memberDisplayName = (name, username) => { + const suppliedName = String(name || '').trim(); + if (suppliedName && suppliedName.toLowerCase() !== 'member') { + return suppliedName; + } + const suppliedUsername = String(username || '').trim(); + if (!suppliedUsername || suppliedUsername.toLowerCase() === 'member') { + return null; + } + return suppliedUsername + .split(/[_-]+/) + .filter(Boolean) + .map(part => + part.length <= 2 + ? part.toUpperCase() + : `${part.slice(0, 1).toUpperCase()}${part.slice(1)}`, + ) + .join(' '); +}; From 5173708d038e63cfd448398272c9a86505b9c375 Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:18:27 -0400 Subject: [PATCH 07/20] Classify official Floor notices separately (#6) * Classify official Floor notices separately * Format Floor attention classification --- .../floorAttentionClassification.test.js | 31 +++++++++++++++++++ js/product/ProductScreens.js | 23 +++++++++----- js/product/floorAttention.js | 13 ++++++++ 3 files changed, 60 insertions(+), 7 deletions(-) create mode 100644 js/__tests__/floorAttentionClassification.test.js create mode 100644 js/product/floorAttention.js diff --git a/js/__tests__/floorAttentionClassification.test.js b/js/__tests__/floorAttentionClassification.test.js new file mode 100644 index 000000000..06c55c1b8 --- /dev/null +++ b/js/__tests__/floorAttentionClassification.test.js @@ -0,0 +1,31 @@ +import { floorAttentionState } from '../product/floorAttention'; + +describe('Floor attention classification', () => { + test('does not present an official zero-reply announcement as needing a reply', () => { + expect( + floorAttentionState({ + posts_count: 1, + an_network_activity_class: 'owner_editorial', + }), + ).toEqual({ label: 'OFFICIAL', icon: 'bullhorn', needsReply: false }); + }); + + test('preserves attention for a genuinely unanswered member discussion', () => { + expect( + floorAttentionState({ + posts_count: 1, + an_network_activity_class: 'member_activity', + }), + ).toEqual({ label: 'NEEDS A REPLY', icon: 'question', needsReply: true }); + }); + + test('fails closed for unknown and replied topic classes', () => { + expect(floorAttentionState({ posts_count: 1 }).needsReply).toBe(false); + expect( + floorAttentionState({ + posts_count: 2, + an_network_activity_class: 'member_activity', + }), + ).toEqual({ label: 'ACTIVE', icon: 'comments', needsReply: false }); + }); +}); diff --git a/js/product/ProductScreens.js b/js/product/ProductScreens.js index d3fd6bd76..866f4fdff 100644 --- a/js/product/ProductScreens.js +++ b/js/product/ProductScreens.js @@ -34,6 +34,7 @@ import { loadCommunity, topicPath, } from './ProductData'; +import { floorAttentionState } from './floorAttention'; import { elevation, floorV2, radius, spacing, type } from './DesignSystem'; import { adjusterNetwork } from '../adjusterNetworkConfig'; export { default as LoungeScreen } from './NativeLoungeScreen'; @@ -365,6 +366,7 @@ const FloorActivityRow = ({ topic, site, navigation, openUrl, category }) => { const FloorAttentionCard = ({ topic, site, category, openUrl, cardWidth }) => { const colors = useProductTheme(); const replies = Math.max(0, (topic.posts_count || 1) - 1); + const attentionState = floorAttentionState(topic); const username = topic.last_poster_username || 'Network member'; const categoryColor = /^#?[0-9a-f]{6}$/i.test(category?.color || '') ? `#${String(category.color).replace('#', '')}` @@ -373,7 +375,7 @@ const FloorAttentionCard = ({ topic, site, category, openUrl, cardWidth }) => { openUrl(`${site.url}${topicPath(topic)}`)} style={({ pressed }) => [ @@ -404,24 +406,31 @@ const FloorAttentionCard = ({ topic, site, category, openUrl, cardWidth }) => { style={[ styles.floorAttentionState, { - backgroundColor: - replies === 0 ? colors.brandAccentSoft : colors.accentSoft, + backgroundColor: attentionState.needsReply + ? colors.brandAccentSoft + : colors.accentSoft, }, ]} > - {replies === 0 ? 'NEEDS A REPLY' : 'ACTIVE'} + {attentionState.label} diff --git a/js/product/floorAttention.js b/js/product/floorAttention.js new file mode 100644 index 000000000..6f135c169 --- /dev/null +++ b/js/product/floorAttention.js @@ -0,0 +1,13 @@ +/* @flow */ +'use strict'; + +export const floorAttentionState = topic => { + if (topic?.an_network_activity_class === 'owner_editorial') { + return { label: 'OFFICIAL', icon: 'bullhorn', needsReply: false }; + } + const replies = Math.max(0, (topic?.posts_count || 1) - 1); + if (topic?.an_network_activity_class === 'member_activity' && replies === 0) { + return { label: 'NEEDS A REPLY', icon: 'question', needsReply: true }; + } + return { label: 'ACTIVE', icon: 'comments', needsReply: false }; +}; From fdb83141879f7b1df60d46a488343563d3bb156e Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Mon, 31 Aug 2026 20:09:12 -0400 Subject: [PATCH 08/20] Handle short native search queries safely (#7) --- js/__tests__/communityData.test.js | 51 ++++++++++++++++++++++ js/__tests__/memberUtilitySurfaces.test.js | 22 ++++++++++ js/product/NativeMemberUtilityScreens.js | 5 ++- js/product/memberUtilities.js | 6 +++ 4 files changed, 83 insertions(+), 1 deletion(-) diff --git a/js/__tests__/communityData.test.js b/js/__tests__/communityData.test.js index eb75835ba..fcf29432e 100644 --- a/js/__tests__/communityData.test.js +++ b/js/__tests__/communityData.test.js @@ -115,6 +115,57 @@ describe('community startup recovery', () => { expect(site.jsonApi).toHaveBeenCalledTimes(4); }); + test('authoritative author and avatar changes replace a stale shared topic snapshot', async () => { + const stale = { + id: 97, + last_poster_username: 'alex', + posters: [{ username: 'alex', avatar_template: '/alex/{size}.png' }], + }; + const editorial = { + id: 97, + last_poster_username: 'an_editorial', + posters: [ + { + username: 'an_editorial', + avatar_template: '/an_editorial/{size}/174_2.png', + description: 'Original Poster, Most Recent Poster', + }, + ], + }; + let topic = stale; + const site = { + jsonApi: jest.fn(path => + Promise.resolve( + path === '/latest.json' + ? { topic_list: { topics: [topic] } } + : { categories: [] }, + ), + ), + }; + + await expect(loadCommunity(site)).resolves.toMatchObject({ + topics: [stale], + }); + topic = editorial; + await expect(loadCommunity(site)).resolves.toMatchObject({ + topics: [editorial], + }); + expect(cachedCommunity(site)).toMatchObject({ topics: [editorial] }); + + const relaunchedSite = { + jsonApi: jest.fn(path => + Promise.resolve( + path === '/latest.json' + ? { topic_list: { topics: [editorial] } } + : { categories: [] }, + ), + ), + }; + await expect(loadCommunity(relaunchedSite)).resolves.toMatchObject({ + topics: [editorial], + }); + }); + test('retains the last successful snapshot when a later refresh fails', async () => { const site = { jsonApi: jest.fn(path => diff --git a/js/__tests__/memberUtilitySurfaces.test.js b/js/__tests__/memberUtilitySurfaces.test.js index ee0577660..0ff3f7740 100644 --- a/js/__tests__/memberUtilitySurfaces.test.js +++ b/js/__tests__/memberUtilitySurfaces.test.js @@ -3,6 +3,11 @@ import fs from 'fs'; import path from 'path'; +import { + discussionSearchEligible, + memberSearchResults, + searchResults, +} from '../product/memberUtilities'; const read = file => fs.readFileSync(path.join(__dirname, '..', file), 'utf8'); @@ -53,6 +58,23 @@ describe('native member utility surfaces', () => { expect(source).not.toContain('WebView'); }); + test.each([ + ['qa', false], + ['QA', false], + ['reviewer', true], + ['admin', true], + ['roof', true], + ])('discussion search eligibility for %s is %s', (query, expected) => { + expect(discussionSearchEligible(query)).toBe(expected); + }); + + test('a two-character member query can return the normal empty state', () => { + expect(searchResults({ topics: [], posts: [], users: [] })).toEqual([]); + expect( + memberSearchResults({ schema: 'an.member-search.v1', results: [] }), + ).toEqual([]); + }); + test('member search uses only contract-returned professional metadata', () => { const helper = read('product/memberUtilities.js'); expect(helper).toContain("payload?.schema !== 'an.member-search.v1'"); diff --git a/js/product/NativeMemberUtilityScreens.js b/js/product/NativeMemberUtilityScreens.js index 742967792..b2120d50c 100644 --- a/js/product/NativeMemberUtilityScreens.js +++ b/js/product/NativeMemberUtilityScreens.js @@ -32,6 +32,7 @@ import { } from '../notificationStatus'; import { bookmarkDeletePath, + discussionSearchEligible, memberSearchResults, searchResults, supportedNotificationPreferences, @@ -597,7 +598,9 @@ export function NativeSearchScreen({ navigation, screenProps }) { memberError: null, }); const [contentResponse, memberResponse] = await Promise.allSettled([ - site.jsonApi(`/search.json?q=${encodeURIComponent(term)}`), + discussionSearchEligible(term) + ? site.jsonApi(`/search.json?q=${encodeURIComponent(term)}`) + : Promise.resolve({ topics: [], posts: [], users: [] }), site.jsonApi( `/native/v1/member-search?q=${encodeURIComponent(term)}&limit=10`, ), diff --git a/js/product/memberUtilities.js b/js/product/memberUtilities.js index ca6593ea6..e51830f66 100644 --- a/js/product/memberUtilities.js +++ b/js/product/memberUtilities.js @@ -1,6 +1,12 @@ /* @flow */ 'use strict'; +export const DISCUSSION_SEARCH_MIN_LENGTH = 3; + +export const discussionSearchEligible = query => + typeof query === 'string' && + query.trim().length >= DISCUSSION_SEARCH_MIN_LENGTH; + export const searchResults = payload => { const topics = Array.isArray(payload?.topics) ? payload.topics : []; const posts = Array.isArray(payload?.posts) ? payload.posts : []; From 6846b9e3aae5977a3c23e505be46f495ac6a0a2a Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Thu, 3 Sep 2026 08:29:58 -0400 Subject: [PATCH 09/20] fix(auth): reconcile production-shipped stale-identity fix into the live lineage (#8) * fix(auth): start every authorization in an ephemeral browser session A non-ephemeral ASWebAuthenticationSession shares the system Safari data store. Deleting the app clears the container, AsyncStorage, the client ID, the RSA keys and the stored User API Key, but not that shared cookie jar, so a reinstalled app presented an already-authenticated Discourse session and bound the new User API Key to the previous account without prompting. Physical TestFlight Build 8 production testing confirmed the auth session reused qa_test instead of allowing cert_probe_01 to sign in (server-side evidence commit 35d9d27). The native bridge already accepted an ephemeral argument and applied it as prefersEphemeralWebBrowserSession. The defect was entirely in JavaScript: site_manager passed a hardcoded false and requestIOSAuth defaulted to false. EPHEMERAL_AUTH_SESSION is now a module constant rather than a caller-supplied parameter, so no call site can reintroduce a persistent session. The adjusternetwork://auth_redirect callback contract, the one-shot nonce/client-ID binding, the canonical-origin admission check, and the fail-closed channel-derived environment resolution are unchanged. OTA-shippable on runtime an-ios-android-1.0.0-native-2: the shipped Build 8 executable already exports requestAuth:callbackURLScheme:ephemeral:resolver: rejecter: and references setPrefersEphemeralWebBrowserSession:, so the JS/native interface is unchanged and no new binary is required. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR * docs(ota): record the stale production group reference POST-SUBMISSION-HOLD.md still names group 39eb1e9b-8b72-480b-99f1-f52ad6d351fc as production. A read-only channel check on 2026-09-02 shows production now serves group 70eebadf-5736-4cd6-a7db-2980a69f0494 at source fdb83141879f, published 2026-09-01. That document is the stated rollback target during the Apple-review freeze, so the stale value is worth correcting. The hold document is untracked in this lineage, so the correction is recorded in the fix lane's backlog rather than applied to it. Documentation only; it does not affect the staged OTA, which was published from fdd5963e. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR * fix(auth): make account selection independent of browser session state The ephemeral browser-auth session alone did not resolve the stale-identity failure on a physical device, and it cannot: it is not the only carrier of a previous member identity, and the app cannot observe whether iOS honoured it. Three carriers survive an app delete and reinstall: - the Keychain User API Key and RSA material, which outlive the app container and are restored without any staleness check; - the shared Safari cookie jar, which app deletion never clears; - js/admissionHandoff.js, which opens the real Safari app through Linking.openURL, where prefersEphemeralWebBrowserSession has no effect. SiteManager.resetAuthorizationIdentity retires all of the client-side carriers together - browser cookies, stored User API Key, RSA material, the recorded authorization profile and the client ID - and the sign-in screen exposes it as "Use a different account". This does not depend on iOS honouring an ephemeral session and it never revokes a server-side credential. A staging-gated diagnostics block on the sign-in screen reports the active OTA git SHA, update ID, channel and embedded-versus-remote source, plus whether a Keychain credential survived reinstall, so bundle activation is provable in the product instead of over USB. It also opens the canonical session endpoint inside the same browser-auth context, which shows directly whether that context is anonymous. The gate is the trusted OTA channel, so the block cannot render on production. The ephemeral session is retained: it is correct, and it is the cheaper of the two mechanisms when iOS honours it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR * docs(backlog): record the Edit Profile photo defect and correct its diagnosis Physical testing found no avatar controls in the native Edit Profile screen. The native editor is in fact already built: NativeProfileScreen renders the current avatar, a Change photo action and a Remove photo action, backed by uploadProfilePhoto/removeProfilePhoto against /native/v1/profile/photo, which delegates to Discourse's own avatar system rather than a parallel one, with type validation, permission and error states, and immediate cross-surface refresh through avatarAuthority. The block is gated on the server capability card.photo.enabled, and POST-SUBMISSION-HOLD.md deliberately keeps structured_profile_photo_enabled false during Apple review. The controls were therefore absent because the capability is off, not because the UI is missing. Remaining work is the deferred server/privacy activation lane, one genuine native gap (camera capture, library picking only today), and a web parity audit that cannot be performed from this repository. Recorded only. No product code changed during auth certification. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR --------- Co-authored-by: Claude Opus 5 (1M context) --- js/Discourse.js | 32 ++ js/__tests__/authEphemeralSession.test.js | 384 ++++++++++++++++++ js/__tests__/stagingDiagnostics.test.js | 94 +++++ js/iosAuthSession.js | 12 +- js/product/ProductScreens.js | 93 ++++- js/site_manager.js | 33 +- js/stagingDiagnostics.js | 38 ++ scripts/verify-ios-auth-presentation.mjs | 9 + testing/native-auth-stale-identity/BACKLOG.md | 125 ++++++ testing/native-auth-stale-identity/README.md | 198 +++++++++ 10 files changed, 1014 insertions(+), 4 deletions(-) create mode 100644 js/__tests__/authEphemeralSession.test.js create mode 100644 js/__tests__/stagingDiagnostics.test.js create mode 100644 js/stagingDiagnostics.js create mode 100644 testing/native-auth-stale-identity/BACKLOG.md create mode 100644 testing/native-auth-stale-identity/README.md diff --git a/js/Discourse.js b/js/Discourse.js index 7e1c8fb3f..276b674f7 100644 --- a/js/Discourse.js +++ b/js/Discourse.js @@ -980,6 +980,37 @@ class Discourse extends React.Component { } } + // A member must never be trapped behind an identity they did not choose in + // this attempt. Retire every client-side identity carrier, then start a + // normal authorization. This does not depend on the browser honouring an + // ephemeral session, and it never revokes the server-side credential of an + // account the member may still want. + async useDifferentAccount() { + if (this.state.connecting) return; + Alert.alert( + 'Use a different account?', + 'Adjuster Network will forget the saved sign-in on this device and ask for credentials again. Your account is not deleted.', + [ + { text: 'Cancel', style: 'cancel' }, + { + text: 'Continue', + onPress: async () => { + this.setState({ connecting: true }); + try { + await this._siteManager.resetAuthorizationIdentity(); + securityEvent('auth.identity.reset'); + } catch { + securityEvent('auth.identity.reset_failed'); + } finally { + this.setState({ connecting: false }); + } + await this.connectCanonical(); + }, + }, + ], + ); + } + async connectCanonical() { if (!adjusterNetwork.canonicalOrigin) { Alert.alert( @@ -1160,6 +1191,7 @@ class Discourse extends React.Component { this.connectCanonical()} + onUseDifferentAccount={() => this.useDifferentAccount()} /> {this.state.privacyShield && this._blurView(theme.name)} diff --git a/js/__tests__/authEphemeralSession.test.js b/js/__tests__/authEphemeralSession.test.js new file mode 100644 index 000000000..9fb1663e7 --- /dev/null +++ b/js/__tests__/authEphemeralSession.test.js @@ -0,0 +1,384 @@ +import fs from 'fs'; +import path from 'path'; + +jest.mock('react-native-safari-web-auth', () => ({ requestAuth: jest.fn() })); +jest.mock('@react-native-community/push-notification-ios', () => ({})); +jest.mock('react-native-key-pair', () => ({ generate: jest.fn() })); +jest.mock('react-native-device-info', () => ({ + getDeviceName: jest.fn(() => Promise.resolve('Adjuster Network - Test')), +})); +jest.mock('@react-native-cookies/cookies', () => ({ + clearAll: jest.fn(() => Promise.resolve()), +})); +jest.mock('@react-native-async-storage/async-storage', () => { + const store = new Map(); + return { + getItem: jest.fn(key => Promise.resolve(store.get(key) ?? null)), + setItem: jest.fn((key, value) => { + store.set(key, value); + return Promise.resolve(); + }), + removeItem: jest.fn(key => { + store.delete(key); + return Promise.resolve(); + }), + }; +}); +jest.mock('../secureCredentialStore', () => ({ + credentialStore: { + storeSiteToken: jest.fn(() => Promise.resolve()), + removeSiteToken: jest.fn(() => Promise.resolve()), + removeRSAKeys: jest.fn(() => Promise.resolve()), + readRSAKeys: jest.fn(() => Promise.resolve(null)), + }, +})); + +import SafariWebAuth from 'react-native-safari-web-auth'; +import SiteManager from '../site_manager'; +import { credentialStore } from '../secureCredentialStore'; +import { EPHEMERAL_AUTH_SESSION, requestIOSAuth } from '../iosAuthSession'; +import { isSafeAuthCallback } from '../adjusterNetworkSecurity'; +import { AUTH_REDIRECT } from '../authorizationConsent'; +import { + AUTHORIZATION_PROFILE_ID, + REQUIRED_AUTHORIZATION_SCOPES, +} from '../authorizationProfile'; +import { + adjusterNetwork, + canonicalOriginForChannel, + trustedPushEnvironment, + trustedUpdateChannel, +} from '../adjusterNetworkConfig'; + +const readSource = (...segments) => + fs.readFileSync(path.join(__dirname, '..', ...segments), 'utf8'); + +// A SiteManager built without its constructor: the authorization binding logic +// under test must not depend on storage load, key generation, or device state. +function authManager(activeSite) { + const manager = Object.create(SiteManager.prototype); + manager.sites = []; + manager.activeSite = activeSite || null; + manager.customScheme = 'adjusternetwork'; + manager.urlScheme = AUTH_REDIRECT; + manager._nonce = null; + manager._nonceSite = null; + manager.save = jest.fn(); + manager._onChange = jest.fn(); + return manager; +} + +const CLIENT_ID = 'synthetic-client-id'; + +function authorizationProfile(clientId = CLIENT_ID) { + return { + profile_id: AUTHORIZATION_PROFILE_ID, + client_id: clientId, + exact_match: true, + granted_scopes: [...REQUIRED_AUTHORIZATION_SCOPES], + required_scopes: [...REQUIRED_AUTHORIZATION_SCOPES], + }; +} + +function memberSite(url, clientId = CLIENT_ID) { + return { + url, + clientId, + authToken: null, + credentialRetired: false, + logoff: jest.fn(), + refresh: jest.fn(() => Promise.resolve()), + // A fresh authorization is only accepted after the server confirms the + // profile is bound to this client ID. + jsonApi: jest.fn(() => Promise.resolve(authorizationProfile(clientId))), + }; +} + +beforeEach(() => jest.clearAllMocks()); + +describe('native authorization starts in a fresh browser-auth context', () => { + test('every authorization launches an ephemeral session', async () => { + SafariWebAuth.requestAuth.mockResolvedValueOnce( + `${AUTH_REDIRECT}?payload=opaque`, + ); + + await requestIOSAuth( + 'https://adjusternetwork.org/user-api-key/new', + 'adjusternetwork', + ); + + expect(EPHEMERAL_AUTH_SESSION).toBe(true); + expect(SafariWebAuth.requestAuth).toHaveBeenCalledWith( + 'https://adjusternetwork.org/user-api-key/new', + 'adjusternetwork', + true, + ); + }); + + test('the SiteManager call site cannot opt out of the ephemeral session', async () => { + const site = memberSite('https://adjusternetwork.org'); + const manager = authManager(site); + SafariWebAuth.requestAuth.mockResolvedValueOnce(AUTH_REDIRECT); + + await manager.requestAuth('https://adjusternetwork.org/user-api-key/new'); + + expect(SafariWebAuth.requestAuth.mock.calls[0][2]).toBe(true); + }); + + test('no source path can request a persistent shared-Safari session', () => { + const sessionSource = readSource('iosAuthSession.js'); + const managerSource = readSource('site_manager.js'); + + // The flag is a module constant, never a caller-supplied argument. + expect(sessionSource).toContain( + 'export const EPHEMERAL_AUTH_SESSION = true', + ); + expect(sessionSource).toMatch( + /requestIOSAuth\(url, callbackScheme\)[\s\S]*EPHEMERAL_AUTH_SESSION/, + ); + expect(sessionSource).not.toMatch(/ephemeral\s*=\s*false/); + expect(managerSource).toContain('requestIOSAuth(url, this.customScheme)'); + expect(managerSource).not.toMatch(/requestIOSAuth\([^)]*false/); + }); + + test('the shipped native bridge honours the ephemeral flag on the same interface', () => { + const nativeSource = fs.readFileSync( + path.join( + __dirname, + '..', + '..', + 'vendor', + 'react-native-safari-web-auth', + 'ios', + 'SafariWebAuth.mm', + ), + 'utf8', + ); + + // This JS/native interface is unchanged by the stale-identity fix, which is + // what keeps the fix inside the existing runtime contract and OTA-eligible. + expect(nativeSource).toContain('ephemeral:(BOOL)ephemeral'); + expect(nativeSource).toContain( + 'session.prefersEphemeralWebBrowserSession = ephemeral;', + ); + }); +}); + +describe('a prior account cannot bind a subsequent authorization', () => { + test('a replayed prior-account payload is rejected and binds nothing', async () => { + const accountA = memberSite('https://adjusternetwork.org'); + const manager = authManager(accountA); + manager._nonceSite = accountA; + manager._nonce = 'nonce-a'; + manager.decryptHelper = jest.fn(() => + JSON.stringify({ nonce: 'nonce-a', key: 'qa-test-key' }), + ); + + await expect(manager.handleAuthPayload('payload-a')).resolves.toBe(true); + expect(accountA.authToken).toBe('qa-test-key'); + + // The pending attempt is one-shot. Replaying the same prior-account + // callback after it is consumed must not rebind anything. + const later = memberSite('https://adjusternetwork.org'); + const secondAttempt = authManager(later); + secondAttempt.decryptHelper = jest.fn(() => + JSON.stringify({ nonce: 'nonce-a', key: 'qa-test-key' }), + ); + + await expect(secondAttempt.handleAuthPayload('payload-a')).resolves.toBe( + false, + ); + expect(later.authToken).toBeNull(); + }); + + test('a callback whose nonce does not match this attempt is rejected', async () => { + const accountB = memberSite('https://adjusternetwork.org'); + const manager = authManager(accountB); + manager._nonceSite = accountB; + manager._nonce = 'nonce-b'; + manager.decryptHelper = jest.fn(() => + JSON.stringify({ nonce: 'nonce-a', key: 'qa-test-key' }), + ); + + await expect(manager.handleAuthPayload('stale')).resolves.toBe(false); + expect(accountB.authToken).toBeNull(); + expect(credentialStore.storeSiteToken).not.toHaveBeenCalled(); + }); + + test('a rejected payload fails the authorization instead of silently continuing', async () => { + const site = memberSite('https://adjusternetwork.org'); + const manager = authManager(site); + manager._nonceSite = site; + manager._nonce = 'nonce-b'; + manager.decryptHelper = jest.fn(() => + JSON.stringify({ nonce: 'nonce-a', key: 'qa-test-key' }), + ); + SafariWebAuth.requestAuth.mockResolvedValueOnce( + `${AUTH_REDIRECT}?payload=stale`, + ); + + await expect( + manager.requestAuth('https://adjusternetwork.org/user-api-key/new'), + ).rejects.toThrow('auth_payload_rejected'); + expect(site.authToken).toBeNull(); + }); +}); + +describe('signing in as account B binds the User API key to B', () => { + test('the key is stored against the site of the pending attempt only', async () => { + const accountA = memberSite('https://adjusternetwork.org'); + accountA.authToken = 'account-a-key'; + const accountB = memberSite('https://adjusternetwork.org'); + const manager = authManager(accountB); + manager.sites = [accountA, accountB]; + manager._nonceSite = accountB; + manager._nonce = 'nonce-b'; + manager.decryptHelper = jest.fn(() => + JSON.stringify({ + nonce: 'nonce-b', + key: 'account-b-key', + push: false, + api: 2, + }), + ); + + await expect(manager.handleAuthPayload('payload-b')).resolves.toBe(true); + + expect(accountB.authToken).toBe('account-b-key'); + expect(accountA.authToken).toBe('account-a-key'); + expect(credentialStore.storeSiteToken).toHaveBeenCalledWith( + 'https://adjusternetwork.org', + 'account-b-key', + ); + }); +}); + +describe('the authorization callback stays on the governed redirect', () => { + test('only the Adjuster Network redirect is accepted', () => { + expect(AUTH_REDIRECT).toBe( + 'adjusternetwork://adjusternetwork.org/auth_redirect', + ); + expect(isSafeAuthCallback(AUTH_REDIRECT)).toBe(true); + expect(isSafeAuthCallback(`${AUTH_REDIRECT}?payload=opaque`)).toBe(true); + + for (const hostile of [ + 'evil://adjusternetwork.org/auth_redirect?payload=x', + 'adjusternetwork://adjusternetwork.org/auth_redirect.evil?payload=x', + 'adjusternetwork://evil.example.com/auth_redirect?payload=x', + // The pre-hardening unqualified redirect is no longer accepted. + 'adjusternetwork://auth_redirect?payload=x', + 'https://adjusternetwork.org/auth_redirect?payload=x', + 'about:blank', + ]) { + expect(isSafeAuthCallback(hostile)).toBe(false); + } + }); + + test('an unapproved callback from the ephemeral session is refused', async () => { + SafariWebAuth.requestAuth.mockResolvedValueOnce( + 'evil://auth_redirect?payload=x', + ); + await expect( + requestIOSAuth('https://adjusternetwork.org/auth', 'adjusternetwork'), + ).rejects.toThrow('auth_callback_invalid'); + }); + + test('the authorization request keeps the governed redirect contract', () => { + const managerSource = readSource('site_manager.js'); + expect(managerSource).toContain('urlScheme = AUTH_REDIRECT'); + expect(managerSource).toContain('auth_redirect: this.urlScheme'); + }); +}); + +describe('environment resolution is unchanged and fail-closed', () => { + test('only the two governed channels resolve an origin', () => { + expect(adjusterNetwork.canonicalOrigin).toBe('https://adjusternetwork.org'); + expect(canonicalOriginForChannel('production')).toBe( + 'https://adjusternetwork.org', + ); + expect(canonicalOriginForChannel('staging')).toBe( + 'https://staging.adjusternetwork.org', + ); + for (const untrusted of [null, undefined, '', 'preview', 'PRODUCTION']) { + expect(trustedUpdateChannel(untrusted)).toBeNull(); + expect(canonicalOriginForChannel(untrusted)).toBeNull(); + } + }); + + test('push environment resolution stays iOS-only and fail-closed', () => { + expect(trustedPushEnvironment('ios', 'production')).toBe('production'); + expect(trustedPushEnvironment('ios', 'staging')).toBe('staging'); + expect(trustedPushEnvironment('ios', 'preview')).toBeNull(); + expect(trustedPushEnvironment('ios', undefined)).toBeNull(); + expect(trustedPushEnvironment('android', 'production')).toBeNull(); + }); + + test('authorization refuses a site outside the resolved canonical origin', async () => { + const manager = authManager(null); + await expect( + manager.generateAuthURL({ url: 'https://evil.example.com' }), + ).rejects.toThrow('auth_origin_not_allowed'); + await expect(manager.generateAuthURL(null)).rejects.toThrow( + 'auth_origin_not_allowed', + ); + }); +}); + +describe('an account switch retires every client-side identity carrier', () => { + test('cookies, Keychain token, RSA material, profile and client ID are cleared', async () => { + const AsyncStorage = require('@react-native-async-storage/async-storage'); + const CookieManager = require('@react-native-cookies/cookies'); + const site = memberSite('https://adjusternetwork.org'); + site.authToken = 'qa-test-key'; + const manager = authManager(site); + manager.sites = [site]; + manager.clientId = CLIENT_ID; + manager.rsaKeys = { public: 'pub', private: 'priv' }; + manager._nonce = 'nonce'; + manager._nonceSite = site; + + await manager.resetAuthorizationIdentity(); + + // The browser cookie jar and every stored credential carrier are gone. + expect(CookieManager.clearAll).toHaveBeenCalledWith(true); + expect(credentialStore.removeSiteToken).toHaveBeenCalledWith( + 'https://adjusternetwork.org', + ); + expect(credentialStore.removeRSAKeys).toHaveBeenCalled(); + expect(AsyncStorage.removeItem).toHaveBeenCalledWith('@ClientId'); + expect(AsyncStorage.removeItem).toHaveBeenCalledWith('@Discourse.rsaKeys'); + + // In-memory authorization state cannot survive the switch either. + expect(manager.rsaKeys).toBeNull(); + expect(manager.clientId).toBeNull(); + expect(manager._nonce).toBeNull(); + expect(manager._nonceSite).toBeNull(); + expect(site.logoff).toHaveBeenCalled(); + }); + + test('a failing carrier never aborts the switch', async () => { + const CookieManager = require('@react-native-cookies/cookies'); + CookieManager.clearAll.mockRejectedValueOnce(new Error('cookie failure')); + credentialStore.removeRSAKeys.mockRejectedValueOnce(new Error('keychain')); + const site = memberSite('https://adjusternetwork.org'); + const manager = authManager(site); + manager.sites = [site]; + manager.clientId = CLIENT_ID; + + await expect(manager.resetAuthorizationIdentity()).resolves.toBeUndefined(); + expect(manager.clientId).toBeNull(); + }); + + test('the next authorization after a switch still launches ephemerally', async () => { + const site = memberSite('https://adjusternetwork.org'); + const manager = authManager(site); + manager.sites = [site]; + manager.clientId = CLIENT_ID; + await manager.resetAuthorizationIdentity(); + + SafariWebAuth.requestAuth.mockResolvedValueOnce(AUTH_REDIRECT); + await manager.requestAuth('https://adjusternetwork.org/user-api-key/new'); + + expect(SafariWebAuth.requestAuth.mock.calls[0][2]).toBe(true); + }); +}); diff --git a/js/__tests__/stagingDiagnostics.test.js b/js/__tests__/stagingDiagnostics.test.js new file mode 100644 index 000000000..b391f8c39 --- /dev/null +++ b/js/__tests__/stagingDiagnostics.test.js @@ -0,0 +1,94 @@ +jest.mock('../secureCredentialStore', () => ({ + credentialStore: { readSiteToken: jest.fn() }, +})); + +import { credentialStore } from '../secureCredentialStore'; +import { + browserSessionProbeUrl, + collectStagingDiagnostics, + stagingDiagnosticsEnabled, +} from '../stagingDiagnostics'; + +beforeEach(() => jest.clearAllMocks()); + +describe('staging certification diagnostics', () => { + test('render only on the staging channel and fail closed elsewhere', () => { + expect(stagingDiagnosticsEnabled('staging')).toBe(true); + for (const channel of [ + 'production', + 'preview', + null, + undefined, + '', + 'STAGING', + ]) { + expect(stagingDiagnosticsEnabled(channel)).toBe(false); + } + }); + + test('the browser probe targets the canonical session endpoint only', () => { + expect(browserSessionProbeUrl('https://adjusternetwork.org')).toBe( + 'https://adjusternetwork.org/session/current.json', + ); + expect(browserSessionProbeUrl(null)).toBeNull(); + }); + + test('reports the active update identity for OTA activation proof', async () => { + credentialStore.readSiteToken.mockResolvedValueOnce(null); + const diagnostics = await collectStagingDiagnostics({ + isEnabled: true, + updateId: 'update-id', + runtimeVersion: 'an-ios-android-1.0.0-native-2', + channel: 'staging', + isEmbeddedLaunch: false, + isEmergencyLaunch: false, + manifest: { extra: { ota: { gitSha: 'abc123' } } }, + }); + + expect(diagnostics).toMatchObject({ + gitSha: 'abc123', + updateId: 'update-id', + channel: 'staging', + source: 'remote', + runtimeVersion: 'an-ios-android-1.0.0-native-2', + retainedCredential: 'absent', + }); + }); + + test('distinguishes an embedded launch from an activated remote update', async () => { + credentialStore.readSiteToken.mockResolvedValueOnce(null); + const diagnostics = await collectStagingDiagnostics({ + isEnabled: true, + updateId: null, + channel: 'staging', + isEmbeddedLaunch: true, + manifest: {}, + }); + + expect(diagnostics.source).toBe('embedded'); + expect(diagnostics.gitSha).toBeNull(); + }); + + test('reports a credential that survived reinstall without exposing it', async () => { + credentialStore.readSiteToken.mockResolvedValueOnce('secret-user-api-key'); + const diagnostics = await collectStagingDiagnostics({ + isEnabled: true, + channel: 'staging', + manifest: {}, + }); + + expect(diagnostics.retainedCredential).toBe('present'); + expect(JSON.stringify(diagnostics)).not.toContain('secret-user-api-key'); + }); + + test('a Keychain read failure never blocks the diagnostics surface', async () => { + credentialStore.readSiteToken.mockRejectedValueOnce(new Error('locked')); + const diagnostics = await collectStagingDiagnostics({ + isEnabled: true, + channel: 'staging', + manifest: {}, + }); + + expect(diagnostics.retainedCredential).toBe('unreadable'); + }); +}); diff --git a/js/iosAuthSession.js b/js/iosAuthSession.js index 6ad04b8f9..d5f6581cb 100644 --- a/js/iosAuthSession.js +++ b/js/iosAuthSession.js @@ -4,11 +4,19 @@ import SafariWebAuth from 'react-native-safari-web-auth'; import { isSafeAuthCallback } from './adjusterNetworkSecurity'; -export async function requestIOSAuth(url, callbackScheme, ephemeral = false) { +// Adjuster Network authorization must never inherit an identity the member did +// not choose in this attempt. A non-ephemeral ASWebAuthenticationSession shares +// the system Safari data store, so a previously signed-in account survives app +// deletion and reinstall and silently binds the next User API Key. Every +// authorization therefore starts in a fresh, ephemeral browser-auth context. +// This is not caller-configurable: no call site may opt out. +export const EPHEMERAL_AUTH_SESSION = true; + +export async function requestIOSAuth(url, callbackScheme) { const callback = await SafariWebAuth.requestAuth( url, callbackScheme, - ephemeral, + EPHEMERAL_AUTH_SESSION, ); if (!callback || !isSafeAuthCallback(callback)) { throw new Error('auth_callback_invalid'); diff --git a/js/product/ProductScreens.js b/js/product/ProductScreens.js index 866f4fdff..5bc57617b 100644 --- a/js/product/ProductScreens.js +++ b/js/product/ProductScreens.js @@ -16,6 +16,13 @@ import { import { SafeAreaView } from 'react-native-safe-area-context'; import FontAwesome5 from '@react-native-vector-icons/fontawesome5'; import { useAssets } from 'expo-asset'; +import { Alert } from 'react-native'; +import { + browserSessionProbeUrl, + collectStagingDiagnostics, + stagingDiagnosticsEnabled, +} from '../stagingDiagnostics'; +import { requestIOSAuth } from '../iosAuthSession'; import { Action, Avatar, @@ -119,12 +126,43 @@ const FloorHeader = ({ navigation, screenProps }) => { ); }; -export function WelcomeScreen({ onConnect, busy }) { +export function WelcomeScreen({ onConnect, onUseDifferentAccount, busy }) { const colors = useProductTheme(); const { width, fontScale } = useWindowDimensions(); const [brandAssets] = useAssets([ require('../../img/adjuster-network-logo.png'), ]); + const diagnosticsVisible = stagingDiagnosticsEnabled(); + const [diagnostics, setDiagnostics] = useState(null); + useEffect(() => { + if (!diagnosticsVisible) return; + let active = true; + collectStagingDiagnostics() + .then(result => { + if (active) setDiagnostics(result); + }) + .catch(() => {}); + return () => { + active = false; + }; + }, [diagnosticsVisible]); + const probeBrowserSession = useCallback(async () => { + const url = browserSessionProbeUrl(); + if (!url) return; + try { + // The probe never completes an authorization: it opens the canonical + // origin in the same browser-auth context the sign-in flow uses so the + // rendered session JSON can be read, then it is dismissed by hand. + await requestIOSAuth(url, 'adjusternetwork'); + } catch (error) { + if (error?.code !== 'auth_user_cancelled') { + Alert.alert( + 'Browser session probe', + 'The probe sheet closed. Read the rendered JSON before dismissing it.', + ); + } + } + }, []); return ( + {onUseDifferentAccount ? ( + + + Use a different account + + + ) : null} + {diagnosticsVisible && ( + + + Staging certification diagnostics + + + {diagnostics + ? [ + `gitSha: ${diagnostics.gitSha || 'unset'}`, + `updateId: ${diagnostics.updateId || 'none'}`, + `channel: ${diagnostics.channel || 'none'}`, + `source: ${diagnostics.source}`, + `runtime: ${diagnostics.runtimeVersion || 'none'}`, + `retainedCredential: ${diagnostics.retainedCredential}`, + ].join('\n') + : 'Reading…'} + + + + Probe browser session + + + + )} {}); + const origins = new Set(this.sites.map(site => site.url)); + if (adjusterNetwork.canonicalOrigin) { + origins.add(adjusterNetwork.canonicalOrigin); + } + for (const origin of origins) { + await credentialStore.removeSiteToken(origin).catch(() => {}); + } + const clientId = this.clientId || (await this.getClientId()); + await clearAuthorizationProfile(clientId).catch(() => {}); + await credentialStore.removeRSAKeys().catch(() => {}); + await AsyncStorage.removeItem('@Discourse.rsaKeys').catch(() => {}); + await AsyncStorage.removeItem('@ClientId').catch(() => {}); + this.rsaKeys = null; + this.clientId = null; + this._nonce = null; + this._nonceSite = null; + this.sites.forEach(site => site.logoff()); + this.save(); + this._onChange(); + } + setActiveSite(site) { return new Promise(resolve => { if (typeof site === 'string' || site instanceof String) { @@ -585,7 +616,7 @@ class SiteManager { } async requestAuth(url) { - const authRequest = await requestIOSAuth(url, this.customScheme, false); + const authRequest = await requestIOSAuth(url, this.customScheme); const urlParams = this.parseURLparameters(authRequest); let acceptedPayload = false; diff --git a/js/stagingDiagnostics.js b/js/stagingDiagnostics.js new file mode 100644 index 000000000..cfcdffa04 --- /dev/null +++ b/js/stagingDiagnostics.js @@ -0,0 +1,38 @@ +/* @flow */ +'use strict'; + +import * as Updates from 'expo-updates'; +import { getOtaDiagnostics } from './otaDiagnostics'; +import { credentialStore } from './secureCredentialStore'; +import { adjusterNetwork } from './adjusterNetworkConfig'; + +// A temporary certification aid for the permanent internal staging client. +// It is gated on the trusted OTA channel, so it can never render on a +// production binary or a production OTA even if this bundle is republished. +export function stagingDiagnosticsEnabled(channel = Updates.channel) { + return channel === 'staging'; +} + +// Opened inside the same ASWebAuthenticationSession the sign-in flow uses. +// An ephemeral session has an empty cookie jar, so the canonical origin must +// report an anonymous visitor. Anything else proves the browser-auth context +// is still sharing the system Safari session. +export function browserSessionProbeUrl( + origin = adjusterNetwork.canonicalOrigin, +) { + return origin ? `${origin}/session/current.json` : null; +} + +export async function collectStagingDiagnostics(updates = Updates) { + const ota = getOtaDiagnostics(updates); + let retainedCredential = 'unknown'; + try { + const origin = adjusterNetwork.canonicalOrigin; + const token = origin ? await credentialStore.readSiteToken(origin) : null; + // Never surface the credential itself, only whether one survived. + retainedCredential = token ? 'present' : 'absent'; + } catch { + retainedCredential = 'unreadable'; + } + return Object.freeze({ ...ota, retainedCredential }); +} diff --git a/scripts/verify-ios-auth-presentation.mjs b/scripts/verify-ios-auth-presentation.mjs index 20c4fd3f8..45add6b37 100644 --- a/scripts/verify-ios-auth-presentation.mjs +++ b/scripts/verify-ios-auth-presentation.mjs @@ -45,6 +45,15 @@ const assertions = [ ), ), ], + [ + 'honours an ephemeral browser-auth context', + source.includes('ephemeral:(BOOL)ephemeral') && + source.includes('session.prefersEphemeralWebBrowserSession = ephemeral;'), + ], + [ + 'never pins a persistent shared-Safari session', + !source.includes('prefersEphemeralWebBrowserSession = NO'), + ], [ 'links AuthenticationServices explicitly', podspec.includes('s.frameworks = "AuthenticationServices", "UIKit"'), diff --git a/testing/native-auth-stale-identity/BACKLOG.md b/testing/native-auth-stale-identity/BACKLOG.md new file mode 100644 index 000000000..4a88490b2 --- /dev/null +++ b/testing/native-auth-stale-identity/BACKLOG.md @@ -0,0 +1,125 @@ +# Non-blocking follow-ups + +Opened alongside the stale-identity authorization fix. None of these gate that +fix, and none may delay its staging or promotion. + +## 1. Misleading reviewer-specific denial copy + +`principal_classification_required` is a generic admission denial, but the +native copy presents it as reviewer-specific. Replace it with accurate generic +copy that does not imply a reviewer-only condition. Reviewer-visible copy is +frozen during Apple review of Build 8; schedule after the freeze lifts. + +## 2. `/native/v1/authorization-profile` returns 500 on a bad User-Api-Key + +A malformed or missing `User-Api-Key` produces a 500 instead of a clean 401. +Server-side fix. It must remain fail-closed: an unauthenticated or malformed +request is denied, only the status and body shape change. Do not weaken +User API authorization while fixing the status code. + +## 3. Legal acceptance screen is too long + +Consolidate into a concise single acknowledgement UI while preserving +per-instrument and per-version acceptance evidence. The recorded evidence +granularity is the constraint; the presentation is not. Reviewer-visible; +schedule after the Apple review freeze. + +## 4. Governed cleanup of qa_test production credentials + +Production User API keys 111 and 112 belong to `qa_test`, plus one stale August +session. These need governed revocation with an audit record. Do not touch +`cert_probe_01`. Do not mutate production server state outside an approved +cleanup window. + +## 5. `POST-SUBMISSION-HOLD.md` names a stale production OTA group + +`testing/native-app-store-readiness/POST-SUBMISSION-HOLD.md` records the +production channel as group `39eb1e9b-8b72-480b-99f1-f52ad6d351fc` +(iOS update `01a01ff3-d501-73b3-b799-2b4cf353efcb`, source +`3fb9de379e499736513d6ade7227b5ce32201ba1`). A read-only channel check on +2026-09-02 shows production has since moved twice and now serves group +`70eebadf-5736-4cd6-a7db-2980a69f0494`, source +`fdb83141879f7b1df60d46a488343563d3bb156e`, published 2026-09-01. + +The stale value matters because that document is the stated rollback target +during the Apple-review freeze. Correct it to the live group. + +That file is untracked in this lineage — it exists only in the canonical +repository's working tree — so this correction is recorded here rather than +applied to it. Documentation only; it does not gate any fix. + +## 6. Founder Production Approval UX / Deployment Gate V2 + +Production promotion is currently a founder-run CLI step with no in-product or +dashboard approval surface, and the governed rollback target lives only in a +document that has already gone stale twice. Design a deployment gate that +records the approver, the exact certified group, the rollback target, and the +freeze state in one authoritative place. + +## 7. Written-but-unwired modules on the certification path + +`js/otaDiagnostics.js` (`getOtaDiagnostics`) and +`js/authorizationConsent.js` (`USER_API_KEY_SCOPE_COPY`) are implemented and +unit-tested but imported by nothing. The missing OTA surface is why proving +which bundle a device had loaded required a laptop and a USB cable, which +directly cost a certification cycle. The staging diagnostics block added in +this lane is deliberately temporary; decide whether a permanent, governed +build-identity surface belongs in the product before removing it. + +## 8. HIGH — Edit Profile shows no profile photo controls on device + +Physical iPhone testing found no way to add, change, or remove the member +avatar in the native Edit Profile screen. Recorded for the immediate +post-certification profile polish pass; not touched during auth certification. + +### Correction to the diagnosis + +The native photo editor is already built. `js/product/NativeProfileScreen.js` +renders, at the top of the editor, the current `MemberAvatar` (or the pending +local preview), a **Change photo** action and a **Remove photo** action. The +supporting pipeline exists and is wired end to end: + +| Requirement | Status | +| ----------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Avatar shown prominently at top of Edit Profile | Built | +| Change photo action | Built | +| Choose from Photo Library | Built (`ImagePicker.launchImageLibraryAsync`, square crop, quality 0.85) | +| Take photo with Camera | **Missing — the only genuine UI gap** | +| Remove/reset photo | Built, with a confirmation alert | +| Permission / error / loading states | Built (`photo_permission_denied`, `profileSaveErrorMessage`, `submitting`) | +| Reuses Discourse's avatar pipeline | Built — `uploadProfilePhoto` posts to `/native/v1/profile/photo` and requires `card.photo.delegatedTo === 'discourse-avatar'`. No parallel image system exists | +| Image type/size validation | Built (`unsupported_profile_photo_type` in `js/product/profileSaveState.js`) | +| Immediate avatar refresh across native UI | Built (`js/product/avatarAuthority.js` publishes the new template) | + +The entire block is gated on the server capability `card?.photo.enabled`, and +`uploadProfilePhoto` additionally fails closed with `photo_capability_disabled` +unless `photo.enabled`, `photo.editable`, and the `discourse-avatar` delegation +are all present. + +**So the controls were absent on device because the server capability is off, +not because the UI is missing.** +`testing/native-app-store-readiness/POST-SUBMISSION-HOLD.md` states this +deliberately: "Keep `structured_profile_photo_enabled=false` during review", +and names profile-photo activation as the first bounded post-freeze +server/privacy certification lane. + +### Actual work remaining + +1. **Server/privacy lane (the real blocker).** Complete the deferred + certification the hold document already scopes — storage and privacy audit, + MIME and size validation, EXIF/GPS stripping, signed-out visibility, + moderation/removal/cache behavior, deletion behavior — then enable + `structured_profile_photo_enabled`. The native UI needs no change to appear. +2. **Camera capture (native, small).** Add a "Take photo" option beside + "Choose from library", using `ImagePicker.requestCameraPermissionsAsync` and + `launchCameraAsync` with the same crop, quality, and + `normalizeProfilePhotoPickerAsset` normalization. `NSCameraUsageDescription` + is already declared. Note that App Store readiness item P1.7 proposes + removing `NSMicrophoneUsageDescription`; the camera string must stay. +3. **Web parity audit (unverified here).** Whether the web member profile + exposes the same capability could not be checked from this repository. Audit + before activation so both surfaces agree, and confirm + `/native/v1/profile/photo` stays compatible with the Discourse avatar + contract rather than diverging. +4. **Re-verify on device after activation** — the capability gate means this + cannot be certified while the flag is false. diff --git a/testing/native-auth-stale-identity/README.md b/testing/native-auth-stale-identity/README.md new file mode 100644 index 000000000..d562005e4 --- /dev/null +++ b/testing/native-auth-stale-identity/README.md @@ -0,0 +1,198 @@ +# Native stale-identity authorization fix + +Recorded: 2026-09-02 (America/New_York) +Base lineage: `fdb83141879f7b1df60d46a488343563d3bb156e` +(`fix/auth-failure-classification-20260830`), the commit currently live on both +the `production` and `staging` OTA channels +Lane: `system` (auth boundary) + +## Base correction + +This fix was first written on `codex/notification-tap-routing` (`879bea6a`). +A read-only channel check before staging showed that branch is not the live +lineage: it forks from the shared ancestor `126ec8e7` and omits the 44 commits +that reached production on 2026-09-01, including the Build 8 submission commit +`2eb3ef6c`, the host-qualified authorization redirect, server-verified +authorization profiles, onboarding gates, and the share-extension work. Staging +that branch would have published a 44-commit regression to the certification +channel. The fix was therefore cherry-picked onto `fdb83141`, and its tests were +updated to the current governed contracts. The defect was confirmed still +present on `fdb83141` before the cherry-pick. + +## Defect + +Server/E2E evidence commit `35d9d27` proved a production native authorization +defect: a stale `ASWebAuthenticationSession` identity survived app delete and +reinstall. Physical TestFlight Build 8 production testing confirmed the auth +session reused `qa_test` instead of allowing `cert_probe_01` to sign in. + +`ASWebAuthenticationSession` defaults to the shared system Safari data store. +Deleting the app clears the app container and `AsyncStorage` (and therefore the +client ID, RSA keys, nonce, and stored User API Key), but it does not clear the +shared Safari cookie jar. The reinstalled app therefore presented an +already-authenticated Discourse session and bound the new User API Key to the +previous account without ever prompting for credentials. + +Web and server lifecycle behavior was already certified; production server +release `228ab5bc8d77c3e89f9b520aba79748bbefa6fb5` is unchanged by this work. + +## Call site + +The flow is **native Swift/Objective-C `ASWebAuthenticationSession`**, not +Expo/JS `WebBrowser.openAuthSessionAsync`. `expo-web-browser` is not a +dependency of this project. + +| Layer | File | +| --------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------- | +| Product entry points | `js/Discourse.js:344`, `js/Discourse.js:730`, `js/screens/HomeScreen.js:78`, `js/screens/WebViewScreenComponents/WebViewComponent.js:456` | +| Authorization orchestration | `js/site_manager.js` — `requestAuth()` | +| JS bridge wrapper | `js/iosAuthSession.js` — `requestIOSAuth()` | +| Native bridge | `vendor/react-native-safari-web-auth/ios/SafariWebAuth.mm` | + +The native bridge already accepted an `ephemeral` argument and already applied +it as `session.prefersEphemeralWebBrowserSession = ephemeral`. The defect was +entirely in JavaScript: `js/site_manager.js` passed a hardcoded `false`, and +`requestIOSAuth` defaulted the flag to `false`. + +## Fix + +`js/iosAuthSession.js` now exports `EPHEMERAL_AUTH_SESSION = true` and always +passes it to the native bridge. The flag is no longer a caller-supplied +parameter, so no call site can reintroduce a persistent shared-Safari session. +`js/site_manager.js` calls `requestIOSAuth(url, this.customScheme)`. + +Preserved unchanged: + +- the governed `AUTH_REDIRECT` callback contract + (`adjusternetwork://adjusternetwork.org/auth_redirect`) and its + `isSafeAuthCallback` allowlist; +- the one-shot nonce/client-ID binding in `handleAuthPayload`, including its + server-verified `/native/v1/authorization-profile` exact-match check and the + restore-previous-authorization path; +- `generateAuthURL`'s canonical-origin admission check; +- channel-derived, fail-closed production/staging environment resolution; +- scopes, policy, and User API authorization. + +No separate "Use a different account" control is required. Because every +authorization now begins in a fresh ephemeral browser-auth context, the browser +never carries a prior identity into a new attempt, so a member cannot be trapped +behind a stale one. The existing `Log out of this device` control in +`PrivacyAccountScreen` remains the in-session account-switch path; it revokes +the User API Key, removes the stored token, and clears cookies. + +## OTA or binary: OTA + +This is OTA-shippable on the existing runtime `an-ios-android-1.0.0-native-2`. +The ruling is based on inspection of the shipped artifact, not on inference: + +- Build 8 submitted source `2eb3ef6c6783181d30a59a2783e42f7c723a2769` already + contains `ephemeral:(BOOL)ephemeral` and + `session.prefersEphemeralWebBrowserSession = ephemeral` in `SafariWebAuth.mm`. +- The Build 8 archive executable + (`~/Library/Developer/Xcode/Archives/2026-08-20/AdjusterNetwork-1.0-8.xcarchive`) + exports the selector `requestAuth:callbackURLScheme:ephemeral:resolver:rejecter:` + and references `setPrefersEphemeralWebBrowserSession:`. + +The fix therefore changes only a JavaScript argument value inside an unchanged +JS/native interface. Per `docs/NATIVE-OTA-OPERATIONS.md` this stays inside the +existing runtime contract: no native source, dependency, entitlement, +permission, capability, Expo module, or interface change, and no runtime version +bump. `app.config.js` and `eas.json` are untouched. + +## Tests + +`js/__tests__/authEphemeralSession.test.js` (14 tests, green on `fdb83141` +alongside the full suite at 89 suites / 614 tests): + +- the auth session is launched ephemerally, and the `SiteManager` call site + cannot opt out; +- no source path can request a persistent shared-Safari session; +- the shipped native bridge honours the flag on an unchanged interface; +- a replayed or nonce-mismatched prior-account payload binds nothing, and a + rejected payload fails the authorization instead of silently continuing; +- signing in as account B binds the User API Key to B and leaves A untouched; +- the callback stays restricted to the governed `AUTH_REDIRECT`, and the + pre-hardening unqualified `adjusternetwork://auth_redirect` is rejected; +- production/staging environment resolution is unchanged and fail-closed, + including the canonical-origin admission check on `generateAuthURL`. + +`scripts/verify-ios-auth-presentation.mjs` gained two native assertions so a +future native regression away from the ephemeral context fails the verifier. + +## Pre-push audit + +- `yarn format:check`, `yarn lint`: PASS +- `yarn test:unit --runInBand`: 60 suites, 417 tests, PASS +- `yarn verify:ota`: 17/17 PASS +- `yarn verify:ios-auth`: 12/12 PASS +- `scripts/native-devex-native.mjs --configuration=Release`: BUILD SUCCEEDED +- `yarn native:lane` classifies the change as the `system` lane +- `yarn validate:system`: PASS, evidence at + `.local/evidence/native-devex/last-run.json` + +The first system-lane run failed inside `ReactCodegen` with missing +`ios/build/generated/**` inputs. That is the known React Native codegen +script-phase race, not a product regression: the artifacts are present, the +change touches no native source, and an immediate rerun of the identical native +build succeeded. + +The three GitHub workflows (`linting`, `jest-tests`, `ios-tests`) only trigger +on `pull_request` or a push to `main`. This branch is the working trunk, more +than a hundred commits ahead of `main`, and no pull request exists for it, so +the push did not start a run. `yarn lint`, `yarn format:check` and +`yarn test:unit` — the complete `linting` and `jest-tests` job commands — were +run locally and pass. The `ios-tests` Detox simulator suite has not been run; +opening a pull request against `main` to trigger it is a release-picture +decision for the founder, not a CI convenience, and per `AGENTS.md` simulator +evidence could not certify this native API behavior in any case. + +`PHYSICAL_REQUIRED` remains open by design. The system lane requires physical +device evidence, which is the founder's `cert_probe_01` retest below. + +## Founder action required + +Production OTA promotion is not performed. It needs founder authorization, and +`testing/native-app-store-readiness/POST-SUBMISSION-HOLD.md` additionally +freezes production OTA repointing while Apple review of Build 8 is underway. + +Governed promotion path once authorized: + +0. Commit or stash the working tree. `yarn ota:stage` refuses a dirty tree, and + the repository still carries pre-existing uncommitted Build 8 certification + evidence (`testing/native-app-store-readiness/`, + `testing/native-media-attachments/`) that is not this lane's work to commit. + +1. `yarn ota:stage` — publishes to the `staging` channel. A dry run confirms the + exact command it will issue for this fix: + + ``` + npx eas-cli@latest update --branch staging --platform all \ + --message "Staging " --non-interactive + ``` + + with `AN_OTA_CHANNEL=staging` and `AN_OTA_GIT_SHA=`, both derived + automatically. Verify with `yarn ota:stage --dry-run` first. + +2. Physically certify on the permanent staging app with `cert_probe_01`: + install, sign in, sign out, delete the app, reinstall, and confirm the + authorization prompt asks for credentials and does not reuse `qa_test`. +3. `yarn ota:promote --group=` — republishes that + exact certified update group to `production`. Do not rebundle. + +Live channel state read on 2026-09-02, which supersedes the group recorded in +`testing/native-app-store-readiness/POST-SUBMISSION-HOLD.md`: + +| Channel | Group | Source SHA | Runtime | Published | +| ---------- | -------------------------------------- | -------------- | ------------------------------- | ---------- | +| production | `70eebadf-5736-4cd6-a7db-2980a69f0494` | `fdb83141879f` | `an-ios-android-1.0.0-native-2` | 2026-09-01 | +| staging | `4fbf12ad-69f3-47a0-b88a-7238bde5214c` | `fdb83141879f` | `an-ios-android-1.0.0-native-2` | 2026-09-01 | + +The production group above is the rollback target. The hold document's +`39eb1e9b-8b72-480b-99f1-f52ad6d351fc` is stale and should be corrected. + +`cert_probe_01` must remain intact and is not modified by this work. No +production server state is mutated. + +## Non-blocking follow-ups + +Tracked separately in `BACKLOG.md`; none of them gate this fix. From ce66da9960bf0107500e366457d19e1fc6bf3b11 Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Fri, 4 Sep 2026 17:51:38 -0400 Subject: [PATCH 10/20] fix(identity): refresh the signed-in member identity while signed in (#9) The member identity was captured once, during handleAuthPayload's call to Site.refresh, and never refetched. Site.refresh is the only writer of site.username, it is reached only through SiteManager.refreshSites, and that loop was deliberately removed from the authenticated lifecycle to stop it extending rate limits: _refreshAuthenticatedResources refreshes notification state only, and the 30s timer calls refreshSites solely while signed out with no active site. The value therefore froze at authorization, and because it is persisted in AsyncStorage under @Discourse.sites, a relaunch rehydrated the same stale handle rather than refetching it. A server-side rename then left every username-dependent surface pointing at the old handle: /u/{username}.json and the bookmarks route 404, and the self-checks behind can-edit-own-card, Lounge own-message deletion, moderation and Ask idempotency all stop matching, so a renamed member silently loses those abilities. The Floor greeting was additionally never able to show a display name at all, because Site never assigned this.name and 'name' was absent from Site.FIELDS, so memberDisplayName always fell through to the title-cased handle. Site.refreshIdentity reads /session/current.json, the narrowest authenticated endpoint carrying both username and name and already covered by the granted session_info scope, so no server contract changes. SiteManager .refreshActiveIdentity refreshes the active site only, shares one in-flight request, persists solely on change, and swallows failures so a malformed payload or a lost connection preserves the last known identity instead of blanking it. The foreground lifecycle calls it inside the existing 30s guard; the retired multi-site loop stays retired. handleAuthPayload now clears the identity before binding a new credential and repopulates it once the authorization is accepted, so a newly authorized account cannot inherit the previous member's handle. logoff clears the name with the username. 'name' is additive in Site.FIELDS: existing records read it as undefined and repopulate on the first refresh, so no migration, logout or reinstall. Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR Co-authored-by: Claude Opus 5 (1M context) --- js/Discourse.js | 4 + js/__tests__/authEphemeralSession.test.js | 1 + js/__tests__/authorizationUpgrade.test.js | 1 + js/__tests__/memberIdentityRefresh.test.js | 339 +++++++++++++++++++++ js/site.js | 30 ++ js/site_manager.js | 37 +++ 6 files changed, 412 insertions(+) create mode 100644 js/__tests__/memberIdentityRefresh.test.js diff --git a/js/Discourse.js b/js/Discourse.js index 276b674f7..5325b26d5 100644 --- a/js/Discourse.js +++ b/js/Discourse.js @@ -876,6 +876,10 @@ class Discourse extends React.Component { if (now - this._lastForegroundRefreshAt < 30000) return false; this._lastForegroundRefreshAt = now; const generation = ++this._foregroundRefreshGeneration; + // A server-side rename must reach the app without a logout or reinstall. + // This refreshes the active site only and reuses the guard above, so it + // cannot reintroduce the retired multi-site refresh loop. + await this._siteManager.refreshActiveIdentity().catch(() => false); await this._siteManager.refreshNotificationState(reason).catch(() => []); if (generation !== this._foregroundRefreshGeneration) return false; this.setState(current => ({ diff --git a/js/__tests__/authEphemeralSession.test.js b/js/__tests__/authEphemeralSession.test.js index 9fb1663e7..f8306038d 100644 --- a/js/__tests__/authEphemeralSession.test.js +++ b/js/__tests__/authEphemeralSession.test.js @@ -88,6 +88,7 @@ function memberSite(url, clientId = CLIENT_ID) { credentialRetired: false, logoff: jest.fn(), refresh: jest.fn(() => Promise.resolve()), + refreshIdentity: jest.fn(() => Promise.resolve(false)), // A fresh authorization is only accepted after the server confirms the // profile is bound to this client ID. jsonApi: jest.fn(() => Promise.resolve(authorizationProfile(clientId))), diff --git a/js/__tests__/authorizationUpgrade.test.js b/js/__tests__/authorizationUpgrade.test.js index 3f8d67df0..42942d2ac 100644 --- a/js/__tests__/authorizationUpgrade.test.js +++ b/js/__tests__/authorizationUpgrade.test.js @@ -59,6 +59,7 @@ test('attests exact scopes before replacing the stored credential', async () => required_scopes: [...REQUIRED_AUTHORIZATION_SCOPES], }), refresh: jest.fn().mockResolvedValue(), + refreshIdentity: jest.fn().mockResolvedValue(false), }; const manager = managerWith(site); await expect(manager.handleAuthPayload('encrypted')).resolves.toBe(true); diff --git a/js/__tests__/memberIdentityRefresh.test.js b/js/__tests__/memberIdentityRefresh.test.js new file mode 100644 index 000000000..339f9e26f --- /dev/null +++ b/js/__tests__/memberIdentityRefresh.test.js @@ -0,0 +1,339 @@ +jest.mock('react-native-safari-web-auth', () => ({ requestAuth: jest.fn() })); +jest.mock('@react-native-community/push-notification-ios', () => ({})); +jest.mock('react-native-key-pair', () => ({ generate: jest.fn() })); +jest.mock('react-native-device-info', () => ({ + getDeviceName: jest.fn(() => Promise.resolve('Adjuster Network - Test')), +})); +jest.mock('@react-native-cookies/cookies', () => ({ + clearAll: jest.fn(() => Promise.resolve()), +})); +jest.mock('@react-native-async-storage/async-storage', () => { + const store = new Map(); + return { + __store: store, + getItem: jest.fn(key => Promise.resolve(store.get(key) ?? null)), + setItem: jest.fn((key, value) => { + store.set(key, value); + return Promise.resolve(); + }), + removeItem: jest.fn(key => { + store.delete(key); + return Promise.resolve(); + }), + }; +}); +jest.mock('../secureCredentialStore', () => ({ + credentialStore: { + storeSiteToken: jest.fn(() => Promise.resolve()), + removeSiteToken: jest.fn(() => Promise.resolve()), + removeRSAKeys: jest.fn(() => Promise.resolve()), + readRSAKeys: jest.fn(() => Promise.resolve(null)), + }, +})); + +import AsyncStorage from '@react-native-async-storage/async-storage'; +import Site from '../site'; +import SiteManager from '../site_manager'; +import { memberDisplayName } from '../product/floorPresentation'; + +const ORIGIN = 'https://adjusternetwork.org'; + +// A Site driven by a scripted /session/current.json, with no transport. +function identitySite(responses, initial = {}) { + const site = Object.create(Site.prototype); + site.url = ORIGIN; + site.authToken = 'token'; + site.username = initial.username ?? null; + site.name = initial.name ?? null; + site.jsonApi = jest.fn(path => { + expect(path).toBe('/session/current.json'); + const next = responses.shift(); + if (next instanceof Error) return Promise.reject(next); + return Promise.resolve(next); + }); + return site; +} + +function manager(site) { + const instance = Object.create(SiteManager.prototype); + instance.sites = site ? [site] : []; + instance.activeSite = site || null; + instance._identityRefresh = null; + instance.save = jest.fn(); + instance._onChange = jest.fn(); + return instance; +} + +const session = (username, name) => ({ current_user: { username, name } }); + +beforeEach(() => jest.clearAllMocks()); + +describe('member identity is refreshed from the current session', () => { + test('authorizing as A stores both the username and the display name', async () => { + const site = identitySite([session('finale2e', 'Finale E2E')]); + await expect(site.refreshIdentity()).resolves.toBe(true); + expect(site.username).toBe('finale2e'); + expect(site.name).toBe('Finale E2E'); + }); + + test('a server-side rename to B is picked up without reauthorization', async () => { + const site = identitySite([session('tomrodriguez', 'Tom Rodriguez')], { + username: 'finale2e', + name: 'Finale E2E', + }); + const instance = manager(site); + + await expect(instance.refreshActiveIdentity()).resolves.toBe(true); + + expect(site.username).toBe('tomrodriguez'); + expect(site.name).toBe('Tom Rodriguez'); + // The refreshed identity is persisted, so a relaunch cannot resurrect A. + expect(instance.save).toHaveBeenCalled(); + expect(instance._onChange).toHaveBeenCalled(); + }); + + test('the Floor greeting renders the display name, not the handle', () => { + expect(memberDisplayName('Tom Rodriguez', 'tomrodriguez')).toBe( + 'Tom Rodriguez', + ); + // Before the fix there was no stored name, so the handle was title-cased. + expect(memberDisplayName(null, 'tomrodriguez')).toBe('Tomrodriguez'); + expect(memberDisplayName(null, 'finale2e')).toBe('Finale2e'); + expect(memberDisplayName(null, null)).toBeNull(); + }); + + test('an unchanged identity does not rewrite storage', async () => { + const site = identitySite([session('tomrodriguez', 'Tom Rodriguez')], { + username: 'tomrodriguez', + name: 'Tom Rodriguez', + }); + const instance = manager(site); + + await expect(instance.refreshActiveIdentity()).resolves.toBe(false); + expect(instance.save).not.toHaveBeenCalled(); + }); +}); + +describe('identity refresh fails closed', () => { + test('a transport error preserves the last known identity', async () => { + const site = identitySite([new Error('offline')], { + username: 'tomrodriguez', + name: 'Tom Rodriguez', + }); + const instance = manager(site); + + await expect(instance.refreshActiveIdentity()).resolves.toBe(false); + expect(site.username).toBe('tomrodriguez'); + expect(site.name).toBe('Tom Rodriguez'); + expect(instance.save).not.toHaveBeenCalled(); + }); + + test('a malformed or empty payload never blanks the identity', async () => { + for (const payload of [ + null, + {}, + { current_user: null }, + { current_user: {} }, + { current_user: { username: ' ' } }, + ]) { + const site = identitySite([payload], { + username: 'tomrodriguez', + name: 'Tom Rodriguez', + }); + await expect(site.refreshIdentity()).resolves.toBe(false); + expect(site.username).toBe('tomrodriguez'); + expect(site.name).toBe('Tom Rodriguez'); + } + }); + + test('a signed-out site is never asked for an identity', async () => { + const site = identitySite([session('a', 'A')]); + site.authToken = null; + await expect(site.refreshIdentity()).resolves.toBe(false); + expect(site.jsonApi).not.toHaveBeenCalled(); + }); + + test('a missing display name falls back to the handle without corrupting state', async () => { + const site = identitySite([session('tomrodriguez', '')]); + await expect(site.refreshIdentity()).resolves.toBe(true); + expect(site.name).toBeNull(); + expect(memberDisplayName(site.name, site.username)).toBe('Tomrodriguez'); + }); +}); + +describe('refresh is bounded and cannot loop', () => { + test('concurrent foreground triggers share one in-flight request', async () => { + const site = identitySite([session('tomrodriguez', 'Tom Rodriguez')]); + const instance = manager(site); + + const results = await Promise.all([ + instance.refreshActiveIdentity(), + instance.refreshActiveIdentity(), + instance.refreshActiveIdentity(), + ]); + + expect(site.jsonApi).toHaveBeenCalledTimes(1); + expect(results).toEqual([true, true, true]); + // The in-flight slot is released so a later foreground can refresh again. + expect(instance._identityRefresh).toBeNull(); + }); + + test('the retired multi-site refresh loop is not revived', async () => { + const site = identitySite([session('tomrodriguez', 'Tom Rodriguez')]); + const instance = manager(site); + instance.refreshSites = jest.fn(); + instance._throttledRefreshSites = jest.fn(); + + await instance.refreshActiveIdentity(); + + expect(instance.refreshSites).not.toHaveBeenCalled(); + expect(instance._throttledRefreshSites).not.toHaveBeenCalled(); + expect(site.jsonApi).toHaveBeenCalledTimes(1); + }); + + test('no authenticated site means no request at all', async () => { + const instance = manager(null); + await expect(instance.refreshActiveIdentity()).resolves.toBe(false); + expect(instance.save).not.toHaveBeenCalled(); + }); +}); + +describe('persistence and backward compatibility', () => { + test('name is serialized so a relaunch keeps the refreshed identity', () => { + expect(Site.FIELDS).toContain('name'); + expect(Site.FIELDS).toContain('username'); + + const site = new Site({ + url: ORIGIN, + username: 'tomrodriguez', + name: 'Tom Rodriguez', + }); + const persisted = JSON.parse(JSON.stringify(site)); + expect(persisted.name).toBe('Tom Rodriguez'); + expect(persisted.username).toBe('tomrodriguez'); + // Rehydration is what a cold launch performs. + const rehydrated = new Site(persisted); + expect(memberDisplayName(rehydrated.name, rehydrated.username)).toBe( + 'Tom Rodriguez', + ); + }); + + test('an existing record with no name upgrades without migration', async () => { + // A record written by a build that never stored a display name. + const legacy = new Site({ url: ORIGIN, username: 'finale2e' }); + expect(legacy.name).toBeUndefined(); + expect(memberDisplayName(legacy.name, legacy.username)).toBe('Finale2e'); + + legacy.authToken = 'token'; + legacy.jsonApi = jest.fn(() => + Promise.resolve(session('tomrodriguez', 'Tom Rodriguez')), + ); + + await expect(legacy.refreshIdentity()).resolves.toBe(true); + expect(memberDisplayName(legacy.name, legacy.username)).toBe( + 'Tom Rodriguez', + ); + }); + + test('logging off clears the whole identity, not just the handle', () => { + const site = new Site({ url: ORIGIN }); + site.authToken = 'token'; + site.username = 'tomrodriguez'; + site.name = 'Tom Rodriguez'; + + site.logoff(); + + expect(site.username).toBeNull(); + expect(site.name).toBeNull(); + expect(site.authToken).toBeNull(); + }); +}); + +describe('username-dependent surfaces follow the refreshed identity', () => { + test('routes and self-checks all read the refreshed username', async () => { + const site = identitySite([session('tomrodriguez', 'Tom Rodriguez')], { + username: 'finale2e', + name: null, + }); + await site.refreshIdentity(); + + // Own-profile and bookmarks routes are built from site.username. + expect(`/u/${encodeURIComponent(site.username)}.json`).toBe( + '/u/tomrodriguez.json', + ); + expect( + `/u/${encodeURIComponent(site.username)}/activity/bookmarks.json`, + ).toBe('/u/tomrodriguez/activity/bookmarks.json'); + + // Self-detection used by can-edit, Lounge self-delete and moderation. + expect('tomrodriguez' === site.username).toBe(true); + expect('finale2e' === site.username).toBe(false); + }); +}); + +describe('a new authorization never inherits the previous identity', () => { + test('the prior identity is cleared before the new credential is used', async () => { + const previous = new Site({ + url: ORIGIN, + username: 'finale2e', + name: 'Finale E2E', + }); + previous.authToken = 'old-token'; + + // handleAuthPayload clears identity before binding; model that boundary. + previous.username = null; + previous.name = null; + previous.authToken = 'new-token'; + previous.jsonApi = jest.fn(() => + Promise.resolve(session('cert_probe_01', 'Cert Probe 01')), + ); + + await expect(previous.refreshIdentity()).resolves.toBe(true); + expect(previous.username).toBe('cert_probe_01'); + expect(previous.name).toBe('Cert Probe 01'); + }); + + test('handleAuthPayload clears identity and refreshes it', () => { + const fs = require('fs'); + const path = require('path'); + const source = fs.readFileSync( + path.join(__dirname, '..', 'site_manager.js'), + 'utf8', + ); + expect(source).toContain('nonceSite.username = null'); + expect(source).toContain('nonceSite.name = null'); + expect(source).toContain('await nonceSite.refreshIdentity()'); + }); + + test('the foreground lifecycle refreshes identity before notifications', () => { + const fs = require('fs'); + const path = require('path'); + const source = fs.readFileSync( + path.join(__dirname, '..', 'Discourse.js'), + 'utf8', + ); + expect(source).toContain('await this._siteManager.refreshActiveIdentity()'); + // The retired multi-site loop stays retired on the authenticated path. + const foreground = source.slice( + source.indexOf('async _refreshAuthenticatedResources'), + source.indexOf('async _refresh()'), + ); + expect(foreground).not.toContain('refreshSites()'); + }); +}); + +test('AsyncStorage is the identity store of record', async () => { + const site = new Site({ url: ORIGIN, username: 'tomrodriguez' }); + site.name = 'Tom Rodriguez'; + const instance = manager(site); + instance.save = SiteManager.prototype.save.bind(instance); + + instance.save(); + + const raw = await AsyncStorage.getItem('@Discourse.sites'); + const parsed = JSON.parse(raw); + expect(parsed[0].username).toBe('tomrodriguez'); + expect(parsed[0].name).toBe('Tom Rodriguez'); + // The credential is never written to AsyncStorage. + expect(raw).not.toContain('authToken":"'); +}); diff --git a/js/site.js b/js/site.js index 855e53235..64670aa71 100644 --- a/js/site.js +++ b/js/site.js @@ -31,6 +31,7 @@ class Site { 'lastVisitedPath', 'lastVisitedPathAt', 'loginRequired', + 'name', 'queueCount', 'title', 'totalNew', @@ -339,9 +340,38 @@ class Site { logoff() { this.authToken = null; this.username = null; + this.name = null; this.isStaff = null; } + // The signed-in member identity is captured once at authorization and then + // never refetched by the notification lifecycle, so a server-side rename + // leaves every username-dependent surface pointing at the old handle. Read + // the current session instead: /session/current.json is the narrowest + // authenticated endpoint carrying both fields and is already covered by the + // granted session_info scope, so no server contract changes. + // + // Fail closed. A malformed or empty payload preserves the last known good + // identity rather than blanking it; transport errors propagate to the + // caller, which treats them the same way. + async refreshIdentity() { + if (!this.authToken) { + return false; + } + + const payload = await this.jsonApi('/session/current.json'); + const current = payload?.current_user; + const username = String(current?.username || '').trim(); + if (!username) { + return false; + } + const name = String(current?.name || '').trim() || null; + const changed = this.username !== username || this.name !== name; + this.username = username; + this.name = name; + return changed; + } + // Only the canonical server 401 + machine-readable credential tuple means // the stored User API credential no longer exists server-side. Clearing it // in memory is not enough: without telling the diff --git a/js/site_manager.js b/js/site_manager.js index 6029b3be5..911d2b9a2 100644 --- a/js/site_manager.js +++ b/js/site_manager.js @@ -149,6 +149,38 @@ class SiteManager { this._onChange(); } + // Refresh the signed-in member identity for the active site only. The broad + // legacy refreshSites() loop stays retired: it fans out across every site and + // was deliberately removed from the authenticated lifecycle to stop it + // extending rate limits. A single in-flight request is shared so sibling + // foreground triggers cannot stack, and the record is persisted only when the + // identity actually changed. A failed refresh preserves the last known + // identity rather than corrupting it. + refreshActiveIdentity() { + if (this._identityRefresh) return this._identityRefresh; + const site = + this.activeSite || this.sites.find(candidate => candidate.authToken); + if (!site?.authToken) return Promise.resolve(false); + + const request = site + .refreshIdentity() + .then(changed => { + if (changed) { + this.save(); + this._onChange(); + } + return changed; + }) + .catch(() => false) + .finally(() => { + if (this._identityRefresh === request) { + this._identityRefresh = null; + } + }); + this._identityRefresh = request; + return request; + } + setActiveSite(site) { return new Promise(resolve => { if (typeof site === 'string' || site instanceof String) { @@ -508,6 +540,10 @@ class SiteManager { await credentialStore.removeSiteToken(nonceSite.url); } }; + // A fresh authorization may be for a different member. Drop the previous + // identity before the new credential is used so nothing can inherit it. + nonceSite.username = null; + nonceSite.name = null; nonceSite.authToken = decrypted.key; nonceSite.hasPush = decrypted.push; nonceSite.apiVersion = decrypted.api; @@ -536,6 +572,7 @@ class SiteManager { // A verified fresh authorization supersedes any earlier retirement. nonceSite.credentialRetired = false; nonceSite.credentialRetiredReason = null; + await nonceSite.refreshIdentity().catch(() => false); this.save(); // cause we want to stop rendering connect From 5b6d988c4aebd7661ab7a7182c345aca96aa4428 Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Sun, 6 Sep 2026 14:12:39 -0400 Subject: [PATCH 11/20] fix(avatars): authenticate private member photos and bind the media credential to the trusted origin (#10) * fix(avatars): authenticate private member photo requests Every member avatar in the app renders through one component - Avatar in js/product/ProductComponents.js, re-exported as MemberAvatar - and it passed source={{ uri }} with no headers. React Native's image loader runs its own native pipeline and attaches none of the app's credentials, so those requests went out unauthenticated. Only site.jsonApi sends User-Api-Key. Against the new /renaissance/member-photo/:username/:size/:version contract that means an ordinary member photo would resolve to the anonymous letter-avatar redirect rather than the member's bytes. memberImageSource attaches User-Api-Key and User-Api-Client-Id, but only when the resolved URL is on the trusted origin. An avatar_template can carry an absolute URL to an external host, and isCanonicalUrl rejects both foreign origins and plain HTTP, so the credential cannot leak to an arbitrary image host or travel unencrypted. It stays in request headers: never in the URL, query string, or cache key. A missing or unauthenticated source returns no headers, and Avatar's existing onError path falls back to the letter avatar, so a rejected request degrades rather than breaking a screen. resetAuthorizationIdentity now clears every cached avatar record, so switching accounts cannot render bytes that were resolved under the previous member's admission. Logout already cleared the per-site records through remove(). This reuses the credential pattern already certified for secure media in DiscourseMedia.js, with the origin guard that path does not currently apply. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR * fix(media): bind the secure-media credential to the trusted origin SecureMediaImage and the attachment viewer attached User-Api-Key and User-Api-Client-Id to whatever URL the media item carried. Those URLs are derived from post cooked HTML by absoluteUrl(), which returns any absolute http(s) URL unchanged, so member-authored content could point an image or attachment at an external host and receive other members' User API keys. Both now use authenticatedOriginHeaders, the same guard the member-photo work introduced: the credential is attached only for the canonical HTTPS origin. Media semantics are unchanged - canonical-origin secure uploads still authenticate, and a pre-signed off-origin object never needed the header. The PDF viewer test built its site on the staging origin while the suite mocks the production OTA channel, so its own fixture was not the canonical origin. The fixture now matches the mocked channel, and a companion test proves the viewer refuses to authenticate an off-origin attachment. Adversarial coverage spans look-alike domains, subdomains, embedded-URL query and fragment tricks, credentials in the authority, a non-default port, plain HTTP, and non-HTTP schemes including data:, file: and javascript:. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR --------- Co-authored-by: Claude Opus 5 (1M context) --- js/__tests__/memberImageSource.test.js | 187 ++++++++++++++++++++ js/__tests__/nativeMediaAttachments.test.js | 38 +++- js/product/DiscourseMedia.js | 25 +-- js/product/ProductComponents.js | 3 +- js/product/memberImageSource.js | 35 ++++ js/site_manager.js | 9 +- 6 files changed, 282 insertions(+), 15 deletions(-) create mode 100644 js/__tests__/memberImageSource.test.js create mode 100644 js/product/memberImageSource.js diff --git a/js/__tests__/memberImageSource.test.js b/js/__tests__/memberImageSource.test.js new file mode 100644 index 000000000..fb49de59a --- /dev/null +++ b/js/__tests__/memberImageSource.test.js @@ -0,0 +1,187 @@ +import { + authenticatedOriginHeaders, + memberImageSource, +} from '../product/memberImageSource'; + +const CANONICAL = 'https://adjusternetwork.org'; +const site = { url: CANONICAL, authToken: 'user-api-key', clientId: 'client' }; + +describe('authenticated member image requests', () => { + test('attaches the User API credential for the governed origin', () => { + const source = memberImageSource( + site, + `${CANONICAL}/renaissance/member-photo/tomrodriguez/120/7`, + ); + + expect(source).toEqual({ + uri: `${CANONICAL}/renaissance/member-photo/tomrodriguez/120/7`, + headers: { + 'User-Api-Key': 'user-api-key', + 'User-Api-Client-Id': 'client', + }, + }); + }); + + test('never sends the credential to any other host', () => { + for (const uri of [ + 'https://evil.example.com/renaissance/member-photo/tomrodriguez/120/7', + 'https://adjusternetwork.org.evil.example.com/a.png', + 'https://cdn.adjusternetwork.org/a.png', + 'https://staging.adjusternetwork.org/renaissance/member-photo/x/120/7', + 'https://www.gravatar.com/avatar/abc.png', + ]) { + expect(memberImageSource(site, uri)).toEqual({ uri }); + } + }); + + test('never sends the credential over plain HTTP', () => { + const uri = 'http://adjusternetwork.org/renaissance/member-photo/x/120/7'; + expect(memberImageSource(site, uri)).toEqual({ uri }); + }); + + test('a signed-out viewer sends no credential', () => { + const uri = `${CANONICAL}/renaissance/member-photo/tomrodriguez/120/7`; + expect(memberImageSource({ url: CANONICAL }, uri)).toEqual({ uri }); + expect(memberImageSource(null, uri)).toEqual({ uri }); + expect(memberImageSource({ ...site, authToken: null }, uri)).toEqual({ + uri, + }); + }); + + test('no credential ever reaches the URL, query string or cache key', () => { + const source = memberImageSource( + site, + `${CANONICAL}/renaissance/member-photo/tomrodriguez/120/7`, + ); + expect(source.uri).not.toContain('user-api-key'); + expect(source.uri).not.toContain('User-Api-Key'); + expect(source.uri).toBe( + `${CANONICAL}/renaissance/member-photo/tomrodriguez/120/7`, + ); + expect(source.uri.includes('?')).toBe(false); + }); + + test('a missing URL yields no source so the letter avatar renders', () => { + expect(memberImageSource(site, null)).toBeNull(); + expect(memberImageSource(site, '')).toBeNull(); + expect(memberImageSource(site, undefined)).toBeNull(); + }); + + test('a missing client id still sends a well-formed header pair', () => { + const source = memberImageSource( + { url: CANONICAL, authToken: 'key' }, + `${CANONICAL}/renaissance/member-photo/x/120/7`, + ); + expect(source.headers['User-Api-Client-Id']).toBe(''); + expect(source.headers['User-Api-Key']).toBe('key'); + }); +}); + +describe('the shared Avatar is the only member-photo loader', () => { + const fs = require('fs'); + const path = require('path'); + const read = file => + fs.readFileSync(path.join(__dirname, '..', file), 'utf8'); + + test('Avatar routes its image through the authenticated source', () => { + const source = read('product/ProductComponents.js'); + expect(source).toContain('memberImageSource(site, resolvedUri)'); + // The unauthenticated form must not survive anywhere in the component. + expect(source).not.toContain('source={{ uri: resolvedUri }}'); + expect(source).toContain('export const MemberAvatar = Avatar'); + }); + + test('an account switch clears every cached avatar record', () => { + const source = read('site_manager.js'); + const reset = source.slice( + source.indexOf('async resetAuthorizationIdentity'), + source.indexOf('setActiveSite(site)'), + ); + expect(reset).toContain('clearAvatarAuthorities()'); + // Logout already clears the per-site records. + expect(source).toContain('clearAvatarAuthorityForSite(removableSite)'); + }); +}); + +describe('adversarial origins never receive the User API credential', () => { + // Post cooked HTML is member-authored, so a hostile absolute URL can reach + // the media loader directly. Every one of these must come back bare. + const HOSTILE = [ + 'https://evil.example.com/uploads/a.png', + 'https://adjusternetwork.org.evil.example.com/uploads/a.png', + 'https://evil.example.com/?next=https://adjusternetwork.org/uploads/a.png', + 'https://evil.example.com#https://adjusternetwork.org/uploads/a.png', + 'https://cdn.adjusternetwork.org/uploads/a.png', + 'https://staging.adjusternetwork.org/uploads/a.png', + 'https://adjusternetwork.org.example.com/uploads/a.png', + 'https://adjusternetwork.orgevil.com/uploads/a.png', + 'http://adjusternetwork.org/uploads/a.png', + 'http://evil.example.com/uploads/a.png', + 'https://user:pass@evil.example.com/uploads/a.png', + 'https://adjusternetwork.org:8443/uploads/a.png', + 'ftp://adjusternetwork.org/uploads/a.png', + 'file:///etc/passwd', + 'data:image/png;base64,AAAA', + 'javascript:alert(1)', + '//evil.example.com/uploads/a.png', + ]; + + test.each(HOSTILE)('no credential for %s', uri => { + expect(authenticatedOriginHeaders(site, uri)).toBeUndefined(); + expect(memberImageSource(site, uri)).toEqual({ uri }); + }); + + test('the trusted origin still authenticates for both surfaces', () => { + for (const uri of [ + `${CANONICAL}/renaissance/member-photo/tomrodriguez/120/7`, + `${CANONICAL}/secure-uploads/original/1X/abc.png`, + `${CANONICAL}/uploads/default/original/1X/abc.png`, + ]) { + expect(authenticatedOriginHeaders(site, uri)).toEqual({ + 'User-Api-Key': 'user-api-key', + 'User-Api-Client-Id': 'client', + }); + } + }); + + test('a signed-out or missing viewer never authenticates', () => { + const uri = `${CANONICAL}/secure-uploads/original/1X/abc.png`; + expect(authenticatedOriginHeaders(null, uri)).toBeUndefined(); + expect(authenticatedOriginHeaders({ url: CANONICAL }, uri)).toBeUndefined(); + expect(authenticatedOriginHeaders(site, null)).toBeUndefined(); + expect(authenticatedOriginHeaders(site, '')).toBeUndefined(); + }); +}); + +describe('secure media reuses the same origin guard', () => { + const fs = require('fs'); + const path = require('path'); + const source = fs.readFileSync( + path.join(__dirname, '..', 'product', 'DiscourseMedia.js'), + 'utf8', + ); + + test('media images and the attachment viewer are both guarded', () => { + expect(source).toContain( + 'const headers = authenticatedOriginHeaders(site, state.url)', + ); + expect(source).toContain( + 'const headers = authenticatedOriginHeaders(site, state.authorizedUrl)', + ); + }); + + test('no unguarded credential construction remains anywhere', () => { + // The only place these header names may appear is the guarded helper. + expect(source).not.toContain("'User-Api-Key': site.authToken"); + const helper = fs.readFileSync( + path.join(__dirname, '..', 'product', 'memberImageSource.js'), + 'utf8', + ); + expect(helper).toContain('isCanonicalUrl(uri)'); + const components = fs.readFileSync( + path.join(__dirname, '..', 'product', 'ProductComponents.js'), + 'utf8', + ); + expect(components).not.toContain("'User-Api-Key'"); + }); +}); diff --git a/js/__tests__/nativeMediaAttachments.test.js b/js/__tests__/nativeMediaAttachments.test.js index 9ad484235..4996a933f 100644 --- a/js/__tests__/nativeMediaAttachments.test.js +++ b/js/__tests__/nativeMediaAttachments.test.js @@ -249,7 +249,7 @@ describe('native Discourse media attachments', () => { test('opens the real PDF route in an authenticated in-app viewer', async () => { const site = { - url: 'https://staging.adjusternetwork.org', + url: 'https://adjusternetwork.org', authToken: 'test-user-api-key', clientId: 'test-client', }; @@ -287,6 +287,42 @@ describe('native Discourse media attachments', () => { }); }); + test('never sends the credential to an off-origin attachment', async () => { + // The OTA channel is production in this suite, so any other host - the + // staging origin included - must not receive the User API credential. + const site = { + url: 'https://adjusternetwork.org', + authToken: 'test-user-api-key', + clientId: 'test-client', + }; + const url = + 'https://staging.adjusternetwork.org/secure-uploads/original/1X/synthetic.pdf'; + const refreshMedia = jest.fn().mockResolvedValue(url); + let renderer; + act(() => { + renderer = TestRenderer.create( + , + ); + }); + + await act(async () => { + renderer.root + .findByProps({ accessibilityLabel: 'Open attachment field-notes.pdf' }) + .props.onPress(); + await Promise.resolve(); + }); + + const viewer = renderer.root.findByProps({ + accessibilityLabel: 'Attachment field-notes.pdf', + }); + expect(viewer.props.source).toEqual({ uri: url, headers: undefined }); + }); + test('supports canonical and legacy Discourse attachment representations', () => { const site = { url: 'https://staging.adjusternetwork.org' }; expect( diff --git a/js/product/DiscourseMedia.js b/js/product/DiscourseMedia.js index b78ad4dab..98da1d64d 100644 --- a/js/product/DiscourseMedia.js +++ b/js/product/DiscourseMedia.js @@ -16,6 +16,7 @@ import { decode } from 'html-entities'; import { SafeAreaView } from 'react-native-safe-area-context'; import { WebView } from 'react-native-webview'; import { radius, spacing, type } from './DesignSystem'; +import { authenticatedOriginHeaders } from './memberImageSource'; import { useProductTheme } from './ProductComponents'; const SIGNED_ACCESS_REFRESH_MS = 240000; @@ -153,12 +154,12 @@ function SecureMediaImage({ refreshing: false, error: null, }); - const headers = site?.authToken - ? { - 'User-Api-Key': site.authToken, - 'User-Api-Client-Id': site.clientId || '', - } - : undefined; + // Media URLs come from post cooked HTML, which passes absolute external + // URLs through unchanged. The credential must therefore be bound to the + // trusted HTTPS origin, exactly as member photos are. Canonical-origin + // secure uploads are unaffected; a pre-signed off-origin object never + // needed the header. + const headers = authenticatedOriginHeaders(site, state.url); useEffect(() => { resolvedAt.current = Date.now(); @@ -263,12 +264,12 @@ function SecureMediaFile({ item, site, resourceKey, refreshMedia }) { error: null, authorizedUrl: null, }); - const headers = site?.authToken - ? { - 'User-Api-Key': site.authToken, - 'User-Api-Client-Id': site.clientId || '', - } - : undefined; + // Media URLs come from post cooked HTML, which passes absolute external + // URLs through unchanged. The credential must therefore be bound to the + // trusted HTTPS origin, exactly as member photos are. Canonical-origin + // secure uploads are unaffected; a pre-signed off-origin object never + // needed the header. + const headers = authenticatedOriginHeaders(site, state.authorizedUrl); const open = useCallback(async () => { if (state.opening) return; setState(current => ({ ...current, opening: true, error: null })); diff --git a/js/product/ProductComponents.js b/js/product/ProductComponents.js index 08628c801..5cd355359 100644 --- a/js/product/ProductComponents.js +++ b/js/product/ProductComponents.js @@ -14,6 +14,7 @@ import FontAwesome5 from '@react-native-vector-icons/fontawesome5'; import { ThemeContext } from '../ThemeContext'; import { productTheme, radius, spacing, type } from './DesignSystem'; import { useAvatarAuthorityRecord } from './avatarAuthority'; +import { memberImageSource } from './memberImageSource'; export const useProductTheme = () => productTheme(useContext(ThemeContext).name); @@ -405,7 +406,7 @@ export const Avatar = ({ key={resolvedUri} accessibilityLabel={`${label} profile photo`} onError={() => setFailedUri(resolvedUri)} - source={{ uri: resolvedUri }} + source={memberImageSource(site, resolvedUri)} style={style} /> ); diff --git a/js/product/memberImageSource.js b/js/product/memberImageSource.js new file mode 100644 index 000000000..7f1435883 --- /dev/null +++ b/js/product/memberImageSource.js @@ -0,0 +1,35 @@ +/* @flow */ +'use strict'; + +import { isCanonicalUrl } from '../adjusterNetworkSecurity'; + +// Private member photos are served by the governed origin and require the same +// User API credential the JSON API already sends. React Native's image loader +// runs its own native pipeline and attaches none of the app's credentials, so +// an authenticated image request must carry them explicitly. +// +// The credential is attached only when the resolved URL is on the trusted +// Adjuster Network origin. An avatar_template may carry an absolute URL to an +// external host - letter avatars, gravatar-style services, a CDN - and those +// must never receive a User API key. isCanonicalUrl also rejects plain HTTP, +// so the credential cannot leave over an unencrypted connection. +// +// Nothing is placed in the URL, query string, or any log: the credential +// travels only as a request header. +export function authenticatedOriginHeaders(site, uri) { + if (!uri || !site?.authToken || !isCanonicalUrl(uri)) { + return undefined; + } + return { + 'User-Api-Key': site.authToken, + 'User-Api-Client-Id': site.clientId || '', + }; +} + +export function memberImageSource(site, uri) { + if (!uri) { + return null; + } + const headers = authenticatedOriginHeaders(site, uri); + return headers ? { uri, headers } : { uri }; +} diff --git a/js/site_manager.js b/js/site_manager.js index 911d2b9a2..6d661a2a8 100644 --- a/js/site_manager.js +++ b/js/site_manager.js @@ -26,7 +26,10 @@ import { recordNotificationDiagnostic, supportedNotification, } from './notificationState'; -import { clearAvatarAuthorityForSite } from './product/avatarAuthority'; +import { + clearAvatarAuthorities, + clearAvatarAuthorityForSite, +} from './product/avatarAuthority'; import { clearAuthorizationProfile, markAuthorizationProfileCurrent, @@ -144,6 +147,10 @@ class SiteManager { this.clientId = null; this._nonce = null; this._nonceSite = null; + // Member photos are authenticated per viewer. Drop every cached avatar + // record so a switched account cannot render bytes resolved under the + // previous member's admission. + clearAvatarAuthorities(); this.sites.forEach(site => site.logoff()); this.save(); this._onChange(); From f662cf3bafdf281342fb456ea23d672d211e6b71 Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Sun, 6 Sep 2026 18:21:49 -0400 Subject: [PATCH 12/20] fix(avatars): authenticate only the private member-photo route (#11) Production regression from 5b6d988c. avatar_template values are relative paths such as /user_avatar/adjusternetwork.org//120/1234_2.png, so resolving them against the site URL puts every avatar on the canonical origin. The origin guard then attached User-Api-Key to all of them, turning each rendered avatar into a counted user-API request. Those requests are issued by React Native's native image pipeline, which sits outside requestOrchestrator and its cooldowns, so they were invisible to the app's own rate limiting while still consuming the member's quota. A screen renders many avatars at once, so the quota drained and the real API calls behind them started failing. The device request ledger shows the result: repeated cooldown_begin on the global-user-api bucket with 4xx, and GET /latest.json, /native/v1/profile, /u/.json and /chat/api/me/channels.json all settling as 4xx failures. Floor still rendered from its cached community snapshot; Discussions had no snapshot to fall back on, so it did not load. Only /renaissance/member-photo/ now carries the credential. Ordinary Discourse avatars load unauthenticated exactly as they did before 5b6d988c, restoring the previous request cost. The private member-photo route keeps its credential and its origin and HTTPS guard, and secure media is untouched: it authenticated before this work, so its request count never changed. Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR Co-authored-by: Claude Opus 5 (1M context) --- js/__tests__/memberImageSource.test.js | 70 +++++++++++++++++++++++++- js/product/memberImageSource.js | 20 +++++++- 2 files changed, 88 insertions(+), 2 deletions(-) diff --git a/js/__tests__/memberImageSource.test.js b/js/__tests__/memberImageSource.test.js index fb49de59a..2c22e827c 100644 --- a/js/__tests__/memberImageSource.test.js +++ b/js/__tests__/memberImageSource.test.js @@ -1,5 +1,6 @@ import { authenticatedOriginHeaders, + isMemberPhotoUrl, memberImageSource, } from '../product/memberImageSource'; @@ -131,7 +132,7 @@ describe('adversarial origins never receive the User API credential', () => { expect(memberImageSource(site, uri)).toEqual({ uri }); }); - test('the trusted origin still authenticates for both surfaces', () => { + test('the trusted origin still authenticates media on the shared guard', () => { for (const uri of [ `${CANONICAL}/renaissance/member-photo/tomrodriguez/120/7`, `${CANONICAL}/secure-uploads/original/1X/abc.png`, @@ -185,3 +186,70 @@ describe('secure media reuses the same origin guard', () => { expect(components).not.toContain("'User-Api-Key'"); }); }); + +describe('only the private member-photo route authenticates an avatar', () => { + // Regression guard. Authenticating ordinary Discourse avatars turned every + // rendered avatar into a counted user-API request. Those requests are issued + // by the native image pipeline, outside the app's request orchestrator, so + // they exhausted the member's rate limit and starved /latest.json - which is + // what Discussions needs to load. + test('ordinary Discourse avatars are loaded without any credential', () => { + for (const path of [ + '/user_avatar/adjusternetwork.org/tomrodriguez/120/1234_2.png', + '/letter_avatar_proxy/v4/letter/t/abc/120.png', + '/uploads/default/original/1X/abc.png', + '/images/avatar.png', + ]) { + const uri = `${CANONICAL}${path}`; + expect(memberImageSource(site, uri)).toEqual({ uri }); + expect(isMemberPhotoUrl(uri)).toBe(false); + } + }); + + test('the governed member-photo route still authenticates', () => { + const uri = `${CANONICAL}/renaissance/member-photo/tomrodriguez/120/7`; + expect(isMemberPhotoUrl(uri)).toBe(true); + expect(memberImageSource(site, uri)).toEqual({ + uri, + headers: { + 'User-Api-Key': 'user-api-key', + 'User-Api-Client-Id': 'client', + }, + }); + }); + + test('a member-photo path on an untrusted origin is still refused', () => { + for (const uri of [ + 'https://evil.example.com/renaissance/member-photo/tomrodriguez/120/7', + 'http://adjusternetwork.org/renaissance/member-photo/tomrodriguez/120/7', + 'https://adjusternetwork.org.evil.example.com/renaissance/member-photo/x/1/1', + ]) { + expect(memberImageSource(site, uri)).toEqual({ uri }); + } + }); + + test('a look-alike path prefix does not qualify', () => { + for (const path of [ + '/renaissance/member-photos/x/120/7', + '/renaissance/member-photo', + '/x/renaissance/member-photo/x/120/7', + '/renaissance/member-photox/x/120/7', + ]) { + expect(isMemberPhotoUrl(`${CANONICAL}${path}`)).toBe(false); + } + }); + + test('secure media keeps its own guard and is unaffected', () => { + // Media already authenticated before this work, so its request count is + // unchanged; only the origin restriction was added. + expect( + authenticatedOriginHeaders( + site, + `${CANONICAL}/secure-uploads/original/1X/abc.png`, + ), + ).toEqual({ + 'User-Api-Key': 'user-api-key', + 'User-Api-Client-Id': 'client', + }); + }); +}); diff --git a/js/product/memberImageSource.js b/js/product/memberImageSource.js index 7f1435883..b75b1b85e 100644 --- a/js/product/memberImageSource.js +++ b/js/product/memberImageSource.js @@ -1,7 +1,14 @@ /* @flow */ 'use strict'; -import { isCanonicalUrl } from '../adjusterNetworkSecurity'; +import { isCanonicalUrl, parseHttpsUrl } from '../adjusterNetworkSecurity'; + +// Only the governed private member-photo route requires the User API +// credential. Ordinary Discourse avatars are served from /user_avatar/ and are +// readable without one; authenticating them turns every rendered avatar into a +// counted user-API request, which exhausts the member's rate limit and starves +// the real API calls behind it. +const MEMBER_PHOTO_PATH = /^\/renaissance\/member-photo\//; // Private member photos are served by the governed origin and require the same // User API credential the JSON API already sends. React Native's image loader @@ -26,10 +33,21 @@ export function authenticatedOriginHeaders(site, uri) { }; } +export function isMemberPhotoUrl(uri) { + const url = parseHttpsUrl(String(uri || '')); + return !!url && MEMBER_PHOTO_PATH.test(url.pathname); +} + export function memberImageSource(site, uri) { if (!uri) { return null; } + // Avatars render many-per-screen and are loaded by the native image + // pipeline, outside the app's request orchestrator and its rate-limit + // cooldowns. Only the private member-photo route may carry the credential. + if (!isMemberPhotoUrl(uri)) { + return { uri }; + } const headers = authenticatedOriginHeaders(site, uri); return headers ? { uri, headers } : { uri }; } From 4d0fa14c5e0ec1bd6c8b2f99b02f5d998f692433 Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Tue, 8 Sep 2026 11:19:27 -0400 Subject: [PATCH 13/20] fix(ratelimit): honor Retry-After and recover avatars from transient failures P1 - Retry-After was silently discarded. RequestOrchestrator.beginCooldown called retryAfterDelayMs(response, retryIndex), but retryAfterDelayMs takes the Retry-After header VALUE, not a response, so it returned null on the first type check. The cooldown was then set to now() + null, which is now(), so it expired immediately and every subsequent request sailed straight through an active limiter window. That is the request amplification seen in production: 27 further requests issued while Retry-After was active, and 104 rejected 429s in a day. The production request ledger corroborates the mechanism precisely - every recorded cooldown_begin carried durationClass "short", which is what null < 10000 evaluates to. beginCooldown now uses rateLimitDelayMs, which reads the header off the response and falls back to a bounded backoff when it is absent. Cooldowns stay clamped to RATE_LIMIT_MAX_MS, so honoring a hostile or very long Retry-After cannot deadlock auth, logout or session recovery. site.jsonApi additionally waits on the IP bucket before issuing a request. An IP-scoped 429 previously set a cooldown that only retries consulted, so fresh requests kept amplifying inside that window too. P2 - a transient image failure latched the letter initial for the lifetime of the mounted Avatar. onError set failedUri, and the reset only ran when the URI changed, which it never does for a given member and size. Because stack screens remount on every navigation and the member-photo route sends Cache-Control: private, no-store, each mount issued a fresh authenticated request and a single 429 was terminal. Recovery is now bounded: two delayed retries at 1.5s and 6s, counted per URI, then the initial stands. The pending timer is cleared on unmount. P4 - member profile reads had no TTL, so remounting refetched /u/:username.json every navigation. They now coalesce inside a 15s TTL without serving stale. Chat message loads are deliberately left uncached because they are real-time. The private member-photo credential boundary is unchanged: only /renaissance/member-photo/ receives User API credentials, ordinary /user_avatar/ stays unauthenticated, and the canonical-origin and secure-media guards are untouched. Two pre-existing sitePrivacy tests asserted Retry-After waiting but could only pass vacuously while the cooldown was zero-length. They now exercise the real wait, so they advance timers instead of counting microtasks. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR --- js/__tests__/rateLimitResilience.test.js | 218 +++++++++++++++++++++++ js/__tests__/sitePrivacy.test.js | 16 +- js/product/ProductComponents.js | 39 +++- js/product/avatarRecovery.js | 30 ++++ js/requestOrchestrator.js | 10 +- js/site.js | 25 ++- 6 files changed, 319 insertions(+), 19 deletions(-) create mode 100644 js/__tests__/rateLimitResilience.test.js create mode 100644 js/product/avatarRecovery.js diff --git a/js/__tests__/rateLimitResilience.test.js b/js/__tests__/rateLimitResilience.test.js new file mode 100644 index 000000000..491a51ab7 --- /dev/null +++ b/js/__tests__/rateLimitResilience.test.js @@ -0,0 +1,218 @@ +import { + RATE_LIMIT_MAX_MS, + RATE_LIMIT_MIN_MS, + rateLimitDelayMs, + retryAfterDelayMs, +} from '../apiRateLimit'; +import { RequestOrchestrator } from '../requestOrchestrator'; +import { + AVATAR_RECOVERY_MAX_ATTEMPTS, + avatarRecoveryDelayMs, + avatarShowsImage, + shouldAttemptAvatarRecovery, +} from '../product/avatarRecovery'; + +const responseWith = retryAfter => ({ + status: 429, + headers: { get: name => (name === 'Retry-After' ? retryAfter : null) }, +}); + +describe('P1: Retry-After is honored by the shared cooldown', () => { + test('regression: passing the response set a zero-length cooldown', () => { + // retryAfterDelayMs takes the header VALUE. Handing it the response + // returned null, and now() + null === now(), so the cooldown expired + // immediately and every request sailed through an active limiter window. + expect(retryAfterDelayMs(responseWith('30'))).toBeNull(); + expect(Date.now() + null).toBe(Date.now() + 0); + }); + + test('rateLimitDelayMs reads Retry-After off the response', () => { + expect(rateLimitDelayMs(responseWith('30'), 0)).toBe(30000); + expect(rateLimitDelayMs(responseWith('5'), 0)).toBe(5000); + }); + + test('a 429 now produces a real cooldown that blocks new requests', async () => { + let clock = 1000; + const slept = []; + const orchestrator = new RequestOrchestrator({ + now: () => clock, + sleep: ms => { + slept.push(ms); + clock += ms; + return Promise.resolve(); + }, + }); + + const delay = orchestrator.beginCooldown('b', responseWith('30'), 0); + expect(delay).toBe(30000); + expect(orchestrator.cooldowns.get('b')).toBe(1000 + 30000); + + await orchestrator.waitForBucket('b'); + // The waiter actually slept for the directed window. + expect(slept.reduce((a, b) => a + b, 0)).toBeGreaterThanOrEqual(30000); + }); + + test('an absent Retry-After falls back to a bounded backoff, never zero', () => { + let clock = 0; + const orchestrator = new RequestOrchestrator({ + now: () => clock, + sleep: () => Promise.resolve(), + }); + const first = orchestrator.beginCooldown('b', responseWith(null), 0); + expect(first).toBeGreaterThanOrEqual(RATE_LIMIT_MIN_MS); + expect(orchestrator.cooldowns.get('b')).toBeGreaterThan(clock); + }); + + test('cooldowns stay bounded so auth and logout cannot deadlock', () => { + expect(rateLimitDelayMs(responseWith('99999'), 0)).toBe(RATE_LIMIT_MAX_MS); + expect(rateLimitDelayMs(responseWith('-5'), 0)).toBeLessThanOrEqual( + RATE_LIMIT_MAX_MS, + ); + }); + + test('a longer directed window never shortens an existing cooldown', () => { + let clock = 0; + const orchestrator = new RequestOrchestrator({ + now: () => clock, + sleep: () => Promise.resolve(), + }); + orchestrator.beginCooldown('b', responseWith('40'), 0); + orchestrator.beginCooldown('b', responseWith('5'), 1); + expect(orchestrator.cooldowns.get('b')).toBe(40000); + }); + + test('no amplification: concurrent waiters observe one window', async () => { + let clock = 0; + let sleeps = 0; + const orchestrator = new RequestOrchestrator({ + now: () => clock, + sleep: ms => { + sleeps += 1; + clock += ms; + return Promise.resolve(); + }, + }); + orchestrator.beginCooldown('b', responseWith('10'), 0); + await Promise.all([ + orchestrator.waitForBucket('b'), + orchestrator.waitForBucket('b'), + orchestrator.waitForBucket('b'), + ]); + // Once the window has elapsed the bucket is cleared, not re-slept forever. + expect(orchestrator.cooldowns.has('b')).toBe(false); + expect(sleeps).toBeLessThanOrEqual(3); + }); +}); + +describe('P1: new requests are gated on the IP bucket as well', () => { + const fs = require('fs'); + const path = require('path'); + const source = fs.readFileSync(path.join(__dirname, '..', 'site.js'), 'utf8'); + + test('site.jsonApi waits on user-api, ip and endpoint buckets', () => { + expect(source).toContain( + 'await requestOrchestrator.waitForBucket(globalUserBucket)', + ); + expect(source).toContain( + 'await requestOrchestrator.waitForBucket(ipBucket)', + ); + expect(source).toContain( + 'await requestOrchestrator.waitForBucket(fallbackBucket)', + ); + expect(source).toContain("errorCode: 'ip_60_secs_limit'"); + }); +}); + +describe('P2: avatar recovers from a transient failure, boundedly', () => { + test('the initial is not terminal while attempts remain', () => { + const uri = + 'https://adjusternetwork.org/renaissance/member-photo/t/72/67.png'; + expect( + avatarShowsImage({ resolvedUri: uri, failedUri: null, attempt: 0 }), + ).toBe(true); + // A failure with attempts remaining still resolves to the image. + expect( + avatarShowsImage({ resolvedUri: uri, failedUri: uri, attempt: 0 }), + ).toBe(true); + expect( + avatarShowsImage({ resolvedUri: uri, failedUri: uri, attempt: 1 }), + ).toBe(true); + // Exhausted: the initial now stands. + expect( + avatarShowsImage({ + resolvedUri: uri, + failedUri: uri, + attempt: AVATAR_RECOVERY_MAX_ATTEMPTS, + }), + ).toBe(false); + expect( + avatarShowsImage({ resolvedUri: null, failedUri: null, attempt: 0 }), + ).toBe(false); + }); + + test('recovery is bounded and never infinite', () => { + expect(avatarRecoveryDelayMs(0)).toBe(1500); + expect(avatarRecoveryDelayMs(1)).toBe(6000); + expect(avatarRecoveryDelayMs(AVATAR_RECOVERY_MAX_ATTEMPTS)).toBeNull(); + expect(avatarRecoveryDelayMs(99)).toBeNull(); + expect(avatarRecoveryDelayMs(-1)).toBeNull(); + expect(shouldAttemptAvatarRecovery(AVATAR_RECOVERY_MAX_ATTEMPTS)).toBe( + false, + ); + }); + + test('delays back off rather than hammering the limiter', () => { + const delays = [avatarRecoveryDelayMs(0), avatarRecoveryDelayMs(1)]; + expect(delays[1]).toBeGreaterThan(delays[0]); + expect(delays[0]).toBeGreaterThanOrEqual(1000); + }); +}); + +describe('P2: Avatar wiring', () => { + const fs = require('fs'); + const path = require('path'); + const source = fs.readFileSync( + path.join(__dirname, '..', 'product', 'ProductComponents.js'), + 'utf8', + ); + + test('the permanent latch is gone and attempts reset per URI', () => { + expect(source).toContain('setRecoveryAttempt(current => current + 1)'); + expect(source).toContain('key={`${resolvedUri}#${recoveryAttempt}`}'); + // Changing URI clears both the failure and the attempt count. + expect(source).toMatch(/setFailedUri\(null\);\s*setRecoveryAttempt\(0\);/); + // The pending timer is cleared on unmount so no work escapes the instance. + expect(source).toContain('clearTimeout(recoveryTimer.current)'); + }); + + test('the private member-photo credential boundary is untouched', () => { + expect(source).toContain('source={memberImageSource(site, resolvedUri)}'); + const helper = fs.readFileSync( + path.join(__dirname, '..', 'product', 'memberImageSource.js'), + 'utf8', + ); + expect(helper).toContain( + 'MEMBER_PHOTO_PATH = /^\\/renaissance\\/member-photo\\//', + ); + expect(helper).toContain('isCanonicalUrl(uri)'); + }); +}); + +describe('P4: duplicate profile reads are coalesced, chat is not', () => { + const fs = require('fs'); + const path = require('path'); + const source = fs.readFileSync(path.join(__dirname, '..', 'site.js'), 'utf8'); + + test('member profile reads get a short TTL without serving stale', () => { + expect(source).toContain( + 'const ttlMs = isNativeRead ? 30000 : isMemberRead ? 15000 : 0;', + ); + expect(source).toContain('allowStale: isNativeRead,'); + }); + + test('only /u/:username.json GETs qualify, never chat or mutations', () => { + const re = /\/\^\\\/u\\\/\[\^\/\]\+\\\.json\//; + expect(re.test(source)).toBe(true); + expect(source).not.toMatch(/isMemberRead[\s\S]{0,80}chat/); + }); +}); diff --git a/js/__tests__/sitePrivacy.test.js b/js/__tests__/sitePrivacy.test.js index c2f2da807..2c6595a44 100644 --- a/js/__tests__/sitePrivacy.test.js +++ b/js/__tests__/sitePrivacy.test.js @@ -80,11 +80,9 @@ describe('site privacy serialization', () => { authToken: 'synthetic-key', }); const pending = site.jsonApi('/latest.json'); - await Promise.resolve(); - await Promise.resolve(); - await Promise.resolve(); + await jest.advanceTimersByTimeAsync(0); expect(fetch).toHaveBeenCalledTimes(1); - await jest.runOnlyPendingTimersAsync(); + await jest.advanceTimersByTimeAsync(20000); await expect(pending).resolves.toEqual({ ok: true }); expect(fetch).toHaveBeenCalledTimes(2); jest.useRealTimers(); @@ -113,10 +111,9 @@ describe('site privacy serialization', () => { }); const floor = site.jsonApi('/latest.json'); const notifications = site.jsonApi('/notifications.json'); - await Promise.resolve(); - await Promise.resolve(); + await jest.advanceTimersByTimeAsync(0); expect(fetch).toHaveBeenCalledTimes(2); - await jest.runOnlyPendingTimersAsync(); + await jest.advanceTimersByTimeAsync(20000); await expect(Promise.all([floor, notifications])).resolves.toEqual([ { topics: true }, { notifications: true }, @@ -140,9 +137,8 @@ describe('site privacy serialization', () => { message: 'api_rate_limited', status: 429, }); - await Promise.resolve(); - await jest.advanceTimersByTimeAsync(2000); - await jest.advanceTimersByTimeAsync(5000); + await jest.advanceTimersByTimeAsync(0); + await jest.advanceTimersByTimeAsync(20000); await rejection; expect(fetch).toHaveBeenCalledTimes(3); apiRateLimitCoordinator.reset(); diff --git a/js/product/ProductComponents.js b/js/product/ProductComponents.js index 5cd355359..b468aab48 100644 --- a/js/product/ProductComponents.js +++ b/js/product/ProductComponents.js @@ -1,7 +1,7 @@ /* @flow */ 'use strict'; -import React, { useContext, useEffect, useState } from 'react'; +import React, { useContext, useEffect, useRef, useState } from 'react'; import { ActivityIndicator, Image, @@ -15,6 +15,7 @@ import { ThemeContext } from '../ThemeContext'; import { productTheme, radius, spacing, type } from './DesignSystem'; import { useAvatarAuthorityRecord } from './avatarAuthority'; import { memberImageSource } from './memberImageSource'; +import { avatarRecoveryDelayMs } from './avatarRecovery'; export const useProductTheme = () => productTheme(useContext(ThemeContext).name); @@ -395,17 +396,47 @@ export const Avatar = ({ uri || avatarUri(site, authority ? authority.template : avatarTemplate, size); const [failedUri, setFailedUri] = useState(null); - useEffect(() => setFailedUri(null), [resolvedUri]); + // Bounded recovery from a transient image failure. Counted per URI so a + // changed photo always starts fresh, and capped so a genuinely broken image + // settles on the initial instead of retrying forever. + const [recoveryAttempt, setRecoveryAttempt] = useState(0); + const recoveryTimer = useRef(null); + useEffect(() => { + setFailedUri(null); + setRecoveryAttempt(0); + }, [resolvedUri]); + useEffect( + () => () => { + if (recoveryTimer.current) clearTimeout(recoveryTimer.current); + }, + [], + ); const style = [ { width: size, height: size, borderRadius: size / 2 }, suppliedStyle, ]; + const scheduleRecovery = () => { + const delay = avatarRecoveryDelayMs(recoveryAttempt); + if (delay === null) return; + if (recoveryTimer.current) clearTimeout(recoveryTimer.current); + recoveryTimer.current = setTimeout(() => { + recoveryTimer.current = null; + // Clearing failedUri re-mounts the same URI for one more attempt. The + // attempt counter is what terminates this, not the URI changing. + setFailedUri(null); + setRecoveryAttempt(current => current + 1); + }, delay); + }; + if (resolvedUri && failedUri !== resolvedUri) { return ( setFailedUri(resolvedUri)} + onError={() => { + setFailedUri(resolvedUri); + scheduleRecovery(); + }} source={memberImageSource(site, resolvedUri)} style={style} /> diff --git a/js/product/avatarRecovery.js b/js/product/avatarRecovery.js new file mode 100644 index 000000000..f142a5c30 --- /dev/null +++ b/js/product/avatarRecovery.js @@ -0,0 +1,30 @@ +/* @flow */ +'use strict'; + +// A transient image failure must not permanently show the letter initial for +// the lifetime of a mounted Avatar. Stack screens remount on every navigation +// and the private member-photo route sends Cache-Control: private, no-store, +// so each mount issues a fresh authenticated request; a single 429 inside a +// limiter window used to latch the initial until the component unmounted. +// +// Recovery is bounded, not infinite: a small number of delayed attempts, then +// the initial stands. Attempts are per URI, so a changed photo starts fresh. +export const AVATAR_RECOVERY_MAX_ATTEMPTS = 2; +export const AVATAR_RECOVERY_DELAYS_MS = Object.freeze([1500, 6000]); + +export function avatarRecoveryDelayMs(attempt) { + if (!Number.isFinite(attempt) || attempt < 0) return null; + if (attempt >= AVATAR_RECOVERY_MAX_ATTEMPTS) return null; + return AVATAR_RECOVERY_DELAYS_MS[attempt] ?? null; +} + +export function shouldAttemptAvatarRecovery(attempt) { + return avatarRecoveryDelayMs(attempt) !== null; +} + +// The initial is shown only once recovery is exhausted for this exact URI. +export function avatarShowsImage({ resolvedUri, failedUri, attempt }) { + if (!resolvedUri) return false; + if (failedUri !== resolvedUri) return true; + return shouldAttemptAvatarRecovery(attempt); +} diff --git a/js/requestOrchestrator.js b/js/requestOrchestrator.js index 19c47a03f..a229ee7b0 100644 --- a/js/requestOrchestrator.js +++ b/js/requestOrchestrator.js @@ -1,7 +1,7 @@ /* @flow */ 'use strict'; -import { retryAfterDelayMs } from './apiRateLimit'; +import { rateLimitDelayMs } from './apiRateLimit'; import { recordRequestLedger } from './requestLedgerDiagnostics'; const MAX_CONCURRENCY = 3; @@ -93,7 +93,13 @@ export class RequestOrchestrator { } beginCooldown(bucket, response, retryIndex) { - const delay = retryAfterDelayMs(response, retryIndex); + // retryAfterDelayMs takes the Retry-After header value; passing the whole + // response made it return null, so every cooldown was set to now() and + // expired instantly. That is why the client kept issuing requests inside + // an active limiter window, and why every recorded cooldown_begin carried + // durationClass "short". rateLimitDelayMs reads the header off the + // response and falls back to a bounded backoff when it is absent. + const delay = rateLimitDelayMs(response, retryIndex, this.now()); this.cooldowns.set( bucket, Math.max(this.cooldowns.get(bucket) || 0, this.now() + delay), diff --git a/js/site.js b/js/site.js index 64670aa71..ac02e9d88 100644 --- a/js/site.js +++ b/js/site.js @@ -143,12 +143,21 @@ class Site { jsonApi(path, method, data) { const normalizedMethod = method || 'GET'; const key = this.apiRequestKey(path, normalizedMethod); - const ttlMs = - normalizedMethod === 'GET' && path.startsWith('/native/v1/') ? 30000 : 0; + // Member profile reads are refetched on every navigation because stack + // screens remount, and each one consumes User API budget. A short TTL + // coalesces those repeats without changing what a screen displays. + // Chat message loads are deliberately excluded: they are real-time and + // caching them would show stale conversation. + const isNativeRead = + normalizedMethod === 'GET' && path.startsWith('/native/v1/'); + const isMemberRead = + normalizedMethod === 'GET' && /^\/u\/[^/]+\.json/.test(path); + const ttlMs = isNativeRead ? 30000 : isMemberRead ? 15000 : 0; return requestOrchestrator.request({ key, ttlMs, - allowStale: normalizedMethod === 'GET' && path.startsWith('/native/v1/'), + // Member reads never serve stale; they only deduplicate inside the TTL. + allowStale: isNativeRead, priority: normalizedMethod === 'GET' ? 'visible' : 'bootstrap', task: () => this._jsonApi(path, normalizedMethod, data), }); @@ -191,7 +200,17 @@ class Site { path, errorCode: 'user_api_key_limiter_60_secs', }); + // The IP buckets gate new requests too. Without this, an IP-scoped 429 + // set a cooldown that only retries consulted, so fresh requests kept + // amplifying inside an active limiter window. + const ipBucket = limiterBucket({ + origin: this.url, + clientId: this.clientId, + path, + errorCode: 'ip_60_secs_limit', + }); await requestOrchestrator.waitForBucket(globalUserBucket); + await requestOrchestrator.waitForBucket(ipBucket); await requestOrchestrator.waitForBucket(fallbackBucket); let req = new Request(this.url + path, { headers: headers, From 0061df498acdbf0e6f027aa95604ff2d7e649499 Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Tue, 8 Sep 2026 11:24:30 -0400 Subject: [PATCH 14/20] test(ratelimit): pin limiter bucket semantics for the pre-device review Adds the evidence the review asked for rather than asserting it: which bucket a user_api_key_limiter_60_secs response enters, that the user-api bucket is path independent so every authenticated jsonApi path waits on it, that the IP codes map to a genuinely separate bucket, that a different client id is not blocked, that every wait is clamped so auth and logout cannot deadlock, that mutations are never replayed, and what the 60s clamp actually does to a 136s directive. No implementation change. --- js/__tests__/limiterBucketReview.test.js | 250 +++++++++++++++++++++++ 1 file changed, 250 insertions(+) create mode 100644 js/__tests__/limiterBucketReview.test.js diff --git a/js/__tests__/limiterBucketReview.test.js b/js/__tests__/limiterBucketReview.test.js new file mode 100644 index 000000000..2e519f683 --- /dev/null +++ b/js/__tests__/limiterBucketReview.test.js @@ -0,0 +1,250 @@ +import Site from '../site'; +import fetch from '../../lib/fetch'; +import { + RATE_LIMIT_MAX_MS, + apiRateLimitCoordinator, + rateLimitDelayMs, +} from '../apiRateLimit'; +import { limiterBucket, requestOrchestrator } from '../requestOrchestrator'; + +jest.mock('../../lib/fetch', () => jest.fn()); + +const ORIGIN = 'https://adjusternetwork.org'; +const bucketFor = (errorCode, path = '/latest.json', clientId = 'client-A') => + limiterBucket({ origin: ORIGIN, clientId, path, errorCode }); + +const limited = (retryAfter, code) => ({ + status: 429, + headers: { + get: name => + name === 'Retry-After' + ? retryAfter + : name === 'Discourse-Rate-Limit-Error-Code' + ? code + : null, + }, +}); + +beforeEach(() => { + fetch.mockReset(); + apiRateLimitCoordinator.reset(); + requestOrchestrator.reset(); +}); + +describe('Q1: which bucket a user-api 429 enters', () => { + test('user_api_key_limiter_60_secs keys on origin + User API client id', () => { + expect(bucketFor('user_api_key_limiter_60_secs')).toBe( + `${ORIGIN}:user-api:client-A`, + ); + // The daily limiter shares that bucket, matching the server keying. + expect(bucketFor('user_api_key_limiter_1_day')).toBe( + `${ORIGIN}:user-api:client-A`, + ); + // A different key is a different bucket: one member cannot stall another. + expect( + bucketFor('user_api_key_limiter_60_secs', '/latest.json', 'client-B'), + ).toBe(`${ORIGIN}:user-api:client-B`); + }); + + test('the user-api bucket is path independent', () => { + for (const path of [ + '/native/v1/profile', + '/u/tomrodriguez.json', + '/chat/api/me/channels.json', + '/site.json', + '/latest.json', + ]) { + expect(bucketFor('user_api_key_limiter_60_secs', path)).toBe( + `${ORIGIN}:user-api:client-A`, + ); + } + }); +}); + +describe('Q2: which later paths a user-api cooldown blocks', () => { + test.each([ + '/native/v1/profile', + '/u/tomrodriguez.json', + '/chat/api/me/channels.json', + '/site.json', + '/latest.json', + ])('%s waits on the shared user-api cooldown', async path => { + jest.useFakeTimers(); + // Seed a live cooldown on the user-api bucket for this client. + requestOrchestrator.beginCooldown( + `${ORIGIN}:user-api:client-A`, + limited('30', 'user_api_key_limiter_60_secs'), + 0, + ); + fetch.mockResolvedValue({ + status: 200, + json: () => Promise.resolve({ ok: 1 }), + }); + const site = new Site({ + url: ORIGIN, + authToken: 'k', + clientId: 'client-A', + }); + + const pending = site.jsonApi(path); + await jest.advanceTimersByTimeAsync(0); + // Blocked: no request issued inside the window. + expect(fetch).not.toHaveBeenCalled(); + + await jest.advanceTimersByTimeAsync(31000); + await expect(pending).resolves.toEqual({ ok: 1 }); + expect(fetch).toHaveBeenCalledTimes(1); + jest.useRealTimers(); + }); +}); + +describe('Q3/Q4: IP bucket is literal and separate from user-api', () => { + test('the IP codes map to their own bucket, not a general label', () => { + expect(bucketFor('ip_10_secs_limit')).toBe(`${ORIGIN}:ip`); + expect(bucketFor('ip_60_secs_limit')).toBe(`${ORIGIN}:ip`); + // It is client-id independent, which is what an IP limiter means. + expect(bucketFor('ip_60_secs_limit', '/latest.json', 'client-B')).toBe( + `${ORIGIN}:ip`, + ); + }); + + test('user-api and IP cooldowns are separate and do not leak into each other', async () => { + jest.useFakeTimers(); + requestOrchestrator.beginCooldown( + `${ORIGIN}:ip`, + limited('30', 'ip_60_secs_limit'), + 0, + ); + // The user-api bucket is untouched by an IP cooldown. + expect( + requestOrchestrator.cooldowns.has(`${ORIGIN}:user-api:client-A`), + ).toBe(false); + expect(requestOrchestrator.cooldowns.has(`${ORIGIN}:ip`)).toBe(true); + jest.useRealTimers(); + }); + + test('native and endpoint-class buckets are also distinct', () => { + expect(bucketFor('an_admission_required')).toBe( + `${ORIGIN}:native:an_admission_required`, + ); + expect(bucketFor(null, '/native/v1/profile')).toBe( + `${ORIGIN}:class:profile`, + ); + expect(bucketFor(null, '/chat/api/me/channels.json')).toBe( + `${ORIGIN}:class:/chat/api`, + ); + }); +}); + +describe('Q5: correctness of the wait', () => { + test('the wait expires and the bucket is cleared', async () => { + let clock = 0; + const orchestrator = new requestOrchestrator.constructor({ + now: () => clock, + sleep: ms => { + clock += ms; + return Promise.resolve(); + }, + }); + orchestrator.beginCooldown( + 'b', + limited('30', 'user_api_key_limiter_60_secs'), + 0, + ); + expect(orchestrator.cooldowns.get('b')).toBe(30000); + await orchestrator.waitForBucket('b'); + expect(orchestrator.cooldowns.has('b')).toBe(false); + }); + + test('a different client id is not blocked: unrelated traffic proceeds', async () => { + jest.useFakeTimers(); + requestOrchestrator.beginCooldown( + `${ORIGIN}:user-api:client-A`, + limited('30', 'user_api_key_limiter_60_secs'), + 0, + ); + fetch.mockResolvedValue({ + status: 200, + json: () => Promise.resolve({ ok: 2 }), + }); + // Signed out: clientId absent, so the bucket is :user-api:unknown. + const other = new Site({ url: ORIGIN }); + const pending = other.jsonApi('/site.json'); + await jest.advanceTimersByTimeAsync(0); + expect(fetch).toHaveBeenCalledTimes(1); + await expect(pending).resolves.toEqual({ ok: 2 }); + jest.useRealTimers(); + }); + + test('auth, logout and session recovery cannot deadlock: every wait is bounded', async () => { + let clock = 0; + const orchestrator = new requestOrchestrator.constructor({ + now: () => clock, + sleep: ms => { + clock += ms; + return Promise.resolve(); + }, + }); + // Even a hostile Retry-After cannot hold a request longer than the clamp. + orchestrator.beginCooldown( + 'b', + limited('999999', 'user_api_key_limiter_60_secs'), + 0, + ); + expect(orchestrator.cooldowns.get('b')).toBe(RATE_LIMIT_MAX_MS); + await orchestrator.waitForBucket('b'); + expect(clock).toBeLessThanOrEqual(RATE_LIMIT_MAX_MS + 1000); + expect(orchestrator.cooldowns.has('b')).toBe(false); + }); + + test('a rate-limited mutation is never replayed automatically', async () => { + jest.useFakeTimers(); + fetch.mockResolvedValue(limited('30', 'user_api_key_limiter_60_secs')); + const site = new Site({ + url: ORIGIN, + authToken: 'k', + clientId: 'client-A', + }); + const rejection = expect( + site.jsonApi('/native/v1/profile', 'PATCH', { a: 1 }), + ).rejects.toMatchObject({ + status: 429, + }); + await jest.advanceTimersByTimeAsync(0); + await rejection; + // One attempt only: writes are not retried after a limiter response. + expect(fetch).toHaveBeenCalledTimes(1); + jest.useRealTimers(); + }); +}); + +describe('Q6: the 60s clamp against production values up to 136s', () => { + test('Retry-After above the clamp is truncated to 60s', () => { + expect( + rateLimitDelayMs(limited('136', 'user_api_key_limiter_60_secs'), 0), + ).toBe(RATE_LIMIT_MAX_MS); + expect(RATE_LIMIT_MAX_MS).toBe(60000); + }); + + test('truncation under-waits, so a 136s directive costs bounded extra 429s', async () => { + jest.useFakeTimers(); + // Server keeps limiting for longer than the clamp. + fetch.mockResolvedValue(limited('136', 'user_api_key_limiter_60_secs')); + const site = new Site({ + url: ORIGIN, + authToken: 'k', + clientId: 'client-A', + }); + const rejection = expect( + site.jsonApi('/latest.json'), + ).rejects.toMatchObject({ + status: 429, + }); + await jest.advanceTimersByTimeAsync(0); + await jest.advanceTimersByTimeAsync(400000); + await rejection; + // Bounded: initial attempt plus RATE_LIMIT_MAX_RETRIES, never a storm. + expect(fetch).toHaveBeenCalledTimes(3); + jest.useRealTimers(); + }); +}); From 6ed8fa797000d36bac4e241d0c226834658aa4e4 Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Tue, 8 Sep 2026 11:37:52 -0400 Subject: [PATCH 15/20] fix(ratelimit): separate cooldown lifetime from per-request wait ceiling The single RATE_LIMIT_MAX_MS ceiling conflated two different concerns. Clamping the cooldown to it meant a production Retry-After of 136s was recorded as 60s, so GET chains re-entered a known-active window at roughly 60s and 120s and generated avoidable 429s. Two ceilings now: - RATE_LIMIT_COOLDOWN_MAX_MS = 180000 bounds the server-directed cooldown lifetime, so a 136s directive is recorded in full. - RATE_LIMIT_MAX_MS = 60000 still bounds how long one request may block. waitForBucket fails fast with the existing bounded api_rate_limited error when the remaining cooldown exceeds the per-request ceiling, rather than sending a request into an active window or holding a caller for minutes. The cooldown is left intact, so later requests keep observing it until it genuinely expires; once the remainder falls inside the per-request ceiling it becomes a normal bounded wait again. Auth, logout and session recovery therefore cannot deadlock: nothing sleeps longer than 60s and the fail-fast path returns immediately. GET retry counts stay bounded, mutations are still never replayed, and a repeated 429 extends the shared window without fan-out. Also reverts the IP-bucket pre-request wait added earlier in this package. The proven production defect is the User API limiter; no IP-limiter incident has been observed, so the request path stays minimal to the evidence. The pre-existing IP bucket mapping and its tests are untouched. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR --- js/__tests__/limiterBucketReview.test.js | 207 ++++++++++++++++++++--- js/__tests__/rateLimitResilience.test.js | 11 +- js/apiRateLimit.js | 43 ++++- js/requestLedgerDiagnostics.js | 1 + js/requestOrchestrator.js | 27 ++- js/site.js | 10 -- 6 files changed, 252 insertions(+), 47 deletions(-) diff --git a/js/__tests__/limiterBucketReview.test.js b/js/__tests__/limiterBucketReview.test.js index 2e519f683..e219558ac 100644 --- a/js/__tests__/limiterBucketReview.test.js +++ b/js/__tests__/limiterBucketReview.test.js @@ -1,9 +1,9 @@ import Site from '../site'; import fetch from '../../lib/fetch'; import { + RATE_LIMIT_COOLDOWN_MAX_MS, RATE_LIMIT_MAX_MS, apiRateLimitCoordinator, - rateLimitDelayMs, } from '../apiRateLimit'; import { limiterBucket, requestOrchestrator } from '../requestOrchestrator'; @@ -98,6 +98,35 @@ describe('Q2: which later paths a user-api cooldown blocks', () => { }); }); +describe('the IP-bucket pre-request wait was removed from this package', () => { + const fs = require('fs'); + const path = require('path'); + const source = fs.readFileSync(path.join(__dirname, '..', 'site.js'), 'utf8'); + + test('jsonApi waits only on the user-api and endpoint-class buckets', () => { + expect(source).toContain( + 'await requestOrchestrator.waitForBucket(globalUserBucket)', + ); + expect(source).toContain( + 'await requestOrchestrator.waitForBucket(fallbackBucket)', + ); + // Scope kept minimal to the proven User API limiter defect. + expect(source).not.toContain('ipBucket'); + expect(source).not.toContain("errorCode: 'ip_60_secs_limit'"); + }); + + test('the pre-existing IP bucket machinery is preserved', () => { + expect( + limiterBucket({ + origin: ORIGIN, + clientId: 'c', + path: '/x', + errorCode: 'ip_60_secs_limit', + }), + ).toBe(`${ORIGIN}:ip`); + }); +}); + describe('Q3/Q4: IP bucket is literal and separate from user-api', () => { test('the IP codes map to their own bucket, not a general label', () => { expect(bucketFor('ip_10_secs_limit')).toBe(`${ORIGIN}:ip`); @@ -176,7 +205,7 @@ describe('Q5: correctness of the wait', () => { jest.useRealTimers(); }); - test('auth, logout and session recovery cannot deadlock: every wait is bounded', async () => { + test('auth, logout and session recovery cannot deadlock', async () => { let clock = 0; const orchestrator = new requestOrchestrator.constructor({ now: () => clock, @@ -185,16 +214,22 @@ describe('Q5: correctness of the wait', () => { return Promise.resolve(); }, }); - // Even a hostile Retry-After cannot hold a request longer than the clamp. + // A hostile Retry-After is capped at the cooldown ceiling, and no single + // request waits for it: the waiter fails fast instead of hanging. orchestrator.beginCooldown( 'b', limited('999999', 'user_api_key_limiter_60_secs'), 0, ); - expect(orchestrator.cooldowns.get('b')).toBe(RATE_LIMIT_MAX_MS); - await orchestrator.waitForBucket('b'); - expect(clock).toBeLessThanOrEqual(RATE_LIMIT_MAX_MS + 1000); - expect(orchestrator.cooldowns.has('b')).toBe(false); + expect(orchestrator.cooldowns.get('b')).toBe(RATE_LIMIT_COOLDOWN_MAX_MS); + await expect(orchestrator.waitForBucket('b')).rejects.toMatchObject({ + message: 'api_rate_limited', + status: 429, + }); + // Nothing slept, so no caller can be held. + expect(clock).toBe(0); + // The cooldown is preserved for later requests rather than cleared. + expect(orchestrator.cooldowns.get('b')).toBe(RATE_LIMIT_COOLDOWN_MAX_MS); }); test('a rate-limited mutation is never replayed automatically', async () => { @@ -218,32 +253,164 @@ describe('Q5: correctness of the wait', () => { }); }); -describe('Q6: the 60s clamp against production values up to 136s', () => { - test('Retry-After above the clamp is truncated to 60s', () => { - expect( - rateLimitDelayMs(limited('136', 'user_api_key_limiter_60_secs'), 0), - ).toBe(RATE_LIMIT_MAX_MS); +describe('Q6: cooldown lifetime vs per-request ceiling', () => { + const makeOrchestrator = () => { + const state = { clock: 0 }; + const orchestrator = new requestOrchestrator.constructor({ + now: () => state.clock, + sleep: ms => { + state.clock += ms; + return Promise.resolve(); + }, + }); + return { orchestrator, state }; + }; + + test('the two ceilings are distinct', () => { expect(RATE_LIMIT_MAX_MS).toBe(60000); + expect(RATE_LIMIT_COOLDOWN_MAX_MS).toBe(180000); }); - test('truncation under-waits, so a 136s directive costs bounded extra 429s', async () => { + test.each([ + ['30', 30000, 'waits'], + ['60', 60000, 'waits'], + ['136', 136000, 'fails fast'], + ['300', RATE_LIMIT_COOLDOWN_MAX_MS, 'fails fast'], + ])( + 'Retry-After %s records a %s ms cooldown and then %s', + async (header, expected) => { + const { orchestrator, state } = makeOrchestrator(); + const delay = orchestrator.beginCooldown( + 'b', + limited(header, 'user_api_key_limiter_60_secs'), + 0, + ); + expect(delay).toBe(expected); + expect(orchestrator.cooldowns.get('b')).toBe(expected); + + if (expected > RATE_LIMIT_MAX_MS) { + await expect(orchestrator.waitForBucket('b')).rejects.toMatchObject({ + status: 429, + }); + expect(state.clock).toBe(0); + } else { + await orchestrator.waitForBucket('b'); + expect(state.clock).toBeGreaterThanOrEqual(expected); + expect(orchestrator.cooldowns.has('b')).toBe(false); + } + }, + ); + + test('no request is sent inside an active 136s cooldown', async () => { jest.useFakeTimers(); - // Server keeps limiting for longer than the clamp. - fetch.mockResolvedValue(limited('136', 'user_api_key_limiter_60_secs')); + requestOrchestrator.beginCooldown( + `${ORIGIN}:user-api:client-A`, + limited('136', 'user_api_key_limiter_60_secs'), + 0, + ); + fetch.mockResolvedValue({ + status: 200, + json: () => Promise.resolve({ ok: 1 }), + }); const site = new Site({ url: ORIGIN, authToken: 'k', clientId: 'client-A', }); - const rejection = expect( - site.jsonApi('/latest.json'), - ).rejects.toMatchObject({ + await expect(site.jsonApi('/latest.json')).rejects.toMatchObject({ + message: 'api_rate_limited', + status: 429, + }); + // Fail fast: nothing reached the network inside the window. + expect(fetch).not.toHaveBeenCalled(); + jest.useRealTimers(); + }); + + test('later requests keep observing the cooldown until it truly expires', async () => { + const { orchestrator } = makeOrchestrator(); + orchestrator.beginCooldown( + 'b', + limited('136', 'user_api_key_limiter_60_secs'), + 0, + ); + await expect(orchestrator.waitForBucket('b')).rejects.toMatchObject({ + status: 429, + }); + // Still active after the per-request ceiling would have elapsed. + orchestrator.now = () => 61000; + await expect(orchestrator.waitForBucket('b')).rejects.toMatchObject({ status: 429, }); + // Inside the final minute it becomes a normal bounded wait again. + orchestrator.now = () => 100000; + await orchestrator.waitForBucket('b'); + }); + + test('normal requests resume after the cooldown expires', async () => { + jest.useFakeTimers(); + requestOrchestrator.beginCooldown( + `${ORIGIN}:user-api:client-A`, + limited('30', 'user_api_key_limiter_60_secs'), + 0, + ); + fetch.mockResolvedValue({ + status: 200, + json: () => Promise.resolve({ ok: 3 }), + }); + const site = new Site({ + url: ORIGIN, + authToken: 'k', + clientId: 'client-A', + }); + const pending = site.jsonApi('/latest.json'); + await jest.advanceTimersByTimeAsync(0); + expect(fetch).not.toHaveBeenCalled(); + await jest.advanceTimersByTimeAsync(31000); + await expect(pending).resolves.toEqual({ ok: 3 }); + expect(fetch).toHaveBeenCalledTimes(1); + jest.useRealTimers(); + }); + + test('a repeated 429 extends the cooldown without fan-out', async () => { + const { orchestrator } = makeOrchestrator(); + orchestrator.beginCooldown( + 'b', + limited('30', 'user_api_key_limiter_60_secs'), + 0, + ); + expect(orchestrator.cooldowns.get('b')).toBe(30000); + // A longer directive extends it. + orchestrator.beginCooldown( + 'b', + limited('136', 'user_api_key_limiter_60_secs'), + 1, + ); + expect(orchestrator.cooldowns.get('b')).toBe(136000); + // A shorter directive never shortens it. + orchestrator.beginCooldown( + 'b', + limited('5', 'user_api_key_limiter_60_secs'), + 2, + ); + expect(orchestrator.cooldowns.get('b')).toBe(136000); + // One shared window, not one per caller. + expect(orchestrator.cooldowns.size).toBe(1); + }); + + test('GET retry count stays bounded when the window is short', async () => { + jest.useFakeTimers(); + fetch.mockResolvedValue(limited('2', 'user_api_key_limiter_60_secs')); + const site = new Site({ + url: ORIGIN, + authToken: 'k', + clientId: 'client-A', + }); + const rejection = expect( + site.jsonApi('/latest.json'), + ).rejects.toMatchObject({ status: 429 }); await jest.advanceTimersByTimeAsync(0); - await jest.advanceTimersByTimeAsync(400000); + await jest.advanceTimersByTimeAsync(60000); await rejection; - // Bounded: initial attempt plus RATE_LIMIT_MAX_RETRIES, never a storm. expect(fetch).toHaveBeenCalledTimes(3); jest.useRealTimers(); }); diff --git a/js/__tests__/rateLimitResilience.test.js b/js/__tests__/rateLimitResilience.test.js index 491a51ab7..2d84c7936 100644 --- a/js/__tests__/rateLimitResilience.test.js +++ b/js/__tests__/rateLimitResilience.test.js @@ -104,22 +104,21 @@ describe('P1: Retry-After is honored by the shared cooldown', () => { }); }); -describe('P1: new requests are gated on the IP bucket as well', () => { +describe('P1: new requests are gated on the shared user-api cooldown', () => { const fs = require('fs'); const path = require('path'); const source = fs.readFileSync(path.join(__dirname, '..', 'site.js'), 'utf8'); - test('site.jsonApi waits on user-api, ip and endpoint buckets', () => { + test('site.jsonApi waits on the user-api and endpoint buckets only', () => { expect(source).toContain( 'await requestOrchestrator.waitForBucket(globalUserBucket)', ); - expect(source).toContain( - 'await requestOrchestrator.waitForBucket(ipBucket)', - ); expect(source).toContain( 'await requestOrchestrator.waitForBucket(fallbackBucket)', ); - expect(source).toContain("errorCode: 'ip_60_secs_limit'"); + // The IP-bucket pre-request wait was reverted: scope stays minimal to the + // proven User API limiter defect. + expect(source).not.toContain('ipBucket'); }); }); diff --git a/js/apiRateLimit.js b/js/apiRateLimit.js index 83d4d25ad..f5c820b20 100644 --- a/js/apiRateLimit.js +++ b/js/apiRateLimit.js @@ -3,21 +3,38 @@ export const RATE_LIMIT_FALLBACK_MS = Object.freeze([2000, 5000]); export const RATE_LIMIT_MIN_MS = 1000; +// Two independent ceilings. RATE_LIMIT_MAX_MS bounds how long a single request +// may block, so auth, logout and session recovery can never hang. +// RATE_LIMIT_COOLDOWN_MAX_MS bounds the recorded cooldown lifetime, which the +// server directs through Retry-After. Production has returned values up to +// 136s; clamping the cooldown to the per-request ceiling made GET chains +// re-enter a known-active window at ~60s and ~120s. export const RATE_LIMIT_MAX_MS = 60000; +export const RATE_LIMIT_COOLDOWN_MAX_MS = 180000; export const RATE_LIMIT_MAX_RETRIES = 2; -const boundedDelay = value => - Math.min(RATE_LIMIT_MAX_MS, Math.max(RATE_LIMIT_MIN_MS, value)); +const bounded = (value, max) => + Math.min(max, Math.max(RATE_LIMIT_MIN_MS, value)); +const boundedDelay = value => bounded(value, RATE_LIMIT_MAX_MS); +const boundedCooldown = value => bounded(value, RATE_LIMIT_COOLDOWN_MAX_MS); -export function retryAfterDelayMs(value, now = Date.now()) { +function parseRetryAfterMs(value, now) { if (typeof value !== 'string' || !value.trim()) return null; const seconds = Number(value.trim()); - if (Number.isFinite(seconds) && seconds >= 0) { - return boundedDelay(seconds * 1000); - } + if (Number.isFinite(seconds) && seconds >= 0) return seconds * 1000; const timestamp = Date.parse(value); if (!Number.isFinite(timestamp)) return null; - return boundedDelay(Math.max(0, timestamp - now)); + return Math.max(0, timestamp - now); +} + +export function retryAfterDelayMs(value, now = Date.now()) { + const raw = parseRetryAfterMs(value, now); + return raw === null ? null : boundedDelay(raw); +} + +export function retryAfterCooldownMs(value, now = Date.now()) { + const raw = parseRetryAfterMs(value, now); + return raw === null ? null : boundedCooldown(raw); } export function rateLimitDelayMs(response, retryIndex, now = Date.now()) { @@ -28,6 +45,18 @@ export function rateLimitDelayMs(response, retryIndex, now = Date.now()) { return directed ?? RATE_LIMIT_FALLBACK_MS[retryIndex] ?? RATE_LIMIT_MAX_MS; } +// The cooldown lifetime honors the directed value up to the cooldown ceiling. +export function rateLimitCooldownMs(response, retryIndex, now = Date.now()) { + const directed = retryAfterCooldownMs( + response?.headers?.get?.('Retry-After'), + now, + ); + return ( + directed ?? + boundedCooldown(RATE_LIMIT_FALLBACK_MS[retryIndex] ?? RATE_LIMIT_MAX_MS) + ); +} + export class ApiRateLimitCoordinator { constructor({ now = () => Date.now(), sleep } = {}) { this.now = now; diff --git a/js/requestLedgerDiagnostics.js b/js/requestLedgerDiagnostics.js index 14bae4d39..43bf77ad9 100644 --- a/js/requestLedgerDiagnostics.js +++ b/js/requestLedgerDiagnostics.js @@ -13,6 +13,7 @@ const ALLOWED_EVENTS = new Set([ 'cache_hit', 'cooldown_begin', 'cooldown_wait', + 'cooldown_reject', ]); let write = Promise.resolve(); diff --git a/js/requestOrchestrator.js b/js/requestOrchestrator.js index a229ee7b0..150beef16 100644 --- a/js/requestOrchestrator.js +++ b/js/requestOrchestrator.js @@ -1,7 +1,7 @@ /* @flow */ 'use strict'; -import { rateLimitDelayMs } from './apiRateLimit'; +import { RATE_LIMIT_MAX_MS, rateLimitCooldownMs } from './apiRateLimit'; import { recordRequestLedger } from './requestLedgerDiagnostics'; const MAX_CONCURRENCY = 3; @@ -80,6 +80,23 @@ export class RequestOrchestrator { async waitForBucket(bucket) { const until = this.cooldowns.get(bucket) || 0; const remaining = until - this.now(); + // No single request may block longer than the per-request ceiling. When the + // shared cooldown still has more than that left, fail fast with the same + // bounded rate-limit error instead of sending a request into an active + // window. The cooldown itself is untouched, so later requests keep waiting + // on it until it genuinely expires. + if (remaining > RATE_LIMIT_MAX_MS) { + this.record({ + event: 'cooldown_reject', + bucket, + status: 429, + durationClass: 'long', + }); + const error = new Error('api_rate_limited'); + error.status = 429; + error.retryAfterMs = remaining; + throw error; + } if (remaining > 0) { this.record({ event: 'cooldown_wait', @@ -97,9 +114,11 @@ export class RequestOrchestrator { // response made it return null, so every cooldown was set to now() and // expired instantly. That is why the client kept issuing requests inside // an active limiter window, and why every recorded cooldown_begin carried - // durationClass "short". rateLimitDelayMs reads the header off the - // response and falls back to a bounded backoff when it is absent. - const delay = rateLimitDelayMs(response, retryIndex, this.now()); + // durationClass "short". rateLimitCooldownMs reads the header off the + // response and falls back to a bounded backoff when it is absent, honoring + // the directed value up to the cooldown ceiling rather than the + // per-request ceiling. + const delay = rateLimitCooldownMs(response, retryIndex, this.now()); this.cooldowns.set( bucket, Math.max(this.cooldowns.get(bucket) || 0, this.now() + delay), diff --git a/js/site.js b/js/site.js index ac02e9d88..80ceba9ef 100644 --- a/js/site.js +++ b/js/site.js @@ -200,17 +200,7 @@ class Site { path, errorCode: 'user_api_key_limiter_60_secs', }); - // The IP buckets gate new requests too. Without this, an IP-scoped 429 - // set a cooldown that only retries consulted, so fresh requests kept - // amplifying inside an active limiter window. - const ipBucket = limiterBucket({ - origin: this.url, - clientId: this.clientId, - path, - errorCode: 'ip_60_secs_limit', - }); await requestOrchestrator.waitForBucket(globalUserBucket); - await requestOrchestrator.waitForBucket(ipBucket); await requestOrchestrator.waitForBucket(fallbackBucket); let req = new Request(this.url + path, { headers: headers, From 18e812ac4e6efe58488ba1b4ce4e77aa58fb48d1 Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:50:35 -0400 Subject: [PATCH 16/20] docs(release): record the native rate-limit resilience certification (#14) * docs(release): record the native rate-limit resilience certification Captures the production certification of dad0af191716aca8d33c6afe58900e70d2be29e4: shipped OTA identifiers, the rollback pointer to the prior f662cf3b production group, the defects corrected, founder device results, and the server lane's read-only observation. Two earlier hypotheses are recorded as corrections so they are not repeated: message-bus does not carry User-Api-Key and does not consume the User API bucket, and real pre-change pressure came from chat/api, member-photo, /u/*.json and /native/v1/*. The scope limit is recorded plainly: no natural 429 occurred during the run, so Retry-After handling, the fail-fast path and avatar recovery remain code- and CI-validated rather than production-trigger validated. Also files the granted_badge notification defect as a separate P2. Type 12 does map to a badge endpoint in DiscourseUtils, so the fault is downstream of that mapping; it is not a regression from this change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR * docs(ota): require provenance tags and record the superseded avatar diagnostic Adds a release-process requirement that every production OTA artifact receives an immutable annotated tag at promotion time, named ota-- and carrying the group, platform update IDs, runtime, full SHA and rollback pointer. Squash and rebase merges both rewrite commits, so four shipped SHAs became unreachable from trunk before this rule existed. Also records the avatar-resolution diagnostic design that PR #12 carried, since that PR is closed as superseded rather than merged. --------- Co-authored-by: Claude Opus 5 (1M context) --- docs/NATIVE-OTA-OPERATIONS.md | 28 ++++ testing/native-auth-stale-identity/BACKLOG.md | 20 +++ testing/native-ratelimit-resilience/README.md | 126 ++++++++++++++++++ 3 files changed, 174 insertions(+) create mode 100644 testing/native-ratelimit-resilience/README.md diff --git a/docs/NATIVE-OTA-OPERATIONS.md b/docs/NATIVE-OTA-OPERATIONS.md index fbd335ad5..efb36890a 100644 --- a/docs/NATIVE-OTA-OPERATIONS.md +++ b/docs/NATIVE-OTA-OPERATIONS.md @@ -61,6 +61,34 @@ channel, signer key ID, rollout percentage, and the verification result. The update must contain no credentials or private member payloads. Never print the private key, auth tokens, or notification payloads in release evidence. +## OTA provenance tags (required) + +Every OTA artifact published or promoted to production must receive an +immutable annotated git tag **at promotion time**, before any merge can rewrite +or drop the shipped commit. Squash and rebase merges both rewrite commits, so a +shipped SHA recorded in EAS routinely becomes unreachable from the trunk +minutes after it ships. Four production artifacts were lost this way before the +requirement existed. + +Naming: `ota--` — the first 8 characters of the update +group UUID and of the full commit SHA. + +The tag message must record: + +- OTA group ID +- iOS and Android update IDs +- runtime version +- full git SHA +- rollback pointer (the group being superseded) + +Tag before merging the pull request. A tag created afterwards can only be +justified by authoritative EAS records; never create one from recollection or +from inferred content equivalence, because a rewritten commit with identical +content is a different artifact for audit purposes. + +Verify with `git ls-remote --tags origin | grep ota-`, and cross-check a group's +shipped SHA with `eas update:view --json`. + ## Recovery and kill switch - Pause a rollout or revert it to the control update for an immediate rollout diff --git a/testing/native-auth-stale-identity/BACKLOG.md b/testing/native-auth-stale-identity/BACKLOG.md index 4a88490b2..3e86a1e12 100644 --- a/testing/native-auth-stale-identity/BACKLOG.md +++ b/testing/native-auth-stale-identity/BACKLOG.md @@ -123,3 +123,23 @@ server/privacy certification lane. contract rather than diverging. 4. **Re-verify on device after activation** — the capability gate means this cannot be certified while the flag is false. + +## 9. P2 — granted_badge notification tap has no destination + +A `granted_badge` notification (observed with Autobiographer) marks itself read +when tapped but produces no navigation: the member is left where they were with +no destination opened. + +Filed during the 2026-09-08 production certification of +`dad0af191716aca8d33c6afe58900e70d2be29e4`. **Not a regression from that +change** — it is a pre-existing native UX defect in notification routing and was +explicitly excluded from that certification. + +`js/DiscourseUtils.js:38-40` does map notification type 12 to +`/badges/{badge_id}/basic?username={username}`, so an endpoint is produced. The +defect is therefore downstream of that mapping: either `data.badge_id` is absent +on the payload, or the resulting web route does not open a native destination. +Confirm which before changing the mapping. + +Marking-as-read succeeding while navigation silently does nothing is the part +that matters: either open a destination or leave the notification unread. diff --git a/testing/native-ratelimit-resilience/README.md b/testing/native-ratelimit-resilience/README.md new file mode 100644 index 000000000..fe5f86d96 --- /dev/null +++ b/testing/native-ratelimit-resilience/README.md @@ -0,0 +1,126 @@ +# Native rate-limit resilience — production certification + +Recorded: 2026-09-08 (America/New_York) +Verdict: **PRODUCTION OTA CERTIFIED** + +## Shipped artifact + +| Field | Value | +| -------------------- | -------------------------------------------------------------------------------------------------------- | +| Certified SHA | `dad0af191716aca8d33c6afe58900e70d2be29e4` | +| Tag | `ota-a4a5e4ee-dad0af19` | +| Production OTA group | `a4a5e4ee-f11d-4804-9756-50c0ede8cd54` | +| iOS update | `01a08212-1e11-7fec-83a3-9c20b9d80de1` | +| Android update | `01a08212-1e11-73a9-af10-65ad49debf68` | +| Runtime | `an-ios-android-1.0.0-native-2` | +| Rollback target | group `a81e298f-1e6f-4b5f-8c7c-ea466833e57b`, iOS `01a078d2-f294-701b-950b-af3ac6864835`, SHA `f662cf3b` | + +Rollback command: `yarn ota:promote --group=a81e298f-1e6f-4b5f-8c7c-ea466833e57b` + +This was a **direct production publish**, not a staging republish, because staging +was deliberately left untouched. Its manifest therefore carries +`expo-channel-name: production` and `useEmbeddedUpdate: true`, unlike earlier +promotions that were republished from staging. + +## Defects corrected + +**Retry-After was silently discarded.** `RequestOrchestrator.beginCooldown` +called `retryAfterDelayMs(response, retryIndex)`, but that helper takes the +Retry-After header _value_, so it returned `null` on its first type check. The +cooldown was then set to `now() + null`, which is `now()`, and expired +instantly. Retry-After was never honored and the limiter never blocked anything. +The production ledger corroborated it exactly: every recorded `cooldown_begin` +carried `durationClass: "short"`, which is what `null < 10000` evaluates to. + +**One ceiling was doing two jobs.** Clamping the cooldown to the per-request +ceiling meant a production `Retry-After: 136` was recorded as 60s, so GET chains +re-entered a known-active window at roughly 60s and 120s. The two concerns are +now separate: `RATE_LIMIT_COOLDOWN_MAX_MS` (180s) bounds the server-directed +cooldown lifetime; `RATE_LIMIT_MAX_MS` (60s) bounds how long one request may +block. When the remaining cooldown exceeds the per-request ceiling, +`waitForBucket` fails fast with the existing bounded `api_rate_limited` error +rather than sending a request into an active window, and leaves the cooldown +intact so later requests keep observing it. + +**Avatars latched the letter initial permanently.** `onError` set `failedUri`, +and the reset only ran when the URI changed — which never happens for a fixed +member and size. Stack screens remount on every navigation and the member-photo +route sends `Cache-Control: private, no-store`, so each mount issued a fresh +authenticated request and a single 429 was terminal for that instance. Recovery +is now bounded: two delayed retries (1.5s, 6s) counted per URI, then the initial +stands. The pending timer is cleared on unmount. + +**Duplicate profile reads.** `/u/:username.json` had no TTL, so remounting +refetched it every navigation. It now coalesces inside a 15s TTL without serving +stale. Chat message loads are deliberately excluded because they are real-time. + +## Corrections to earlier analysis + +Two of our working hypotheses were wrong and are recorded so they are not +repeated: + +- **Message-bus does not carry `User-Api-Key` and does not consume the User API + bucket.** The earlier attribution of ~65% of bucket consumption to + message-bus was incorrect. Native has no message-bus client at all; the + hypothesis that the `WKWebView` Discourse session was consuming the User API + bucket is also disproved. +- **Real pre-change User API pressure** came primarily from `/chat/api/*`, + member-photo image loads, `/u/*.json`, and `/native/v1/*`. + +## Physical device certification + +Founder-led on the production iPhone, with the server lane observing read-only. + +Floor, Discussions, Ask, Lounge, Intel, You, Member Profile, Edit Profile, +back-navigation and remounts, Notifications, background/foreground, +force-close/reopen, and post-reopen You → Member Profile → Edit Profile: all +PASS. Avatar rendered as a photo on every profile surface including across +remounts. No crashes or hangs. + +Server observation over ~6 minutes: 94 device requests, 92 bucket-consuming, +peak rolling 10s = 9, peak rolling 60s = 21. **User API 429s = 0**, other 429s = +0, 5xx = 0. member-photo 16/16 200, `/u/*.json` 5/5 200, `/native/v1/*` 19/19 +200, `/chat/api/*` 14/14 200, `/site.json` 8/8 200, `/latest.json` 8/8 200. No +server-side contradiction to the founder results. + +### Scope limit of this certification + +**No natural 429 occurred during the run.** Retry-After handling, the fail-fast +path above the per-request ceiling, and avatar recovery from a transient failure +therefore remain **code- and CI-validated, not production-trigger validated**. +The run proves the change is healthy under normal load and did not regress +anything; it does not prove the limiter behaviour under a live 429. + +## Validation + +CI green 4/4 on the exact SHA: lint, Jest, iPhone Detox (54m5s), iPad Detox +(54m21s). Locally: 94 suites / 719 tests, `verify:ota` 17/17, +`verify:ios-auth` 12/12, native Release BUILD SUCCEEDED. + +Preserved throughout: private member-photo credential boundary (only +`/renaissance/member-photo/` receives User API credentials), ordinary +`/user_avatar/` unauthenticated, canonical-origin guard, secure-media origin +guard, identity refresh, logout and account-switch cleanup. No server settings, +runtime, or binary change. The 100/min server setting is not to be reopened. + +## Avatar-resolution diagnostic (superseded, not merged) + +Branch `diag/native-avatar-resolution-20260908`, draft PR #12, final SHA +`a4b82b6ddf34183c00b57cbf62ba2417c2735f94`. Never merged and never promoted to +production; retained on the branch for reuse. + +It instrumented the shared `Avatar` with a monotonic per-instance id, a UTC +timestamp with milliseconds, mount/unmount, the React key, source-object +recreation, the navigator kind, the resolved path, the authority key and +presence, the `memberImageSource` classification, whether `failedUri` matched +the resolved URI, the fallback branch taken, and the image lifecycle with the +exact `nativeEvent.error`. Instance ids mattered because `no-store` plus stack +remounting means several Image instances share one URI and their events +interleave. + +It was never run: the fixture iPhone was on the production channel and could not +consume a staging OTA, and no second device was available. The root cause was +instead proven from source plus the server lane's 429 evidence, so the design is +recorded here rather than carried in trunk. The two ideas worth keeping are the +per-instance id for correlating interleaved events, and a UTC wall clock with +milliseconds for aligning a specific client instance against an edge log. From cb4d286b4b45e515b632f53bee7543b7436f05b2 Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Tue, 8 Sep 2026 20:17:00 -0400 Subject: [PATCH 17/20] ci(detox): give the logged-out launch its real budget and retry runner flake (#17) Backlog #10. The logged-out suite has failed intermittently across trunk and unrelated pull requests, most recently on PR #16 with: Exceeded timeout of 120000 ms for a hook. The hook is the beforeEach in e2e/onboarding.test.js, which performs a full device.launchApp({ delete: true, newInstance: true }) reinstall. jest.config's testTimeout governs hooks as well as tests, so a cold macOS runner could exhaust the entire 120s budget inside the reinstall, before the element waits ever ran. The same run makes that plain: a sibling test asserting on the same welcome element passed at 119027ms, one second under the old budget, and the iPad job passed on identical product code. testTimeout goes to 180s, and the iPhone Detox job gains --retries 2 to match the iPad job that has always had it. That asymmetry is the only reason this flake blocked iPhone and not iPad. No assertion is weakened. The 15s and 30s element waits in loggedOutLaunch.js are unchanged, the logged-out-welcome-scroll checks remain, no sleeps were added, and a test that fails every retry still fails the job. Comments now record why the budget is what it is, so it is not trimmed back later. CI configuration only; no product code. --- .github/workflows/ios-tests.yml | 6 +++++- e2e/jest.config.js | 7 ++++++- e2e/loggedOutLaunch.js | 4 ++++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ios-tests.yml b/.github/workflows/ios-tests.yml index 696a66585..ef51fdf3b 100644 --- a/.github/workflows/ios-tests.yml +++ b/.github/workflows/ios-tests.yml @@ -51,8 +51,12 @@ jobs: -quiet \ build + # --retries matches the iPad job. It covers macOS runner flake - cold + # simulator boots have repeatedly exceeded the launch budget - not + # product assertions. A test that fails every attempt still fails the + # job; retries never convert a real regression into a pass. - name: Detox iPhone tests - run: yarn detox test --configuration ios.sim.release --cleanup --record-logs all + run: yarn detox test --configuration ios.sim.release --cleanup --record-logs all --retries 2 - name: Upload artifacts if: failure() diff --git a/e2e/jest.config.js b/e2e/jest.config.js index da4e7aa29..edb677036 100644 --- a/e2e/jest.config.js +++ b/e2e/jest.config.js @@ -3,7 +3,12 @@ module.exports = { rootDir: '..', roots: ['/e2e'], testMatch: ['/e2e/**/*.test.js'], - testTimeout: 120000, + // Governs hooks as well as tests. The logged-out suite's beforeEach performs + // a full device.launchApp({ delete: true }) reinstall, and on a cold macOS + // runner that has exceeded 120s outright - one observed sibling test passed + // at 119027ms, a second under the old budget. 180s accommodates the + // documented slowness without touching any element matcher. + testTimeout: 180000, maxWorkers: 1, globalSetup: 'detox/runners/jest/globalSetup', globalTeardown: 'detox/runners/jest/globalTeardown', diff --git a/e2e/loggedOutLaunch.js b/e2e/loggedOutLaunch.js index a3e0ab286..cf539ada2 100644 --- a/e2e/loggedOutLaunch.js +++ b/e2e/loggedOutLaunch.js @@ -1,5 +1,9 @@ import { by, device, element, waitFor } from 'detox'; +// The element-level waits below are deliberately unchanged. The flake this +// helper is associated with was never the matcher: it was the outer Jest +// hook/test budget in e2e/jest.config.js, which a cold-boot reinstall could +// exceed before these waits had a chance to run. Keep the assertions strict. export async function waitForLoggedOutWelcome() { const welcome = element(by.id('logged-out-welcome-scroll')); From 64940a16128fb464dfd019c4182e42ce1fad5d7b Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Wed, 9 Sep 2026 08:06:52 -0400 Subject: [PATCH 18/20] fix(notifications): route valid first-party destinations instead of dropping them (#16) Tapping an Autobiographer notification marked it read and then did nothing visible. The type-12 mapping was never the problem: DiscourseUtils produced a well-formed /badges/{id}/basic?username={user} URL from a payload that carries badge_id and username. The destination was lost one layer later. classifyFirstPartyMemberRoute is an allowlist of native screens and returned 'rejected' for anything outside it, and openUrl handled only 'native' and 'privileged_external'. A 'rejected' result therefore fell off the end of the function: no navigation, no error, no feedback. Read-marking had already fired, which is why the server saw /notifications/read return 200 while the UI stayed put. This was never badge-specific. The same silent drop affected group message summaries, consolidated likes, accepted membership requests, and both chat mention and chat message notifications - seven classes in total, chat included. A 'first_party_web' disposition now names valid canonical-origin member destinations that have no native screen, and openUrl sends them to the already-registered authenticated Discourse WebView. It is an explicit path allowlist, not a blanket "anything internal opens" rule, and it is evaluated after every native pattern and after the /admin boundary so it cannot widen an already-denied destination. Off-origin URLs, plain HTTP, unauthenticated callers, non-staff /admin, unknown types and empty endpoints all stay rejected, and a malformed badge payload fails safely because a non-numeric badge id does not match. openUrl now handles every disposition explicitly and records a security event for a denial, so no recognised disposition can silently fall through again. No native badge, group or chat screen was invented; the existing WebView destination is used. Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR Co-authored-by: Claude Opus 5 (1M context) --- js/Discourse.js | 14 ++ js/__tests__/notificationRouting.test.js | 267 +++++++++++++++++++++++ js/nativeMemberRouting.js | 26 +++ 3 files changed, 307 insertions(+) create mode 100644 js/__tests__/notificationRouting.test.js diff --git a/js/Discourse.js b/js/Discourse.js index 5325b26d5..6da48f6b0 100644 --- a/js/Discourse.js +++ b/js/Discourse.js @@ -979,9 +979,23 @@ class Discourse extends React.Component { } return; } + // A valid first-party member destination with no native screen opens in the + // authenticated Discourse WebView. Without this branch such destinations + // fell through and the tap did nothing at all: notification read-marking + // had already succeeded, so a granted_badge notification went read with no + // visible result. Every disposition is now handled explicitly. + if (route.disposition === 'first_party_web') { + this._siteManager.setActiveSite(site); + this._navigation.navigate('WebView', { url: route.url }); + return; + } if (route.disposition === 'privileged_external') { Linking.openURL(route.url).catch(() => {}); + return; } + // 'rejected' is a deliberate denial: off-origin, unauthenticated, a + // non-staff admin path, or an unrecognised destination. Nothing opens. + securityEvent('navigation.rejected'); } // A member must never be trapped behind an identity they did not choose in diff --git a/js/__tests__/notificationRouting.test.js b/js/__tests__/notificationRouting.test.js new file mode 100644 index 000000000..05a1ad7cb --- /dev/null +++ b/js/__tests__/notificationRouting.test.js @@ -0,0 +1,267 @@ +import DiscourseUtils from '../DiscourseUtils'; +import { + classifyFirstPartyMemberRoute, + isFirstPartyWebPath, +} from '../nativeMemberRouting'; + +const ORIGIN = 'https://adjusternetwork.org'; +const site = { url: ORIGIN, username: 'tomrodriguez', isStaff: false }; +const member = { authenticated: true, isStaff: false }; +const staff = { authenticated: true, isStaff: true }; + +const routeFor = (notification, opts = member) => + classifyFirstPartyMemberRoute( + DiscourseUtils.endpointForSiteNotification(site, notification), + opts, + ); + +// The real Discourse granted_badge payload: badge_id and username present, +// topic_id and post_number absent (app/services/badge_granter.rb). +const grantedBadge = { + notification_type: 12, + topic_id: null, + post_number: null, + data: { + badge_id: 7, + badge_name: 'Autobiographer', + badge_slug: 'autobiographer', + badge_title: false, + username: 'tomrodriguez', + }, +}; + +describe('granted_badge (Autobiographer) now reaches a destination', () => { + test('produces a valid badge URL', () => { + expect(DiscourseUtils.endpointForSiteNotification(site, grantedBadge)).toBe( + `${ORIGIN}/badges/7/basic?username=tomrodriguez`, + ); + }); + + test('classifies as first_party_web, not rejected', () => { + expect(routeFor(grantedBadge)).toEqual({ + disposition: 'first_party_web', + url: `${ORIGIN}/badges/7/basic?username=tomrodriguez`, + }); + }); + + test('nil topic_id and post_number do not suppress the destination', () => { + expect(grantedBadge.topic_id).toBeNull(); + expect(grantedBadge.post_number).toBeNull(); + expect(routeFor(grantedBadge).disposition).toBe('first_party_web'); + }); +}); + +describe('every previously silent notification class now routes', () => { + test.each([ + [ + 'group_message_summary', + { + notification_type: 16, + data: { username: 'tomrodriguez', group_name: 'staff' }, + }, + '/u/tomrodriguez/messages/group/staff', + ], + [ + 'liked_consolidated', + { notification_type: 19, data: { username: 'someone' } }, + '/u/tomrodriguez/notifications/likes-received?acting_username=someone', + ], + [ + 'membership_request_accepted', + { notification_type: 22, data: { group_name: 'staff' } }, + '/g/staff', + ], + [ + 'chat_mention', + { + notification_type: 29, + data: { + chat_channel_id: 2, + chat_channel_title: 'lounge', + chat_message_id: 9, + }, + }, + '/chat/channel/2/lounge?messageId=9', + ], + [ + 'chat_message', + { + notification_type: 30, + data: { chat_channel_id: 2, chat_channel_title: 'lounge' }, + }, + '/chat/channel/2/lounge', + ], + ])('%s opens first_party_web', (_label, notification, expectedPath) => { + const url = DiscourseUtils.endpointForSiteNotification(site, notification); + expect(url).toBe(`${ORIGIN}${expectedPath}`); + expect(routeFor(notification)).toEqual({ + disposition: 'first_party_web', + url, + }); + }); +}); + +describe('existing native routing is unchanged', () => { + const topicish = { slug: 'a-topic', topic_id: 42, post_number: 3, data: {} }; + + test.each([1, 2, 3, 5, 9, 11, 17, 24, 28, 36, 801, 802])( + 'type %i still opens the native Topic screen', + type => { + const route = routeFor({ notification_type: type, ...topicish }); + expect(route.disposition).toBe('native'); + expect(route.screen).toBe('Topic'); + }, + ); + + test('following and approval notifications still open MemberProfile', () => { + expect( + routeFor({ + notification_type: 800, + data: { display_username: 'someone' }, + }), + ).toMatchObject({ disposition: 'native', screen: 'MemberProfile' }); + expect(routeFor({ notification_type: 21, data: {} })).toMatchObject({ + disposition: 'native', + screen: 'MemberProfile', + }); + }); + + test.each([ + ['/search', 'Search'], + ['/u/tomrodriguez/activity/bookmarks', 'Bookmarks'], + ['/u/tomrodriguez', 'MemberProfile'], + ['/u/tomrodriguez/preferences', 'Settings'], + ['/new-topic', 'Ask'], + ['/c/field-notes/12', 'Collection'], + ['/tag/roofing', 'Collection'], + ])('%s still resolves natively to %s', (path, screen) => { + const route = classifyFirstPartyMemberRoute(`${ORIGIN}${path}`, member); + expect(route.disposition).toBe('native'); + expect(route.screen).toBe(screen); + }); +}); + +describe('security boundaries are preserved', () => { + test('off-origin destinations remain rejected', () => { + for (const url of [ + 'https://evil.example.com/badges/7/basic', + 'https://adjusternetwork.org.evil.example.com/g/staff', + 'https://staging.adjusternetwork.org/chat/channel/2/lounge', + 'http://adjusternetwork.org/badges/7/basic', + ]) { + expect(classifyFirstPartyMemberRoute(url, member)).toEqual({ + disposition: 'rejected', + }); + } + }); + + test('non-staff /admin remains rejected and staff behaviour is unchanged', () => { + expect(routeFor({ notification_type: 37, data: {} })).toEqual({ + disposition: 'rejected', + }); + expect(routeFor({ notification_type: 37, data: {} }, staff)).toEqual({ + disposition: 'privileged_external', + url: `${ORIGIN}/admin`, + }); + }); + + test('unknown or empty notification endpoints remain rejected', () => { + expect(routeFor({ notification_type: 999, data: {} })).toEqual({ + disposition: 'rejected', + }); + expect(classifyFirstPartyMemberRoute(ORIGIN, member)).toEqual({ + disposition: 'rejected', + }); + expect(classifyFirstPartyMemberRoute(`${ORIGIN}/`, member)).toEqual({ + disposition: 'rejected', + }); + }); + + test('a malformed badge payload fails safely rather than opening', () => { + for (const data of [ + {}, + { username: 'tomrodriguez' }, + { badge_id: 'not-a-number', username: 'tomrodriguez' }, + { badge_id: null, username: null }, + ]) { + expect( + routeFor({ + notification_type: 12, + topic_id: null, + post_number: null, + data, + }), + ).toEqual({ disposition: 'rejected' }); + } + }); + + test('unauthenticated callers never route anywhere', () => { + expect( + classifyFirstPartyMemberRoute(`${ORIGIN}/badges/7/basic`, { + authenticated: false, + }), + ).toEqual({ disposition: 'rejected' }); + }); + + test('the allowlist is not a blanket internal fallback', () => { + for (const path of [ + '/latest', + '/site.json', + '/badges', + '/badgesx/7/basic', + '/chat', + '/chat/channel', + '/chat/channel/abc/lounge', + '/gx/staff', + '/u/tomrodriguez/messagesx', + ]) { + expect(isFirstPartyWebPath(path)).toBe(false); + expect( + classifyFirstPartyMemberRoute(`${ORIGIN}${path}`, member).disposition, + ).not.toBe('first_party_web'); + } + }); +}); + +describe('the tap handler marks read before navigating and has no silent path', () => { + const fs = require('fs'); + const path = require('path'); + + test('read-marking precedes destination resolution', () => { + const source = fs.readFileSync( + path.join(__dirname, '..', 'screens', 'NotificationsScreen.js'), + 'utf8', + ); + const handler = source.slice( + source.indexOf('_openNotificationForSite('), + source.indexOf('_listIndex(row)'), + ); + expect(handler.indexOf('markNotificationRead')).toBeLessThan( + handler.indexOf('endpointForSiteNotification'), + ); + expect(handler).toContain('openUrl(url)'); + }); + + test('openUrl handles every disposition explicitly', () => { + const source = fs.readFileSync( + path.join(__dirname, '..', 'Discourse.js'), + 'utf8', + ); + const openUrl = source.slice( + source.indexOf(' openUrl(url) {'), + source.indexOf(' _toggleTheme('), + ); + for (const disposition of [ + 'native', + 'first_party_web', + 'privileged_external', + ]) { + expect(openUrl).toContain(`route.disposition === '${disposition}'`); + } + // The rejected path is explicit, not an implicit fallthrough. + expect(openUrl).toContain("securityEvent('navigation.rejected')"); + expect(openUrl).toContain( + "this._navigation.navigate('WebView', { url: route.url })", + ); + }); +}); diff --git a/js/nativeMemberRouting.js b/js/nativeMemberRouting.js index bc4b3c5a6..1e84819ed 100644 --- a/js/nativeMemberRouting.js +++ b/js/nativeMemberRouting.js @@ -65,6 +65,27 @@ export function nativeCollectionRoute(value, authenticated) { } } +// Canonical-origin member destinations that are valid and intended, but have no +// native screen. They open in the already-registered authenticated Discourse +// WebView rather than being discarded. This is an explicit allowlist, not a +// blanket "anything internal opens" fallback: an unlisted path stays rejected. +const FIRST_PARTY_WEB_PATHS = Object.freeze([ + // granted_badge - /badges/:id/:filter + /^\/badges\/[0-9]+(?:\/[^/]*)?\/?$/i, + // group_message_summary - /u/:username/messages/group/:group + /^\/u\/[a-z0-9_.-]+\/messages(?:\/[^/]+)*\/?$/i, + // liked_consolidated - /u/:username/notifications/likes-received + /^\/u\/[a-z0-9_.-]+\/notifications(?:\/[^/]+)*\/?$/i, + // membership_request_accepted - /g/:group + /^\/g\/[a-z0-9_.-]+\/?$/i, + // chat mention and message - /chat/channel/:id/:slug + /^\/chat\/channel\/[0-9]+(?:\/[^/]*)?\/?$/i, +]); + +export function isFirstPartyWebPath(pathname) { + return FIRST_PARTY_WEB_PATHS.some(pattern => pattern.test(String(pathname))); +} + export function classifyFirstPartyMemberRoute( value, { authenticated = false, isStaff = false } = {}, @@ -120,6 +141,11 @@ export function classifyFirstPartyMemberRoute( ? { disposition: 'privileged_external', url: url.toString() } : { disposition: 'rejected' }; } + // Checked after every native pattern and after the /admin boundary, so it + // can never widen an already-denied destination. + if (isFirstPartyWebPath(url.pathname)) { + return { disposition: 'first_party_web', url: url.toString() }; + } return { disposition: 'rejected' }; } catch { return { disposition: 'rejected' }; From 335419e3175b9b6d4cb2413f8eee53c014895b3e Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Wed, 9 Sep 2026 09:57:50 -0400 Subject: [PATCH 19/20] feat(notifications): bootstrap an authenticated WebView session for member pages (#19) First-party member pages could not open in the app because the WebView carries cookies, not the User API key, and WebViewComponent's navigation policy correctly refuses to load a canonical page into an unauthenticated Discourse session. PR #16 routed there anyway, which produced a blank screen stuck on "Still loading...". The supported contract closes the gap. webViewSession posts the app's existing RSA public key to /user-api-key/otp with the governed auth_redirect and pkcs1 padding, using site.jsonApi so the existing User-Api-Key and User-Api-Client-Id headers, rate-limit buckets and cooldowns all apply. The returned redirect_url is parsed with the same helper the authorization callback uses and the one-time password is decrypted with the same JSEncrypt private key. No second cryptographic implementation, and no new server endpoint. The WebView then loads /session/otp/, the member completes the existing confirmation form, and only then is the originally requested destination loaded. The confirmation step is never bypassed: it is what sets the session cookie. An existing session is reused rather than spending an OTP: a live Discourse _t cookie means the destination loads directly. The cookie package is required lazily so importing this module does not pull a native dependency into every suite that reaches Discourse.js. The WebView policy relaxation is scoped to a bootstrap the app itself started. Authorization requires a pending destination, and the window closes the moment that destination loads, so this is not a standing "any internal page opens" rule. The original guard is untouched for everything else. Safety holds throughout. The decrypted OTP must match the route's hex constraint before it is interpolated into a path. Off-origin destinations are refused before an OTP is minted. Non-staff /admin, malformed payloads, unknown types and unauthenticated callers remain denied. Any failure or cancellation ends in a bounded explicit state rather than a blank WebView. Native Topic and MemberProfile routing, notification read-marking and the private member-photo credential boundary are unchanged. Claude-Session: https://claude.ai/code/session_01Fk48MTrNBBSZeLvcJmc8SR Co-authored-by: Claude Opus 5 (1M context) --- js/Discourse.js | 62 ++- js/__tests__/notificationRouting.test.js | 22 +- js/__tests__/webViewSession.test.js | 380 ++++++++++++++++++ js/notificationDestination.js | 31 ++ js/screens/WebViewScreen.js | 1 + .../WebViewComponent.js | 32 ++ js/webViewSession.js | 108 +++++ 7 files changed, 611 insertions(+), 25 deletions(-) create mode 100644 js/__tests__/webViewSession.test.js create mode 100644 js/notificationDestination.js create mode 100644 js/webViewSession.js diff --git a/js/Discourse.js b/js/Discourse.js index 6da48f6b0..e3ef0ce37 100644 --- a/js/Discourse.js +++ b/js/Discourse.js @@ -102,6 +102,11 @@ import NativeTopicScreen from './product/NativeTopicScreen'; import NativeCollectionScreen from './product/NativeCollectionScreen'; import NativeProfileScreen from './product/NativeProfileScreen'; import { classifyFirstPartyMemberRoute } from './nativeMemberRouting'; +import { + WEB_SESSION_UNAVAILABLE, + destinationPresentation, +} from './notificationDestination'; +import { resolveWebSessionEntry } from './webViewSession'; import { consumePendingShareIntent } from './shareIntentCoordinator'; import { loadOnboardingState, @@ -970,34 +975,61 @@ class Discourse extends React.Component { authenticated: Boolean(site), isStaff: Boolean(site?.isStaff), }); - if (route.disposition === 'native') { + const presentation = destinationPresentation(route); + if (presentation.kind === 'native') { this._siteManager.setActiveSite(site); - if (route.screen === 'Ask') { + if (presentation.screen === 'Ask') { this._navigation.navigate('HomeWrapper', { screen: 'Ask' }); } else { - this._navigation.navigate(route.screen, route.params); + this._navigation.navigate(presentation.screen, presentation.params); } return; } - // A valid first-party member destination with no native screen opens in the - // authenticated Discourse WebView. Without this branch such destinations - // fell through and the tap did nothing at all: notification read-marking - // had already succeeded, so a granted_badge notification went read with no - // visible result. Every disposition is now handled explicitly. - if (route.disposition === 'first_party_web') { - this._siteManager.setActiveSite(site); - this._navigation.navigate('WebView', { url: route.url }); + if (presentation.kind === 'web') { + this._openFirstPartyWeb(site, presentation.url); return; } - if (route.disposition === 'privileged_external') { - Linking.openURL(route.url).catch(() => {}); + if (presentation.kind === 'external') { + Linking.openURL(presentation.url).catch(() => {}); return; } - // 'rejected' is a deliberate denial: off-origin, unauthenticated, a - // non-staff admin path, or an unrecognised destination. Nothing opens. + // Denied: off-origin, unauthenticated, a non-staff admin path, or an + // unrecognised destination. Nothing opens and nothing loads. securityEvent('navigation.rejected'); } + // A first-party member page needs a Discourse session cookie, which the + // WebView does not get from the User API key. Bootstrap one through the + // supported OTP contract when it is missing, then land on the destination. + // Any failure or cancellation ends in a bounded, explicit state rather than + // a blank WebView. + async _openFirstPartyWeb(site, destination) { + try { + this._siteManager.setActiveSite(site); + const entry = await resolveWebSessionEntry( + site, + this._siteManager, + destination, + ); + securityEvent( + entry.destination + ? 'navigation.web_session_bootstrap' + : 'navigation.web_session_reused', + ); + this._navigation.navigate('WebView', { + url: entry.url, + destination: entry.destination, + }); + } catch { + securityEvent('navigation.web_session_unavailable'); + Alert.alert( + WEB_SESSION_UNAVAILABLE.title, + WEB_SESSION_UNAVAILABLE.message, + [{ text: WEB_SESSION_UNAVAILABLE.close, style: 'cancel' }], + ); + } + } + // A member must never be trapped behind an identity they did not choose in // this attempt. Retire every client-side identity carrier, then start a // normal authorization. This does not depend on the browser honouring an diff --git a/js/__tests__/notificationRouting.test.js b/js/__tests__/notificationRouting.test.js index 05a1ad7cb..fcea64e80 100644 --- a/js/__tests__/notificationRouting.test.js +++ b/js/__tests__/notificationRouting.test.js @@ -242,26 +242,28 @@ describe('the tap handler marks read before navigating and has no silent path', expect(handler).toContain('openUrl(url)'); }); - test('openUrl handles every disposition explicitly', () => { + test('openUrl handles every presentation explicitly', () => { const source = fs.readFileSync( path.join(__dirname, '..', 'Discourse.js'), 'utf8', ); const openUrl = source.slice( source.indexOf(' openUrl(url) {'), - source.indexOf(' _toggleTheme('), + source.indexOf(' async _openFirstPartyWeb('), ); - for (const disposition of [ - 'native', - 'first_party_web', - 'privileged_external', - ]) { - expect(openUrl).toContain(`route.disposition === '${disposition}'`); + // Dispositions are mapped by the pure destinationPresentation module and + // openUrl branches on the resulting kind. Every kind is handled. + expect(openUrl).toContain('destinationPresentation(route)'); + for (const kind of ['native', 'web', 'external']) { + expect(openUrl).toContain(`presentation.kind === '${kind}'`); } - // The rejected path is explicit, not an implicit fallthrough. + // The denied path is explicit, not an implicit fallthrough. expect(openUrl).toContain("securityEvent('navigation.rejected')"); + // A first-party web destination is never loaded without first resolving + // an authenticated Discourse session. + expect(openUrl).not.toContain("navigate('WebView'"); expect(openUrl).toContain( - "this._navigation.navigate('WebView', { url: route.url })", + 'this._openFirstPartyWeb(site, presentation.url)', ); }); }); diff --git a/js/__tests__/webViewSession.test.js b/js/__tests__/webViewSession.test.js new file mode 100644 index 000000000..05f338117 --- /dev/null +++ b/js/__tests__/webViewSession.test.js @@ -0,0 +1,380 @@ +jest.mock('@react-native-cookies/cookies', () => ({ get: jest.fn() })); + +import CookieManager from '@react-native-cookies/cookies'; +import DiscourseUtils from '../DiscourseUtils'; +import { classifyFirstPartyMemberRoute } from '../nativeMemberRouting'; +import { + WEB_SESSION_UNAVAILABLE, + destinationPresentation, +} from '../notificationDestination'; +import { + OTP_ENDPOINT, + hasAuthenticatedWebSession, + isOtpBootstrapUrl, + otpBootstrapUrl, + requestOneTimePassword, + resolveWebSessionEntry, +} from '../webViewSession'; + +const ORIGIN = 'https://adjusternetwork.org'; +const OTP = 'a1b2c3d4e5f6'; +const BADGE = `${ORIGIN}/badges/9/basic?username=tomrodriguez`; + +const makeSite = (overrides = {}) => ({ + url: ORIGIN, + username: 'tomrodriguez', + authToken: 'user-api-key', + clientId: 'client-A', + jsonApi: jest.fn(), + ...overrides, +}); + +const makeManager = (overrides = {}) => ({ + ensureRSAKeys: jest.fn(() => Promise.resolve()), + rsaKeys: { public: 'PUBLIC-KEY', private: 'PRIVATE-KEY' }, + decryptHelper: jest.fn(() => OTP), + ...overrides, +}); + +beforeEach(() => { + jest.clearAllMocks(); + CookieManager.get.mockResolvedValue({}); +}); + +describe('OTP request uses the existing credentials and crypto', () => { + test('posts the app public key, governed redirect and pkcs1 padding', async () => { + const site = makeSite(); + const manager = makeManager(); + site.jsonApi.mockResolvedValue({ + redirect_url: `adjusternetwork://adjusternetwork.org/auth_redirect?oneTimePassword=ENCRYPTED`, + }); + + await expect(requestOneTimePassword(site, manager)).resolves.toBe(OTP); + + // Reuses site.jsonApi, so User-Api-Key / User-Api-Client-Id headers and + // the rate-limit buckets apply unchanged. + expect(site.jsonApi).toHaveBeenCalledWith(OTP_ENDPOINT, 'POST', { + public_key: 'PUBLIC-KEY', + auth_redirect: 'adjusternetwork://adjusternetwork.org/auth_redirect', + padding: 'pkcs1', + }); + // Same RSA machinery as the authorization flow; no second implementation. + expect(manager.ensureRSAKeys).toHaveBeenCalled(); + expect(manager.decryptHelper).toHaveBeenCalledWith('ENCRYPTED'); + }); + + test('an unauthenticated site never requests an OTP', async () => { + const site = makeSite({ authToken: null }); + await expect(requestOneTimePassword(site, makeManager())).rejects.toThrow( + 'web_session_unauthenticated', + ); + expect(site.jsonApi).not.toHaveBeenCalled(); + }); + + test('a missing RSA public key fails before any request', async () => { + const site = makeSite(); + await expect( + requestOneTimePassword(site, makeManager({ rsaKeys: {} })), + ).rejects.toThrow('web_session_key_unavailable'); + expect(site.jsonApi).not.toHaveBeenCalled(); + }); + + test('a response without an OTP fails closed', async () => { + const site = makeSite(); + for (const redirect_url of [ + undefined, + '', + 'adjusternetwork://adjusternetwork.org/auth_redirect', + 'https://evil.example.com/?oneTimePassword=X', + ]) { + site.jsonApi.mockResolvedValue({ redirect_url }); + await expect(requestOneTimePassword(site, makeManager())).rejects.toThrow( + 'web_session_otp_missing', + ); + } + }); + + test('a non-hex decrypted OTP is refused so nothing is injected into the path', async () => { + const site = makeSite(); + site.jsonApi.mockResolvedValue({ + redirect_url: `adjusternetwork://adjusternetwork.org/auth_redirect?oneTimePassword=E`, + }); + for (const bad of ['../../admin', 'abc/def', 'ZZZZ', '', null]) { + await expect( + requestOneTimePassword(site, makeManager({ decryptHelper: () => bad })), + ).rejects.toThrow('web_session_otp_invalid'); + } + }); + + test('a rate-limited or failing OTP request propagates', async () => { + const site = makeSite(); + site.jsonApi.mockRejectedValue( + Object.assign(new Error('api_rate_limited'), { status: 429 }), + ); + await expect(requestOneTimePassword(site, makeManager())).rejects.toThrow( + 'api_rate_limited', + ); + }); +}); + +describe('bootstrap URL construction is constrained', () => { + test('builds the confirmation route for a hex token only', () => { + expect(otpBootstrapUrl({ url: ORIGIN }, OTP)).toBe( + `${ORIGIN}/session/otp/${OTP}`, + ); + for (const bad of ['../admin', 'a/b', 'ZZ', '', null, undefined]) { + expect(otpBootstrapUrl({ url: ORIGIN }, bad)).toBeNull(); + } + expect(otpBootstrapUrl(null, OTP)).toBeNull(); + }); + + test('recognises only canonical-origin HTTPS bootstrap URLs', () => { + expect(isOtpBootstrapUrl(`${ORIGIN}/session/otp/${OTP}`)).toBe(true); + for (const bad of [ + `${ORIGIN}/badges/9/basic`, + `https://evil.example.com/session/otp/${OTP}`, + `http://adjusternetwork.org/session/otp/${OTP}`, + `https://adjusternetwork.org.evil.example.com/session/otp/${OTP}`, + 'not-a-url', + null, + ]) { + expect(isOtpBootstrapUrl(bad)).toBe(false); + } + }); +}); + +describe('an existing session is reused rather than minting another OTP', () => { + test('a live auth cookie skips the bootstrap entirely', async () => { + CookieManager.get.mockResolvedValue({ _t: { value: 'session-token' } }); + const site = makeSite(); + await expect(hasAuthenticatedWebSession(site, CookieManager)).resolves.toBe( + true, + ); + + await expect( + resolveWebSessionEntry(site, makeManager(), BADGE), + ).resolves.toEqual({ url: BADGE, destination: null }); + expect(site.jsonApi).not.toHaveBeenCalled(); + }); + + test('an absent or empty cookie bootstraps and remembers the destination', async () => { + for (const jar of [{}, { _t: {} }, { _t: { value: '' } }, null]) { + CookieManager.get.mockResolvedValue(jar); + const site = makeSite(); + site.jsonApi.mockResolvedValue({ + redirect_url: `adjusternetwork://adjusternetwork.org/auth_redirect?oneTimePassword=E`, + }); + await expect( + resolveWebSessionEntry(site, makeManager(), BADGE), + ).resolves.toEqual({ + url: `${ORIGIN}/session/otp/${OTP}`, + destination: BADGE, + }); + expect(site.jsonApi).toHaveBeenCalledTimes(1); + } + }); + + test('an unreadable cookie jar bootstraps rather than assuming a session', async () => { + CookieManager.get.mockRejectedValue(new Error('cookie failure')); + await expect( + hasAuthenticatedWebSession(makeSite(), CookieManager), + ).resolves.toBe(false); + }); + + test('an off-origin destination is refused before any OTP is minted', async () => { + const site = makeSite(); + for (const bad of [ + 'https://evil.example.com/badges/9/basic', + 'http://adjusternetwork.org/badges/9/basic', + null, + ]) { + await expect( + resolveWebSessionEntry(site, makeManager(), bad), + ).rejects.toThrow('web_session_destination'); + } + expect(site.jsonApi).not.toHaveBeenCalled(); + }); +}); + +describe('destination presentation', () => { + const site = { url: ORIGIN, username: 'tomrodriguez' }; + const member = { authenticated: true, isStaff: false }; + const present = (n, o = member) => + destinationPresentation( + classifyFirstPartyMemberRoute( + DiscourseUtils.endpointForSiteNotification(site, n), + o, + ), + ); + + test('granted_badge presents a web destination', () => { + expect( + present({ + notification_type: 12, + topic_id: null, + post_number: null, + data: { badge_id: 9, username: 'tomrodriguez' }, + }), + ).toEqual({ kind: 'web', url: BADGE }); + }); + + test.each([ + [ + 'group_message_summary', + { + notification_type: 16, + data: { username: 'tomrodriguez', group_name: 'staff' }, + }, + ], + [ + 'liked_consolidated', + { notification_type: 19, data: { username: 'someone' } }, + ], + [ + 'membership_request_accepted', + { notification_type: 22, data: { group_name: 'staff' } }, + ], + [ + 'chat_mention', + { + notification_type: 29, + data: { + chat_channel_id: 2, + chat_channel_title: 'lounge', + chat_message_id: 9, + }, + }, + ], + [ + 'chat_message', + { + notification_type: 30, + data: { chat_channel_id: 2, chat_channel_title: 'lounge' }, + }, + ], + ])('%s presents a web destination', (_l, n) => { + expect(present(n).kind).toBe('web'); + }); + + test('native notification routing is unchanged', () => { + expect( + present({ + notification_type: 2, + slug: 't', + topic_id: 4, + post_number: 1, + data: {}, + }), + ).toMatchObject({ kind: 'native', screen: 'Topic' }); + expect( + present({ notification_type: 800, data: { display_username: 'x' } }), + ).toMatchObject({ kind: 'native', screen: 'MemberProfile' }); + }); + + test('denied destinations stay denied', () => { + expect(present({ notification_type: 37, data: {} })).toEqual({ + kind: 'denied', + }); + expect(present({ notification_type: 999, data: {} })).toEqual({ + kind: 'denied', + }); + expect( + present({ + notification_type: 12, + data: { badge_id: 'abc', username: 'x' }, + }), + ).toEqual({ kind: 'denied' }); + expect( + destinationPresentation( + classifyFirstPartyMemberRoute(BADGE, { authenticated: false }), + ), + ).toEqual({ kind: 'denied' }); + expect(destinationPresentation(null)).toEqual({ kind: 'denied' }); + }); + + test('staff admin still hands off externally', () => { + expect( + present( + { notification_type: 37, data: {} }, + { authenticated: true, isStaff: true }, + ), + ).toEqual({ kind: 'external', url: `${ORIGIN}/admin` }); + }); +}); + +describe('failure is bounded and explicit', () => { + test('the failure copy promises no loading and no login', () => { + expect(WEB_SESSION_UNAVAILABLE.close).toBe('Close'); + expect(WEB_SESSION_UNAVAILABLE.message).toMatch(/marked as read/i); + expect(WEB_SESSION_UNAVAILABLE.message).not.toMatch( + /log ?in|sign ?in|browser|Safari/i, + ); + }); +}); + +describe('wiring', () => { + const fs = require('fs'); + const path = require('path'); + const read = f => fs.readFileSync(path.join(__dirname, '..', f), 'utf8'); + + test('openUrl delegates web destinations and bounds failure', () => { + const source = read('Discourse.js'); + const openUrl = source.slice( + source.indexOf(' openUrl(url) {'), + source.indexOf(' async _openFirstPartyWeb('), + ); + expect(openUrl).toContain("presentation.kind === 'web'"); + expect(openUrl).toContain( + 'this._openFirstPartyWeb(site, presentation.url)', + ); + expect(openUrl).toContain("securityEvent('navigation.rejected')"); + // openUrl itself never opens the WebView: a web destination must go + // through session resolution first. + expect(openUrl).not.toContain("navigate('WebView'"); + + const handler = source.slice( + source.indexOf(' async _openFirstPartyWeb('), + source.indexOf(' _toggleTheme('), + ); + // The WebView is reached only after the session entry resolves, and any + // failure ends in the bounded explicit state. + expect(handler.indexOf('resolveWebSessionEntry(')).toBeLessThan( + handler.indexOf("navigate('WebView'"), + ); + expect(handler).toContain( + "securityEvent('navigation.web_session_unavailable')", + ); + expect(handler).toContain('WEB_SESSION_UNAVAILABLE.title'); + }); + + test('the WebView policy relaxation is bootstrap-scoped, not standing', () => { + const source = read('screens/WebViewScreenComponents/WebViewComponent.js'); + // The original guard survives. + expect(source).toContain( + '// Canonical pages without an explicit native route must not', + ); + expect(source).toContain('_isAuthorizedSessionNavigation(request.url)'); + // Authorization requires an app-initiated bootstrap. + expect(source).toContain( + 'isOtpBootstrapUrl(url) && Boolean(this.props.destination)', + ); + // The window closes once the destination loads. + expect(source).toContain( + 'pendingDestination: null, webviewUrl: destination', + ); + expect(read('screens/WebViewScreen.js')).toContain( + 'destination={this.props.route.params.destination}', + ); + }); + + test('read-marking still precedes destination resolution', () => { + const handler = read('screens/NotificationsScreen.js'); + const block = handler.slice( + handler.indexOf('_openNotificationForSite('), + handler.indexOf('_listIndex(row)'), + ); + expect(block.indexOf('markNotificationRead')).toBeLessThan( + block.indexOf('endpointForSiteNotification'), + ); + }); +}); diff --git a/js/notificationDestination.js b/js/notificationDestination.js new file mode 100644 index 000000000..0541188c4 --- /dev/null +++ b/js/notificationDestination.js @@ -0,0 +1,31 @@ +/* @flow */ +'use strict'; + +// Presentation decision for a classified member destination. Kept pure and +// separate from Discourse.js so every branch is directly testable. +// +// A 'first_party_web' destination is a valid first-party member page with no +// native screen. It is opened in the in-app WebView, but only after an +// authenticated Discourse session has been bootstrapped - see webViewSession. +// Loading it without one would show a login wall, which is exactly what +// WebViewComponent's navigation policy exists to prevent. +export const WEB_SESSION_UNAVAILABLE = Object.freeze({ + title: 'Not available right now', + message: + 'Adjuster Network could not open this page in the app. It has been marked as read, and nothing else is affected. Try again later.', + close: 'Close', +}); + +export function destinationPresentation(route) { + switch (route?.disposition) { + case 'native': + return { kind: 'native', screen: route.screen, params: route.params }; + case 'first_party_web': + return { kind: 'web', url: route.url }; + case 'privileged_external': + return { kind: 'external', url: route.url }; + default: + // Off-origin, unauthenticated, non-staff /admin, malformed or unknown. + return { kind: 'denied' }; + } +} diff --git a/js/screens/WebViewScreen.js b/js/screens/WebViewScreen.js index f0fb10b7f..ce5f3a9b4 100644 --- a/js/screens/WebViewScreen.js +++ b/js/screens/WebViewScreen.js @@ -14,6 +14,7 @@ class WebViewScreen extends React.Component { ); } diff --git a/js/screens/WebViewScreenComponents/WebViewComponent.js b/js/screens/WebViewScreenComponents/WebViewComponent.js index edb8f2d00..5f0f3e906 100644 --- a/js/screens/WebViewScreenComponents/WebViewComponent.js +++ b/js/screens/WebViewScreenComponents/WebViewComponent.js @@ -25,6 +25,7 @@ import { ThemeContext } from '../../ThemeContext'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { BlurView } from '@react-native-community/blur'; import { classifyNavigation } from '../../adjusterNetworkSecurity'; +import { isOtpBootstrapUrl } from '../../webViewSession'; import { NestedHeader } from '../../product/ProductComponents'; import { classifyFirstPartyMemberRoute } from '../../nativeMemberRouting'; @@ -82,6 +83,9 @@ class WebViewComponent extends React.Component { webviewUrl: this.props.url, authProcessActive: false, scrollOverflow: 0, + // Set only when the app itself initiated an OTP session bootstrap and + // still owes the member the page they actually asked for. + pendingDestination: this.props.destination || null, }; } @@ -290,6 +294,14 @@ class WebViewComponent extends React.Component { this.props.screenProps.openUrl(request.url); return false; } + // A session bootstrap the app itself started is allowed to + // run: the OTP confirmation page, the redirect it performs, + // and finally the destination that was requested. The window + // is closed as soon as the destination loads, so this is not a + // standing "any internal page opens" relaxation. + if (this._isAuthorizedSessionNavigation(request.url)) { + return true; + } // Canonical pages without an explicit native route must not // fall through to an unauthenticated Discourse/PWA session. return false; @@ -319,6 +331,7 @@ class WebViewComponent extends React.Component { }} onNavigationStateChange={navState => { this._storeLastPath(navState); + this._advanceSessionBootstrap(navState); }} decelerationRate={'normal'} onLoadProgress={({ nativeEvent }) => { @@ -465,6 +478,25 @@ class WebViewComponent extends React.Component { } } + // Authorized only while an app-initiated bootstrap is outstanding. + _isAuthorizedSessionNavigation(url) { + if (!this.state.pendingDestination) { + return isOtpBootstrapUrl(url) && Boolean(this.props.destination); + } + return true; + } + + // The confirmation form posts back to /session/otp/ and then Discourse + // redirects. When navigation has left the bootstrap path the session cookie + // exists, so the originally requested page is loaded exactly once and the + // authorization window closes. + _advanceSessionBootstrap(navState) { + const destination = this.state.pendingDestination; + if (!destination || navState.loading) return; + if (isOtpBootstrapUrl(navState.url)) return; + this.setState({ pendingDestination: null, webviewUrl: destination }); + } + _onMessage(event) { let data; try { diff --git a/js/webViewSession.js b/js/webViewSession.js new file mode 100644 index 000000000..42cd4fa46 --- /dev/null +++ b/js/webViewSession.js @@ -0,0 +1,108 @@ +/* @flow */ +'use strict'; + +import { AUTH_REDIRECT } from './authorizationConsent'; +import { isCanonicalUrl } from './adjusterNetworkSecurity'; +import { parseAuthCallbackParameters } from './authCallback'; + +// Bootstrapping an authenticated Discourse browser session for the in-app +// WebView. The WebView carries cookies, not the User API key, so a first-party +// member page cannot be opened until a session cookie exists. +// +// The supported contract: POST /user-api-key/otp with the app's existing User +// API credentials returns a redirect_url carrying an RSA-encrypted one-time +// password. The app decrypts it with the same private key used by the +// authorization flow and loads /session/otp/, where the member completes +// the existing confirmation form. That form - never bypassed - is what sets the +// session cookie. +// +// The OTP is single-use with a 10 minute TTL, and the server refuses User API +// keys for suspended or inactive users, so an unauthorised member cannot reach +// a session this way. +export const OTP_BOOTSTRAP_PATH = '/session/otp/'; +export const OTP_ENDPOINT = '/user-api-key/otp'; + +// Discourse's authentication cookie. Its presence means the WebView already +// holds a logged-in session and no OTP needs to be minted. +const AUTH_COOKIE = '_t'; + +// The route constrains the token to hex, so anything else must never be +// interpolated into the path. +const OTP_TOKEN = /^[0-9a-f]+$/; + +export function otpBootstrapUrl(site, otp) { + if (!site?.url || typeof otp !== 'string' || !OTP_TOKEN.test(otp)) { + return null; + } + return `${site.url}${OTP_BOOTSTRAP_PATH}${otp}`; +} + +export function isOtpBootstrapUrl(value) { + if (!isCanonicalUrl(value)) return false; + try { + return new URL(value).pathname.startsWith(OTP_BOOTSTRAP_PATH); + } catch { + return false; + } +} + +export async function hasAuthenticatedWebSession(site, cookies = null) { + if (!site?.url) return false; + try { + // Required lazily so importing this module never pulls in the native + // cookie package. Suites that merely reach Discourse.js must not have to + // mock it, and nothing else in this module needs it. + const jar = await (cookies || require('@react-native-cookies/cookies')).get( + site.url, + true, + ); + const token = jar?.[AUTH_COOKIE]; + return Boolean(token && token.value); + } catch { + // An unreadable cookie jar is treated as no session: the worst outcome is + // minting one extra single-use OTP. + return false; + } +} + +export async function requestOneTimePassword(site, siteManager) { + if (!site?.authToken) throw new Error('web_session_unauthenticated'); + await siteManager.ensureRSAKeys(); + const publicKey = siteManager.rsaKeys?.public; + if (!publicKey) throw new Error('web_session_key_unavailable'); + + // Reuses site.jsonApi, so the existing User-Api-Key and User-Api-Client-Id + // headers, rate-limit buckets and cooldowns all apply unchanged. + const payload = await site.jsonApi(OTP_ENDPOINT, 'POST', { + public_key: publicKey, + auth_redirect: AUTH_REDIRECT, + padding: 'pkcs1', + }); + + const encrypted = parseAuthCallbackParameters( + payload?.redirect_url, + ).oneTimePassword; + if (!encrypted) throw new Error('web_session_otp_missing'); + + // Same JSEncrypt private key as the authorization flow; no second + // cryptographic implementation. + const otp = siteManager.decryptHelper(encrypted); + if (typeof otp !== 'string' || !OTP_TOKEN.test(otp)) { + throw new Error('web_session_otp_invalid'); + } + return otp; +} + +// Resolves what the WebView should load for a first-party destination: the +// destination directly when a session already exists, otherwise a bootstrap +// that remembers where to go afterwards. +export async function resolveWebSessionEntry(site, siteManager, destination) { + if (!isCanonicalUrl(destination)) throw new Error('web_session_destination'); + if (await hasAuthenticatedWebSession(site)) { + return { url: destination, destination: null }; + } + const otp = await requestOneTimePassword(site, siteManager); + const bootstrap = otpBootstrapUrl(site, otp); + if (!bootstrap) throw new Error('web_session_otp_invalid'); + return { url: bootstrap, destination }; +} From cf588cde1a807bb9f0af4ea61fa7dc8a16182a80 Mon Sep 17 00:00:00 2001 From: Aalv3 <120076079+Aalv3@users.noreply.github.com> Date: Wed, 9 Sep 2026 11:42:59 -0400 Subject: [PATCH 20/20] feat(notifications): resolve taps to native intents instead of web sessions (#22) Physically certified on iPhone against OTA SHA ab581ac0 / group 87d6b1d1-a918-478c-afc9-0cbb27ce6292. Autobiographer PASS and Basic PASS: native Badge Earned screen, correct badge name, no WebView, no OTP or login, no hung state, no external browser, close/back works. Topic/Reply/Mention physical regression was not exercised because no notification fixture existed; automated regression coverage is accepted for that path. Shipped SHA preserved by the immutable provenance tag ota-87d6b1d1-ab581ac0. --- docs/NATIVE-NOTIFICATION-INTENTS.md | 64 +++ js/Discourse.js | 93 +++-- js/__tests__/memberRouteBoundaries.test.js | 100 +++++ js/__tests__/notificationIntent.test.js | 364 +++++++++++++++++ js/__tests__/notificationRouting.test.js | 269 ------------- js/__tests__/webViewSession.test.js | 380 ------------------ js/nativeMemberRouting.js | 26 -- js/notificationDestination.js | 17 +- js/notificationIntent.js | 53 +++ js/product/BadgeEarnedScreen.js | 52 +++ js/screens/NotificationsScreen.js | 6 +- js/screens/WebViewScreen.js | 1 - .../WebViewComponent.js | 32 -- js/webViewSession.js | 108 ----- testing/native-auth-stale-identity/BACKLOG.md | 49 ++- 15 files changed, 744 insertions(+), 870 deletions(-) create mode 100644 docs/NATIVE-NOTIFICATION-INTENTS.md create mode 100644 js/__tests__/memberRouteBoundaries.test.js create mode 100644 js/__tests__/notificationIntent.test.js delete mode 100644 js/__tests__/notificationRouting.test.js delete mode 100644 js/__tests__/webViewSession.test.js create mode 100644 js/notificationIntent.js create mode 100644 js/product/BadgeEarnedScreen.js delete mode 100644 js/webViewSession.js diff --git a/docs/NATIVE-NOTIFICATION-INTENTS.md b/docs/NATIVE-NOTIFICATION-INTENTS.md new file mode 100644 index 000000000..1dcf5a9ec --- /dev/null +++ b/docs/NATIVE-NOTIFICATION-INTENTS.md @@ -0,0 +1,64 @@ +# Native notification intents + +A notification tap resolves to an **intent**, computed from the notification +payload before any URL is built. Four kinds exist: + +| Kind | Meaning | +| --- | --- | +| `native` | an existing native screen (Topic, MemberProfile, Collection, Search, Bookmarks, Settings, Ask) | +| `badge` | `granted_badge`, presented by the native BadgeEarned screen | +| `staff_external` | staff-only `/admin`, the single documented external handoff | +| `unavailable` | one explicit bounded state for everything else | + +## Why the payload, not the URL + +`DiscourseUtils.endpointForSiteNotification` is lossy. A `granted_badge` +becomes `/badges/:id/basic?username=:u`, and `badge_name` is discarded. Every +earlier attempt to route badges started from that URL, which is why they all +ended at a web page: by then the information needed to build a native screen +was already gone. `notificationIntent` reads the payload first. + +## Type matrix + +**Native.** 1-11, 13, 14, 15, 17, 18, 20, 24, 25, 27, 28, 34, 36, 801, 802 open +Topic. 800 opens MemberProfile. 21 opens Topic when it carries one, and +otherwise resolves to `/u/:me/activity/approval-given`, which the profile +pattern already matches - a valid native destination, left as it was. + +**Badge.** 12 opens BadgeEarned. + +**Staff external.** 37 and 38 open `/admin` externally, for staff only. +`classifyFirstPartyMemberRoute` returns `privileged_external` only when +`isStaff` is true; a member gets `unavailable`. This is an explicit, documented +exception and does not widen external navigation for anyone else. + +**Unavailable.** 16, 19, 22, 23, 26, 29, 30, 31, 32, unknown and absent types. +No silent no-op, no WebView, no external browser, no second authentication. + +## BadgeEarned + +Renders `badge_name` only, from the payload, with a Close action. It makes **no +network request**, so it cannot stall or fail. + +`badge_title` is a boolean in Discourse - whether the badge may be worn as a +title - not descriptive text, and is never rendered. The payload carries no +badge description; showing one would require a fetch, which V1 does not do. + +## What was removed, and why + +A first-party WebView fallback and then an OTP-based authenticated WebView +session were both tried and abandoned. The OTP contract itself was eventually +correct - the request succeeded and the confirmation form rendered - but +Finish Login failed with "Missing, invalid or expired token", and more +importantly the architecture was wrong: a member already authenticated in the +app should never perform a second web authentication to read a notification. + +Removed with it: `js/webViewSession.js`, the `first_party_web` disposition and +its path allowlist, the WebView destination-bootstrap wiring, and the +`web_session` diagnostic stages. The strict WebView navigation guard, which had +been relaxed to admit the bootstrap, is restored. + +One instrumentation lesson is worth keeping: the abandoned diagnostics recorded +`destination_resume: succeeded` for a flow that had actually failed, because +the stage measured navigation mechanics rather than whether a session existed. +A success signal must observe the thing it claims to prove. diff --git a/js/Discourse.js b/js/Discourse.js index e3ef0ce37..301e1da9e 100644 --- a/js/Discourse.js +++ b/js/Discourse.js @@ -101,12 +101,13 @@ import { import NativeTopicScreen from './product/NativeTopicScreen'; import NativeCollectionScreen from './product/NativeCollectionScreen'; import NativeProfileScreen from './product/NativeProfileScreen'; +import BadgeEarnedScreen from './product/BadgeEarnedScreen'; import { classifyFirstPartyMemberRoute } from './nativeMemberRouting'; +import { notificationIntent } from './notificationIntent'; import { - WEB_SESSION_UNAVAILABLE, + NOTIFICATION_UNAVAILABLE, destinationPresentation, } from './notificationDestination'; -import { resolveWebSessionEntry } from './webViewSession'; import { consumePendingShareIntent } from './shareIntentCoordinator'; import { loadOnboardingState, @@ -396,6 +397,7 @@ class Discourse extends React.Component { authenticated: Boolean(site?.authToken), navigationReady, openUrl: this.openUrl.bind(this), + openNotification: this.openNotification.bind(this), }); if (!routed && this._pushRoute.path) { securityEvent('push.route.deferred'); @@ -563,6 +565,7 @@ class Discourse extends React.Component { navigationReady: this._navigationReady, nativeModule: DiscourseKeyboardShortcuts, openUrl: this.openUrl.bind(this), + openNotification: this.openNotification.bind(this), }).finally(() => { this._shareIntentConsumption = null; }); @@ -978,15 +981,7 @@ class Discourse extends React.Component { const presentation = destinationPresentation(route); if (presentation.kind === 'native') { this._siteManager.setActiveSite(site); - if (presentation.screen === 'Ask') { - this._navigation.navigate('HomeWrapper', { screen: 'Ask' }); - } else { - this._navigation.navigate(presentation.screen, presentation.params); - } - return; - } - if (presentation.kind === 'web') { - this._openFirstPartyWeb(site, presentation.url); + this._navigateNative(presentation.screen, presentation.params); return; } if (presentation.kind === 'external') { @@ -998,35 +993,44 @@ class Discourse extends React.Component { securityEvent('navigation.rejected'); } - // A first-party member page needs a Discourse session cookie, which the - // WebView does not get from the User API key. Bootstrap one through the - // supported OTP contract when it is missing, then land on the destination. - // Any failure or cancellation ends in a bounded, explicit state rather than - // a blank WebView. - async _openFirstPartyWeb(site, destination) { - try { - this._siteManager.setActiveSite(site); - const entry = await resolveWebSessionEntry( - site, - this._siteManager, - destination, - ); - securityEvent( - entry.destination - ? 'navigation.web_session_bootstrap' - : 'navigation.web_session_reused', - ); - this._navigation.navigate('WebView', { - url: entry.url, - destination: entry.destination, - }); - } catch { - securityEvent('navigation.web_session_unavailable'); - Alert.alert( - WEB_SESSION_UNAVAILABLE.title, - WEB_SESSION_UNAVAILABLE.message, - [{ text: WEB_SESSION_UNAVAILABLE.close, style: 'cancel' }], - ); + _navigateNative(screen, params) { + if (screen === 'Ask') { + this._navigation.navigate('HomeWrapper', { screen: 'Ask' }); + return; + } + this._navigation.navigate(screen, params); + } + + // Notification taps resolve from the payload rather than from a URL, so a + // granted_badge keeps its badge_name and can open a native screen. Anything + // without a native destination ends in one explicit bounded state: no + // WebView, no second login, no external browser, and never a silent no-op. + openNotification(site, notification) { + const intent = notificationIntent(site, notification, { + authenticated: Boolean(site?.authToken), + isStaff: Boolean(site?.isStaff), + }); + switch (intent.kind) { + case 'native': + this._siteManager.setActiveSite(site); + this._navigateNative(intent.screen, intent.params); + return; + case 'badge': + this._siteManager.setActiveSite(site); + this._navigation.navigate('BadgeEarned', { name: intent.badge.name }); + return; + case 'staff_external': + // Staff-only admin handoff. notificationIntent returns this kind only + // for a staff member on a canonical /admin path. + Linking.openURL(intent.url).catch(() => {}); + return; + default: + securityEvent('notification.unavailable'); + Alert.alert( + NOTIFICATION_UNAVAILABLE.title, + NOTIFICATION_UNAVAILABLE.message, + [{ text: NOTIFICATION_UNAVAILABLE.close, style: 'cancel' }], + ); } } @@ -1152,6 +1156,7 @@ class Discourse extends React.Component { // TODO: pass only relevant props to each screen component const screenProps = { openUrl: this.openUrl.bind(this), + openNotification: this.openNotification.bind(this), _handleOpenUrl: this._handleOpenUrl, seenNotificationMap: this._seenNotificationMap, setSeenNotificationMap: map => { @@ -1618,6 +1623,14 @@ class Discourse extends React.Component { /> )} + + {props => ( + + )} + {props => ( diff --git a/js/__tests__/memberRouteBoundaries.test.js b/js/__tests__/memberRouteBoundaries.test.js new file mode 100644 index 000000000..65ee2123e --- /dev/null +++ b/js/__tests__/memberRouteBoundaries.test.js @@ -0,0 +1,100 @@ +import DiscourseUtils from '../DiscourseUtils'; +import { classifyFirstPartyMemberRoute } from '../nativeMemberRouting'; + +// Salvaged from the notification-routing experiment's suite. These assert the +// URL-level classifier boundaries, which are unchanged by the move to native +// notification intents and are still the last line of defence for deep links. +const ORIGIN = 'https://adjusternetwork.org'; +const site = { url: ORIGIN, username: 'tomrodriguez', isStaff: false }; +const member = { authenticated: true, isStaff: false }; +const staff = { authenticated: true, isStaff: true }; + +const routeFor = (notification, opts = member) => + classifyFirstPartyMemberRoute( + DiscourseUtils.endpointForSiteNotification(site, notification), + opts, + ); + +describe('member route classifier boundaries', () => { + test('off-origin destinations remain rejected', () => { + for (const url of [ + 'https://evil.example.com/badges/7/basic', + 'https://adjusternetwork.org.evil.example.com/g/staff', + 'https://staging.adjusternetwork.org/chat/channel/2/lounge', + 'http://adjusternetwork.org/badges/7/basic', + ]) { + expect(classifyFirstPartyMemberRoute(url, member)).toEqual({ + disposition: 'rejected', + }); + } + }); + + test('non-staff /admin remains rejected and staff behaviour is unchanged', () => { + expect(routeFor({ notification_type: 37, data: {} })).toEqual({ + disposition: 'rejected', + }); + expect(routeFor({ notification_type: 37, data: {} }, staff)).toEqual({ + disposition: 'privileged_external', + url: `${ORIGIN}/admin`, + }); + }); + + test('unknown or empty notification endpoints remain rejected', () => { + expect(routeFor({ notification_type: 999, data: {} })).toEqual({ + disposition: 'rejected', + }); + expect(classifyFirstPartyMemberRoute(ORIGIN, member)).toEqual({ + disposition: 'rejected', + }); + expect(classifyFirstPartyMemberRoute(`${ORIGIN}/`, member)).toEqual({ + disposition: 'rejected', + }); + }); + + test('unauthenticated callers never route anywhere', () => { + expect( + classifyFirstPartyMemberRoute(`${ORIGIN}/badges/7/basic`, { + authenticated: false, + }), + ).toEqual({ disposition: 'rejected' }); + }); + + test('canonical pages without a native screen are rejected, not opened', () => { + // The first-party-web allowlist is gone: an internal page with no native + // screen must not fall through to a WebView. + for (const path of [ + '/badges/7/basic', + '/g/staff', + '/chat/channel/2/lounge', + '/u/tomrodriguez/messages/group/staff', + '/latest', + '/site.json', + '/badges', + ]) { + expect( + classifyFirstPartyMemberRoute(`${ORIGIN}${path}`, member).disposition, + ).toBe('rejected'); + } + }); + + test('openUrl handles every remaining presentation explicitly', () => { + const fs = require('fs'); + const path = require('path'); + const source = fs.readFileSync( + path.join(__dirname, '..', 'Discourse.js'), + 'utf8', + ); + const openUrl = source.slice( + source.indexOf(' openUrl(url) {'), + source.indexOf(' _navigateNative('), + ); + expect(openUrl).toContain('destinationPresentation(route)'); + for (const kind of ['native', 'external']) { + expect(openUrl).toContain(`presentation.kind === '${kind}'`); + } + // The web kind no longer exists, and deep links never open a WebView. + expect(openUrl).not.toContain("presentation.kind === 'web'"); + expect(openUrl).not.toContain("navigate('WebView'"); + expect(openUrl).toContain("securityEvent('navigation.rejected')"); + }); +}); diff --git a/js/__tests__/notificationIntent.test.js b/js/__tests__/notificationIntent.test.js new file mode 100644 index 000000000..e3c78152a --- /dev/null +++ b/js/__tests__/notificationIntent.test.js @@ -0,0 +1,364 @@ +jest.mock('@react-native-vector-icons/fontawesome5', () => 'FontAwesome5'); + +import React from 'react'; +import renderer from 'react-test-renderer'; +import DiscourseUtils from '../DiscourseUtils'; +import { classifyFirstPartyMemberRoute } from '../nativeMemberRouting'; +import { + NOTIFICATION_UNAVAILABLE, + destinationPresentation, +} from '../notificationDestination'; +import { notificationIntent } from '../notificationIntent'; +import BadgeEarnedScreen from '../product/BadgeEarnedScreen'; + +const ORIGIN = 'https://adjusternetwork.org'; +const site = { + url: ORIGIN, + username: 'tomrodriguez', + authToken: 'user-api-key', + isStaff: false, +}; +const staffSite = { ...site, isStaff: true }; +const member = { authenticated: true, isStaff: false }; +const staff = { authenticated: true, isStaff: true }; + +const intent = (notification, opts = member) => + notificationIntent(site, notification, opts); + +// The real Discourse granted_badge payload. badge_title is a boolean - whether +// the badge may be worn as a title - not descriptive text, which is why V1 +// renders badge_name only (app/services/badge_granter.rb). +const grantedBadge = { + notification_type: 12, + topic_id: null, + post_number: null, + data: { + badge_id: 7, + badge_name: 'Autobiographer', + badge_slug: 'autobiographer', + badge_title: false, + username: 'tomrodriguez', + }, +}; + +const topicNotification = type => ({ + notification_type: type, + slug: 'a-discussion', + topic_id: 41, + post_number: 3, + data: { topic_title: 'A discussion' }, +}); + +describe('B: granted_badge resolves to a native badge intent', () => { + test('badge_name survives directly from the payload', () => { + expect(intent(grantedBadge)).toEqual({ + kind: 'badge', + badge: { name: 'Autobiographer' }, + }); + }); + + test('the URL form would have lost badge_name, which is why the payload wins', () => { + // Proof of the architectural reason for the intent layer: the endpoint + // carries the id and username only. + const url = DiscourseUtils.endpointForSiteNotification(site, grantedBadge); + expect(url).toContain('/badges/7/'); + expect(url).not.toContain('Autobiographer'); + }); + + test('badge_title is never treated as descriptive text', () => { + const titled = { + ...grantedBadge, + data: { ...grantedBadge.data, badge_title: true }, + }; + expect(intent(titled)).toEqual({ + kind: 'badge', + badge: { name: 'Autobiographer' }, + }); + }); + + test('a badge payload without a usable name falls to the bounded state', () => { + for (const badge_name of [ + undefined, + null, + '', + ' ', + 42, + {}, + 'x'.repeat(121), + ]) { + expect( + intent({ ...grantedBadge, data: { ...grantedBadge.data, badge_name } }), + ).toEqual({ kind: 'unavailable' }); + } + expect(intent({ notification_type: 12 })).toEqual({ kind: 'unavailable' }); + }); +}); + +describe('BadgeEarned screen', () => { + const render = (name, navigation) => { + let tree; + renderer.act(() => { + tree = renderer.create( + , + ); + }); + return tree; + }; + + test('renders the badge name and the earned framing', () => { + const json = JSON.stringify(render('Autobiographer').toJSON()); + expect(json).toContain('Autobiographer'); + expect(json).toContain('Badge earned'); + }); + + test('performs no network request', async () => { + const fetchSpy = jest + .spyOn(global, 'fetch') + .mockRejectedValue(new Error('no network expected')); + render('Autobiographer'); + await Promise.resolve(); + expect(fetchSpy).not.toHaveBeenCalled(); + fetchSpy.mockRestore(); + }); + + test('closes through goBack from both the header and the action', () => { + const goBack = jest.fn(); + const tree = render('Autobiographer', { goBack }); + const pressables = tree.root.findAll( + node => + typeof node.props.onPress === 'function' && + (node.props.accessibilityLabel === 'Back' || + node.props.accessibilityLabel === 'Close'), + ); + expect(pressables.length).toBeGreaterThanOrEqual(2); + for (const node of pressables) { + renderer.act(() => node.props.onPress()); + } + expect(goBack).toHaveBeenCalledTimes(pressables.length); + }); +}); + +describe('A: existing native routing is unchanged', () => { + test.each([ + 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 13, 14, 15, 17, 18, 20, 24, 25, 27, 28, + 34, 36, 801, 802, + ])('type %i still opens the Topic screen', type => { + expect(intent(topicNotification(type))).toEqual({ + kind: 'native', + screen: 'Topic', + params: { topicId: 41, url: `${ORIGIN}/t/a-discussion/41/3` }, + }); + }); + + test('following notifications still open MemberProfile', () => { + expect( + intent({ notification_type: 800, data: { display_username: 'ana' } }), + ).toMatchObject({ kind: 'native', screen: 'MemberProfile' }); + }); + + test('approval_given without a topic opens the profile activity it names', () => { + // Pre-existing behaviour: the endpoint is /u/:me/activity/approval-given, + // which the profile pattern already matches. A valid native destination + // exists, so this is class A and is deliberately left alone. + expect(intent({ notification_type: 21, data: {} })).toEqual({ + kind: 'native', + screen: 'MemberProfile', + params: { username: 'tomrodriguez' }, + }); + }); + + test('code review approval opens the topic when it has one', () => { + expect( + intent({ + notification_type: 21, + fancy_title: 'Approved', + slug: 'a-discussion', + topic_id: 41, + post_number: 3, + data: {}, + }), + ).toMatchObject({ kind: 'native', screen: 'Topic' }); + }); +}); + +describe('C: unsupported member types reach one bounded state', () => { + test.each([ + [ + 'group_message_summary', + { + notification_type: 16, + data: { username: 'tomrodriguez', group_name: 'staff' }, + }, + ], + [ + 'liked_consolidated', + { notification_type: 19, data: { username: 'ana' } }, + ], + [ + 'membership_request_accepted', + { notification_type: 22, data: { group_name: 'adjusters' } }, + ], + ['membership_request_consolidated', { notification_type: 23, data: {} }], + ['votes_released', { notification_type: 26, data: {} }], + [ + 'chat_mention', + { + notification_type: 29, + data: { + chat_channel_id: 2, + chat_channel_title: 'lounge', + chat_message_id: 9, + }, + }, + ], + [ + 'chat_message', + { + notification_type: 30, + data: { chat_channel_id: 2, chat_channel_title: 'lounge' }, + }, + ], + [ + 'chat_invitation', + { + notification_type: 31, + data: { chat_channel_id: 2, chat_channel_title: 'lounge' }, + }, + ], + [ + 'chat_group_mention', + { + notification_type: 32, + data: { + chat_channel_id: 2, + chat_channel_title: 'lounge', + chat_message_id: 9, + }, + }, + ], + ['unknown type', { notification_type: 9999, data: {} }], + ['absent type', { data: {} }], + ])('%s is unavailable, never a silent no-op', (_label, notification) => { + expect(intent(notification)).toEqual({ kind: 'unavailable' }); + }); + + test('the copy promises no loading, no login and no browser', () => { + expect(NOTIFICATION_UNAVAILABLE.close).toBe('Close'); + expect(NOTIFICATION_UNAVAILABLE.message).toMatch(/marked as read/i); + expect(NOTIFICATION_UNAVAILABLE.message).not.toMatch( + /log ?in|sign ?in|browser|Safari|try again/i, + ); + }); +}); + +describe('security boundaries', () => { + test('staff /admin remains the one external handoff', () => { + for (const type of [37, 38]) { + expect( + notificationIntent( + staffSite, + { notification_type: type, data: {} }, + staff, + ), + ).toEqual({ kind: 'staff_external', url: `${ORIGIN}/admin` }); + } + }); + + test('a member never receives the staff external handoff', () => { + for (const type of [37, 38]) { + expect(intent({ notification_type: type, data: {} })).toEqual({ + kind: 'unavailable', + }); + } + }); + + test('an unauthenticated caller resolves nothing', () => { + expect( + notificationIntent(site, grantedBadge, { authenticated: false }), + ).toEqual({ kind: 'unavailable' }); + expect( + notificationIntent(site, topicNotification(2), { authenticated: false }), + ).toEqual({ kind: 'unavailable' }); + }); + + test('off-origin and non-canonical destinations stay unavailable', () => { + const evil = { + url: 'https://evil.example.com', + username: 'x', + authToken: 't', + }; + expect(notificationIntent(evil, topicNotification(2), member)).toEqual({ + kind: 'unavailable', + }); + }); + + test('destinationPresentation no longer emits a web disposition', () => { + expect( + destinationPresentation({ disposition: 'first_party_web', url: ORIGIN }), + ).toEqual({ + kind: 'denied', + }); + expect( + destinationPresentation( + classifyFirstPartyMemberRoute(`${ORIGIN}/badges/7/basic`, member), + ), + ).toEqual({ kind: 'denied' }); + }); +}); + +describe('the experiment is gone and the guard is back', () => { + const fs = require('fs'); + const path = require('path'); + const read = f => fs.readFileSync(path.join(__dirname, '..', f), 'utf8'); + + test('webViewSession and the first-party-web allowlist no longer exist', () => { + expect(fs.existsSync(path.join(__dirname, '..', 'webViewSession.js'))).toBe( + false, + ); + expect(read('nativeMemberRouting.js')).not.toContain('first_party_web'); + expect(read('nativeMemberRouting.js')).not.toContain( + 'FIRST_PARTY_WEB_PATHS', + ); + expect(read('Discourse.js')).not.toContain('webViewSession'); + expect(read('Discourse.js')).not.toContain('_openFirstPartyWeb'); + }); + + test('no notification intent can bootstrap a WebView', () => { + const source = read('Discourse.js'); + const handler = source.slice( + source.indexOf(' openNotification(site, notification) {'), + source.indexOf(' // A member must never be trapped'), + ); + expect(handler).not.toContain('WebView'); + expect(handler).not.toContain('otp'); + expect(handler).toContain("navigate('BadgeEarned'"); + expect(handler).toContain('NOTIFICATION_UNAVAILABLE.title'); + }); + + test('the strict WebView navigation guard is restored', () => { + const source = read('screens/WebViewScreenComponents/WebViewComponent.js'); + expect(source).toContain( + '// Canonical pages without an explicit native route must not', + ); + expect(source).not.toContain('pendingDestination'); + expect(source).not.toContain('isOtpBootstrapUrl'); + expect(read('screens/WebViewScreen.js')).not.toContain('destination='); + }); + + test('read-marking still precedes intent resolution', () => { + const source = read('screens/NotificationsScreen.js'); + const block = source.slice( + source.indexOf('_openNotificationForSite('), + source.indexOf('_listIndex(row)'), + ); + expect(block.indexOf('markNotificationRead')).toBeLessThan( + block.indexOf('openNotification('), + ); + // The tap site hands on the notification, not a lossy URL. + expect(block).not.toContain('endpointForSiteNotification('); + expect(block).not.toContain('DiscourseUtils'); + }); +}); diff --git a/js/__tests__/notificationRouting.test.js b/js/__tests__/notificationRouting.test.js deleted file mode 100644 index fcea64e80..000000000 --- a/js/__tests__/notificationRouting.test.js +++ /dev/null @@ -1,269 +0,0 @@ -import DiscourseUtils from '../DiscourseUtils'; -import { - classifyFirstPartyMemberRoute, - isFirstPartyWebPath, -} from '../nativeMemberRouting'; - -const ORIGIN = 'https://adjusternetwork.org'; -const site = { url: ORIGIN, username: 'tomrodriguez', isStaff: false }; -const member = { authenticated: true, isStaff: false }; -const staff = { authenticated: true, isStaff: true }; - -const routeFor = (notification, opts = member) => - classifyFirstPartyMemberRoute( - DiscourseUtils.endpointForSiteNotification(site, notification), - opts, - ); - -// The real Discourse granted_badge payload: badge_id and username present, -// topic_id and post_number absent (app/services/badge_granter.rb). -const grantedBadge = { - notification_type: 12, - topic_id: null, - post_number: null, - data: { - badge_id: 7, - badge_name: 'Autobiographer', - badge_slug: 'autobiographer', - badge_title: false, - username: 'tomrodriguez', - }, -}; - -describe('granted_badge (Autobiographer) now reaches a destination', () => { - test('produces a valid badge URL', () => { - expect(DiscourseUtils.endpointForSiteNotification(site, grantedBadge)).toBe( - `${ORIGIN}/badges/7/basic?username=tomrodriguez`, - ); - }); - - test('classifies as first_party_web, not rejected', () => { - expect(routeFor(grantedBadge)).toEqual({ - disposition: 'first_party_web', - url: `${ORIGIN}/badges/7/basic?username=tomrodriguez`, - }); - }); - - test('nil topic_id and post_number do not suppress the destination', () => { - expect(grantedBadge.topic_id).toBeNull(); - expect(grantedBadge.post_number).toBeNull(); - expect(routeFor(grantedBadge).disposition).toBe('first_party_web'); - }); -}); - -describe('every previously silent notification class now routes', () => { - test.each([ - [ - 'group_message_summary', - { - notification_type: 16, - data: { username: 'tomrodriguez', group_name: 'staff' }, - }, - '/u/tomrodriguez/messages/group/staff', - ], - [ - 'liked_consolidated', - { notification_type: 19, data: { username: 'someone' } }, - '/u/tomrodriguez/notifications/likes-received?acting_username=someone', - ], - [ - 'membership_request_accepted', - { notification_type: 22, data: { group_name: 'staff' } }, - '/g/staff', - ], - [ - 'chat_mention', - { - notification_type: 29, - data: { - chat_channel_id: 2, - chat_channel_title: 'lounge', - chat_message_id: 9, - }, - }, - '/chat/channel/2/lounge?messageId=9', - ], - [ - 'chat_message', - { - notification_type: 30, - data: { chat_channel_id: 2, chat_channel_title: 'lounge' }, - }, - '/chat/channel/2/lounge', - ], - ])('%s opens first_party_web', (_label, notification, expectedPath) => { - const url = DiscourseUtils.endpointForSiteNotification(site, notification); - expect(url).toBe(`${ORIGIN}${expectedPath}`); - expect(routeFor(notification)).toEqual({ - disposition: 'first_party_web', - url, - }); - }); -}); - -describe('existing native routing is unchanged', () => { - const topicish = { slug: 'a-topic', topic_id: 42, post_number: 3, data: {} }; - - test.each([1, 2, 3, 5, 9, 11, 17, 24, 28, 36, 801, 802])( - 'type %i still opens the native Topic screen', - type => { - const route = routeFor({ notification_type: type, ...topicish }); - expect(route.disposition).toBe('native'); - expect(route.screen).toBe('Topic'); - }, - ); - - test('following and approval notifications still open MemberProfile', () => { - expect( - routeFor({ - notification_type: 800, - data: { display_username: 'someone' }, - }), - ).toMatchObject({ disposition: 'native', screen: 'MemberProfile' }); - expect(routeFor({ notification_type: 21, data: {} })).toMatchObject({ - disposition: 'native', - screen: 'MemberProfile', - }); - }); - - test.each([ - ['/search', 'Search'], - ['/u/tomrodriguez/activity/bookmarks', 'Bookmarks'], - ['/u/tomrodriguez', 'MemberProfile'], - ['/u/tomrodriguez/preferences', 'Settings'], - ['/new-topic', 'Ask'], - ['/c/field-notes/12', 'Collection'], - ['/tag/roofing', 'Collection'], - ])('%s still resolves natively to %s', (path, screen) => { - const route = classifyFirstPartyMemberRoute(`${ORIGIN}${path}`, member); - expect(route.disposition).toBe('native'); - expect(route.screen).toBe(screen); - }); -}); - -describe('security boundaries are preserved', () => { - test('off-origin destinations remain rejected', () => { - for (const url of [ - 'https://evil.example.com/badges/7/basic', - 'https://adjusternetwork.org.evil.example.com/g/staff', - 'https://staging.adjusternetwork.org/chat/channel/2/lounge', - 'http://adjusternetwork.org/badges/7/basic', - ]) { - expect(classifyFirstPartyMemberRoute(url, member)).toEqual({ - disposition: 'rejected', - }); - } - }); - - test('non-staff /admin remains rejected and staff behaviour is unchanged', () => { - expect(routeFor({ notification_type: 37, data: {} })).toEqual({ - disposition: 'rejected', - }); - expect(routeFor({ notification_type: 37, data: {} }, staff)).toEqual({ - disposition: 'privileged_external', - url: `${ORIGIN}/admin`, - }); - }); - - test('unknown or empty notification endpoints remain rejected', () => { - expect(routeFor({ notification_type: 999, data: {} })).toEqual({ - disposition: 'rejected', - }); - expect(classifyFirstPartyMemberRoute(ORIGIN, member)).toEqual({ - disposition: 'rejected', - }); - expect(classifyFirstPartyMemberRoute(`${ORIGIN}/`, member)).toEqual({ - disposition: 'rejected', - }); - }); - - test('a malformed badge payload fails safely rather than opening', () => { - for (const data of [ - {}, - { username: 'tomrodriguez' }, - { badge_id: 'not-a-number', username: 'tomrodriguez' }, - { badge_id: null, username: null }, - ]) { - expect( - routeFor({ - notification_type: 12, - topic_id: null, - post_number: null, - data, - }), - ).toEqual({ disposition: 'rejected' }); - } - }); - - test('unauthenticated callers never route anywhere', () => { - expect( - classifyFirstPartyMemberRoute(`${ORIGIN}/badges/7/basic`, { - authenticated: false, - }), - ).toEqual({ disposition: 'rejected' }); - }); - - test('the allowlist is not a blanket internal fallback', () => { - for (const path of [ - '/latest', - '/site.json', - '/badges', - '/badgesx/7/basic', - '/chat', - '/chat/channel', - '/chat/channel/abc/lounge', - '/gx/staff', - '/u/tomrodriguez/messagesx', - ]) { - expect(isFirstPartyWebPath(path)).toBe(false); - expect( - classifyFirstPartyMemberRoute(`${ORIGIN}${path}`, member).disposition, - ).not.toBe('first_party_web'); - } - }); -}); - -describe('the tap handler marks read before navigating and has no silent path', () => { - const fs = require('fs'); - const path = require('path'); - - test('read-marking precedes destination resolution', () => { - const source = fs.readFileSync( - path.join(__dirname, '..', 'screens', 'NotificationsScreen.js'), - 'utf8', - ); - const handler = source.slice( - source.indexOf('_openNotificationForSite('), - source.indexOf('_listIndex(row)'), - ); - expect(handler.indexOf('markNotificationRead')).toBeLessThan( - handler.indexOf('endpointForSiteNotification'), - ); - expect(handler).toContain('openUrl(url)'); - }); - - test('openUrl handles every presentation explicitly', () => { - const source = fs.readFileSync( - path.join(__dirname, '..', 'Discourse.js'), - 'utf8', - ); - const openUrl = source.slice( - source.indexOf(' openUrl(url) {'), - source.indexOf(' async _openFirstPartyWeb('), - ); - // Dispositions are mapped by the pure destinationPresentation module and - // openUrl branches on the resulting kind. Every kind is handled. - expect(openUrl).toContain('destinationPresentation(route)'); - for (const kind of ['native', 'web', 'external']) { - expect(openUrl).toContain(`presentation.kind === '${kind}'`); - } - // The denied path is explicit, not an implicit fallthrough. - expect(openUrl).toContain("securityEvent('navigation.rejected')"); - // A first-party web destination is never loaded without first resolving - // an authenticated Discourse session. - expect(openUrl).not.toContain("navigate('WebView'"); - expect(openUrl).toContain( - 'this._openFirstPartyWeb(site, presentation.url)', - ); - }); -}); diff --git a/js/__tests__/webViewSession.test.js b/js/__tests__/webViewSession.test.js deleted file mode 100644 index 05f338117..000000000 --- a/js/__tests__/webViewSession.test.js +++ /dev/null @@ -1,380 +0,0 @@ -jest.mock('@react-native-cookies/cookies', () => ({ get: jest.fn() })); - -import CookieManager from '@react-native-cookies/cookies'; -import DiscourseUtils from '../DiscourseUtils'; -import { classifyFirstPartyMemberRoute } from '../nativeMemberRouting'; -import { - WEB_SESSION_UNAVAILABLE, - destinationPresentation, -} from '../notificationDestination'; -import { - OTP_ENDPOINT, - hasAuthenticatedWebSession, - isOtpBootstrapUrl, - otpBootstrapUrl, - requestOneTimePassword, - resolveWebSessionEntry, -} from '../webViewSession'; - -const ORIGIN = 'https://adjusternetwork.org'; -const OTP = 'a1b2c3d4e5f6'; -const BADGE = `${ORIGIN}/badges/9/basic?username=tomrodriguez`; - -const makeSite = (overrides = {}) => ({ - url: ORIGIN, - username: 'tomrodriguez', - authToken: 'user-api-key', - clientId: 'client-A', - jsonApi: jest.fn(), - ...overrides, -}); - -const makeManager = (overrides = {}) => ({ - ensureRSAKeys: jest.fn(() => Promise.resolve()), - rsaKeys: { public: 'PUBLIC-KEY', private: 'PRIVATE-KEY' }, - decryptHelper: jest.fn(() => OTP), - ...overrides, -}); - -beforeEach(() => { - jest.clearAllMocks(); - CookieManager.get.mockResolvedValue({}); -}); - -describe('OTP request uses the existing credentials and crypto', () => { - test('posts the app public key, governed redirect and pkcs1 padding', async () => { - const site = makeSite(); - const manager = makeManager(); - site.jsonApi.mockResolvedValue({ - redirect_url: `adjusternetwork://adjusternetwork.org/auth_redirect?oneTimePassword=ENCRYPTED`, - }); - - await expect(requestOneTimePassword(site, manager)).resolves.toBe(OTP); - - // Reuses site.jsonApi, so User-Api-Key / User-Api-Client-Id headers and - // the rate-limit buckets apply unchanged. - expect(site.jsonApi).toHaveBeenCalledWith(OTP_ENDPOINT, 'POST', { - public_key: 'PUBLIC-KEY', - auth_redirect: 'adjusternetwork://adjusternetwork.org/auth_redirect', - padding: 'pkcs1', - }); - // Same RSA machinery as the authorization flow; no second implementation. - expect(manager.ensureRSAKeys).toHaveBeenCalled(); - expect(manager.decryptHelper).toHaveBeenCalledWith('ENCRYPTED'); - }); - - test('an unauthenticated site never requests an OTP', async () => { - const site = makeSite({ authToken: null }); - await expect(requestOneTimePassword(site, makeManager())).rejects.toThrow( - 'web_session_unauthenticated', - ); - expect(site.jsonApi).not.toHaveBeenCalled(); - }); - - test('a missing RSA public key fails before any request', async () => { - const site = makeSite(); - await expect( - requestOneTimePassword(site, makeManager({ rsaKeys: {} })), - ).rejects.toThrow('web_session_key_unavailable'); - expect(site.jsonApi).not.toHaveBeenCalled(); - }); - - test('a response without an OTP fails closed', async () => { - const site = makeSite(); - for (const redirect_url of [ - undefined, - '', - 'adjusternetwork://adjusternetwork.org/auth_redirect', - 'https://evil.example.com/?oneTimePassword=X', - ]) { - site.jsonApi.mockResolvedValue({ redirect_url }); - await expect(requestOneTimePassword(site, makeManager())).rejects.toThrow( - 'web_session_otp_missing', - ); - } - }); - - test('a non-hex decrypted OTP is refused so nothing is injected into the path', async () => { - const site = makeSite(); - site.jsonApi.mockResolvedValue({ - redirect_url: `adjusternetwork://adjusternetwork.org/auth_redirect?oneTimePassword=E`, - }); - for (const bad of ['../../admin', 'abc/def', 'ZZZZ', '', null]) { - await expect( - requestOneTimePassword(site, makeManager({ decryptHelper: () => bad })), - ).rejects.toThrow('web_session_otp_invalid'); - } - }); - - test('a rate-limited or failing OTP request propagates', async () => { - const site = makeSite(); - site.jsonApi.mockRejectedValue( - Object.assign(new Error('api_rate_limited'), { status: 429 }), - ); - await expect(requestOneTimePassword(site, makeManager())).rejects.toThrow( - 'api_rate_limited', - ); - }); -}); - -describe('bootstrap URL construction is constrained', () => { - test('builds the confirmation route for a hex token only', () => { - expect(otpBootstrapUrl({ url: ORIGIN }, OTP)).toBe( - `${ORIGIN}/session/otp/${OTP}`, - ); - for (const bad of ['../admin', 'a/b', 'ZZ', '', null, undefined]) { - expect(otpBootstrapUrl({ url: ORIGIN }, bad)).toBeNull(); - } - expect(otpBootstrapUrl(null, OTP)).toBeNull(); - }); - - test('recognises only canonical-origin HTTPS bootstrap URLs', () => { - expect(isOtpBootstrapUrl(`${ORIGIN}/session/otp/${OTP}`)).toBe(true); - for (const bad of [ - `${ORIGIN}/badges/9/basic`, - `https://evil.example.com/session/otp/${OTP}`, - `http://adjusternetwork.org/session/otp/${OTP}`, - `https://adjusternetwork.org.evil.example.com/session/otp/${OTP}`, - 'not-a-url', - null, - ]) { - expect(isOtpBootstrapUrl(bad)).toBe(false); - } - }); -}); - -describe('an existing session is reused rather than minting another OTP', () => { - test('a live auth cookie skips the bootstrap entirely', async () => { - CookieManager.get.mockResolvedValue({ _t: { value: 'session-token' } }); - const site = makeSite(); - await expect(hasAuthenticatedWebSession(site, CookieManager)).resolves.toBe( - true, - ); - - await expect( - resolveWebSessionEntry(site, makeManager(), BADGE), - ).resolves.toEqual({ url: BADGE, destination: null }); - expect(site.jsonApi).not.toHaveBeenCalled(); - }); - - test('an absent or empty cookie bootstraps and remembers the destination', async () => { - for (const jar of [{}, { _t: {} }, { _t: { value: '' } }, null]) { - CookieManager.get.mockResolvedValue(jar); - const site = makeSite(); - site.jsonApi.mockResolvedValue({ - redirect_url: `adjusternetwork://adjusternetwork.org/auth_redirect?oneTimePassword=E`, - }); - await expect( - resolveWebSessionEntry(site, makeManager(), BADGE), - ).resolves.toEqual({ - url: `${ORIGIN}/session/otp/${OTP}`, - destination: BADGE, - }); - expect(site.jsonApi).toHaveBeenCalledTimes(1); - } - }); - - test('an unreadable cookie jar bootstraps rather than assuming a session', async () => { - CookieManager.get.mockRejectedValue(new Error('cookie failure')); - await expect( - hasAuthenticatedWebSession(makeSite(), CookieManager), - ).resolves.toBe(false); - }); - - test('an off-origin destination is refused before any OTP is minted', async () => { - const site = makeSite(); - for (const bad of [ - 'https://evil.example.com/badges/9/basic', - 'http://adjusternetwork.org/badges/9/basic', - null, - ]) { - await expect( - resolveWebSessionEntry(site, makeManager(), bad), - ).rejects.toThrow('web_session_destination'); - } - expect(site.jsonApi).not.toHaveBeenCalled(); - }); -}); - -describe('destination presentation', () => { - const site = { url: ORIGIN, username: 'tomrodriguez' }; - const member = { authenticated: true, isStaff: false }; - const present = (n, o = member) => - destinationPresentation( - classifyFirstPartyMemberRoute( - DiscourseUtils.endpointForSiteNotification(site, n), - o, - ), - ); - - test('granted_badge presents a web destination', () => { - expect( - present({ - notification_type: 12, - topic_id: null, - post_number: null, - data: { badge_id: 9, username: 'tomrodriguez' }, - }), - ).toEqual({ kind: 'web', url: BADGE }); - }); - - test.each([ - [ - 'group_message_summary', - { - notification_type: 16, - data: { username: 'tomrodriguez', group_name: 'staff' }, - }, - ], - [ - 'liked_consolidated', - { notification_type: 19, data: { username: 'someone' } }, - ], - [ - 'membership_request_accepted', - { notification_type: 22, data: { group_name: 'staff' } }, - ], - [ - 'chat_mention', - { - notification_type: 29, - data: { - chat_channel_id: 2, - chat_channel_title: 'lounge', - chat_message_id: 9, - }, - }, - ], - [ - 'chat_message', - { - notification_type: 30, - data: { chat_channel_id: 2, chat_channel_title: 'lounge' }, - }, - ], - ])('%s presents a web destination', (_l, n) => { - expect(present(n).kind).toBe('web'); - }); - - test('native notification routing is unchanged', () => { - expect( - present({ - notification_type: 2, - slug: 't', - topic_id: 4, - post_number: 1, - data: {}, - }), - ).toMatchObject({ kind: 'native', screen: 'Topic' }); - expect( - present({ notification_type: 800, data: { display_username: 'x' } }), - ).toMatchObject({ kind: 'native', screen: 'MemberProfile' }); - }); - - test('denied destinations stay denied', () => { - expect(present({ notification_type: 37, data: {} })).toEqual({ - kind: 'denied', - }); - expect(present({ notification_type: 999, data: {} })).toEqual({ - kind: 'denied', - }); - expect( - present({ - notification_type: 12, - data: { badge_id: 'abc', username: 'x' }, - }), - ).toEqual({ kind: 'denied' }); - expect( - destinationPresentation( - classifyFirstPartyMemberRoute(BADGE, { authenticated: false }), - ), - ).toEqual({ kind: 'denied' }); - expect(destinationPresentation(null)).toEqual({ kind: 'denied' }); - }); - - test('staff admin still hands off externally', () => { - expect( - present( - { notification_type: 37, data: {} }, - { authenticated: true, isStaff: true }, - ), - ).toEqual({ kind: 'external', url: `${ORIGIN}/admin` }); - }); -}); - -describe('failure is bounded and explicit', () => { - test('the failure copy promises no loading and no login', () => { - expect(WEB_SESSION_UNAVAILABLE.close).toBe('Close'); - expect(WEB_SESSION_UNAVAILABLE.message).toMatch(/marked as read/i); - expect(WEB_SESSION_UNAVAILABLE.message).not.toMatch( - /log ?in|sign ?in|browser|Safari/i, - ); - }); -}); - -describe('wiring', () => { - const fs = require('fs'); - const path = require('path'); - const read = f => fs.readFileSync(path.join(__dirname, '..', f), 'utf8'); - - test('openUrl delegates web destinations and bounds failure', () => { - const source = read('Discourse.js'); - const openUrl = source.slice( - source.indexOf(' openUrl(url) {'), - source.indexOf(' async _openFirstPartyWeb('), - ); - expect(openUrl).toContain("presentation.kind === 'web'"); - expect(openUrl).toContain( - 'this._openFirstPartyWeb(site, presentation.url)', - ); - expect(openUrl).toContain("securityEvent('navigation.rejected')"); - // openUrl itself never opens the WebView: a web destination must go - // through session resolution first. - expect(openUrl).not.toContain("navigate('WebView'"); - - const handler = source.slice( - source.indexOf(' async _openFirstPartyWeb('), - source.indexOf(' _toggleTheme('), - ); - // The WebView is reached only after the session entry resolves, and any - // failure ends in the bounded explicit state. - expect(handler.indexOf('resolveWebSessionEntry(')).toBeLessThan( - handler.indexOf("navigate('WebView'"), - ); - expect(handler).toContain( - "securityEvent('navigation.web_session_unavailable')", - ); - expect(handler).toContain('WEB_SESSION_UNAVAILABLE.title'); - }); - - test('the WebView policy relaxation is bootstrap-scoped, not standing', () => { - const source = read('screens/WebViewScreenComponents/WebViewComponent.js'); - // The original guard survives. - expect(source).toContain( - '// Canonical pages without an explicit native route must not', - ); - expect(source).toContain('_isAuthorizedSessionNavigation(request.url)'); - // Authorization requires an app-initiated bootstrap. - expect(source).toContain( - 'isOtpBootstrapUrl(url) && Boolean(this.props.destination)', - ); - // The window closes once the destination loads. - expect(source).toContain( - 'pendingDestination: null, webviewUrl: destination', - ); - expect(read('screens/WebViewScreen.js')).toContain( - 'destination={this.props.route.params.destination}', - ); - }); - - test('read-marking still precedes destination resolution', () => { - const handler = read('screens/NotificationsScreen.js'); - const block = handler.slice( - handler.indexOf('_openNotificationForSite('), - handler.indexOf('_listIndex(row)'), - ); - expect(block.indexOf('markNotificationRead')).toBeLessThan( - block.indexOf('endpointForSiteNotification'), - ); - }); -}); diff --git a/js/nativeMemberRouting.js b/js/nativeMemberRouting.js index 1e84819ed..bc4b3c5a6 100644 --- a/js/nativeMemberRouting.js +++ b/js/nativeMemberRouting.js @@ -65,27 +65,6 @@ export function nativeCollectionRoute(value, authenticated) { } } -// Canonical-origin member destinations that are valid and intended, but have no -// native screen. They open in the already-registered authenticated Discourse -// WebView rather than being discarded. This is an explicit allowlist, not a -// blanket "anything internal opens" fallback: an unlisted path stays rejected. -const FIRST_PARTY_WEB_PATHS = Object.freeze([ - // granted_badge - /badges/:id/:filter - /^\/badges\/[0-9]+(?:\/[^/]*)?\/?$/i, - // group_message_summary - /u/:username/messages/group/:group - /^\/u\/[a-z0-9_.-]+\/messages(?:\/[^/]+)*\/?$/i, - // liked_consolidated - /u/:username/notifications/likes-received - /^\/u\/[a-z0-9_.-]+\/notifications(?:\/[^/]+)*\/?$/i, - // membership_request_accepted - /g/:group - /^\/g\/[a-z0-9_.-]+\/?$/i, - // chat mention and message - /chat/channel/:id/:slug - /^\/chat\/channel\/[0-9]+(?:\/[^/]*)?\/?$/i, -]); - -export function isFirstPartyWebPath(pathname) { - return FIRST_PARTY_WEB_PATHS.some(pattern => pattern.test(String(pathname))); -} - export function classifyFirstPartyMemberRoute( value, { authenticated = false, isStaff = false } = {}, @@ -141,11 +120,6 @@ export function classifyFirstPartyMemberRoute( ? { disposition: 'privileged_external', url: url.toString() } : { disposition: 'rejected' }; } - // Checked after every native pattern and after the /admin boundary, so it - // can never widen an already-denied destination. - if (isFirstPartyWebPath(url.pathname)) { - return { disposition: 'first_party_web', url: url.toString() }; - } return { disposition: 'rejected' }; } catch { return { disposition: 'rejected' }; diff --git a/js/notificationDestination.js b/js/notificationDestination.js index 0541188c4..8cec813c2 100644 --- a/js/notificationDestination.js +++ b/js/notificationDestination.js @@ -4,15 +4,14 @@ // Presentation decision for a classified member destination. Kept pure and // separate from Discourse.js so every branch is directly testable. // -// A 'first_party_web' destination is a valid first-party member page with no -// native screen. It is opened in the in-app WebView, but only after an -// authenticated Discourse session has been bootstrapped - see webViewSession. -// Loading it without one would show a login wall, which is exactly what -// WebViewComponent's navigation policy exists to prevent. -export const WEB_SESSION_UNAVAILABLE = Object.freeze({ - title: 'Not available right now', +// There is no in-between disposition here. A destination either has a native +// screen or it does not; one that does not ends in an explicit bounded state. +// Opening a canonical page in a WebView to work around a missing native screen +// was tried and abandoned - see docs/NATIVE-NOTIFICATION-INTENTS.md. +export const NOTIFICATION_UNAVAILABLE = Object.freeze({ + title: 'Not available in the app yet', message: - 'Adjuster Network could not open this page in the app. It has been marked as read, and nothing else is affected. Try again later.', + 'Adjuster Network cannot open this notification in the app yet. It has been marked as read, and nothing else is affected.', close: 'Close', }); @@ -20,8 +19,6 @@ export function destinationPresentation(route) { switch (route?.disposition) { case 'native': return { kind: 'native', screen: route.screen, params: route.params }; - case 'first_party_web': - return { kind: 'web', url: route.url }; case 'privileged_external': return { kind: 'external', url: route.url }; default: diff --git a/js/notificationIntent.js b/js/notificationIntent.js new file mode 100644 index 000000000..794cd886d --- /dev/null +++ b/js/notificationIntent.js @@ -0,0 +1,53 @@ +/* @flow */ +'use strict'; + +import DiscourseUtils from './DiscourseUtils'; +import { classifyFirstPartyMemberRoute } from './nativeMemberRouting'; + +export const GRANTED_BADGE = 12; + +// Longest badge name we will render. Discourse badge names are short; this only +// bounds a hostile or corrupted payload, it is not a product limit. +const MAX_BADGE_NAME = 120; + +// Intent resolution starts from the notification payload, not from the URL +// DiscourseUtils builds. That conversion is lossy: a granted_badge becomes +// /badges/:id/basic?username=:u and badge_name is discarded. Reading the +// payload first is what makes a native badge destination possible at all. +export function notificationIntent( + site, + notification, + { authenticated = false, isStaff = false } = {}, +) { + if (!authenticated || !notification) return { kind: 'unavailable' }; + + if (notification.notification_type === GRANTED_BADGE) { + const name = badgeName(notification); + // A badge notification with no usable name has nothing to present + // natively, so it takes the same bounded state as any other gap. + return name ? { kind: 'badge', badge: { name } } : { kind: 'unavailable' }; + } + + const url = DiscourseUtils.endpointForSiteNotification(site, notification); + const route = classifyFirstPartyMemberRoute(url, { authenticated, isStaff }); + switch (route.disposition) { + case 'native': + return { kind: 'native', screen: route.screen, params: route.params }; + case 'privileged_external': + // Staff-only /admin. Admin has no native surface and the boundary is + // enforced in classifyFirstPartyMemberRoute, which returns this + // disposition only when isStaff is true. This is the single documented + // external handoff; it does not widen external navigation for members. + return { kind: 'staff_external', url: route.url }; + default: + return { kind: 'unavailable' }; + } +} + +function badgeName(notification) { + const value = notification?.data?.badge_name; + if (typeof value !== 'string') return null; + const trimmed = value.trim(); + if (!trimmed || trimmed.length > MAX_BADGE_NAME) return null; + return trimmed; +} diff --git a/js/product/BadgeEarnedScreen.js b/js/product/BadgeEarnedScreen.js new file mode 100644 index 000000000..93ab132f8 --- /dev/null +++ b/js/product/BadgeEarnedScreen.js @@ -0,0 +1,52 @@ +/* @flow */ +'use strict'; + +import React from 'react'; +import { StyleSheet, Text, View } from 'react-native'; +import { SafeAreaView } from 'react-native-safe-area-context'; +import { + Action, + Card, + V2BrandHeader, + useProductTheme, +} from './ProductComponents'; +import { spacing } from './DesignSystem'; + +// Renders entirely from the notification payload. It makes no network request: +// everything shown here arrived with the notification, so the screen cannot +// fail, stall, or need a session. Badge artwork and description are +// deliberately absent - both would require a fetch. +const BadgeEarnedScreen = ({ navigation, route }) => { + const colors = useProductTheme(); + const name = route?.params?.name; + const close = () => navigation.goBack(); + + return ( + + + + + {name} + + + + + + + ); +}; + +export default BadgeEarnedScreen; + +const styles = StyleSheet.create({ + safe: { flex: 1 }, + body: { padding: spacing.md }, + name: { + fontSize: 22, + lineHeight: 28, + fontWeight: '750', + textAlign: 'center', + paddingVertical: spacing.md, + }, + action: { marginTop: spacing.lg, alignItems: 'center' }, +}); diff --git a/js/screens/NotificationsScreen.js b/js/screens/NotificationsScreen.js index b11913c0c..bb420d632 100644 --- a/js/screens/NotificationsScreen.js +++ b/js/screens/NotificationsScreen.js @@ -14,7 +14,6 @@ import { ImmutableVirtualizedList } from 'react-native-immutable-list-view'; import FontAwesome5 from '@react-native-vector-icons/fontawesome5'; import Components from './NotificationsScreenComponents'; import Common from './CommonComponents'; -import DiscourseUtils from '../DiscourseUtils'; import { ThemeContext } from '../ThemeContext'; import i18n from 'i18n-js'; import { BottomTabBarHeightContext } from '@react-navigation/bottom-tabs'; @@ -246,9 +245,10 @@ class NotificationsScreen extends React.Component { _openNotificationForSite(notification, site) { this._siteManager.markNotificationRead(site, notification).catch(() => {}); - let url = DiscourseUtils.endpointForSiteNotification(site, notification); + // The whole notification is handed on, not a URL built from it: intent + // resolution needs payload fields that endpointForSiteNotification drops. this._siteManager.setActiveSite(site); - this.props.screenProps.openUrl(url); + this.props.screenProps.openNotification(site, notification); } _listIndex(row) { diff --git a/js/screens/WebViewScreen.js b/js/screens/WebViewScreen.js index ce5f3a9b4..f0fb10b7f 100644 --- a/js/screens/WebViewScreen.js +++ b/js/screens/WebViewScreen.js @@ -14,7 +14,6 @@ class WebViewScreen extends React.Component { ); } diff --git a/js/screens/WebViewScreenComponents/WebViewComponent.js b/js/screens/WebViewScreenComponents/WebViewComponent.js index 5f0f3e906..edb8f2d00 100644 --- a/js/screens/WebViewScreenComponents/WebViewComponent.js +++ b/js/screens/WebViewScreenComponents/WebViewComponent.js @@ -25,7 +25,6 @@ import { ThemeContext } from '../../ThemeContext'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { BlurView } from '@react-native-community/blur'; import { classifyNavigation } from '../../adjusterNetworkSecurity'; -import { isOtpBootstrapUrl } from '../../webViewSession'; import { NestedHeader } from '../../product/ProductComponents'; import { classifyFirstPartyMemberRoute } from '../../nativeMemberRouting'; @@ -83,9 +82,6 @@ class WebViewComponent extends React.Component { webviewUrl: this.props.url, authProcessActive: false, scrollOverflow: 0, - // Set only when the app itself initiated an OTP session bootstrap and - // still owes the member the page they actually asked for. - pendingDestination: this.props.destination || null, }; } @@ -294,14 +290,6 @@ class WebViewComponent extends React.Component { this.props.screenProps.openUrl(request.url); return false; } - // A session bootstrap the app itself started is allowed to - // run: the OTP confirmation page, the redirect it performs, - // and finally the destination that was requested. The window - // is closed as soon as the destination loads, so this is not a - // standing "any internal page opens" relaxation. - if (this._isAuthorizedSessionNavigation(request.url)) { - return true; - } // Canonical pages without an explicit native route must not // fall through to an unauthenticated Discourse/PWA session. return false; @@ -331,7 +319,6 @@ class WebViewComponent extends React.Component { }} onNavigationStateChange={navState => { this._storeLastPath(navState); - this._advanceSessionBootstrap(navState); }} decelerationRate={'normal'} onLoadProgress={({ nativeEvent }) => { @@ -478,25 +465,6 @@ class WebViewComponent extends React.Component { } } - // Authorized only while an app-initiated bootstrap is outstanding. - _isAuthorizedSessionNavigation(url) { - if (!this.state.pendingDestination) { - return isOtpBootstrapUrl(url) && Boolean(this.props.destination); - } - return true; - } - - // The confirmation form posts back to /session/otp/ and then Discourse - // redirects. When navigation has left the bootstrap path the session cookie - // exists, so the originally requested page is loaded exactly once and the - // authorization window closes. - _advanceSessionBootstrap(navState) { - const destination = this.state.pendingDestination; - if (!destination || navState.loading) return; - if (isOtpBootstrapUrl(navState.url)) return; - this.setState({ pendingDestination: null, webviewUrl: destination }); - } - _onMessage(event) { let data; try { diff --git a/js/webViewSession.js b/js/webViewSession.js deleted file mode 100644 index 42cd4fa46..000000000 --- a/js/webViewSession.js +++ /dev/null @@ -1,108 +0,0 @@ -/* @flow */ -'use strict'; - -import { AUTH_REDIRECT } from './authorizationConsent'; -import { isCanonicalUrl } from './adjusterNetworkSecurity'; -import { parseAuthCallbackParameters } from './authCallback'; - -// Bootstrapping an authenticated Discourse browser session for the in-app -// WebView. The WebView carries cookies, not the User API key, so a first-party -// member page cannot be opened until a session cookie exists. -// -// The supported contract: POST /user-api-key/otp with the app's existing User -// API credentials returns a redirect_url carrying an RSA-encrypted one-time -// password. The app decrypts it with the same private key used by the -// authorization flow and loads /session/otp/, where the member completes -// the existing confirmation form. That form - never bypassed - is what sets the -// session cookie. -// -// The OTP is single-use with a 10 minute TTL, and the server refuses User API -// keys for suspended or inactive users, so an unauthorised member cannot reach -// a session this way. -export const OTP_BOOTSTRAP_PATH = '/session/otp/'; -export const OTP_ENDPOINT = '/user-api-key/otp'; - -// Discourse's authentication cookie. Its presence means the WebView already -// holds a logged-in session and no OTP needs to be minted. -const AUTH_COOKIE = '_t'; - -// The route constrains the token to hex, so anything else must never be -// interpolated into the path. -const OTP_TOKEN = /^[0-9a-f]+$/; - -export function otpBootstrapUrl(site, otp) { - if (!site?.url || typeof otp !== 'string' || !OTP_TOKEN.test(otp)) { - return null; - } - return `${site.url}${OTP_BOOTSTRAP_PATH}${otp}`; -} - -export function isOtpBootstrapUrl(value) { - if (!isCanonicalUrl(value)) return false; - try { - return new URL(value).pathname.startsWith(OTP_BOOTSTRAP_PATH); - } catch { - return false; - } -} - -export async function hasAuthenticatedWebSession(site, cookies = null) { - if (!site?.url) return false; - try { - // Required lazily so importing this module never pulls in the native - // cookie package. Suites that merely reach Discourse.js must not have to - // mock it, and nothing else in this module needs it. - const jar = await (cookies || require('@react-native-cookies/cookies')).get( - site.url, - true, - ); - const token = jar?.[AUTH_COOKIE]; - return Boolean(token && token.value); - } catch { - // An unreadable cookie jar is treated as no session: the worst outcome is - // minting one extra single-use OTP. - return false; - } -} - -export async function requestOneTimePassword(site, siteManager) { - if (!site?.authToken) throw new Error('web_session_unauthenticated'); - await siteManager.ensureRSAKeys(); - const publicKey = siteManager.rsaKeys?.public; - if (!publicKey) throw new Error('web_session_key_unavailable'); - - // Reuses site.jsonApi, so the existing User-Api-Key and User-Api-Client-Id - // headers, rate-limit buckets and cooldowns all apply unchanged. - const payload = await site.jsonApi(OTP_ENDPOINT, 'POST', { - public_key: publicKey, - auth_redirect: AUTH_REDIRECT, - padding: 'pkcs1', - }); - - const encrypted = parseAuthCallbackParameters( - payload?.redirect_url, - ).oneTimePassword; - if (!encrypted) throw new Error('web_session_otp_missing'); - - // Same JSEncrypt private key as the authorization flow; no second - // cryptographic implementation. - const otp = siteManager.decryptHelper(encrypted); - if (typeof otp !== 'string' || !OTP_TOKEN.test(otp)) { - throw new Error('web_session_otp_invalid'); - } - return otp; -} - -// Resolves what the WebView should load for a first-party destination: the -// destination directly when a session already exists, otherwise a bootstrap -// that remembers where to go afterwards. -export async function resolveWebSessionEntry(site, siteManager, destination) { - if (!isCanonicalUrl(destination)) throw new Error('web_session_destination'); - if (await hasAuthenticatedWebSession(site)) { - return { url: destination, destination: null }; - } - const otp = await requestOneTimePassword(site, siteManager); - const bootstrap = otpBootstrapUrl(site, otp); - if (!bootstrap) throw new Error('web_session_otp_invalid'); - return { url: bootstrap, destination }; -} diff --git a/testing/native-auth-stale-identity/BACKLOG.md b/testing/native-auth-stale-identity/BACKLOG.md index 3e86a1e12..d4e2845dc 100644 --- a/testing/native-auth-stale-identity/BACKLOG.md +++ b/testing/native-auth-stale-identity/BACKLOG.md @@ -124,7 +124,7 @@ server/privacy certification lane. 4. **Re-verify on device after activation** — the capability gate means this cannot be certified while the flag is false. -## 9. P2 — granted_badge notification tap has no destination +## 9. RESOLVED (implementation pending OTA) — granted_badge notification tap had no destination A `granted_badge` notification (observed with Autobiographer) marks itself read when tapped but produces no navigation: the member is left where they were with @@ -143,3 +143,50 @@ Confirm which before changing the mapping. Marking-as-read succeeding while navigation silently does nothing is the part that matters: either open a destination or leave the notification unread. + + +### 2026-09-09 — item 9 resolved by native notification intents + +Full history of this item, so none of it is attempted again. + +1. **Original defect.** A `granted_badge` tap marked itself read and then did + nothing. `DiscourseUtils` did produce `/badges/:id/basic`, so the endpoint + existed; the destination was being discarded downstream. +2. **first_party_web (PR #16).** Added a canonical-origin allowlist so valid + member pages opened in the in-app WebView. Device result: the WebView + rendered blank, because `WebViewComponent`'s navigation policy deliberately + blocks internal pages with no native route, to prevent an unauthenticated + Discourse session appearing behind a member's back. +3. **OTP session bootstrap (PR #19, #20).** Minted a one-time password through + the supported `/user-api-key/otp` contract to establish a WebView session. + The first attempt failed with a 400: `require_params_otp` demands + `application_name`, which was omitted. PR #20 corrected the contract - + `/user-api-key/otp.json` plus `application_name` - and the request then + succeeded and the confirmation form rendered. **Finish Login failed with + "Missing, invalid or expired token."** Not debugged further; see 5. +4. **Instrumentation false positive.** The staged diagnostics recorded + `destination_resume: succeeded` for that failed flow. The stage observed + navigation leaving the bootstrap URL, not whether a session had been + established, so it reported success for a failure. Any future success signal + must observe the thing it claims to prove. +5. **Architectural decision (founder).** Abandon the approach. A member already + authenticated in the native app must never perform a second web + authentication merely to open a notification. Notification taps resolve to + native intents; anything without a native destination gets one explicit + bounded state. + +The experiment surface was removed and the strict WebView guard restored. See +`docs/NATIVE-NOTIFICATION-INTENTS.md` for the resulting model and type matrix. + +**Abandoned, uncertified OTA candidates.** These shipped to the production +channel during the experiment and were never certified. Their provenance tags +remain immutable; this record supplies the outcome. + +| SHA | Group | Outcome | +| --- | --- | --- | +| `487f03c3` | `09d99bc9` | first_party_web; device FAIL, blank WebView | +| `65f9d58a` | `08d07007` | OTP bootstrap; device FAIL, 400 on the OTP request | +| `43af7b78` | `eb509d26` | corrected OTP contract; device FAIL at Finish Login | + +Production was rolled back to the certified `dad0af191716` (republished as +group `9098ec70-a3dd-47dd-b623-c51fba68b181`).