* test(sync): reproduce file-based compaction stranded ops pointer (#9040)
Concurrent split-file compactions can leave the committed sync-ops.json
referencing a snapshot present in neither sync-state.json nor its .bak: the
snapshot is force-written unconditionally while only the ops file is gated, so
the loser of the ops race can clobber the winner's snapshot after backing up an
older generation.
Adds a failing integration test that drives two interleaved compactions against
the stateful MockFileProvider and asserts a fresh client can still hydrate the
committed generation. Fails today (unrecoverable gap); passes once the snapshot
is written under a generation-unique, immutable name.
* fix(sync): write split-compaction snapshots to immutable files (#9040)
Concurrent split-file compactions could strand the winning ops pointer: the
snapshot was force-written to the fixed sync-state.json, so the loser of the
conditional ops-commit race could clobber the winner's snapshot, leaving the
committed sync-ops.json referencing a snapshot absent from both sync-state.json
and its .bak — an unrecoverable gap for a fresh client.
Write the compaction snapshot to a generation+client-unique immutable file
(sync-state__<syncVersion>__<clientId>.json) recorded in snapshotRef.file. A
concurrent compactor writes a DIFFERENT file, so the winning pointer can never
be clobbered. Readers prefer snapshotRef.file and fall back to sync-state.json /
.bak (kept dual-written for pre-#9040 clients). The superseded snapshot is
GC'd O(1) after each commit; a losing compactor's same-generation orphan is a
rare, bounded leak (documented, with a listFiles-prune upgrade path).
Also fixes MockFileProvider to model create-if-absent (If-None-Match: *) so the
fresh-folder concurrent-compaction path is faithfully gated in tests.
Tests: 3 concurrent-compaction integration scenarios (harmful/benign interleave,
fresh-folder race) + 2 unit tests pinning the snapshot resolution order. Full
op-log suite (3427) and sync-providers package (404) green.
* fix(sync): reclaim orphaned snapshot when compaction loses the commit race (#9040)
A concurrent compactor that writes its immutable snapshot but then loses the
conditional ops-commit left that snapshot orphaned — no committed ops file ever
referenced it, and the O(1) predecessor-GC never reclaimed it (same generation).
The losing compactor now deletes the snapshot it just wrote when its commit
throws, eliminating the leak at its only realistic source. Only a crash between
the snapshot write and the commit/cleanup can still orphan a file (rare crash
window; listFiles-prune remains the documented backstop).
Refactors the GC helper into a single-file _removeGenStateFile primitive used by
both the post-commit predecessor delete and the new failure-path cleanup.
Tests assert the loser's orphan is gone after both the near-cap and fresh-folder
concurrent-loss scenarios. Full op-log suite (3427) green.
* fix(sync): only reclaim orphaned snapshot on confirmed rev-mismatch (#9040)
The loser-orphan cleanup deleted the just-written immutable snapshot on ANY
commit failure. A rev-mismatch is a confirmed server rejection (ops did not
commit), but a network/5xx error is ambiguous — the ops PUT may have landed and
committed, in which case the snapshot is still referenced and deleting it would
strand a reader. Restrict the cleanup to UploadRevToMatchMismatchAPIError.
Adds a test asserting the immutable snapshot survives a non-mismatch commit
failure. Full op-log suite (3428) green.
* fix(sync): use a random suffix, not clientId, in immutable snapshot names (#9040)
Filenames are not encrypted, so embedding the clientId in
sync-state__<syncVersion>__<clientId>.json leaked device count/platform and a
per-device counter to the remote — a metadata downgrade for E2EE file-based
users. Replace the clientId with an opaque random suffix, which still gives two
concurrent compactors distinct files. A collision is astronomically unlikely and
self-heals (the reader validates loaded content against snapshotRef, so a wrong
file fails validation and falls back to sync-state.json/.bak). syncVersion stays
in the name for legible ordering and a future listFiles-prune.
Tests now assert on the count of immutable snapshot files (and a captured name)
rather than hardcoding the now-random filenames. Full op-log suite (3428) green.
* docs(plugins): add microsoft 365 calendar provider plan
* docs(plugins): add microsoft 365 calendar provider plan
* docs(ios): plan internal testflight builds
* fix(sync): prevent file-based sync data loss
Preserve post-snapshot operations and stage remote baselines until durable apply.
Use conditional writes and a resumable marker for provider concurrency and legacy migration. Report trustworthy remote timestamps and document transport limits.
Refs #8960
* fix(sync): avoid biased conflict recommendations
Highlight a side only when its timestamp or known change count is strictly greater, leaving unknown and tied metadata neutral.
* test(sync): assert options arg on split-file processRemoteOps
The split-file snapshot path now routes post-snapshot ops through
_processRemoteOpsWithStartupCleanup, which calls processRemoteOps with an
(empty) options object. Update the assertion to match the 2-arg call so the
unit suite passes.
* refactor(sync): dedupe strong-ETag regex, document snapshot-op boundary
- Extract the duplicated RFC 7232 strong-entity-tag pattern into a single
STRONG_ETAG_RE constant with a comment noting why the char class is safe to
interpolate into an If-Match header (no CR/LF header splitting).
- Explain why sv === undefined ops are classified as snapshot-included: they are
legacy migration ops fully contained in the snapshot, and _validateSnapshotRef
enforces a clock-EQUAL boundary. No behavior change.
* fix(sync): prevent destructive full-state sync races
Use causal ordering and server-sequence preconditions for full-state operations, surface snapshot rejection, deduplicate migrations, and retain queued WebSocket downloads.\n\nFixes #8959
* fix(sync): block uploads after rejected imports
Keep incremental operations behind a durable barrier when a local import or restore is rejected. Release the barrier only after a newer full-state snapshot is accepted, and surface the blocked state instead of reporting sync success.
* fix(sync): harden causal repair recovery
Persist repair base cursors across the client, provider, and server so stale snapshots are rejected before quota or history mutation and legacy repairs cannot poison restore state.
Atomically rebase stale repairs after downloading the missing suffix, preserve rejected-import barriers and WebSocket watermarks, and cover PostgreSQL serialization.
* fix(sync): harden repair recovery edge cases
Block dependent uploads after any rejected full-state operation, reset negotiated capabilities across provider config changes, and bound WebSocket download retries.
Update sync test doubles for causal repairs and the expanded duplicate-operation identity.
* fix(sync): make task replay values deterministic
Capture logical dates and timestamps in task actions and backfill legacy operations. Carry per-day task totals so own-operation replay is idempotent while foreign time remains additive.
Fixes#8957
* fix(sync): make time snapshots replay-safe
Exclude pending task-time batches from op-log and file-sync snapshots so their later additive operations cannot overlap snapshot state. Preserve concurrent direct credits and normalize legacy replay dates deterministically.
Fixes#8957
* fix(sync): align snapshots with queued task time
Exclude accumulator and in-flight task-time deltas from operation-log snapshots so later delta operations cannot double-count them. Capture file and direct-upload snapshots at the same operation-log boundary, and flush or clear queued time around destructive state replacement.
* fix(tasks): flush queued time at task boundaries
Persist queued timer deltas before absolute short-syntax edits, and clear them whenever project, schedule, or repeat cleanup deletes the owning task. This prevents stale batches from recreating or overwriting removed task time.
* fix(sync): preserve concurrent task time deltas
Treat concurrent task-time batches as commuting updates on both client and server, while retaining causal stale-operation checks. Reject malformed identities, dates, durations, and unsafe timestamps before replay or persistence.
* test(sync): cover task time snapshot replay
Exercise seeded snapshot and restart invariants plus a real three-client SuperSync convergence path with the initial time inside the snapshot.
* fix(sync): select action type for legacy conflicts
* fix(sync): harden file-based .bak recovery and split gap detection
Post-merge review of the SPAP-8/9/10/11 series found five defects in the
file-based sync adapter; all fixed here with regression tests proven
red/green against the previous code. Design cross-checked by a 7-reviewer
multi-agent pass; its findings are folded in.
- Split gap detection suppressed a syncVersion reset when the remote
clock was EQUAL OR GREATER_THAN the last-seen clock — the exact bug the
SPAP-9 review follow-up removed from the single-file path. A dominating
client's snapshot reset (which compacts ops this client never saw) was
treated as cosmetic, skipping snapshot hydration and silently
diverging. Now EQUAL only, matching the single-file path.
- .bak recovery staged the CORRUPT primary's rev, which setLastServerSeq
then promoted to _lastSeenRevs: every later poll's SPAP-10 pre-check
read "unchanged" and skipped the re-download while the upload path (no
.bak recovery) kept failing on the corrupt primary — sync wedged until
another client rewrote the file. A recovery download now never
stages/promotes the rev (each poll re-recovers and re-seeds the heal
cache), and every site that rewrites _lastSeenRevs drops any stale
staged rev via the shared _commitLastSeenRev.
- Snapshot uploads (force-upload / "Use Local" / E2EE re-encryption) left
the pre-snapshot .bak behind. After a password rotation the stale
old-key .bak was silently "recovered" by a still-old-key client
(suppressing its wrong-password prompt) and heal-uploaded back over the
new-key primary, reverting the rotation. Snapshot uploads now write the
same payload to .bak FIRST, then the primary (_forceUploadWithBakFirst;
deliberately FATAL — aborting pre-primary leaves the remote consistent
for a retry). Applies to sync-data.json.bak and sync-ops.json.bak;
sync-state.json.bak is deliberately exempt: its adoption is
ref-validated (EQUAL clock vs snapshotRef), so a stale copy is inert,
and it must keep serving the compaction crash window it exists for.
- Recovery additionally refuses a PLAINTEXT .bak when encryption is
expected: decoding trusts the file's own prefix flags, so a plaintext
.bak decodes even under a wrong/rotated key — the same
wrong-password-suppression class via mode (rather than key) mismatch.
- The split migration wrote the v3 tombstone BEFORE neutralizing the
legacy .bak (best-effort): a crash between the writes left a live v2
.bak that an OFF client's recovery would resurrect over the tombstone,
forking the folder. Neutralize-first, fatal. Residual: a step-3 failure
after the migration's ops-file commit leaves a live v2 file until the
next snapshot upload (documented at the call site).
- sync-ops.json — the hot file, rewritten on every op-bearing sync — had
no backup at all, so a torn write wedged split sync until a manual
force-upload. It now gets the same backup-before-overwrite + recovery +
heal treatment as the single-file format. deleteAllData deletes the
split files BEFORE the tombstone and treats source-of-truth deletion
failures as errors (success:false) — it previously left every split
file behind and reported success.
The three backup/recover pairs are collapsed into shared _writeBakFile /
_readBakFile helpers (the EQUAL||GREATER_THAN divergence above is exactly
the copy-drift failure mode this prevents); .bak file names move into
FILE_BASED_SYNC_CONSTANTS as remote-format surface.
* fix(sync): show the exact pending-op count in the conflict dialog
Compaction can fold still-unsynced ops into the snapshot baseline clock,
so the dialog's vector-clock delta could report "0 changes" right next to
"N local changes pending" — and the false 0 skipped the secondary
USE_REMOTE overwrite confirmation. The dialog now prefers the EXACT
pending-op count carried on LocalDataConflictError (new optional
ConflictData.localUnsyncedOpsCount): it is precisely "what USE_REMOTE
would discard", so both the displayed count and the >= 20-difference
confirmation threshold work from a truthful figure; the clock delta
remains the fallback when no measured count is supplied.
Display/confirmation-only — no clock or op-log semantics touched. Also
asserts the explicit-null lastSyncedVectorClock contract at the
fresh-client conflict throw sites (test gap from SPAP-7).
* chore(lint): enforce tx-handle-only access in op-log transactions
The SQLite op-log adapter serializes every entry point through a
per-connection FIFO queue; awaiting an adapter method inside a
.transaction() callback enqueues behind the transaction's own slot and
silently deadlocks all op-log persistence. The port contract documents
the precondition and #8849 promised a lint rule — this adds it
(no-adapter-in-tx, scoped to src/app/op-log) with RuleTester specs.
Matching is rename-proof: it flags access on the SAME receiver the
transaction was opened on (plain identifiers and any `this.<field>`), so
it does not depend on the field being named `_adapter`; known heuristic
gaps (extracted callbacks, aliasing, method indirection) are documented.
Also corrects the port doc: IndexedDB serializes only overlapping-scope
transactions; SQLite provides the stronger whole-connection exclusion.
* feat(sync): SPAP-11 opt-in split-file sync format for O(delta) syncs
Splits the single sync-data.json into a small always-read/written sync-ops.json
(the commit point) plus a sync-state.json snapshot rewritten only on
compaction / force-upload / gap-repair / migration, so a normal sync transfers
O(delta) instead of O(dataset). Opt-in via the isUseSplitSyncFiles setting; the
legacy file is left as a v3 tombstone so old clients hard-stop rather than
silently diverge.
Review follow-ups:
- Recompaction triggers at combinedOps > MAX_RECENT_OPS (2000) and trims to
SPLIT_COMPACTION_THRESHOLD (1000), so it no longer rebuilds the snapshot on
every op-bearing sync once the folder crosses 1000 (test b2).
- Split download stages the ops-file rev to _pendingRevs (not _lastSeenRevs),
promoted only in setLastServerSeq after durable apply — mirrors the single-file
crash-safety path (test g).
- Split rev-precheck is gated on sinceSeq > 0, so a forceFromSeq0 download
(USE_REMOTE / rehydrate) always fetches ops + snapshot (test h).
Stacked on SPAP-9 (#8787, merged) and SPAP-10 (#8795): the split path reuses
their per-provider causality (_lastSeenVectorClocks / compareVectorClocks) and
rev (_lastSeenRevs / _pendingRevs) infrastructure.
SPAP-11
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(sync): return whole file-based ops buffer per download page
The file-based download paths (single-file _downloadOps and split-format
_downloadOpsSplit) truncated recentOps to the caller's page size
(DOWNLOAD_PAGE_SIZE = 500) via slice(0, limit), but they return ops by array
index and ignore sinceSeq. The caller (operation-log-download.service) loops on
`hasMore`, advancing sinceSeq by the returned index-based serverSeq — so when the
buffer exceeds the page size the adapter kept re-returning the SAME oldest slice.
A behind client never received its newest ops and the loop spun until it bailed
(MAX_DOWNLOAD_ITERATIONS / in-memory cap), leaving the sync stuck (success:false)
and unable to converge.
This is the common case for the split format, whose ops buffer floor is
SPLIT_COMPACTION_THRESHOLD (1000) — always above DOWNLOAD_PAGE_SIZE. The
single-file path shares it, latent since MAX_RECENT_OPS was raised 500 -> 2000
(before, buffer == page, so hasMore never tripped).
File-based providers have no server-side cursor (they re-download the whole file
each call), so cross-call pagination cannot advance and truncation just drops the
newest ops. The recentOps buffer is already bounded by MAX_RECENT_OPS, so return
it WHOLE with hasMore=false and let the caller's appliedOpIds dedup decide what is
actually new. Returning everything (rather than slicing to a cap) also converges
safely if a future higher-MAX_RECENT_OPS client writes an over-cap buffer, instead
of re-entering the same stranding loop.
Documents the `limit` divergence on the OperationSyncCapable.downloadOps
interface. Regression tests cover a 600-op buffer with page size 500 on both the
split and single-file paths (newest ops delivered, hasMore=false).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(sync): drop redundant recentOps-length clause from gap detection (SPAP-33)
The file-based gap detector treated a trimming gap as real only when the ops
buffer was also at its cap:
oldestOpSyncVersion > sinceSeq + 1 && recentOps.length >= <cap>
The length clause is redundant for correctness — syncVersion is contiguous and
every bump carries at least one op, so oldestOpSyncVersion > sinceSeq + 1 already
proves the op at sinceSeq+1 existed and was trimmed away, and it never
false-positives. But the clause caused a false NEGATIVE: a buffer trimmed at a
SMALLER floor than the current cap — a legacy buffer written by an old client
with a lower MAX_RECENT_OPS, or (in the split format) a buffer trimmed to
SPLIT_COMPACTION_THRESHOLD — has a genuine gap while holding fewer ops than the
cap, so the gap was suppressed and the behind client applied ops without the
snapshot base and silently diverged.
Drop the clause in both the single-file (>= MAX_RECENT_OPS) and split
(>= SPLIT_COMPACTION_THRESHOLD) paths, so a behind client correctly falls back to
the snapshot. Updates the single-file test that asserted the suppressed behavior
and adds a split-path regression (short trimmed buffer -> gap + snapshot load).
Previously deferred as SPAP-33; pulled in as it prevents silent cross-client
divergence and sits in the same download-correctness path as the pagination fix.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Johannes Millan <johannes.millan@gmail.com>
Use vector clocks to resolve file-based sync conflicts by causality instead
of always surfacing the binary USE_LOCAL/USE_REMOTE dialog:
- keep-local when local strictly dominates the remote snapshot
- apply-snapshot (no dialog) when the snapshot strictly dominates local
- dialog when clocks are concurrent and can't be auto-resolved
Cosmetic syncVersion-reset suppression is gated on the last-seen vector
clock and treated as cosmetic ONLY when the clocks are EQUAL. A GREATER_THAN
remote clock proves the writer did more work, not that this client received
the intervening ops (a snapshot can compact ops we never saw), so it now
conservatively triggers a seq-0 resync rather than being silently skipped.
CONCURRENT-snapshot auto-merge defaults OFF and, when enabled, only merges
if the retained recent ops provably bridge the whole gap to the snapshot
clock (local ⊔ recentOps ⊒ snapshot); otherwise it falls back to the
user-recoverable dialog instead of replaying only recentOps and silently
dropping compacted-only entities. Re-enabling default-on is gated on
multi-client E2E validation (SPAP-34).
SPAP-9
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
File-based providers (WebDAV, Dropbox, OneDrive, Nextcloud, LocalFile)
derived "should I encrypt?" from key presence alone, so a silently
dropped credential-store key made the next sync upload the full
sync-data.json in plaintext with no error — same class as GHSA-9v8x,
which only covered SuperSync.
Root cause: there was no durable, per-provider record of encryption
intent. The global sync.isEncryptionEnabled flag is shared across
providers and re-derived from key presence in the settings form, so it
is unreliable in both directions (stale-true after a SuperSync→file
switch; flipped false by a routine save once the key is gone).
Fix — persist intent per provider, enforce at one boundary, recover
uniformly:
- Store isEncryptionEnabled in each file-based provider's privateCfg,
written atomically with the key on enable/disable (closes the
non-atomic disable window) and backfilled from the key present at
save time / first sync for pre-fix configs. Read everywhere as
`isEncryptionEnabled ?? !!encryptKey`.
- WrappedProviderService now derives the adapter's isEncrypt from this
per-provider intent, not the global flag — so a stale global flag no
longer blocks legitimate plaintext sync and privateCfg writes keep the
adapter cache correct.
- The upload service fails closed for file-based intent-on-but-key-gone
via a new adapter hook (isEncryptionKeyMissing), throwing
EncryptNoPasswordError BEFORE either upload loop — never plaintext,
and never the permanent markRejected the snapshot path would apply.
- SyncWrapperService routes EncryptNoPasswordError to the enter-password
recovery dialog on the main sync, force-upload and USE_LOCAL conflict
paths (previously only main sync; the others dead-ended in a snack).
- The encrypt handler keeps throwing on isEncrypt-without-key as a
last-line backstop.
Residual: a pre-fix config whose key is dropped before intent is ever
recorded (never synced/saved after upgrade) cannot be distinguished from
"never encrypted" and is not retroactively protected — no record exists.
Regression tests per layer: adapter hooks, upload-boundary guard,
per-provider intent persistence, form derivation/preservation, and
conflict-path recovery routing.
* fix(sync): never upload plaintext ops for E2EE-mandatory providers
SuperSync's first-time setup ran an initial sync (dialog-sync-cfg save ->
sync(true)) BEFORE the user chose an encryption password, so all local ops
(incl. issue-provider credentials) were uploaded to the server in cleartext.
Completing setup deleted them; aborting left them stored indefinitely, breaking
the E2EE promise (GHSA-9v8x-68pf-p5x7).
Add an optional `isEncryptionMandatory` capability to OperationSyncCapable
(true for SuperSync) and refuse to upload in the op-log upload path while no
usable key is configured. Downloads still run (merge-first) and the encryption-
enable flow performs the first, encrypted upload, so the setup flow and prompt
are unchanged. File-based providers, where unencrypted sync is a legitimate
user choice, leave the flag unset.
* fix(sync): fail closed on plaintext snapshot for E2EE-mandatory providers
Multi-review hardening for GHSA-9v8x-68pf-p5x7. The op-upload guard closes the
reported leak, but SnapshotUploadService.deleteAndReuploadWithNewEncryption could
still push a plaintext snapshot for SuperSync on two adjacent paths: the (today
UI-unreachable) disable-encryption flow, and a keyless import declaring
isEncryptionEnabled:true. Reject an unencrypted snapshot for an encryption-
mandatory provider before any destructive deleteAllData, so the "never transmit
plaintext" invariant holds regardless of caller.
Also lower the mandatory-encryption upload-skip log from warn to normal: it is an
expected by-design skip during the pre-encryption setup window and would
otherwise fire on every auto-sync cycle.
* fix(sync): consolidate op-log into the encryption-enable snapshot
Follow-up to the GHSA-9v8x-68pf-p5x7 upload guard. With the guard, first-time
SuperSync setup leaves the whole local history unsynced until encryption is
enabled; the enable-snapshot then represents that full state, but the ops were
re-uploaded incrementally on top of it on the next sync (redundant server op-log
bloat, and previously untested at whole-history volume).
deleteAndReuploadWithNewEncryption now captures the pending ops the snapshot
subsumes (before the destructive delete, under runWithSyncBlocked + a modal
dialog so the set is stable) and marks them synced after a successful upload —
mirroring planRegularOpsAfterFullStateUpload in the op-log upload path, which
this direct snapshot upload bypasses. Also fixes the same latent redundancy in
the enable-from-settings-with-pending-ops flow.
Adds a multi-client e2e: local task history exists before first-time encrypted
setup, then a second client with the same password receives exactly those tasks
with no duplicates, conflicts, or errors.
* ci(e2e): add optional grep input to manual SuperSync e2e dispatch
* fix(sync): capture subsumed ops before state snapshot to prevent mark-synced data loss
deleteAndReuploadWithNewEncryption captured getUnsynced() AFTER the full-state
snapshot, so an op created in that window was marked synced yet absent from the
snapshot — silently lost. Capture the unsynced set first: every marked-synced op
is then guaranteed present in the snapshot, and a concurrent op arriving after
the capture is left unsynced and re-uploaded next sync (idempotent by op id).
* test(sync): mock WebCrypto so mandatory-encryption guard test reaches the guard
The 'enabling without a usable key' case passed isEncryptionEnabled: true but did
not mock WebCrypto, so the availability check threw WebCryptoNotAvailableError in
non-secure CI before the /unencrypted snapshot/ guard under test.
* fix(sync): verify file-based upload size to catch truncated writes (#8604)
Dropbox and OneDrive enforce no end-to-end integrity, so a truncated upload
(a cut-short body silently accepted) lands a partial gzip/JSON that fails to
decode on every later download until the file is deleted. The #7300 post-upload
verification was WebDAV-only.
Add a shared assertUploadedSizeMatches() helper: compare the stored byte size
(already in the upload response, no extra request) against the bytes sent, but
only for pure-ASCII payloads so the comparison is transport-encoding independent
— avoiding false positives on the native CapacitorHttp path, where a wrong
byte-count assumption would silently re-upload every cycle. Compressed/encrypted
payloads are base64 (ASCII), covering the reported case. On mismatch raise
UploadRevToMatchMismatchAPIError, the error the upload path already surfaces.
This detects and fails the sync loudly; it does not repair the remote.
Wired into Dropbox and OneDrive uploads. WebDAV keeps its stronger content-hash
re-read check.
* perf(sync): detect ASCII without encoding in upload-size check (#8604)
The upload-size guard called TextEncoder.encode(data).length purely to detect
whether the payload is pure-ASCII (byte count === char count), allocating a
full multi-MB Uint8Array on every Dropbox/OneDrive upload — including the
non-ASCII skip path, which is the default-config common case. Detect non-ASCII
with a regex instead and use data.length directly; behaviour is identical
(byteLen === data.length iff pure ASCII) with no allocation and an early exit on
the first non-ASCII unit.
Also add OneDrive upload-size wiring tests (match + ASCII truncation), which the
app-side spec previously exercised only on the skip path.
Surfaced by a second multi-agent review.
A OneDrive token-exchange 400 (issue #8572) surfaced only as a generic
"HTTP 400 Bad Request" / "copy the code exactly" message, hiding the
real cause: the authorize step succeeds, then token redemption fails
because the custom Microsoft Entra app isn't registered as a public
client ("Allow public client flows" disabled -> AADSTS7000218).
- onedrive provider: parse the OAuth error body once, log the safe
short `error` code on every token failure, and route the verbose
AADSTSxxxxx `error_description` to HttpNotOkAPIError.detail (UI only,
per the existing privacy split).
- sync-wrapper: show a OneDrive-specific snack pointing at the Entra
registration fix and interpolating the AADSTS detail; other providers
keep INVALID_AUTH_CODE.
- in-app info text now shows the desktop redirect URI and links to the
setup guide; add a full OneDrive section to the configure-sync wiki
note (it was entirely missing).
- test: auth-code 400 surfaces the detail to the UI, logs the error
code, and keeps the description out of the structured log.
* docs(sync): point Nextcloud user-ID lookup at the WebDAV URL (#7617)
The field hint, the 404 test-connection message, and the wiki all told
users to find their user ID under 'Settings -> Personal info' / 'your
Files URL'. Both are unreliable: Personal info does not clearly surface
the uid, and a folder's address bar shows a folder ID, not the user ID
(reporter followed it and got a folder ID). Redirect all three to the
authoritative source: Files -> settings gear (bottom-left) -> the WebDAV
URL '.../remote.php/dav/files/<user-id>/'.
* feat(sync): auto-detect Nextcloud user ID via OCS (#7617)
The Nextcloud WebDAV files path needs the account's internal user ID,
which differs from the email/login name people enter and is awkward to
find by hand — the root cause of the recurring '404 / connection test
failed' reports. Add a 'Detect user ID' button to the Nextcloud sync
config that asks the server for it.
- packages/sync-providers: discoverNextcloudUserId() calls the OCS
endpoint /ocs/v2.php/cloud/user (OCS-APIRequest header) authenticated
with login + app password and returns ocs.data.id. A 401 reports bad
credentials (cleanly distinct from the 404 wrong-user-id path); a 200
that isn't an OCS payload reports 'not a Nextcloud/OCS server'. A
missing/wrong URL scheme is refused up front (matches the provider's
own _cfgOrError check) so credentials never hit a schemeless host.
- Dialog: button fills the Username field with the detected ID. To keep
auth working, if 'Login name' was empty and the user had typed their
login into 'Username', that login is preserved into 'Login name'
before Username is overwritten with the ID. Result handling split into
_applyDetectedUserIdResult for unit-testability.
- Additive and optional: generic WebDAV and the save/sync path are
untouched, no synced data shapes change.
- Tests: 7 package specs (success, trailing-slash, login fallback, 401,
non-OCS 200, missing id, scheme guard) + 6 dialog specs (login guard,
fill+confirm, 3 login-preservation cases, failure).
* cleanup: remove unused latestSnapshotSeq from HTTP response and client types
latestSnapshotSeq was computed on every download but never read by any
client code. sibling snapshotVectorClock IS consumed — only this field
is dead. The server still computes it internally for the snapshot
optimization logic, but it no longer serializes it in the response.
Removed from:
- DownloadOpsResponse type and response object (sync.routes.ts)
- Zod schema (supersync-http-contract.ts) + associated test
- OpDownloadResponseBase interface (sync-providers)
- Client validator test + stale comment references
- Server integration test assertions
* cleanup: remove deprecated CONFLICT_STALE error code
Server never emits CONFLICT_STALE (fully migrated to
CONFLICT_SUPERSEDED).
* cleanup: remove unused SyncConfig fields (maxOpsPerUpload, downloadLimit, downloadRateLimit)
These three config fields were declared and defaulted but never read by
any code. Routes hardcode their own limits:
- maxOpsPerUpload: gate is MAX_OPS_PER_BATCH constant
(sync.routes.payload.ts)
- downloadLimit: hardcoded Math.min(limit, 1000) in sync.routes.ts
- downloadRateLimit: hardcoded max:200 in route rateLimit config
* cleanup: remove dead/drifted duplicate types from sync.types.ts
Removed 6 types with zero references repo-wide:
- SnapshotResponse: orphaned after previous PR removed its importers
- UploadSnapshotRequest: zero uses, missing 7 fields vs actual Zod
schema
- RestorePointType: zero uses, included dead 'DAILY_BOUNDARY' value
- RestorePoint: zero uses (live version in snapshot.service.ts)
- RestorePointsResponse: zero uses
- RestoreSnapshotResponse: zero uses
* fix(plainspace): don't tint claim-list link icon with accent color
* fix(sync): clarify Nextcloud connection-test 404 (wrong user ID) (#7617)
A base-root 404 in the WebDAV connection test means auth succeeded but the
DAV path /remote.php/dav/files/<userName>/ does not exist — i.e. the
"Username" holds an email/display name instead of the account's user ID
(Nextcloud accepts email/login for auth but the files path needs the uid).
Previously this surfaced as a bare scrubbed hostname in the snack, which
users misread as a stripped URL. Now:
- WebdavApi.testConnection maps thrown errors to a readable, privacy-safe
message plus an HTTP errorCode discriminator (404/401/status).
- The Nextcloud "Test Connection" path shows a specific hint on 404
explaining that "Username" must be the user ID, not email/display name.
- Wiki + field guidance clarified accordingly.
* refactor: remove dead SyncStateCorruptedError, compat exports, and 0-byte sync-providers barrel #8328
* docs(sync): drop stale SyncStateCorruptedError/fail-fast references (#8328)
The fail-fast dependency-resolution subsystem (DependencyResolverService +
SyncStateCorruptedError throw) was removed earlier; OperationApplierService now
bulk-dispatches ops in causal arrival order and returns a failedOp for the caller
to re-validate/retry. Update the sync architecture docs, archive-operations
diagram, and user-data wiki to match. Completes the dead-code cleanup for #8396.
---------
Co-authored-by: Johannes Millan <johannes.millan@gmail.com>
* feat(sync): show actionable error on persistent WebDAV 409
When a WebDAV PUT keeps returning 409 Conflict even after the parent
collection is created, the Base URL / Sync Folder Path is misconfigured
(the classic Synology / raw-WebDAV setup mistake). Previously the raw
"HTTP 409 Conflict" (or a bare "Unknown error") reached the user with no
hint at the cause.
Throw a dedicated WebDavSyncFolderUnusableSPError with a privacy-safe,
actionable message (no path, no response body) at the spot that already
detects this condition. Mirrors NetworkUnavailableSPError: a fixed
user-facing message matched by instanceof.
* ci(release): revive contributors section in release notes
* fix(plugins): stop leaked timers on plugin disable via onUnload hook
* fix(plugins): harden onUnload teardown hook after review
* refactor(plugins): group lifecycle registers into options object
* fix(plugins): close onReady stale-guard gap and timer interleave race
Pre-merge review of #8228 flagged four real items in the otherwise
solid security boundary. All non-blocking; rolled into one pass:
1) IMAGE_PICK_AND_IMPORT was the only renderer-callable handler that
threw raw Errors back across IPC. The FS handlers all funnel
through createSafeIpcError, which strips e.stack so main-bundle
paths can't leak via the renderer's error-handling. Switch
IMAGE_PICK_AND_IMPORT to the same shape: returns Error on
validation failure, null on user cancel, value on success.
Renderer adapter uses `instanceof Error` (parity with fileSyncLoad
et al). New test asserts the returned error has no path content
and no stack.
2) The image picker button was not disabled while the IPC was in
flight. A second click would queue a second IPC, opening a second
dialog after the first resolves; because `replacesId` is computed
from the form value at click time, the first import's id would
never be GC'd. Add an `isPickerBusy` signal and bind the button's
`disabled` to it. New spec asserts the second concurrent click is a
no-op and the busy flag clears in the `finally`.
3) Stale comment in `local-file.model.ts` pointed at
`electron/sync-folder-store.ts`, which doesn't exist (the storage
was inlined into local-file-sync.ts in Phase 2.5). Update to
describe the actual current shape and clarify that the field is
read once on upgrade for the migration breadcrumb and never written
from new code paths.
4) Image input placeholder advertised `file://` URLs as a valid
input. Phase 4 removed the IPC that resolved them, so the
placeholder was a footgun. Drop the `file://` half.
68/68 electron security tests; 8/8 formly-image-input karma; lint +
tsc clean.
End-to-end review of the #8228 sweep flagged one real residual risk
and several smaller items. This commit addresses them in one pass.
The IMAGE_CACHE_IMPORT IPC accepted a renderer-supplied absolute path
with no link to a recent SHOW_OPEN_DIALOG signal. A compromised
renderer could call importImage() with any image-extension path under
5 MB outside userData — same information-disclosure shape the legacy
READ_LOCAL_IMAGE_AS_DATA_URL had, just one-shot instead of repeatable.
Fix: merge the dialog and the import into a single atomic IPC,
IMAGE_PICK_AND_IMPORT. Main opens the picker inline and only calls
importImage() with the file the user actually clicked. The renderer
no longer holds a path at any point in the flow; an attacker who
controls the renderer cannot trigger an image read without a real
user dialog interaction.
Shape change:
- Return null on user-cancel; throw on validation failure. This
preserves the existing UX where cancel is silent and an unreadable
image shows a snack. Renderer-side openFileExplorer is a try/catch.
- Optional { replacesId } arg garbage-collects the previously
cached image when the user picks a new one. Closes the disk-fills
follow-up the review raised (#3) without an extra IPC round-trip.
Smaller items from the review folded in:
- Extract _resolveRelative helper in local-file-sync.ts; the five
FS handlers had the same three-line incantation copy-pasted. The
Phase 2.5 simplification over-corrected by inlining everything.
- Drop the dead __resetSyncFolderCacheForTests export; no caller, and
the tests reset via require.cache surgery anyway.
- image-cache.ts getExt now uses path.extname so Windows backslash
paths are handled correctly (the old slash-only split worked by
accident).
- resolveBgImageToDataUrl: short-circuit on empty id, and log a
console.warn when a legacy file:// is detected (no resolvable IPC
remains; user must re-pick). i18n snack is a documented follow-up.
- Package LocalFileSyncElectron.isReady now logs a critical line via
the injected sync logger when isReady=false but the renderer's
privateCfg still has the legacy syncFolderPath — dev console
surfaces the "needs re-pick" state without dragging i18n into the
security PR.
- Stale comment in electron-file-adapter.ts referenced a deleted file
(sync-folder-store.ts is inlined into local-file-sync.ts now).
Tests:
- electron/local-file-sync.test.cjs gains four IPC-level cases:
legacy IMAGE_CACHE_IMPORT not registered, pick+import round-trip,
cancel returns null, validation failure throws, replacesId GCs the
prior cached file.
- formly-image-input.component.spec rewritten for the new flow:
asserts imagePickAndImport is called with replacesId when one is in
state, snack on rejection but not on null, no setValue on either.
68/68 electron security tests pass; 7/7 formly-image-input karma;
5/5 resolve-bg-image karma; 17/17 sync-providers package
local-file specs. Lint + tsc clean.
What this commit does NOT do (called out so it isn't forgotten):
- CLIPBOARD_COPY_IMAGE_FILE has the same renderer-supplied-path shape
for clipboard images. Parallel issue; out of scope for #8228.
- PICK_DIRECTORY has no defaultPath — could nudge users toward a
dedicated sync subfolder vs picking ~/Documents. Cosmetic.
- Symlink TOCTOU between resolveSyncPath and writeFileSync stands;
O_NOFOLLOW-on-open is the proper fix and needs cross-platform care.
- Migration UX is console-only; a user-visible snack needs i18n
infrastructure not appropriate for the security commit.
Phase 2 of #8228. Closes the renderer-supplied-absolute-path hole: the
file-sync IPCs now take a relative path; main resolves it against a
sync folder it owns and never trusts the renderer's copy of.
Main side
- sync-folder-store: persists the path in simple-store (still inside
userData, still renderer-unreachable via assertPathOutside) with an
in-memory cache so per-IPC lookup doesn't stat the file every call.
initSyncFolderStore() is fire-and-forget at adapter init; each handler
awaits it defensively because it's idempotent via _loadOnce.
- FILE_SYNC_SAVE/LOAD/REMOVE: accept { relativePath, ... }, pass through
resolveSyncPath, write/read/delete the vetted absolute path.
- CHECK_DIR_EXISTS and FILE_SYNC_LIST_FILES: accept an optional
relativePath; default to the sync root itself so the existing
"is the sync folder reachable?" check still works.
- PICK_DIRECTORY: now persists the chosen folder via setSyncFolderPath
before returning the display string to the renderer. The renderer
receiving the string is no longer load-bearing — it's display only.
- New GET_SYNC_FOLDER_PATH IPC returns the current value for display
(settings form, "sync folder: …" labels).
- TO_FILE_URL: pulls in the assertPathOutside(userData) backstop that
was missing on master, so a userData path cannot be laundered into a
file:// URL that later flows through READ_LOCAL_IMAGE_AS_DATA_URL or
the navigation guard.
Package (@sp/sync-providers/local-file)
- LocalFileSyncElectron.isReady now asks main (getMainSyncFolderPath
dep) instead of reading the renderer's privateCfg. This drives the
migration UX: an existing install with a legacy privateCfg value but
no main-side path lands on "not configured" and gets re-prompted by
the picker. A one-cycle "please pick again" is the migration cost; we
decided against silently promoting the renderer's persisted value
because it could be tampered with.
- getFilePath returns the *relative* path (no absolute folder prefix).
The leading-slash strip is preserved so existing call sites that
prepend '/' still work.
- pickDirectory no longer writes to renderer privateCfg — main owns
persistence. The dep contract still returns the display string.
- The privateCfg.syncFolderPath field is marked @deprecated; the
field is kept for migration of older configs and may be removed in a
later pass once the migration has rolled out.
Renderer
- ElectronFileAdapter forwards the relative path verbatim to the IPC.
- The renderer's LocalFileSyncElectron deps wire pickDirectory and the
new getMainSyncFolderPath to the bridge.
Tests
- electron/local-file-sync.test.cjs rewritten for the new signatures:
happy path, traversal escapes, absolute-relativePath rejection,
sync-folder-equals-userData rejection (grant-file forgery defense),
PICK persists main-side, GET returns the configured path, TO_FILE_URL
refuses userData.
- electron/sync-folder-store.test.cjs covers persistence,
init-before-get throw, null/empty handling, idempotent set.
- The package's local-file-sync-electron.spec.ts is updated for the new
deps shape and asserts the package no longer writes to the renderer
credential store.
All 56 electron-side tests pass; 374 sync-providers tests pass.
Delete 29 plan/design docs whose work has shipped or been superseded
(SuperSync slices, sync-core extraction, encryption-at-rest drafts,
document-mode/Stage-A persistence, calendar/CalDAV concepts, focus-mode
time-tracking sync, etc.).
Kept the still-forward-looking docs (e.g. supersync-encryption-at-rest,
sync-core-simplification-roadmap, calendar-two-way-sync-technical-analysis).
Source comments that cited deleted docs are rewritten into self-contained
inline rationale so no "see docs/..." reference dangles.
error-meta.spec.ts was the only test importing via the package
self-reference subpath @sp/sync-providers/log, which exports maps to
./dist/log.mjs — requiring a tsup build that 'npm test' never runs, so
vitest could not resolve it. Import ../../src/log like the other 15
test files and drop the now-dead tsconfig.spec path mapping that only
existed to satisfy that import's typecheck.
* fix(sync): guard fresh-client first sync against data-loss trap & decrypt-race
A genuinely-fresh client seeds example tasks before its first sync, so on first
connect to a populated, encrypted SuperSync dataset it hit a SYNC_IMPORT conflict
dialog whose USE_LOCAL would overwrite the real remote, plus a red
DecryptNoPasswordError.
- A1: SyncImportConflictGateService flags isNeverSynced (\!hasSyncedOps) on the
incoming-import dialog; the dialog guards the destructive USE_LOCAL with a
confirm and focuses the scenario-appropriate safe button via cdkFocusInitial.
- B: OperationLogDownloadService logs 'encrypted ops, no key' quietly only for a
genuinely-fresh client (never synced AND no local encryption flag); it stays
loud (dropped-credential signature) for already-synced clients and for a wiped
op store whose config still flags encryption on, via the new optional provider
method SuperSync.isEncryptionEnabled().
- D: SuperSync.isReady() returns false while isEncryptionEnabled && \!encryptKey,
keeping a self-inconsistent encrypted config out of auto-sync; shares the
_isEncryptionHalfConfigured predicate with getEncryptKey().
Tests: super-sync isReady/isEncryptionEnabled, conflict-gate isNeverSynced,
dialog confirm-guard + focus-by-scenario, download severity branches, and a
sync-service end-to-end first-sync-trap guard.
Plan/rationale: docs/plans/2026-06-03-fresh-client-first-sync-data-loss-trap.md
* docs(sync): add fresh-client first-sync data-loss-trap plan
Documents the shared root cause (example tasks seeded pre-first-sync), the A1/B/D
fixes, and the accepted Fix C residual (example-task pollution onto non-encrypted
accounts) with the rationale for deferring an identity-based cleanup.
* fix(sync): capture never-synced guard pre-download for piggyback SYNC_IMPORT
The SYNC_IMPORT conflict gate's never-synced guard (which blocks a
fresh client from force-overwriting a populated remote via USE_LOCAL)
was computed from a live hasSyncedOps() read on the piggyback-upload
path. By the time that gate runs, the same sync has already persisted
downloaded ops with syncedAt and marked accepted uploads synced, so the
flag flips to "has synced" mid-cycle and the guard disarms itself —
re-opening the data-loss trap on the piggyback path.
Capture the never-synced snapshot once at sync-cycle start, before
download, and thread it through downloadRemoteOps()/uploadPendingOps()
into the gate. The gate falls back to a live read only for standalone
callers (download gate is safe live: nothing is persisted until
processRemoteOps).
Also stop the sync-wrapper catch from re-logging the expected
DecryptNoPasswordError at error level — the download/upload service
already logs it at the right severity, so re-logging undid the
fresh-client onboarding-prompt quieting.
Tests: gate honors a caller-provided isNeverSynced without consulting
live history; piggyback path flags isNeverSynced=true even when the
upload marks ops synced; wrapper threads the pre-download snapshot.
Refresh the plan doc's stale test counts and document the fix (§11).
iOS reads a stale sync-data.json on every WebDAV sync: URLSession's
default configuration caches GET responses, so the client never sees
remote changes and — because the pre-upload conflict check re-reads the
same cached body — blindly overwrites newer remote data. Android (OkHttp,
no cache) and web (fetch `cache: 'no-store'`) are unaffected, which is why
this is iOS-only.
- iOS WebDavHttpPlugin: disable the HTTP cache (urlCache=nil +
reloadIgnoringLocalCacheData) so sync reads always hit the network.
- WebDavHttpAdapter: send `Cache-Control: no-cache, no-store` + `Pragma`
on the native request path, covering an upstream proxy/CDN cache too.
- Log allowlisted, non-sensitive cache-relevant response headers so a
future log capture localises a stale read (client cache vs proxy).
Fixes#7144
Address review items from issue #7797:
- @odata.nextLink absolute-URL pass-through (sovereign clouds), gated by
an HTTPS + Microsoft Graph host allowlist so a tampered nextLink can't
redirect this request's Bearer token to a hostile origin.
- Hard cap (500 pages) on listFiles pagination to prevent unbounded
growth from a buggy or cyclic @odata.nextLink.
- Collapse the missing/mismatched OAuth state errors into a single
i18n'd snack key (T.F.SYNC.S.OAUTH_STATE_INVALID).
- Replace the `as OneDrivePrivateCfg` cast with an explicit field merge
that coerces form-side nulls to undefined; surface a snack if required
fields are missing instead of returning silently.
- Mark the OneDrive provider as experimental in the sync provider
dropdown.
Adds unit tests covering the sovereign-cloud nextLink pass-through, the
pagination cap, and the new bearer-token allowlist (asserts the hostile
host is rejected before fetch is called).
* feat(sync): integrate onedrive with pkce and operation log sync
* fix(sync): add oauth state validation and token refresh concurrency control
* fix(sync): refactor onedrive oauth cleanup and reduce download API calls
- Replace setInterval-based OAuth state pruning with on-demand
_pruneExpiredOAuthStates() to avoid background timer overhead
- Optimize downloadFile to read ETag from content response headers
first, falling back to a metadata request only when needed (reduces
API calls from 2 to 1 in the common case)
- Narrow OneDrive class interface from SyncProviderServiceInterface to
FileSyncProvider for stronger type guarantees
- Accept optional devPath constructor parameter for non-production
folder namespacing
- Extract isOneDriveClientIdRequired to a helper function in
sync-form.const.ts for readability
- Change default OneDrive sync folder name to 'Super Productivity'
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): address PR #7523 review feedback for OneDrive sync
Critical fixes:
- Detect invalid_grant in token refresh responses and clear credentials
- Reset _tokenRefreshInFlightPromise in clearAuthCredentials to prevent
stale refresh results from overwriting cleared state
- Re-read config before persisting refreshed tokens to avoid race with
concurrent clearAuthCredentials calls
Important fixes:
- Extract OAuth state management to shared oauth-state.util.ts to fix
circular dependency between OneDrive provider and callback handler
- Add 401 retry pattern (_is401Retry) matching Dropbox's approach:
on 401, proactively refresh token and retry, only clear credentials
if retry fails
- Remove unused auth status translation keys (AUTH_SUCCESS,
BTN_AUTHENTICATE, BTN_REAUTHENTICATE, REAUTH_SUCCESS,
STATUS_CONFIGURED, STATUS_NOT_CONFIGURED) and form props
- Remove unused requireAuth parameter from _cfgOrError
Minor fixes:
- Replace path-exposing log messages with structured logging
- Remove unused _formatHttpErrorDetails helper
- Return empty ETag fallback in getFileRev instead of throwing
- Add folder cache invalidation comment for path changes
- Add afterEach to restore global fetch in tests
- Add PKCE defence-in-depth comment in auth code dialog
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): use eslint-disable-next-line for @microsoft.graph.conflictBehavior
Replace spread+computed-property workaround with a plain property plus
an explicit eslint-disable-next-line comment. Clearer intent: the
naming violation is a Graph API requirement, not accidental.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): pre-save OneDrive form config in reauth() to prevent MissingCredentialsSPError
Re-auth was failing silently on Linux because getAuthHelper() reads
clientId from IndexedDB (privateCfg), but the form values were never
written before calling configuredAuthForSyncProviderIfNecessary().
- Extract shared BTN_REAUTHENTICATE and REAUTH_SUCCESS translation keys
- Add OneDrive to OAUTH_SYNC_PROVIDERS
- Pre-save form config in reauth() matching the existing save() flow
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): adapt TooManyRequestsAPIError to new constructor signature after rebase
* refactor(sync): migrate OneDrive provider to packages/sync-providers
Move OneDrive core logic into the shared sync-providers package,
matching the pattern used by Dropbox, WebDAV, and other providers.
The app-side code is now a thin adapter with a createOneDriveProvider()
factory function.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): re-throw MissingRefreshTokenAPIError from _requestOAuthToken catch block
The catch block in _requestOAuthToken was swallowing the
MissingRefreshTokenAPIError thrown after clearing credentials on
invalid_grant, causing it to fall through to a generic HttpNotOkAPIError
instead. Now re-throws MissingRefreshTokenAPIError explicitly.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): address PR #7523 second review critical and important items
- Replace _is401Retry instance field with per-call parameter to prevent
concurrent 401 races (Critical #1)
- Fix _requestOAuthToken catch block to re-throw MissingRefreshTokenAPIError
instead of swallowing it (Critical #2)
- Fix _parseOAuthCallback defaulting unknown provider to 'unknown' instead
of 'dropbox' to prevent misrouting (Critical #3 + Important #6)
- Change conflictBehavior from 'replace' to 'fail' to prevent data loss
if a file exists with the same name as the folder (Critical #4)
- Await in-flight token refresh promise in clearAuthCredentials before
clearing, to close the refresh-during-clear race (Important #5)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): address PR #7523 review important and minor items
- Extract duplicate OneDrive pre-auth save block into
_persistOneDriveFormCfgBeforeAuth() method (Important #7)
- Add GET probe in _ensureSyncFolderExists to skip per-segment
creation when folder already exists (Important #9)
- Redact sensitive params (code, code_verifier, refresh_token,
access_token) from token endpoint and Graph error bodies
(Important #11)
- Remove targetPath from _mapAndThrow error messages to prevent
logging user content (Important #12)
- Add OAuth state validation in manual paste flow for OneDrive
(Important #13)
- Fix unknown provider routing in _parseOAuthCallback (Important #14)
- Guard Electron IPC listener against post-destroy emissions
(Important #15)
- Remove dead authStatus Formly field from sync-form.const.ts (Minor)
- Simplify Headers builder in _ensureSyncFolderExists (Minor)
- Fix spec afterEach to restore original fetch instead of undefined
(Minor)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): fix clearAuthCredentials race and add unit tests for OneDrive
Fix a race condition in clearAuthCredentials where _tokenRefreshInFlightPromise
was nulled AFTER awaiting, causing the invalid_grant handler's clearAuthCredentials
to wait on itself. Now captures the promise and nulls the field before awaiting.
Add 5 new unit tests covering critical code paths:
- 401 token refresh and retry
- 401 retry failure with credential clearing
- 400 invalid_grant credential clearing
- 412 precondition failed mapping
- Concurrent token refresh deduplication
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): address PR #7523 third review items
- Simplify clearAuthCredentials: null _tokenRefreshInFlightPromise without
awaiting to avoid deadlock when called from inside the refresh IIFE
- Add superproductivity:// scheme validation in Electron OAuth listener
- Fix invalid_grant test to assert MissingRefreshTokenAPIError
- Fix folder cache test to match GET-probe optimization
- Set setComplete default in beforeEach
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* test(sync): fix OAuthCallbackHandler spec to expect 'unknown' provider
The _parseOAuthCallback now returns 'unknown' instead of 'dropbox' for
URLs without an explicit provider path segment.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): address PR #7523 fourth review items
- Handle generic 401 from token endpoint (clear creds + MissingRefreshTokenAPIError)
- Remove user-supplied paths from error constructors (rule 9 compliance)
- Soften clearAuthCredentials comment to reflect actual TOCTOU guarantee
- Tighten Electron scheme gate to match native path prefix
- Add scheme info to Electron rejection log
- Fix folder-cache test to assert GET probe count
- Remove redundant per-test setComplete stub
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): address PR #7523 fifth review items
- Scope invalid_grant/401 credential clearing to refresh_token grant only
(bad auth code exchange no longer wipes existing credentials)
- Redact OAuth code/state from Electron protocol handler logs
- Add @sp/sync-providers/onedrive TS path alias to tsconfig files
- Default undefined sync config fields instead of overwriting with undefined
- Handle OneDrive in provider-switch branch (preserve encryptKey)
- Update sync-config test expectations to match default values
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): address PR #7523 sixth review items
- Use If-None-Match: * for null-rev uploads to prevent concurrent overwrite
- Guard in-flight refresh against stale credentials (match refreshToken/clientId/tenantId)
- Gate OneDrive provider option behind Electron/Native (web CORS unsupported)
- Don't clear credentials on transient token endpoint errors (429/5xx)
- Preserve null syncProvider instead of defaulting to WebDAV
- Hydrate full OneDrive privateCfg on provider switch
- Remove duplicate syncInterval/isManualSyncOnly Formly controls
- Revert non-English locale changes (zh.json, zh-tw.json)
- Redact URL fragments (#) in Electron protocol handler logs
- listFiles rethrows non-404 errors instead of swallowing all
- Update 401 test expectations for new rethrow behavior
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): guard credential clearing against stale concurrent refresh
Prevent a stale in-flight refresh from wiping newer credentials after
a concurrent re-auth. The refresh IIFE now throws a plain Error (not
MissingRefreshTokenAPIError) when it detects config drift, and all
clearAuthCredentials() call sites now verify the stored config still
matches before clearing. Also fix listFiles() to handle 404 from
_requestJson (which throws HttpNotOkAPIError, not RemoteFileNotFoundAPIError).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): address PR #7523 eighth review items
- Replace If-None-Match:* with @microsoft.graph.conflictBehavior=fail query param for upload create-only semantics
- Add identity change detection in _persistOneDriveFormCfgBeforeAuth to clear tokens when useCustomApp/clientId/tenantId change
- Extract _clearIfConfigMatches helper to guard credential clearing against stale concurrent refresh
- Restrict folder probe fallthrough to 404 only, propagate other errors
- Restore locale files from upstream master
- Centralize IS_ONEDRIVE_SUPPORTED in onedrive-auth-mode.const.ts, use in factory and form config
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): address PR #7523 ninth review items
- Paginate listFiles via @odata.nextLink to avoid silent data loss
- Fix logger misuse (log -> normal with proper string message)
- Replace as any with OneDrivePrivateCfg in dialog-sync-cfg
- Throw NoRevAPIError when uploadFile response lacks eTag
- Omit undefined optional booleans from global config to prevent overwrite
- Move OneDrive dynamic import inside IS_ONEDRIVE_SUPPORTED gate
- Require state param for OneDrive full-URL paste (CSRF)
- Add id_token to sensitive keys for body redaction
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(sync): add OneDrive type alias to electron tsconfig paths
The @sp/sync-providers/onedrive path alias was missing from
electron/tsconfig.electron.json, causing the Electron build to fail
with "Cannot find module '@sp/sync-providers/onedrive'". The alias was
already present in tsconfig.base.json but electron uses its own paths.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
WebdavBaseProvider.clearAuthCredentials() blanked the user-typed
userName/password on a single recoverable AuthFailSPError /
MissingCredentialsSPError. The transient-retry tolerance in
sync-wrapper is SuperSync-only, so a one-off 401 on WebDAV/Nextcloud
irreversibly erased the stored connection settings with no undo.
A WebDAV password is a user-typed, often-irrecoverable secret, not a
refreshable OAuth token. Remove the WebDAV/Nextcloud override so
ProviderManager's existing guard makes the auth-error clear path a
no-op for them; Dropbox/SuperSync keep their token clearing. The
actionable re-configure snackbar still fires unchanged. Document the
optional-hook contract on SyncProviderBase.
Add regression tests for the provider hook (incl. asserting WebDAV
exposes no clearAuthCredentials) and a legacy WebDAV credential
migration repro harness (the migration itself was sound).
Fixes#7616. Related: #7619 (exported logs leak task content).
testConnection() did a PROPFIND only on the configured sync folder.
On first-time setup that folder does not exist yet (created lazily on
first upload), so the server returned 404 and every correctly-configured
new Nextcloud/WebDAV user saw "Connection test failed".
Probe the WebDAV base root instead: it is reachable and auth-checked for
any valid server/credentials, while a wrong username/base path (404) or
bad password (401) still correctly fails. Add regression tests covering
the base-root probe, wrong-base 404, and the bad-credentials safety
invariant.
Address review nits on 058f92e972: anchor the retryable-upload regex
to "unable to resolve host" so future SuperSync op-graph rejection
strings (e.g. "Unable to resolve parent revision") can't be flipped
from permanent to retryable; rename RETRY_BACKOFF_MS to
RETRY_BACKOFF_BASE_MS (it is the linear base, not the total); clarify
the JSDoc that 4.5s is the inter-attempt sleep budget, not wall-clock.
Android UnknownHostException after Doze-mode resume produced a raw
"Unable to resolve host" snackbar because the post-retry error message
didn't match isRetryableUploadError's patterns, so it skipped the
NetworkUnavailableSPError wrap. Bump the native HTTP retry backoff from
1s/2s to 1.5s/3s (~4.5s budget) to better tolerate post-Doze DNS
staleness, and add the Android phrasing to both transient-error checks
so the error now surfaces as the translated network-warning snackbar.
* refactor(sync): tighten extracted package surfaces
Combined polish from the post-extraction review:
- sync-core: strip NgRx-shaped types from EntityConfig/EntityRegistry;
expose host extensions via generic param. Move StateSelector,
PropsStateSelector, SelectByIdFactory, SelectById, EntityUpdateLike,
EntityAdapterLike to a new app-side entity-registry-host.types.ts.
- sync-core: mark OpType.SyncImport/BackupImport/Repair as @deprecated;
hosts should use createFullStateOpTypeHelpers().
- sync-providers: resolve provider.types.ts vs provider-types.ts
duplication; inline implementation into the dashed canonical name.
- sync-providers: drop unused root barrel and "." export; consumers
already use focused subpath barrels (/dropbox, /webdav, etc.).
- sync-providers: replace wildcard "@sp/sync-providers/*" tsconfig path
alias with 11 explicit subpath entries matching package.json exports;
deep-internal imports now fail at typecheck.
- sync-providers: move @sp/sync-core from dependencies to
peerDependencies (kept in devDependencies for tests).
- both packages: add composite: true to enable project references;
introduce tsconfig.build.json overlay so tsup DTS bundler still works.
- gitignore: ignore **/*.tsbuildinfo composite outputs.
* refactor(sync-core): prune 47 unused barrel exports
Removes exports with zero consumers outside the package. Source files
are unchanged; only the public barrel is trimmed. Covers compression
helper classes, sync-file-prefix error/config types, replay coordinator
internals, remote-apply result types, upload/download planning option
and plan types, ports misc, conflict-resolution helper types, and
sync-import-filter decision types.
* refactor(sync-core): drop unused encryption migration path
decryptWithMigration and DecryptResult had no host consumer; they
exposed a structural-migration entry point ("here is your ciphertext
re-encrypted under Argon2id") that nothing in the codebase reads. The
side-channel setLegacyKdfWarningHandler — which IS used — stays.
encryptWithDerivedKey/decryptWithDerivedKey lose their export keyword
and remain as module-internal helpers; encrypt/decrypt/encryptBatch/
decryptBatch still call them. Wire format and legacy-fallback semantics
are unchanged, so existing ciphertext continues to decrypt.
Test imports for compression and sync-file-prefix specs now go via
their source files instead of the trimmed barrel.
* fix(sync-providers): bound dropbox token refresh to single retry; share md5 rev helper
The five hand-rolled token-refresh blocks in Dropbox.{getFileRev,
downloadFile, uploadFile, removeFile, listFiles} recursed on themselves
after refresh. If the post-refresh call still saw a token error (real
case: the refresh token itself was revoked), the recursion would not
terminate. Consolidated into a single _withTokenRefresh helper that
attempts the call, refreshes once on a token error, retries once, then
lets the outer 401 classifier surface AuthFailSPError.
Same log message, same _isTokenError discriminator, same refresh call.
Same five sites still apply their post-call non-token error mapping
(NoRev, InvalidData, RemoteFileNotFound, path-not-found swallow, etc.).
Also extracts md5 content-rev computation duplicated between
LocalFileSyncBase._getLocalRev and WebdavApi._computeContentHash into a
shared file-based/content-rev.ts; both call sites preserve their own
error wrapping at the boundary.
* refactor(sync): split oversized super-sync and conflict-resolution
sync-providers: extract request-ID hashing from super-sync.ts (1017 ->
918 lines) into a new request-id.ts. The helpers were free functions
already in disguise (none referenced this), so the move is mechanical.
HTTP plumbing (_doWebFetch/_doNativeFetch/_fetchApi*) stays as private
methods — it transitively touches 12 instance members and would need
either a wide context object or a separate http-client collaborator
class to extract cleanly. Left as a follow-up.
sync-core: split conflict-resolution.ts into three cohesive files:
- entity-frontier.ts now owns buildEntityFrontier and
adjustForClockCorruption (per-entity vector-clock domain).
- extractEntityFromPayload and extractUpdateChanges move to
operation.types.ts next to the existing extractActionPayload.
- conflict-resolution.ts keeps deep-equality, LWW planning,
partitioning, and identical-conflict detection.
Public barrel exports unchanged; tests now import the moved symbols
from their new homes.
* refactor(sync-core): drop redundant OperationStorePort
OperationStorePort overlapped with RemoteOperationApplyStorePort on the
two state-transition methods (markSynced/markApplied,
markRejected/markFailed) and had zero non-structural consumers — the
only implementer was OperationLogStoreService, which already exposes
the three methods as its own public surface. Removing the port leaves
the service contract intact and removes the verb-pair confusion noted
in the post-extraction review.
Spec contract test still drives the same state transitions; only the
local typing of the test fixture changes from the deleted interface to
Pick<OperationLogStoreService, ...>.
* refactor(sync-providers): decouple SuperSync provider from SP-specific host
Two coupling leaks the package shouldn't carry:
1. SUPER_SYNC_DEFAULT_BASE_URL was an implicit fallback inside
SuperSyncProvider — an SP-specific URL baked into a "framework-
agnostic" package. Make defaultBaseUrl a required SuperSyncDeps
field; the host factory supplies the SP default. The constant stays
exported as a suggested default for hosts targeting the SP-hosted
server.
2. Consumers that wanted the WebSocket path had to do
`provider as unknown as SuperSyncProvider` to call
getWebSocketParams. Introduce SuperSyncWebSocketAccess interface +
isSuperSyncWebSocketAccess structural guard; SuperSyncProvider
implements it. sync-wrapper.service drops its cast in favor of the
guard.
super-sync-restore.service still casts to SuperSyncProvider for the
restore path — same pattern would solve it, but out of scope here.
* test(sync-providers): extract shared test helpers and prefer barrels
Adds tests/helpers/sync-logger.ts and tests/helpers/credential-store.ts
to centralize the noopLogger and CredentialStore mocks that were copy-
pasted across 8 spec files. createStatefulCredentialStore covers the
"load/upsert/clear with state" cases; createMockCredentialStore covers
bare vi.fn() ports. Spec sites that needed a unique mockResolvedValue
chain it after the helper, preserving behavior 1:1.
Also migrates 5 spec files from deep ../src/<file> paths to the
matching sub-barrel (../src/webdav, /http, /super-sync, /platform) for
symbols already exported there. No new barrel exports added — internal
types (WebDavHttpAdapter, WebdavApi, DropboxApi, etc.) stay on deep
paths because they are intentionally not part of the public surface.
super-sync.spec.ts keeps its own credential/logger mocks (special
__asPort wrapper and vi.spyOn against the live NOOP_SYNC_LOGGER) that
the generic helpers cannot reproduce without bloat.
* test(sync): pin vector-clock pruning, error-meta privacy, and sync-import edges
Fills three test gaps surfaced by the post-extraction review:
- vector-clock pruning correctness across clocks: 4 cases pinning that
pruning legitimately flips GREATER_THAN to CONCURRENT/LESS_THAN when
the dropped keys are still present in the comparison clock. This is
the documented behavior (compareVectorClocks is intentionally not
pruning-aware); the protocol handles flips server-side via the
rejected-ops retry loop. preserveClientIds case also covered.
- error-meta privacy boundary: 22 new cases covering urlPathOnly (strip
query/fragment/userinfo, preserve host+path+port, leave non-URLs
intact) and errorMeta (no leakage of headers, response bodies, OAuth
tokens, signed-URL params, user emails, or attached error fields).
Real negative assertions (.not.toContain), not shape checks.
- sync-import-filter edge cases: 8 cases covering empty clocks on
either side, op clock listing the import client at 0, same-client
with equal counter (pinning the strict-greater-than boundary), and
different-client knowledge above the import counter.
sync-core 195 -> 207 tests, sync-providers 319 -> 341 tests; no
production code changed.
* style(sync-core): format sync-file-prefix.spec import line
* fix(sync): address package review feedback
Findings from a multi-agent review of the recent sync extraction:
- Seven error classes in @sp/sync-providers shipped with a leading
space in `name`, and `UploadRevToMatchMismatchAPIError` was further
truncated to ' UploadRevToMatchMismatchAP'. Consumers use instanceof
so runtime behavior was preserved, but stack traces, error envelopes,
and structured-log meta carried the broken names. Added a regression
test asserting instance.name matches ErrCtor.name for all 14 classes.
- @sp/sync-core decryptBatch JSDoc claimed Argon2 errors are never
silently masked as legacy fallbacks, contradicting the actual
catch-and-decryptLegacy path. The fallback is part of the public
wire-format contract; rewrote the comment to match the implementation
and reference the module-level wire-format spec.
- SuperSyncEncryptionToggleService.enable/disableEncryption promised
"Clear cache on success" but never called clearSessionKeyCache().
Added the call on the success path in both methods, matching the
pattern used by EncryptionPasswordChangeService and
FileBasedEncryptionService.
- Removed packages/sync-core/tests/ports.spec.ts — 57 LOC of
vi.fn().mockResolvedValue type-assertion tests with no production
code under test. Port shapes are enforced at the host via
`implements` clauses with real behavioral coverage in sibling specs.
- super-sync: keep the 75s abort timer alive across response.text() on
non-OK responses so a stalled error body still triggers AbortError.
- decryptBatch: hold derived keys in a batch-local map. Previously
Phase 3 read from the LRU session cache, which could evict entries
mid-batch when the input contained more unique salts than the cache
could hold (SESSION_DECRYPT_CACHE_MAX_SIZE = 100), crashing on the
non-null assertion. Adds a 120-item regression test.
- session cache: replace the 32-bit djb2 password identifier with a
length-prefixed full-password key (injective). A djb2 collision
silently returned a key derived from a different password, producing
undecryptable ciphertext on subsequent encrypts.
- docs: bless salt-per-session-per-password semantics explicitly in the
encryption.ts JSDoc and the sync-core README so future readers and
tests know IV uniqueness — not salt uniqueness — is the AES-GCM
invariant being preserved.
Two related improvements to snapshot/full-state handling:
- Persist the incoming op's vector clock as `user_sync_state.latest_full_state_vector_clock`
so downloads can skip the expensive aggregate scan when the snapshot clock is
still valid; falls back to the legacy aggregate when missing or invalid.
- Treat retried snapshot uploads (same opId) as idempotent successes instead of
409/DUPLICATE_OPERATION so a dropped response doesn't push clients into the
download-and-merge path. Per-namespace request dedup keeps ops and snapshot
caches isolated.
