From 503a450b404a39347f5d26a56c8532e6de1c4f39 Mon Sep 17 00:00:00 2001 From: Nariman Jelveh Date: Tue, 11 Aug 2026 18:06:05 -0700 Subject: [PATCH] fix: make feedback daily caps fail closed under concurrent submissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-user and per-user-per-app caps were check-then-insert and the per-app email cap was count-then-send, so parallel requests (or multiple nodes, or the route limiter failing open) could all read a stale under-cap count and push past every limit — the exact scenario the DB-backed caps exist to stop. Now the user caps recount after the insert (own row included) and roll the row back with 429 if a burst breached them, and the email cap claims its slot (email_sent=1) before sending, recounts, and releases the slot if over cap or if the send fails. --- .../feedback/AppFeedbackController.test.ts | 36 +++++++ .../services/feedback/AppFeedbackService.ts | 100 ++++++++++++------ .../stores/appFeedback/AppFeedbackStore.ts | 31 +++++- 3 files changed, 130 insertions(+), 37 deletions(-) diff --git a/src/backend/controllers/feedback/AppFeedbackController.test.ts b/src/backend/controllers/feedback/AppFeedbackController.test.ts index e9701980b..b23bf1bab 100644 --- a/src/backend/controllers/feedback/AppFeedbackController.test.ts +++ b/src/backend/controllers/feedback/AppFeedbackController.test.ts @@ -444,6 +444,42 @@ describe('AppFeedbackController POST /', () => { }); }); + it('rolls back the stored row when a concurrent burst breaches the cap', async () => { + const { userId: ownerId } = await makeUser(); + const app = await makeApp(ownerId, { feedbackEnabled: true }); + const { actor, userId } = await makeUser(); + for (let i = 0; i < AppFeedbackService.PER_USER_APP_DAILY_LIMIT; i++) { + await server.stores.appFeedback.create({ + appId: app.id, + appUid: app.uid, + userId, + message: `seed ${i}`, + }); + } + // Simulate the losing side of the check-then-insert race: the + // pre-insert check reads a stale under-cap count; the post-insert + // recount (real implementation) sees the truth. + vi.spyOn( + server.stores.appFeedback, + 'countByUserAndAppSince', + ).mockResolvedValueOnce(0); + + await expect( + submit(actor, { app: app.name, message: 'raced past the cap' }), + ).rejects.toMatchObject({ + statusCode: 429, + legacyCode: 'too_many_requests', + }); + + const rows = (await server.clients.db.read( + 'SELECT COUNT(*) AS n FROM `app_feedback` WHERE `user_id` = ?', + [userId], + )) as Array<{ n: unknown }>; + expect(Number(rows[0]?.n)).toBe( + AppFeedbackService.PER_USER_APP_DAILY_LIMIT, + ); + }); + it('enforces the per-user daily cap across apps with 429', async () => { const { userId: ownerId } = await makeUser(); const target = await makeApp(ownerId, { feedbackEnabled: true }); diff --git a/src/backend/services/feedback/AppFeedbackService.ts b/src/backend/services/feedback/AppFeedbackService.ts index 7c129a5c9..74d796cbc 100644 --- a/src/backend/services/feedback/AppFeedbackService.ts +++ b/src/backend/services/feedback/AppFeedbackService.ts @@ -188,24 +188,35 @@ export class AppFeedbackService extends PuterService { // Durable caps. Deliberately DB-backed (see class doc); the counts // ride the (user_id, created_at) / (app_id, created_at) indexes. + // `includeOwnRow` distinguishes the pre-insert check (this + // submission not yet counted) from the post-insert recount (it is). const since = Math.floor(Date.now() / 1000) - 24 * 60 * 60; - const [userAppCount, userCount] = await Promise.all([ - this.stores.appFeedback.countByUserAndAppSince( - userId, - appId, - since, - ), - this.stores.appFeedback.countByUserSince(userId, since), - ]); - if ( - userAppCount >= AppFeedbackService.PER_USER_APP_DAILY_LIMIT || - userCount >= AppFeedbackService.PER_USER_DAILY_LIMIT - ) { - throw new HttpError( + const capsBreached = async ( + includeOwnRow: boolean, + ): Promise => { + const slack = includeOwnRow ? 1 : 0; + const [userAppCount, userCount] = await Promise.all([ + this.stores.appFeedback.countByUserAndAppSince( + userId, + appId, + since, + ), + this.stores.appFeedback.countByUserSince(userId, since), + ]); + return ( + userAppCount >= + AppFeedbackService.PER_USER_APP_DAILY_LIMIT + slack || + userCount >= AppFeedbackService.PER_USER_DAILY_LIMIT + slack + ); + }; + const tooManyError = () => + new HttpError( 429, 'You have sent a lot of feedback recently — please try again later', { legacyCode: 'too_many_requests' }, ); + if (await capsBreached(false)) { + throw tooManyError(); } const row = await this.stores.appFeedback.create({ @@ -217,6 +228,16 @@ export class AppFeedbackService extends PuterService { sourceOrigin: sourceOrigin ?? null, }); + // The pre-check is check-then-insert, so parallel submissions (or + // multiple nodes) can all pass it on the same stale count. Recount + // with this row included and roll it back if a concurrent burst + // pushed past a cap — these caps must fail closed, not just usually + // hold. + if (await capsBreached(true)) { + await this.stores.appFeedback.deleteById(row.id); + throw tooManyError(); + } + // Email delivery is best-effort: any failure past this point must // not fail the request — the feedback is already stored. try { @@ -271,9 +292,17 @@ export class AppFeedbackService extends PuterService { } if (!(await this.clients.email.validate(owner.email))) return; + // Claim an email-cap slot *before* sending: flip email_sent, recount + // with the claim included, and release the slot if a concurrent + // burst pushed past the cap. Counting before sending would fail + // open — parallel submissions could each read an under-cap count and + // all send. The cost is that a crash mid-send burns a slot without + // delivering; the cap is an upper bound, not a quota owed. + await this.stores.appFeedback.markEmailSent(feedbackId); const emailedToday = await this.stores.appFeedback.countEmailedByAppSince(appId, since); - if (emailedToday >= AppFeedbackService.PER_APP_DAILY_EMAIL_LIMIT) { + if (emailedToday > AppFeedbackService.PER_APP_DAILY_EMAIL_LIMIT) { + await this.stores.appFeedback.unmarkEmailSent(feedbackId); return; } @@ -287,23 +316,30 @@ export class AppFeedbackService extends PuterService { const senderEmail = sender?.email && sender.email_confirmed ? sender.email : null; - await this.clients.email.send( - owner.email, - 'app-user-feedback', - { - owner_username: owner.username, - sender_username: sender?.username ?? 'A Puter user', - sender_email: senderEmail, - // Collapse whitespace so a crafted title can't break the - // subject header or spoof extra lines in the body. - app_title: String(app.title ?? app.name).replace(/\s+/g, ' '), - app_name: String(app.name), - app_link: `${this.config.origin}/app/${encodeURIComponent(String(app.name))}`, - message, - }, - senderEmail ? { replyTo: senderEmail } : {}, - ); - - await this.stores.appFeedback.markEmailSent(feedbackId); + try { + await this.clients.email.send( + owner.email, + 'app-user-feedback', + { + owner_username: owner.username, + sender_username: sender?.username ?? 'A Puter user', + sender_email: senderEmail, + // Collapse whitespace so a crafted title can't break the + // subject header or spoof extra lines in the body. + app_title: String(app.title ?? app.name).replace( + /\s+/g, + ' ', + ), + app_name: String(app.name), + app_link: `${this.config.origin}/app/${encodeURIComponent(String(app.name))}`, + message, + }, + senderEmail ? { replyTo: senderEmail } : {}, + ); + } catch (e) { + // Release the claimed slot — the mail never went out. + await this.stores.appFeedback.unmarkEmailSent(feedbackId); + throw e; + } } } diff --git a/src/backend/stores/appFeedback/AppFeedbackStore.ts b/src/backend/stores/appFeedback/AppFeedbackStore.ts index d02ff9982..cc0c1ce47 100644 --- a/src/backend/stores/appFeedback/AppFeedbackStore.ts +++ b/src/backend/stores/appFeedback/AppFeedbackStore.ts @@ -89,9 +89,10 @@ export class AppFeedbackStore extends PuterStore { // -- Writes ------------------------------------------------------- /** - * Insert one feedback row. `email_sent` starts false; the service flips it - * with {@link markEmailSent} after the owner email actually goes out, so the - * email cap only counts delivered mail. + * Insert one feedback row. `email_sent` starts false; the service claims it + * with {@link markEmailSent} before sending and releases it with + * {@link unmarkEmailSent} if the cap was breached or the send failed, so the + * email cap fails closed under concurrent submissions. */ async create(fields: { appId: number; @@ -127,11 +128,31 @@ export class AppFeedbackStore extends PuterStore { return { id: Number(insertId), uid }; } - /** Record that the owner email for this row was sent. */ + /** Record that the owner email for this row was sent (claim a cap slot). */ async markEmailSent(id: number): Promise { + await this.#setEmailSent(id, true); + } + + /** + * Revert {@link markEmailSent} — releases a claimed email-cap slot when the + * cap turned out breached or the send failed. + */ + async unmarkEmailSent(id: number): Promise { + await this.#setEmailSent(id, false); + } + + async #setEmailSent(id: number, sent: boolean): Promise { await this.clients.db.write( 'UPDATE `app_feedback` SET `email_sent` = ? WHERE `id` = ?', - [this.clients.db.booleanValue(true), id], + [this.clients.db.booleanValue(sent), id], + ); + } + + /** Delete one feedback row. Backs the service's cap-race rollback. */ + async deleteById(id: number): Promise { + await this.clients.db.write( + 'DELETE FROM `app_feedback` WHERE `id` = ?', + [id], ); } }