From 1460751b4109337b450131f9571f3d01dae55668 Mon Sep 17 00:00:00 2001 From: Nariman Jelveh Date: Tue, 11 Aug 2026 18:16:14 -0700 Subject: [PATCH] fix: refuse feedback when the deployment cannot deliver it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With no email transport configured (the common self-hosted default), submissions were stored in app_feedback — a table with no read path beyond the abuse-cap COUNTs — the owner email was silently skipped, and the sender was still shown 'Feedback sent. Thank you!'. The developer never learns the feedback exists while the user believes it was delivered. Gate acceptsFeedback on clients.email.isConfigured so the pre-flight reports enabled:false (the dialog shows its 'not accepting feedback' pane) and submit returns 403 instead of swallowing messages. Owner-level store-without-email cases (unconfirmed owner email, per-app email cap overflow) keep their existing deliberate semantics. --- .../feedback/AppFeedbackController.test.ts | 54 +++++++++++++++++-- .../services/feedback/AppFeedbackService.ts | 13 ++++- 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/src/backend/controllers/feedback/AppFeedbackController.test.ts b/src/backend/controllers/feedback/AppFeedbackController.test.ts index b23bf1bab..743c7ec15 100644 --- a/src/backend/controllers/feedback/AppFeedbackController.test.ts +++ b/src/backend/controllers/feedback/AppFeedbackController.test.ts @@ -99,6 +99,14 @@ const makeApp = async ( ); }; +// Feedback is only offered when the deployment can deliver it (email +// transport configured); most tests want that baseline without asserting +// anything about the mail itself. +const mockEmailConfigured = () => + vi.spyOn(server.clients.email, 'isConfigured', 'get').mockReturnValue( + true, + ); + const confirmOwnerEmail = async (userId: number) => { await server.clients.db.write( 'UPDATE `user` SET `email_confirmed` = ? WHERE `id` = ?', @@ -222,6 +230,7 @@ describe('AppFeedbackController GET /target', () => { }); it('reports enabled:false for an app that has not opted in', async () => { + mockEmailConfigured(); const { userId: ownerId } = await makeUser(); const app = await makeApp(ownerId); const { actor } = await makeUser(); @@ -236,6 +245,7 @@ describe('AppFeedbackController GET /target', () => { }); it('reports enabled:true with canonical title/name for an opted-in app', async () => { + mockEmailConfigured(); const { userId: ownerId } = await makeUser(); const app = await makeApp(ownerId, { feedbackEnabled: true }); const { actor } = await makeUser(); @@ -253,6 +263,7 @@ describe('AppFeedbackController GET /target', () => { }); it('resolves an opted-in app whose name starts with "app-"', async () => { + mockEmailConfigured(); const { userId: ownerId } = await makeUser(); const name = `app-fdbk-${Math.random().toString(36).slice(2, 10)}`; const app = await makeApp(ownerId, { feedbackEnabled: true, name }); @@ -271,6 +282,7 @@ describe('AppFeedbackController GET /target', () => { }); it('resolves an origin to the app whose index_url it matches', async () => { + mockEmailConfigured(); const { userId: ownerId } = await makeUser(); const app = await makeApp(ownerId, { feedbackEnabled: true }); const origin = new URL(app.index_url).origin; @@ -288,6 +300,22 @@ describe('AppFeedbackController GET /target', () => { }); }); + it('reports enabled:false when the email transport is unconfigured', async () => { + // No mockEmailConfigured(): this is the self-hosted no-SMTP default. + // Feedback that can never be delivered must not be solicited. + const { userId: ownerId } = await makeUser(); + const app = await makeApp(ownerId, { feedbackEnabled: true }); + const { actor } = await makeUser(); + const { res, captured } = makeRes(); + await callRoute( + 'get', + '/target', + makeReq({ query: { app: app.name }, actor }), + res, + ); + expect(captured.body).toMatchObject({ enabled: false }); + }); + it('reports enabled:false for an origin with no registered app', async () => { const { actor } = await makeUser(); const { res, captured } = makeRes(); @@ -330,6 +358,7 @@ describe('AppFeedbackController POST /', () => { }); it('throws 403 feedback_not_enabled when the app has not opted in', async () => { + mockEmailConfigured(); const { userId: ownerId } = await makeUser(); const app = await makeApp(ownerId); const { actor } = await makeUser(); @@ -354,7 +383,23 @@ describe('AppFeedbackController POST /', () => { ).rejects.toMatchObject({ statusCode: 403 }); }); + it('throws 403 feedback_not_enabled when the email transport is unconfigured', async () => { + // No mockEmailConfigured(): the opted-in app must still refuse — a + // stored row nothing can read, sold to the sender as delivered, is + // worse than an honest refusal. + const { userId: ownerId } = await makeUser(); + const app = await makeApp(ownerId, { feedbackEnabled: true }); + const { actor } = await makeUser(); + await expect( + submit(actor, { app: app.name, message: 'into the void' }), + ).rejects.toMatchObject({ + statusCode: 403, + legacyCode: 'feedback_not_enabled', + }); + }); + it('throws 400 when the message exceeds the length limit', async () => { + mockEmailConfigured(); const { userId: ownerId } = await makeUser(); const app = await makeApp(ownerId, { feedbackEnabled: true }); const { actor } = await makeUser(); @@ -369,6 +414,7 @@ describe('AppFeedbackController POST /', () => { }); it('stores a normalized row and responds with an empty object', async () => { + mockEmailConfigured(); const { userId: ownerId } = await makeUser(); const app = await makeApp(ownerId, { feedbackEnabled: true }); const { actor, userId } = await makeUser(); @@ -399,6 +445,7 @@ describe('AppFeedbackController POST /', () => { }); it('records the attested origin on web submissions', async () => { + mockEmailConfigured(); const { userId: ownerId } = await makeUser(); const app = await makeApp(ownerId, { feedbackEnabled: true }); const origin = new URL(app.index_url).origin; @@ -417,6 +464,7 @@ describe('AppFeedbackController POST /', () => { }); it('enforces the per-user-per-app daily cap with 429', async () => { + mockEmailConfigured(); const { userId: ownerId } = await makeUser(); const app = await makeApp(ownerId, { feedbackEnabled: true }); const { actor, userId } = await makeUser(); @@ -445,6 +493,7 @@ describe('AppFeedbackController POST /', () => { }); it('rolls back the stored row when a concurrent burst breaches the cap', async () => { + mockEmailConfigured(); const { userId: ownerId } = await makeUser(); const app = await makeApp(ownerId, { feedbackEnabled: true }); const { actor, userId } = await makeUser(); @@ -481,6 +530,7 @@ describe('AppFeedbackController POST /', () => { }); it('enforces the per-user daily cap across apps with 429', async () => { + mockEmailConfigured(); const { userId: ownerId } = await makeUser(); const target = await makeApp(ownerId, { feedbackEnabled: true }); const other = await makeApp(ownerId, { feedbackEnabled: true }); @@ -503,9 +553,7 @@ describe('AppFeedbackController POST /', () => { describe('AppFeedbackService owner email', () => { const mockEmailReady = () => { - vi.spyOn(server.clients.email, 'isConfigured', 'get').mockReturnValue( - true, - ); + mockEmailConfigured(); return vi .spyOn(server.clients.email, 'send') .mockResolvedValue(undefined); diff --git a/src/backend/services/feedback/AppFeedbackService.ts b/src/backend/services/feedback/AppFeedbackService.ts index 9481971f0..d40343243 100644 --- a/src/backend/services/feedback/AppFeedbackService.ts +++ b/src/backend/services/feedback/AppFeedbackService.ts @@ -114,10 +114,19 @@ export class AppFeedbackService extends PuterService { /** * Whether `app` (a row from AppStore) currently accepts user feedback: the - * developer opted in and the app has an owner to deliver to. + * developer opted in, the app has an owner to deliver to, and this + * deployment can deliver at all (email transport configured). Without a + * transport every submission would be stored-and-lost — the rows have no + * other read path — while the sender is told it was sent, so the feature + * reports itself unavailable instead. */ acceptsFeedback(app: Record | null): boolean { - return Boolean(app && app.feedback_enabled && app.owner_user_id); + return Boolean( + this.clients.email.isConfigured && + app && + app.feedback_enabled && + app.owner_user_id, + ); } /**