Skip to content

fix(notifications): normalize source in getNotificationTimeoutMs and sanitize timer duration - #1808

Closed
hsusul wants to merge 1 commit into
OpenWhispr:mainfrom
hsusul:fix/1807-notification-timer-robustness
Closed

fix(notifications): normalize source in getNotificationTimeoutMs and sanitize timer duration#1808
hsusul wants to merge 1 commit into
OpenWhispr:mainfrom
hsusul:fix/1807-notification-timer-robustness

Conversation

@hsusul

@hsusul hsusul commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #1807

Problem

In src/helpers/notificationTimer.js:

  1. getNotificationTimeoutMs(source) used strict lowercase comparison against "calendar", falling back to 30s detection timeout when source was capitalized or padded with whitespace (e.g. "Calendar").
  2. NotificationDismissTimer.prototype.start(durationMs) did not sanitize durationMs, allowing non-finite or non-positive numbers to disrupt timer scheduling.
  3. NotificationDismissTimer threw a TypeError when the timer expired if this._onTimeout was missing or not a function.

Solution

  • Normalized source with trim().toLowerCase().
  • Sanitized durationMs to ensure a positive finite number before passing to _arm.
  • Guarded callback invocation with typeof this._onTimeout === "function".
  • Added unit tests in test/helpers/notificationTimer.test.js.

Verification

  • node --test test/helpers/notificationTimer.test.js (passes, 10/10 tests)
  • npm run typecheck (passes, 0 errors)
  • npm run lint (passes, 0 errors)
  • npm run i18n:check (passes)
  • npm run build:renderer (passes)
  • git diff --check (clean)

@Chadpiha

Copy link
Copy Markdown
Collaborator

Thanks @hsusul — closing along with #1807 (see the #1840 note for the general convention).

source is an internal two-value enum: the only producers are literal "audio" and "calendar" strings inside meetingDetectionEngine.js, and it never crosses IPC or persistence on the way to this helper. Worth noting the engine itself strict-compares source === "calendar" in two other places (pref gating and the Join button) — so normalizing in just this one reader would imply a tolerance the rest of the pipeline doesn't have. Similarly, the timer's only production start() call receives one of two constants, and the constructor always gets an inline callback — the fallbacks would convert a loud future bug into a notification that silently never dismisses.

Thanks as always for the tidy tests! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

getNotificationTimeoutMs misses mixed-case source and NotificationDismissTimer doesn't validate duration

2 participants