Repository navigation
fix(cli): sync import --dry-run must never replace the live database - #16723
Merged
Merged
Conversation
`omniroute sync import --dry-run` POSTed the backup to /api/db-backups/import with `?dryRun=true`, but the route never read the flag, so a dry run replaced the operator's database wholesale. Route: `dryRun=true` (query string or multipart field) now runs the same validation (type, size, SQLite integrity, required tables) and returns the row counts and schema version a real import would load, without the pre-import backup and without touching the live database. The handler is split into readUpload / validateUploadShape / inspectCandidate / replaceLiveDatabase; the candidate summary query lives in src/lib/db/backup.ts (summarizeImportCandidate). The route also accepts the machine-bound local CLI credential (new isLocalCliCredential helper, shared with resolveOperatorCredential) next to the dashboard session and manage-scoped API keys, so `sync import` on the server host passes auth. The temp candidate handle is now closed on every validation exit. CLI: the import path moves to bin/cli/commands/sync-import.mjs. With --dry-run it never contacts the server (an older server would still ignore the flag): it validates the file locally and prints the summary. A real import goes through apiFetch (local CLI credential on loopback, no retry, 5 min timeout), refuses non-SQLite files such as the JSON written by `sync bundle` before uploading, and turns 400/401/403/404 into a readable error with the usual exit code instead of a bare status.
…nc-import-dry-run
…r helpers After #16721 the sync subcommands share sendSyncRequest/reportFailure/ routeUnavailableHint. The import upload now lives in sync.mjs on those helpers; sync-import.mjs keeps only the local read/header check and the dry run. Drops the authHeaders helper and the getBaseUrl import, both unused after the merge.
Contributor
CI Coverage Report
Coverage artifact was not available for this run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
omniroute sync import --dry-runPOSTed the backup toPOST /api/db-backups/import?dryRun=true, but the route never readdryRun. A "dry run" therefore replaced the live database, which is data loss. The command also sent only the Bearer key. The route only accepted a dashboard session or a manage-scoped API key, so on the server host, with just the local CLI credential, the import came back 401.Fix
Route (
src/app/api/db-backups/import/route.ts): a real server-side dry run.dryRun=true(query string, or a multipartdryRunfield) runs the same validation as a real import:.sqlitetype, size limits, SQLiteintegrity_checkand the required tables. It then returns{ dryRun: true, valid: true, imported: false, connectionCount, nodeCount, comboCount, apiKeyCount, schemaVersion, sizeBytes, filename }. It skips the pre-import backup and leaves the live DB alone (the test checks the file hash).readUpload/validateUploadShape/inspectCandidate/replaceLiveDatabase. The complexity ratchet improved (complexity −2, cognitive −1). The SQL that summarizes the candidate lives insrc/lib/db/backup.ts(summarizeImportCandidate), so the route has no raw SQL. The temp candidate handle is now closed on every validation exit; before, an integrity or tables 400 leaked it.isLocalCliCredential()helper, whichresolveOperatorCredential()now shares. The always-protected classification inrouteGuard.tsis unchanged.CLI (
runSyncImportinbin/cli/commands/sync.mjs; local checks inbin/cli/commands/sync-import.mjs)--dry-runnever contacts the server. An older server still ignoresdryRunand would replace the DB, so the CLI validates the file locally (header, integrity, required tables, counts, schema version) and prints the summary.sendSyncRequest/reportFailure/routeUnavailableHinthelpers, soapiFetchsends the local CLI credential to a loopback server. It runs with no retry (a replace should not be replayed blindly after a 502) and a 5-minute timeout. Files that are not SQLite (for example the JSON fromsync bundle) are refused before upload, with a hint. 400/401/403/404 end inError: sync import failed (HTTP n): <server message>plus a hint and the usual exit code (2/4/…), never a stack trace.release/v3.8.52after fix(cli): map sync push/bundle/initialize onto the real routes; drop sync diff/resolve #16721. TheauthHeadershelper and thegetBaseUrlimport were unused after the merge and are removed.Why a real dry run and not a 400: the validation already existed, and the dry run is just an early return before the destructive branch. That is cheap, and it also protects curl, dashboard and future clients that pass the flag.
Tests (TDD)
tests/unit/db-import-dry-run.test.tsdrives the real route against an isolatedDATA_DIR: query and multipart dry runs leave the live sentinel row and the file hash unchanged, an invalid upload in dry-run mode gets a 400 with no side effect, a request with no credential gets 401, the pipeline-stamped local CLI subject is accepted, and a real import still replaces the DB (regression). RED on base: 5/6 failed. The first dry run returnedimported: trueand wiped the sentinel, and the CLI subject got a 401. GREEN: 6/6.tests/unit/cli-sync-import-dry-run.test.tsdrives the real commander wiring withfetchstubbed:--dry-runmakes zero network calls, a backup missing tables gets exit 2, a JSON bundle is refused locally, an unreadable path gives a clean error, a real import sends POST + octet-stream +x-omniroute-cli-token+?filename=, a 403 gives exit 4 with a hint, and a 400 surfaces the server message. RED on base: 7/7 failed. GREEN: 7/7.Validation
cli-sync-subcommands-routes, credential-export, cli-tokens, cli-i18n): 209/209 passcheck:complexity-ratchets --base-ref <merge-base>: OK (complexity −2, cognitive −1)check:file-size: OKbin/is eslint-ignored); Prettier cleancheck:docs-all,check:openapi-{breaking,coverage,routes,security-tiers},check:cli-i18n,check:route-guard-membership,check:route-validation:t06: all passdocs/openapi.yamlandAPI_REFERENCE.mddocumentdryRun/filenameand the 400/401 responsesNotes / out of scope
sync bundlewrites JSON (/api/sync/bundle), butsync importrestores a.sqlitebackup, so the two do not round-trip.importnow says this clearly instead of uploading the JSON.bundleitself was reworked in fix(cli): map sync push/bundle/initialize onto the real routes; drop sync diff/resolve #16721.