fix: lib/ audit — validators, TaskGraph/RemoteTrigger DartWorker, iOS existingPolicy+executionId

CHANGELOG.md

@@ -5,6 +5,125 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+
+Fixes 3 issues found by a full `lib/` audit (2026-09-23), reviewed in detail
+in the linked commits. Ships as **1.8.3** (1.8.2 is already live on pub.dev
+and immutable).
+
+### Fixed
+
+- **`moveToSharedStorage()`'s `subDir` had no path-traversal check on either
+  platform** — the one genuine cross-platform bypass in this batch. Neither
+  native implementation ever checked it: Android does
+  `File(publicDir, config.subDir)` directly, iOS does
+  `docsURL.appendingPathComponent(subDir)` directly, which does **not**
+  resolve `..` safely. `subDir` feeds `MediaStore.RELATIVE_PATH` on Android
+  and a Documents subfolder on iOS, so
+  `moveToSharedStorage(sourcePath: p, subDir: '../../OtherApp/Camera')`
+  reached native and would have actually escaped the sandbox on both
+  platforms. Everything else validated in the same commit
+  (`multiUpload()`'s url/files, `webSocket()`'s url/storeResponseAt,
+  `ParallelHttpUploadWorker`'s constructor) is Dart-side defense-in-depth,
+  not a closed system-level hole — native's `SecurityValidator`
+  (Kotlin/Swift, at parity) already independently validated those; see the
+  commit for the exact per-worker breakdown.
+- **A `DartWorker` placed in a `TaskGraph` node or a `RemoteTriggerRule`
+  mapping never reached native with a resolved callback handle** —
+  `enqueue()` and task chains converted `DartWorker` to `DartWorkerInternal`
+  (resolving the native callback handle) before sending it; `TaskNode` and
+  `RemoteTriggerRule` never did, so the task was enqueued but its callback
+  could never be resolved and the task silently never ran. Confirmed
+  reachable on both platforms before fixing (neither native worker factory
+  special-cases `DartCallbackWorker`; both require `callbackHandle` and fail
+  cleanly without it).
+  - A chain-step or `TaskGraph`-node `DartWorker` now gets the same
+    `isHeavyTask: true` promotion plain `enqueue()` applies on iOS, instead of
+    silently skipping it. **Correction, checked after first writing this
+    entry:** this is not currently an observable behavior change. `isHeavyTask`
+    only ever affects scheduling on iOS for the `periodic` and `windowed`
+    trigger types (both routed through `BGTaskScheduler`); chain steps and
+    graph nodes have no `TaskTrigger` of their own and always execute inline
+    via a plain `Task {}`, never through `BGTaskScheduler`, so the promoted
+    value is stored correctly but not read anywhere today. Kept for
+    consistency with `enqueue()`'s existing behavior and so a future fix to
+    chain/graph iOS scheduling doesn't need its own audit of this — not
+    claimed as a fix for OS-kill risk on either mechanism today.
+  - **Behavior change:** an unregistered `DartWorker` (`callbackId` not
+    passed to `initialize(dartWorkers:)`) used in a `TaskGraph` node or a
+    `RemoteTriggerRule` mapping now throws a `StateError` at the point of
+    `enqueueGraph()`/`registerRemoteTrigger()`, instead of silently reaching
+    native and never running.
+  - **Bonus fix, found while re-checking this change:** the same unregistered
+    `callbackId` used in a task **chain** used to throw too, but with the
+    wrong message — chains never checked registration at all, so they fell
+    straight into the "should never happen" internal-error branch meant for
+    a genuinely impossible state, printing `INTERNAL ERROR: Callback handle
+    not found... Please report this bug.` for what is actually a completely
+    ordinary mistake. Chains now get the same clear "not registered, here's
+    how to fix it" message `enqueue()` has always given.
+- **iOS's foreground `handleEnqueue` never read `existingPolicy` at all** —
+  found while investigating the item above. Every repeat `enqueue()` call
+  for a reused `taskId` silently started a second, fully independent
+  concurrent execution, regardless of what policy the caller asked for.
+  Confirmed on a simulator: two `DartWorker` executions of one `taskId`,
+  600ms apart, both ran to full completion independently.
+  `existingPolicy: .replace` (the default) now actually cancels the outgoing
+  execution before starting the new one; `.keep` now actually leaves the
+  running execution alone and ignores the new request — both matching
+  Android's WorkManager semantics.
+  - Fixing this alone reproduced **issue #72's exact bug shape on iOS**: the
+    replacement execution resolves almost instantly (same running engine, no
+    boot delay), sees the outgoing execution's cancellation mark, and its own
+    cleanup — previously keyed by bare `taskId` — cleared that mark before
+    the outgoing execution's next poll could observe it. So this also ports
+    issue #72's fix to iOS: `DartTaskCancellationRegistry` is now keyed by a
+    fresh per-execution id (minted in `executeDartWorkerViaMethodChannel`,
+    covering both the foreground and headless paths since they share that
+    function), not by bare `taskId`, mirroring the Android fix. `isTaskCancelled()`
+    now binds this id into a Zone on the foreground/simulator path too
+    (`method_channel.dart`'s `_executeDartCallback`), matching what the
+    headless isolate's `_callbackDispatcher` already did.
+  - Known residual limitation, traced through but deliberately not closed
+    (closing it means threading a pre-minted execution id through every
+    caller of `executeDartWorkerViaMethodChannel` — direct enqueue, chains,
+    `TaskGraph`, `BGTaskScheduler`-resumed tasks, offline queue — more surface
+    area than this PR's blast radius should grow to without its own device
+    verification pass): the per-execution id is minted lazily, inside
+    `executeDartWorkerViaMethodChannel`, once the replacement `Task` actually
+    starts running — not synchronously when `handleEnqueue` decides to
+    replace. In the narrow window between a `.replace` swapping
+    `activeTasks[taskId]` to the new `Task` and that `Task` reaching its
+    first `beginExecution` call, `DartTaskCancellationRegistry`'s
+    `currentExecutionId[taskId]` still points at the OLD (already-replaced)
+    execution. A `cancel(taskId)` — or another `.replace` — landing in that
+    window marks the wrong (stale) execution id; the new one starts moments
+    later unaffected by that mark, so it does **not** stop when the caller
+    thought it just told it to. Pure double/triple-replace with no
+    intervening `cancel()` was traced through and does not corrupt state —
+    only an explicit cancel landing in that specific gap does. The window is
+    on the order of the time from `Task { }` construction to its first
+    `await` inside `executeDartWorkerViaMethodChannel` — real, but requires a
+    caller to `enqueue()`-then-immediately-`cancel()`/`enqueue()` again the
+    same `taskId` back-to-back, not a pattern normal usage hits.
+  - Device-verified on an iOS simulator (both `.replace` and `.keep`); no
+    Android changes were needed (Android's `existingPolicy` handling and
+    `DartTaskCancellationRegistry` were already correct — that's what issue
+    #72 fixed).
+  - **Follow-up found in second-pass review, before this ever shipped**: the
+    `existingPolicy` fix above read `activeTasks[taskId]` as its "is this
+    still running" signal, but that dictionary was never cleared when a
+    direct one-time task finished *naturally* (only explicit cancel ever
+    removed an entry) — a leftover from before anything read it as a
+    liveness signal. Confirmed on a simulator: re-enqueuing a `taskId` whose
+    task had already completed, with `existingPolicy: .keep`, was silently
+    dropped forever, because the stale entry made `.keep` think something
+    was still running. Fixed with a per-enqueue generation id that lets a
+    task's own completion clear its entry — but only if nothing has replaced
+    it in the meantime, the same guard pattern used by
+    `DartTaskCancellationRegistry`. Verified fixed on the same simulator, and
+    the full `Cancellation` device-test group (8 tests) still passes.
+
 ## [1.8.2] - 2026-09-23
 
 ### Added

example/integration_test/device_integration_test.dart

@@ -2070,6 +2070,207 @@ void main() {
       },
     );
 
+    testWidgets(
+      'lib_audit_3: existingPolicy.replace cancels the outgoing execution '
+      'precisely and the replacement runs to completion, not inheriting its '
+      'stale cancel mark (iOS)',
+      (tester) async {
+        // https://github.com/brewkits/native_workmanager — found by the
+        // 2026-09-23 lib/ audit while investigating the iOS analogue of
+        // issue_72 above. Two things were wrong before this fix, discovered
+        // in order:
+        //   1. iOS's foreground handleEnqueue never read existingPolicy at
+        //      all — every repeat enqueue() of one taskId silently started a
+        //      second, fully independent concurrent execution, confirmed by
+        //      running exactly this test's shape pre-fix: both old and new
+        //      ran to full completion (50/50), regardless of policy.
+        //   2. Fixing #1 alone (mark-and-replace on the outgoing execution)
+        //      reproduced issue_72's "direction 1" bug on iOS: the new
+        //      execution resolves almost instantly (same running engine, no
+        //      boot delay), sees the mark, and its own cleanup — keyed by
+        //      bare taskId — cleared it before the outgoing execution's next
+        //      poll could observe it. Confirmed the same way: the new
+        //      execution died at iteration 1, the old one ran all 50.
+        // The real fix needed both: existingPolicy.replace AND per-execution
+        // (executionId-keyed) cancellation tracking, mirroring issue #72's
+        // Android fix, ported to iOS's DartTaskCancellationRegistry.
+        if (!Platform.isIOS) {
+          markTestSkipped('iOS foreground existingPolicy path');
+          return;
+        }
+
+        final id = _id('lib_audit_3_replace');
+        final oldCounterFile =
+            File('${tmpDir.path}/lib_audit_3_replace_old.txt');
+        final newCounterFile =
+            File('${tmpDir.path}/lib_audit_3_replace_new.txt');
+
+        await NativeWorkManager.enqueue(
+          taskId: id,
+          trigger: const TaskTrigger.oneTime(),
+          worker: DartWorker(
+            callbackId: 'dit_cancel_poll',
+            input: {'counterFile': oldCounterFile.path},
+          ),
+        );
+
+        await Future.delayed(const Duration(milliseconds: 600));
+
+        // Default policy is replace.
+        await NativeWorkManager.enqueue(
+          taskId: id,
+          trigger: const TaskTrigger.oneTime(),
+          worker: DartWorker(
+            callbackId: 'dit_cancel_poll',
+            input: {'counterFile': newCounterFile.path},
+          ),
+        );
+
+        await Future.delayed(const Duration(seconds: 12));
+
+        expect(
+          oldCounterFile.existsSync(),
+          isTrue,
+          reason: 'lib_audit_3: the replaced execution must have started',
+        );
+        final oldIterations =
+            int.parse(oldCounterFile.readAsStringSync().trim());
+        expect(
+          oldIterations,
+          lessThan(50),
+          reason: 'lib_audit_3: the replaced execution must have observed '
+              'cancellation and stopped',
+        );
+
+        expect(
+          newCounterFile.existsSync(),
+          isTrue,
+          reason: 'lib_audit_3: the replacement execution must have started',
+        );
+        final newIterations =
+            int.parse(newCounterFile.readAsStringSync().trim());
+        expect(
+          newIterations,
+          equals(50),
+          reason: 'lib_audit_3: the replacement must run to completion — a '
+              'lower count means it inherited the replaced execution\'s '
+              'stale cancellation mark and self-aborted',
+        );
+      },
+    );
+
+    testWidgets(
+      'lib_audit_3: existingPolicy.keep leaves the running execution alone '
+      'and ignores the new request (iOS)',
+      (tester) async {
+        if (!Platform.isIOS) {
+          markTestSkipped('iOS foreground existingPolicy path');
+          return;
+        }
+
+        final id = _id('lib_audit_3_keep');
+        final oldCounterFile = File('${tmpDir.path}/lib_audit_3_keep_old.txt');
+        final newCounterFile = File('${tmpDir.path}/lib_audit_3_keep_new.txt');
+
+        await NativeWorkManager.enqueue(
+          taskId: id,
+          trigger: const TaskTrigger.oneTime(),
+          worker: DartWorker(
+            callbackId: 'dit_cancel_poll',
+            input: {'counterFile': oldCounterFile.path},
+          ),
+        );
+
+        await Future.delayed(const Duration(milliseconds: 600));
+
+        await NativeWorkManager.enqueue(
+          taskId: id,
+          trigger: const TaskTrigger.oneTime(),
+          worker: DartWorker(
+            callbackId: 'dit_cancel_poll',
+            input: {'counterFile': newCounterFile.path},
+          ),
+          existingPolicy: ExistingTaskPolicy.keep,
+        );
+
+        await Future.delayed(const Duration(seconds: 12));
+
+        expect(
+          oldCounterFile.existsSync(),
+          isTrue,
+          reason: 'lib_audit_3: keep must leave the running execution alone',
+        );
+        expect(
+          int.parse(oldCounterFile.readAsStringSync().trim()),
+          equals(50),
+          reason: 'lib_audit_3: keep must not cancel the running execution',
+        );
+        expect(
+          newCounterFile.existsSync(),
+          isFalse,
+          reason: 'lib_audit_3: keep must ignore the new request entirely — '
+              'no second execution should ever have started',
+        );
+      },
+    );
+
+    testWidgets(
+      'lib_audit_3: re-enqueuing a taskId whose previous execution already '
+      'completed runs the new one, regardless of existingPolicy (iOS)',
+      (tester) async {
+        // Found while re-verifying the two tests above: iOS's activeTasks
+        // dict is never cleared when a direct one-time task finishes
+        // NATURALLY (only explicit cancel paths ever call removeValue).
+        // Before existingPolicy read that dict as a liveness signal this was
+        // a harmless leak; the first version of the existingPolicy fix
+        // treated a long-finished taskId as "still running" forever, so
+        // ANY later re-enqueue with existingPolicy.keep was silently
+        // dropped — confirmed with this exact test shape before the second
+        // fix (a per-enqueue generation id, cleared only by the Task that
+        // is still the current occupant of activeTasks[taskId] when it
+        // finishes — mirrors DartTaskCancellationRegistry's "clear only if
+        // still current" guard).
+        if (!Platform.isIOS) {
+          markTestSkipped('iOS foreground existingPolicy path');
+          return;
+        }
+
+        final id = _id('lib_audit_3_keep_after_completion');
+
+        final firstEvent = _waitEvent(id, timeout: const Duration(seconds: 15));
+        await NativeWorkManager.enqueue(
+          taskId: id,
+          trigger: const TaskTrigger.oneTime(),
+          worker: DartWorker(callbackId: 'dit_pass'),
+        );
+        final first = await firstEvent;
+        expect(first?.success, isTrue,
+            reason: 'lib_audit_3: the first execution must complete');
+
+        // Give the natural-completion cleanup a moment to run before
+        // re-enqueuing, so this genuinely exercises the "already finished,
+        // not just finishing" case.
+        await Future.delayed(const Duration(seconds: 2));
+
+        final secondEvent =
+            _waitEvent(id, timeout: const Duration(seconds: 15));
+        await NativeWorkManager.enqueue(
+          taskId: id,
+          trigger: const TaskTrigger.oneTime(),
+          worker: DartWorker(callbackId: 'dit_pass'),
+          existingPolicy: ExistingTaskPolicy.keep,
+        );
+        final second = await secondEvent;
+        expect(
+          second?.success,
+          isTrue,
+          reason: 'lib_audit_3: a taskId reused after its previous execution '
+              'already completed must run the new request — keep must not '
+              'mistake a long-finished task for one still running',
+        );
+      },
+    );
+
     testWidgets(
       'issue_69: cancelling a background-session download actually aborts the transfer (iOS)',
       (tester) async {

ios/native_workmanager/Sources/native_workmanager/NativeWorkmanagerPlugin+Cancel.swift

@@ -16,6 +16,7 @@ extension NativeWorkmanagerPlugin {
             activeTasks.keys.forEach { DartTaskCancellationRegistry.shared.markCancelled($0) }
             activeTasks.values.forEach { $0.cancel() }
             activeTasks.removeAll()
+            activeTaskGenerations.removeAll() // see its doc comment on NativeWorkmanagerPlugin
             taskStates.removeAll()
             taskTags.removeAll()
             workers.values.forEach { $0.stop() }
@@ -53,6 +54,7 @@ extension NativeWorkmanagerPlugin {
                 DartTaskCancellationRegistry.shared.markCancelled(taskId) // issue #66
                 activeTasks[taskId]?.cancel()
                 activeTasks.removeValue(forKey: taskId)
+                activeTaskGenerations.removeValue(forKey: taskId) // see its doc comment
                 taskStates[taskId] = .cancelled
                 taskTags.removeValue(forKey: taskId)
                 workers[taskId]?.stop()
@@ -74,6 +76,7 @@ extension NativeWorkmanagerPlugin {
                 stateQueue.async(flags: .barrier) {
                     self.activeTasks[taskId]?.cancel()
                     self.activeTasks.removeValue(forKey: taskId)
+                    self.activeTaskGenerations.removeValue(forKey: taskId) // see its doc comment
                     self.taskStates[taskId] = .cancelled
                     self.taskTags.removeValue(forKey: taskId)
                     self.workers[taskId]?.stop()