fix(security): harden Local REST API against DNS rebinding and field injection (#6996)

* fix(security): harden Local REST API against DNS rebinding and field injection - Add Host header validation to block DNS rebinding attacks from malicious websites targeting the localhost API - Add IS_ELECTRON guard in handler init() for defense-in-depth - Add allowlist of task fields settable via API (title, notes, isDone, timeEstimate, timeSpent, projectId, tagIds, dueDay, dueWithTime, plannedAt) to prevent state corruption via injected internal fields - Validate source query parameter instead of blind cast - Add tests verifying disallowed fields are stripped from POST and PATCH https://claude.ai/code/session_01YJNc4gUCkHHrAAhXJBEaop * fix: address review findings from daily code review Security hardening: - Block cloud metadata endpoints (169.254.169.254, metadata.google.internal) even when allowPrivateNetwork is true (SSRF protection) - Add HTTP method allowlist to PluginHttp request() to prevent abuse - Add rate limiting (max 50 concurrent) to Local REST API Bug fixes: - Fix hardcoded 'ICAL' issueProviderKey in calendar effects; use event's actual provider key with ICAL fallback - Fix race condition in TimeBlockDeleteSidecarService: push IDs instead of replacing to prevent data loss on rapid bulk deletes - Backfill missing issueProviderKey on cached calendar events from localStorage (backward compatibility) - Fix btoa(String.fromCharCode(...spread)) overflow for long strings by using loop-based approach Minor: - Cap HiddenCalendarEventsService to 500 entries max (prevent unbounded localStorage growth) - Fix misleading "deep clone" comment to "shallow clone" https://claude.ai/code/session_01YJNc4gUCkHHrAAhXJBEaop * test: fix unit test failures from review hardening changes - Replace IS_ELECTRON guard with window.ea?.onLocalRestApiRequest check in handler init() so tests can mock the API without navigator.userAgent - Update plugin-http spec to expect "cloud metadata endpoints" error message (was "private/local") for 169.254.169.254 and metadata.google - Fix allowlist test expectation: title is an allowed field and passes through pickAllowedFields as expected https://claude.ai/code/session_01YJNc4gUCkHHrAAhXJBEaop --------- Co-authored-by: Claude <noreply@anthropic.com>
2026-07-17 16:37:43 +00:00 · 2026-03-29 11:59:27 +02:00 · 2026-03-29 11:59:27 +02:00 · 4b0550be35
commit 4b0550be35
parent 0b19712b53
12 changed files with 175 additions and 19 deletions
--- a/electron/local-rest-api.ts
+++ b/electron/local-rest-api.ts
@ -8,6 +8,7 @@ import { GlobalConfigState } from '../src/app/features/config/global-config.mode
 import {
  LOCAL_REST_API_HOST,
  LOCAL_REST_API_MAX_BODY_BYTES,
+  LOCAL_REST_API_MAX_CONCURRENT_REQUESTS,
  LOCAL_REST_API_PORT,
  LOCAL_REST_API_TIMEOUT_MS,
  LocalRestApiRequestPayload,
@ -107,10 +108,41 @@ const handleResponse = (_event: unknown, payload: LocalRestApiResponsePayload):
  pending.resolve(payload);
 };

+const ALLOWED_HOSTS = new Set([
+  `${LOCAL_REST_API_HOST}:${LOCAL_REST_API_PORT}`,
+  `localhost:${LOCAL_REST_API_PORT}`,
+  LOCAL_REST_API_HOST,
+  'localhost',
+]);
+
 const handleHttpRequest = async (
  req: IncomingMessage,
  res: ServerResponse,
 ): Promise<void> => {
+  // Block DNS rebinding: reject requests with unexpected Host headers
+  const host = req.headers.host;
+  if (!host || !ALLOWED_HOSTS.has(host)) {
+    writeJson(res, 403, {
+      ok: false,
+      error: {
+        code: 'FORBIDDEN',
+        message: 'Invalid Host header',
+      },
+    });
+    return;
+  }
+
+  if (pendingRequests.size >= LOCAL_REST_API_MAX_CONCURRENT_REQUESTS) {
+    writeJson(res, 429, {
+      ok: false,
+      error: {
+        code: 'TOO_MANY_REQUESTS',
+        message: `Too many concurrent requests (limit: ${LOCAL_REST_API_MAX_CONCURRENT_REQUESTS})`,
+      },
+    });
+    return;
+  }
+
  const requestUrl = new URL(req.url ?? '/', `http://${LOCAL_REST_API_HOST}`);
  const method = req.method ?? 'GET';

--- a/electron/shared-with-frontend/local-rest-api.model.ts
+++ b/electron/shared-with-frontend/local-rest-api.model.ts
@ -2,6 +2,7 @@ export const LOCAL_REST_API_HOST = '127.0.0.1';
 export const LOCAL_REST_API_PORT = 3876;
 export const LOCAL_REST_API_TIMEOUT_MS = 15000;
 export const LOCAL_REST_API_MAX_BODY_BYTES = 1024 * 1024;
+export const LOCAL_REST_API_MAX_CONCURRENT_REQUESTS = 50;

 export interface LocalRestApiRequestPayload {
  requestId: string;
--- a/src/app/core/electron/local-rest-api-handler.service.spec.ts
+++ b/src/app/core/electron/local-rest-api-handler.service.spec.ts
@ -339,6 +339,29 @@ describe('LocalRestApiHandlerService', () => {
        expect(response.body.ok).toBe(false);
        expect(response.status).toBe(400);
      });
+
+      it('should strip disallowed fields from the body', async () => {
+        Object.defineProperty(taskServiceMock, 'getByIdOnce$', {
+          get: () => (_id: string) => of(createMockTask('new-task-id')),
+        });
+
+        await sendRequestAndWait(
+          createRequest('POST', '/tasks', {
+            body: {
+              title: 'New Task',
+              notes: 'allowed',
+              id: 'injected-id',
+              subTaskIds: ['injected'],
+              parentId: 'injected-parent',
+            },
+          }),
+        );
+
+        expect(taskServiceMock.add).toHaveBeenCalledWith('New Task', false, {
+          title: 'New Task',
+          notes: 'allowed',
+        });
+      });
    });

    describe('GET /tasks/:id', () => {
@ -408,6 +431,28 @@ describe('LocalRestApiHandlerService', () => {
        expect(response.body.ok).toBe(false);
        expect(response.status).toBe(400);
      });
+
+      it('should strip disallowed fields from PATCH body', async () => {
+        const mockTask = createMockTask('task-1');
+        Object.defineProperty(taskServiceMock, 'getByIdOnce$', {
+          get: () => (_id: string) => of(mockTask),
+        });
+
+        await sendRequestAndWait(
+          createRequest('PATCH', '/tasks/task-1', {
+            body: {
+              title: 'Updated',
+              id: 'injected-id',
+              subTaskIds: ['injected'],
+              parentId: 'injected-parent',
+            },
+          }),
+        );
+
+        expect(taskServiceMock.update).toHaveBeenCalledWith('task-1', {
+          title: 'Updated',
+        });
+      });
    });

    describe('DELETE /tasks/:id', () => {
--- a/src/app/core/electron/local-rest-api-handler.service.ts
+++ b/src/app/core/electron/local-rest-api-handler.service.ts
@ -13,6 +13,30 @@ import {
 const isRecord = (value: unknown): value is Record<string, unknown> =>
  typeof value === 'object' && value !== null && !Array.isArray(value);

+/** Only these fields may be set via the REST API to prevent state corruption. */
+const ALLOWED_TASK_FIELDS = new Set<string>([
+  'title',
+  'notes',
+  'isDone',
+  'timeEstimate',
+  'timeSpent',
+  'projectId',
+  'tagIds',
+  'dueDay',
+  'dueWithTime',
+  'plannedAt',
+]);
+
+const pickAllowedFields = (body: Record<string, unknown>): Partial<Task> => {
+  const result: Record<string, unknown> = {};
+  for (const key of Object.keys(body)) {
+    if (ALLOWED_TASK_FIELDS.has(key)) {
+      result[key] = body[key];
+    }
+  }
+  return result as Partial<Task>;
+};
+
 const getQueryParam = (
  query: Record<string, string | string[]>,
  key: string,
@ -77,7 +101,7 @@ export class LocalRestApiHandlerService {
  private _isInitialized = false;

  init(): void {
-    if (this._isInitialized) {
+    if (this._isInitialized || !window.ea?.onLocalRestApiRequest) {
      return;
    }
    this._isInitialized = true;
@ -222,7 +246,11 @@ export class LocalRestApiHandlerService {
    const projectId = getQueryParam(query, 'projectId');
    const tagId = getQueryParam(query, 'tagId');
    const includeDone = getQueryParamAsBoolean(query, 'includeDone', false);
-    const source = (getQueryParam(query, 'source') as TaskSource) || 'active';
+    const VALID_SOURCES: TaskSource[] = ['active', 'archived', 'all'];
+    const rawSource = getQueryParam(query, 'source') || 'active';
+    const source: TaskSource = VALID_SOURCES.includes(rawSource as TaskSource)
+      ? (rawSource as TaskSource)
+      : 'active';

    let tasks: Task[];

@ -271,9 +299,8 @@ export class LocalRestApiHandlerService {
    }

    const title = body.title.trim();
-    const additionalFields = { ...body };
-    delete additionalFields.title;
-    const taskId = this._taskService.add(title, false, additionalFields as Partial<Task>);
+    const additionalFields = pickAllowedFields(body);
+    const taskId = this._taskService.add(title, false, additionalFields);
    const createdTask = await this._getTaskById(taskId);

    return createSuccessResponse(requestId, 201, createdTask);
@ -311,7 +338,7 @@ export class LocalRestApiHandlerService {
          return createErrorResponse(requestId, 404, 'TASK_NOT_FOUND', 'Task not found');
        }

-        this._taskService.update(taskId, body as Partial<Task>);
+        this._taskService.update(taskId, pickAllowedFields(body));
        return createSuccessResponse(requestId, 200, await this._getTaskById(taskId));
      }

--- a/src/app/features/calendar-integration/calendar-integration.service.ts
+++ b/src/app/features/calendar-integration/calendar-integration.service.ts
@ -374,7 +374,12 @@ export class CalendarIntegrationService {
        // filter out cached past entries
        .map((provider) => ({
          ...provider,
-          items: provider.items.filter((item) => item.start + item.duration >= now),
+          items: provider.items
+            .filter((item) => item.start + item.duration >= now)
+            // Backfill issueProviderKey for events cached before it became required
+            .map((item) =>
+              item.issueProviderKey ? item : { ...item, issueProviderKey: 'ICAL' },
+            ),
        }))
    );
  }
--- a/src/app/features/calendar-integration/hidden-calendar-events.service.ts
+++ b/src/app/features/calendar-integration/hidden-calendar-events.service.ts
@ -8,6 +8,8 @@ import { getCalendarEventIdCandidates } from './get-calendar-event-id-candidates
  providedIn: 'root',
 })
 export class HiddenCalendarEventsService {
+  private static readonly _MAX_HIDDEN_EVENT_IDS = 500;
+
  readonly hiddenEventIds$ = new BehaviorSubject<string[]>(this._loadFromStorage());

  hideEvent(calEv: CalendarIntegrationEvent): void {
@ -18,7 +20,12 @@ export class HiddenCalendarEventsService {
      return;
    }
    const current = this.hiddenEventIds$.getValue();
-    const updated = [...current, ...idsToAdd.filter((id) => !current.includes(id))];
+    let updated = [...current, ...idsToAdd.filter((id) => !current.includes(id))];
+    if (updated.length > HiddenCalendarEventsService._MAX_HIDDEN_EVENT_IDS) {
+      updated = updated.slice(
+        updated.length - HiddenCalendarEventsService._MAX_HIDDEN_EVENT_IDS,
+      );
+    }
    this.hiddenEventIds$.next(updated);
    this._saveToStorage(updated);
  }
--- a/src/app/features/calendar-integration/store/calendar-integration.effects.ts
+++ b/src/app/features/calendar-integration/store/calendar-integration.effects.ts
@ -17,7 +17,7 @@ import { isValidUrl } from '../../../util/is-valid-url';
 import { getPluralKey } from '../../../util/get-plural-key';
 import { distinctUntilChangedObject } from '../../../util/distinct-until-changed-object';
 import { selectCalendarProviders } from '../../issue/store/issue-provider.selectors';
-import { IssueProviderCalendar } from '../../issue/issue.model';
+import { IssueProviderCalendar, IssueProviderKey } from '../../issue/issue.model';
 import { IssueService } from '../../issue/issue.service';
 import { DateService } from '../../../core/date/date.service';
 import { TaskService } from '../../tasks/task.service';
@ -97,7 +97,8 @@ export class CalendarIntegrationEffects {
                            !matchesAnyCalendarEventId(calEv, allIssueIds)
                          ) {
                            this._issueService.addTaskFromIssue({
-                              issueProviderKey: 'ICAL',
+                              issueProviderKey:
+                                (calEv.issueProviderKey as IssueProviderKey) || 'ICAL',
                              issueProviderId: calProvider.id,
                              issueDataReduced: calEv,
                              // from this context we should always add to the default project rather than current context
@ -235,7 +236,7 @@ export class CalendarIntegrationEffects {
            fn: () => {
              this._skipEv(calEv);
              this._issueService.addTaskFromIssue({
-                issueProviderKey: 'ICAL',
+                issueProviderKey: (calEv.issueProviderKey as IssueProviderKey) || 'ICAL',
                issueProviderId: calProvider.id,
                issueDataReduced: calEv,
                // from the banner we should always add to the default project rather than current context
--- a/src/app/features/calendar-integration/time-block/time-block-delete-sidecar.service.ts
+++ b/src/app/features/calendar-integration/time-block/time-block-delete-sidecar.service.ts
@ -10,7 +10,7 @@ export class TimeBlockDeleteSidecarService {
  private _pendingTaskIds: string[] = [];

  set(taskIds: string[]): void {
-    this._pendingTaskIds = taskIds;
+    this._pendingTaskIds.push(...taskIds);
  }

  consume(): string[] {
--- a/src/app/features/issue/dialog-edit-issue-provider/dialog-edit-issue-provider.component.ts
+++ b/src/app/features/issue/dialog-edit-issue-provider/dialog-edit-issue-provider.component.ts
@ -480,7 +480,7 @@ export class DialogEditIssueProviderComponent {
    const pluginConfig = (model as { pluginConfig?: Record<string, unknown> })
      .pluginConfig;
    if (!pluginConfig) return model;
-    // Deep-clone pluginConfig to avoid mutating store state
+    // Shallow-clone pluginConfig to avoid mutating store state
    const migrated = { ...pluginConfig };
    // Migrate old single-calendar calendarId to multi-calendar config shape
    if (
--- a/src/app/features/schedule/ical/get-relevant-events-from-ical.ts
+++ b/src/app/features/schedule/ical/get-relevant-events-from-ical.ts
@ -166,7 +166,12 @@ const generateGoogleCalendarEventUrl = (
  try {
    const cleanId = eventId.replace(/@(group\.calendar\.)?google\.com.*$/, '');
    const raw = `${cleanId} ${calendarEmail}`;
-    const eid = btoa(String.fromCharCode(...new TextEncoder().encode(raw)));
+    const bytes = new TextEncoder().encode(raw);
+    let binary = '';
+    for (let i = 0; i < bytes.length; i++) {
+      binary += String.fromCharCode(bytes[i]);
+    }
+    const eid = btoa(binary);
    return `https://calendar.google.com/calendar/event?eid=${eid}`;
  } catch {
    return undefined;
--- a/src/app/plugins/issue-provider/plugin-http.service.spec.ts
+++ b/src/app/plugins/issue-provider/plugin-http.service.spec.ts
@ -122,7 +122,7 @@ describe('PluginHttpService', () => {
    it('should block 169.254.x.x link-local range', async () => {
      const http = service.createHttpHelper(noopHeaders);
      await expectAsync(http.get('https://169.254.169.254/api')).toBeRejectedWithError(
-        /private\/local/,
+        /cloud metadata endpoints/,
      );
    });

@ -130,7 +130,7 @@ describe('PluginHttpService', () => {
      const http = service.createHttpHelper(noopHeaders);
      await expectAsync(
        http.get('https://metadata.google.internal/computeMetadata/v1/'),
-      ).toBeRejectedWithError(/private\/local/);
+      ).toBeRejectedWithError(/cloud metadata endpoints/);
    });

    it('should block IPv6 ULA (fc00::)', async () => {
--- a/src/app/plugins/issue-provider/plugin-http.service.ts
+++ b/src/app/plugins/issue-provider/plugin-http.service.ts
@ -15,6 +15,27 @@ const BLOCKED_HOSTNAMES = new Set([
  'metadata.google.internal',
 ]);

+// Cloud metadata endpoints that must ALWAYS be blocked, even when allowPrivateNetwork is true
+const ALLOWED_HTTP_METHODS = new Set([
+  'GET',
+  'POST',
+  'PUT',
+  'PATCH',
+  'DELETE',
+  'HEAD',
+  'OPTIONS',
+  'PROPFIND',
+  'REPORT',
+  'MKCALENDAR',
+  'PROPPATCH',
+]);
+
+const CLOUD_METADATA_HOSTNAMES = new Set([
+  '169.254.169.254', // AWS/Azure instance metadata
+  'metadata.google.internal', // GCP instance metadata
+  'fd00:ec2::254', // AWS IMDSv2 IPv6
+]);
+
 const isPrivateIp = (hostname: string): boolean => {
  // IPv4 private ranges
  if (
@ -83,6 +104,10 @@ export class PluginHttpService {
    getHeaders: () => Record<string, string> | Promise<Record<string, string>>,
    allowPrivateNetwork: boolean,
  ): Promise<T> {
+    const upperMethod = method.toUpperCase();
+    if (!ALLOWED_HTTP_METHODS.has(upperMethod)) {
+      throw new Error(`[PluginHttp] HTTP method not allowed: ${method}`);
+    }
    this._validateUrl(url, allowPrivateNetwork);

    const authHeaders = await Promise.resolve(getHeaders());
@ -103,7 +128,7 @@ export class PluginHttpService {
    const responseType = options?.responseType === 'text' ? 'text' : 'json';

    const obs$ = this._http
-      .request(method, url, {
+      .request(upperMethod, url, {
        body,
        headers: new HttpHeaders(mergedHeaders),
        params,
@ -124,9 +149,17 @@ export class PluginHttpService {
    if (parsed.protocol !== 'https:' && parsed.protocol !== 'http:') {
      throw new Error(`[PluginHttp] Unsupported URL scheme: ${parsed.protocol}`);
    }
+    // Strip brackets from IPv6 addresses (URL.hostname may include them)
+    const hostname = parsed.hostname.replace(/^\[|\]$/g, '');
+
+    // Always block cloud metadata endpoints (SSRF protection), even when allowPrivateNetwork is true
+    if (CLOUD_METADATA_HOSTNAMES.has(hostname.toLowerCase())) {
+      throw new Error(
+        `[PluginHttp] Requests to cloud metadata endpoints are not allowed: ${hostname}`,
+      );
+    }
+
    if (!allowPrivateNetwork) {
-      // Strip brackets from IPv6 addresses (URL.hostname may include them)
-      const hostname = parsed.hostname.replace(/^\[|\]$/g, '');
      if (BLOCKED_HOSTNAMES.has(hostname) || isPrivateIp(hostname)) {
        throw new Error(
          `[PluginHttp] Requests to private/local networks are not allowed: ${hostname}`,