fix(usage): close the QA findings on opt-in product usage
A QA exploration of this branch against an isolated rig — own stub collector, SQLite and PostgreSQL — turned up one dead end and a set of signals and controls that did not hold up. This closes all of them. Rotating JWT_SECRET, the documented response to a suspected compromise, made the signing key unreadable. That was already named and documented, but it left no way out: the delete packet can never be signed, so the row stays deletion_pending forever, and enable() refuses because it is not `disabled`. An operator who rotated precisely because the secret was compromised cannot restore it, so the feature was bricked with no control left. POST /usage/abandon is offered only in that state; it drops the local identity and records the receipt as `collector-unconfirmed` rather than claiming a deletion that did not happen. Every failed delivery was retried on the next admin request, and /activity is open to any authenticated admin while the settings ticker fires it every five minutes per open tab — 30 activity calls against a rejecting collector produced 30 outbound requests. Migration 206 adds attempts/next_attempt_at and the unattended sender honours the gate; Retry and opt-out still send immediately, and the tab names the time of the next automatic attempt. Feedback, votes and portal sessions now share an installation-wide budget of 30/hour. They are the only endpoints whose effect is outbound traffic carrying operator-written free text, and the general limiter skips authenticated requests by design. Reading status and withdrawing stay unthrottled. gallery_image_protection was true on a bare install with no galleries: PicPeak ships default_protection_level='standard' and enable_devtools_protection=true, so it reported fleet-wide 100% and could never separate a decision from an untouched default. It now reads only what deviates from the shipped defaults, and the devtools flag is not read at all — being on by default, its only informative state is off, which is the opposite of what the key claims. Also: - the export receipt counted every packet and called the total "usage reports"; reports and participant operations are now counted and named separately - GET /usage/preview no longer persists the custom_css marker, so the transparency view stops changing what will be sent - the feedback route requires every field the packet schema requires, so an API caller gets the missing field named instead of a bare INVALID_PACKET from inside signing - the German strings for this feature use "Sie" throughout, matching the rest of the admin UI; the ignore hint says what ignoring will do rather than stating it as already true - the consent dialog returns focus to the control that opened it - the long buttons wrap instead of running off a 390px viewport - a deletion receipt is labelled as belonging to an earlier participation while a new one is active Regression tests cover each of these, including the delete packet's reuse of the last accepted sequence, which was an unwritten assumption about the collector rather than a defect.
This commit is contained in:
Paul Nothaft
@@ -31,6 +31,7 @@ jest.mock('../../src/services/productUsageService', () =>
|
||||
'dismiss',
|
||||
'enable',
|
||||
'disable',
|
||||
'abandon',
|
||||
'preview',
|
||||
'export',
|
||||
'preferences',
|
||||
@@ -125,6 +126,7 @@ const ROUTES = [
|
||||
['post', '/enable'],
|
||||
['post', '/consent'],
|
||||
['post', '/disable'],
|
||||
['post', '/abandon'],
|
||||
['post', '/retry'],
|
||||
['post', '/dismiss'],
|
||||
['get', '/preview'],
|
||||
@@ -230,3 +232,89 @@ test('only a backup that writes to the configured destination flags S3', () => {
|
||||
['/backup/picpeak/export', false],
|
||||
]);
|
||||
});
|
||||
|
||||
// The route allowlist and the packet schema have to agree. The allowlist used
|
||||
// to let `name`, `allow_public` and `allow_marketing` be omitted while the
|
||||
// schema requires all three, so an API caller got a bare INVALID_PACKET from
|
||||
// deep inside signing instead of being told which field was missing.
|
||||
const VALID_FEEDBACK = {
|
||||
kind: 'feedback',
|
||||
title: 'Title',
|
||||
body: 'Body',
|
||||
name: '',
|
||||
allow_public: false,
|
||||
allow_marketing: false
|
||||
};
|
||||
test.each([
|
||||
['no body at all', {}],
|
||||
['missing name', { ...VALID_FEEDBACK, name: undefined }],
|
||||
['missing allow_public', { ...VALID_FEEDBACK, allow_public: undefined }],
|
||||
['missing allow_marketing', { ...VALID_FEEDBACK, allow_marketing: undefined }],
|
||||
['a boolean sent as a string', { ...VALID_FEEDBACK, allow_public: 'true' }],
|
||||
['a title of only whitespace', { ...VALID_FEEDBACK, title: ' ' }],
|
||||
['an unknown field', { ...VALID_FEEDBACK, ownerId: 7 }]
|
||||
])('feedback rejects %s before anything is signed', async (_label, data) => {
|
||||
const response = await request(app)
|
||||
.post('/api/admin/usage/feedback')
|
||||
.set('Authorization', `Bearer ${token('admin')}`)
|
||||
.send(JSON.parse(JSON.stringify(data)))
|
||||
.expect(400);
|
||||
// Named, not a bare protocol failure the caller cannot act on.
|
||||
expect(response.body.code).toBe('VALIDATION_ERROR');
|
||||
expect(service.command).not.toHaveBeenCalled();
|
||||
});
|
||||
test('feedback accepts the complete payload and mints the id server-side', async () => {
|
||||
await request(app)
|
||||
.post('/api/admin/usage/feedback')
|
||||
.set('Authorization', `Bearer ${token('admin')}`)
|
||||
.send({ ...VALID_FEEDBACK, name: 'QA' })
|
||||
.expect(200);
|
||||
expect(service.command).toHaveBeenCalledWith(
|
||||
'feedback',
|
||||
expect.objectContaining({ name: 'QA', feedback_id: expect.any(String) })
|
||||
);
|
||||
});
|
||||
|
||||
// Runs last on purpose: the limiter's budget is per-process and shared with
|
||||
// every test above that reaches an outbound route, so consuming it here cannot
|
||||
// starve them. The assertion is deliberately about the property — some request
|
||||
// is refused and the service stops being called — rather than an exact count,
|
||||
// which would depend on how much budget earlier tests used.
|
||||
test('the outbound routes are throttled so an admin session cannot flood the collector', async () => {
|
||||
const codes = [];
|
||||
for (let i = 0; i < 45; i += 1) {
|
||||
const response = await request(app)
|
||||
.post('/api/admin/usage/feedback')
|
||||
.set('Authorization', `Bearer ${token('admin')}`)
|
||||
.send({ ...VALID_FEEDBACK, title: `flood ${i}` });
|
||||
codes.push(response.status);
|
||||
if (response.status === 429) {
|
||||
expect(response.body.code).toBe('USAGE_RATE_LIMITED');
|
||||
break;
|
||||
}
|
||||
}
|
||||
expect(codes).toContain(429);
|
||||
expect(service.command.mock.calls.length).toBeLessThan(codes.length);
|
||||
|
||||
// The same budget covers the other two routes that relay to the collector.
|
||||
await request(app)
|
||||
.post('/api/admin/usage/vote')
|
||||
.set('Authorization', `Bearer ${token('admin')}`)
|
||||
.send({ feedback_id: '11111111-1111-4111-8111-111111111111', voted: true })
|
||||
.expect(429);
|
||||
await request(app)
|
||||
.post('/api/admin/usage/portal-session')
|
||||
.set('Authorization', `Bearer ${token('admin')}`)
|
||||
.expect(429);
|
||||
|
||||
// Reading status and withdrawing must never be throttled: those are how an
|
||||
// operator sees what is happening and how they get out.
|
||||
await request(app)
|
||||
.get('/api/admin/usage')
|
||||
.set('Authorization', `Bearer ${token('admin')}`)
|
||||
.expect(200);
|
||||
await request(app)
|
||||
.post('/api/admin/usage/disable')
|
||||
.set('Authorization', `Bearer ${token('admin')}`)
|
||||
.expect(200);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user