mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
fix(local-rest-api): validate PATCH/create field value types (#8738)
PATCH /tasks/:id picked allowed keys but never checked value types, so a wrong-typed value (e.g. tagIds:123, timeEstimate:"abc") was written into the store and the synced op-log, corrupting state locally and tripping typia-as-corrupt on other devices when the op replays. Both PATCH and create now typia.validate the values and return 400 before dispatching. Also harden the Electron server: reset isListening eagerly and call closeAllConnections() on stop so a keep-alive socket can't leave the server stuck and no-op a later re-enable, reject requests with 503 while disabled, and log an actionable message on EADDRINUSE. Closes #8732 Refs #7484
This commit is contained in:
parent
25d7d6a379
commit
08d13fd7d5
4 changed files with 153 additions and 7 deletions
|
|
@ -397,10 +397,10 @@ curl http://127.0.0.1:3876/tags
|
|||
|
||||
| Code | HTTP Status | Description |
|
||||
| ---------------- | ----------- | --------------------- |
|
||||
| `TASK_NOT_FOUND` | 404 | Task does not exist |
|
||||
| `INVALID_INPUT` | 400 | Invalid request body |
|
||||
| `NOT_FOUND` | 404 | Route not found |
|
||||
| `INTERNAL_ERROR` | 500 | Internal server error |
|
||||
| `TASK_NOT_FOUND` | 404 | Task does not exist |
|
||||
| `INVALID_INPUT` | 400 | Invalid request body, or a field has the wrong value type (see `error.details`) |
|
||||
| `NOT_FOUND` | 404 | Route not found |
|
||||
| `INTERNAL_ERROR` | 500 | Internal server error |
|
||||
|
||||
---
|
||||
|
||||
|
|
|
|||
|
|
@ -122,6 +122,20 @@ const handleHttpRequest = async (
|
|||
req: IncomingMessage,
|
||||
res: ServerResponse,
|
||||
): Promise<void> => {
|
||||
// Reject everything while disabled. server.close() stops accepting new
|
||||
// sockets, but an in-flight keep-alive connection could still be served
|
||||
// during the close window; this makes the off switch immediate.
|
||||
if (!isEnabled) {
|
||||
writeJson(res, 503, {
|
||||
ok: false,
|
||||
error: {
|
||||
code: 'API_DISABLED',
|
||||
message: 'Local REST API is disabled',
|
||||
},
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
// Block DNS rebinding: reject requests with unexpected Host headers
|
||||
const host = req.headers.host;
|
||||
if (!host || !ALLOWED_HOSTS.has(host)) {
|
||||
|
|
@ -237,8 +251,15 @@ export const initLocalRestApi = (): void => {
|
|||
void handleHttpRequest(req, res);
|
||||
});
|
||||
|
||||
server.on('error', (error) => {
|
||||
server.on('error', (error: NodeJS.ErrnoException) => {
|
||||
isListening = false;
|
||||
if (error.code === 'EADDRINUSE') {
|
||||
warn(
|
||||
`[local-rest-api] Port ${LOCAL_REST_API_PORT} is in use — API could not start. ` +
|
||||
`Another process is holding it; free it and toggle the API off/on to retry.`,
|
||||
);
|
||||
return;
|
||||
}
|
||||
warn('[local-rest-api] Server error', error);
|
||||
});
|
||||
|
||||
|
|
@ -267,15 +288,23 @@ const stopServer = (): void => {
|
|||
return;
|
||||
}
|
||||
|
||||
// Reset eagerly: server.close() only invokes its callback once every socket
|
||||
// has closed, so a lingering keep-alive connection would otherwise leave
|
||||
// isListening=true forever and make a later re-enable a no-op (#7484).
|
||||
isListening = false;
|
||||
|
||||
server.close((error) => {
|
||||
if (error) {
|
||||
warn('[local-rest-api] Failed to stop server', error);
|
||||
return;
|
||||
}
|
||||
|
||||
isListening = false;
|
||||
log('[local-rest-api] Server stopped');
|
||||
});
|
||||
|
||||
// Force keep-alive sockets shut so the API stops serving immediately on
|
||||
// disable and close() can actually complete.
|
||||
server.closeAllConnections();
|
||||
};
|
||||
|
||||
export const updateLocalRestApiConfig = (cfg: GlobalConfigState): void => {
|
||||
|
|
|
|||
|
|
@ -537,6 +537,19 @@ describe('LocalRestApiHandlerService', () => {
|
|||
});
|
||||
});
|
||||
|
||||
it('should return 400 when an allowed field has an invalid type', async () => {
|
||||
const response = await sendRequestAndWait(
|
||||
createRequest('POST', '/tasks', {
|
||||
body: { title: 'New Task', timeEstimate: 'not-a-number' },
|
||||
}),
|
||||
);
|
||||
|
||||
expect(response.body.ok).toBe(false);
|
||||
expect(response.status).toBe(400);
|
||||
expect((response.body as any).error.code).toBe('INVALID_INPUT');
|
||||
expect(taskServiceMock.add).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should reject subTaskIds in body with 400', async () => {
|
||||
const response = await sendRequestAndWait(
|
||||
createRequest('POST', '/tasks', {
|
||||
|
|
@ -780,6 +793,44 @@ describe('LocalRestApiHandlerService', () => {
|
|||
expect((response.body as any).error.code).toBe('UNSUPPORTED_FIELD');
|
||||
expect(taskServiceMock.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should return 400 (not dispatch) when a field has an invalid type', async () => {
|
||||
const mockTask = createMockTask('task-1');
|
||||
Object.defineProperty(taskServiceMock, 'getByIdOnce$', {
|
||||
get: () => (_id: string) => of(mockTask),
|
||||
});
|
||||
|
||||
const response = await sendRequestAndWait(
|
||||
createRequest('PATCH', '/tasks/task-1', {
|
||||
body: { tagIds: 123, timeEstimate: 'abc' },
|
||||
}),
|
||||
);
|
||||
|
||||
expect(response.body.ok).toBe(false);
|
||||
expect(response.status).toBe(400);
|
||||
expect((response.body as any).error.code).toBe('INVALID_INPUT');
|
||||
expect(taskServiceMock.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should accept valid typed fields and dispatch them', async () => {
|
||||
const mockTask = createMockTask('task-1');
|
||||
Object.defineProperty(taskServiceMock, 'getByIdOnce$', {
|
||||
get: () => (_id: string) => of(mockTask),
|
||||
});
|
||||
|
||||
const response = await sendRequestAndWait(
|
||||
createRequest('PATCH', '/tasks/task-1', {
|
||||
body: { tagIds: ['tag-1'], timeEstimate: 1000, isDone: true },
|
||||
}),
|
||||
);
|
||||
|
||||
expect(response.body.ok).toBe(true);
|
||||
expect(taskServiceMock.update).toHaveBeenCalledWith('task-1', {
|
||||
tagIds: ['tag-1'],
|
||||
timeEstimate: 1000,
|
||||
isDone: true,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('DELETE /tasks/:id', () => {
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import { Injectable, inject } from '@angular/core';
|
||||
import { firstValueFrom } from 'rxjs';
|
||||
import typia from 'typia';
|
||||
import { TaskService } from '../../features/tasks/task.service';
|
||||
import { Task, TaskWithSubTasks } from '../../features/tasks/task.model';
|
||||
import { TaskArchiveService } from '../../features/archive/task-archive.service';
|
||||
|
|
@ -56,6 +57,48 @@ const pickAllowedFields = (body: Record<string, unknown>): Partial<Task> => {
|
|||
return result as Partial<Task>;
|
||||
};
|
||||
|
||||
/**
|
||||
* Value-level types for the fields writable via the REST API. Keys mirror
|
||||
* ALLOWED_TASK_FIELDS; `pickAllowedFields` filters by key only, so this is
|
||||
* where the *values* get checked. Without it a caller could push a wrong-typed
|
||||
* value (e.g. `tagIds: 123`, `timeEstimate: 'abc'`) straight into the store and
|
||||
* the synced op-log, where it corrupts state locally and trips typia-as-corrupt
|
||||
* on other devices when the op replays.
|
||||
*/
|
||||
interface WritableTaskFields {
|
||||
title?: string;
|
||||
notes?: string;
|
||||
isDone?: boolean;
|
||||
timeEstimate?: number;
|
||||
timeSpent?: number;
|
||||
projectId?: string;
|
||||
tagIds?: string[];
|
||||
dueDay?: string | null;
|
||||
dueWithTime?: number | null;
|
||||
plannedAt?: number;
|
||||
}
|
||||
|
||||
type FieldTypeError = { path: string; expected: string };
|
||||
|
||||
/**
|
||||
* Validates the value types of already-key-filtered task fields. The create
|
||||
* path is separately guarded by `typia.assert<Task>` in the task service (a
|
||||
* bad value throws → generic 500); validating here lets both create and PATCH
|
||||
* reject bad input with a clean 400 before anything is dispatched.
|
||||
*/
|
||||
const validateWritableFields = (
|
||||
fields: Partial<Task>,
|
||||
): { ok: true } | { ok: false; errors: FieldTypeError[] } => {
|
||||
const result = typia.validate<WritableTaskFields>(fields);
|
||||
if (result.success) {
|
||||
return { ok: true };
|
||||
}
|
||||
return {
|
||||
ok: false,
|
||||
errors: result.errors.map((e) => ({ path: e.path, expected: e.expected })),
|
||||
};
|
||||
};
|
||||
|
||||
const firstRejectedField = (body: Record<string, unknown>): string | undefined =>
|
||||
REJECTED_TASK_FIELDS.find((field) => field in body);
|
||||
|
||||
|
|
@ -351,6 +394,17 @@ export class LocalRestApiHandlerService {
|
|||
const title = body.title.trim();
|
||||
const additionalFields = pickAllowedFields(body);
|
||||
|
||||
const validation = validateWritableFields(additionalFields);
|
||||
if (!validation.ok) {
|
||||
return createErrorResponse(
|
||||
requestId,
|
||||
400,
|
||||
'INVALID_INPUT',
|
||||
'One or more task fields have an invalid type',
|
||||
validation.errors,
|
||||
);
|
||||
}
|
||||
|
||||
if ('parentId' in body) {
|
||||
if (typeof body.parentId !== 'string' || !body.parentId) {
|
||||
return createErrorResponse(
|
||||
|
|
@ -441,12 +495,24 @@ export class LocalRestApiHandlerService {
|
|||
);
|
||||
}
|
||||
|
||||
const changes = pickAllowedFields(body);
|
||||
const validation = validateWritableFields(changes);
|
||||
if (!validation.ok) {
|
||||
return createErrorResponse(
|
||||
requestId,
|
||||
400,
|
||||
'INVALID_INPUT',
|
||||
'One or more task fields have an invalid type',
|
||||
validation.errors,
|
||||
);
|
||||
}
|
||||
|
||||
const task = await this._getTaskById(taskId);
|
||||
if (!task) {
|
||||
return createErrorResponse(requestId, 404, 'TASK_NOT_FOUND', 'Task not found');
|
||||
}
|
||||
|
||||
this._taskService.update(taskId, pickAllowedFields(body));
|
||||
this._taskService.update(taskId, changes);
|
||||
return createSuccessResponse(requestId, 200, await this._getTaskById(taskId));
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue