From 14439fb4387a0e9143c93bb954c2a164d269931f Mon Sep 17 00:00:00 2001 From: audristroyer Date: Fri, 12 Jun 2026 15:53:22 -0400 Subject: [PATCH 1/6] security(sqlitedb): escape interpolated ids in double-quoted SQL literals (audit 6.1-3) Several sqlitedb queries interpolate branch_id / doc_id / document_id / filename directly into SQL string literals delimited by double quotes, e.g. ['... WHERE doc_id="' document_id '"']. run_sql_query() does not forward bind parameters to mksqlite (it calls do_run_sql_query without varargin), so these values cannot be passed as '?' placeholders; a value containing a double quote could break out of the literal and inject SQL (e.g. a crafted branch name or document id). Add a private escapeSqlLiteral helper that doubles embedded double quotes (inside a double-quoted token SQLite reads "" as an escaped "), mirroring the DID-python _sql_escape (which doubles single quotes for its single-quoted literals), and apply it at all 15 value-interpolation sites (do_add_branch / do_delete_branch / do_get_branch_parent / do_get_sub_branches / do_get_doc_ids / read / remove_doc / open_doc / file path lookups). The existing parameterized run_sql_noOpen('... =?', val) call sites are unchanged. Lockstep with the DID-python field-name validation (same audit item 6.1-3). Authored without a local MATLAB runtime; needs MATLAB to validate/run. --- src/did/+did/+implementations/sqlitedb.m | 47 +++++++++++++++++------- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/src/did/+did/+implementations/sqlitedb.m b/src/did/+did/+implementations/sqlitedb.m index 01f0e5b..cbd0929 100644 --- a/src/did/+did/+implementations/sqlitedb.m +++ b/src/did/+did/+implementations/sqlitedb.m @@ -113,6 +113,25 @@ function delete(this_obj) % disposed when this method returns, using the onCleanup mechanism end % do_run_sql_query() + function s = escapeSqlLiteral(~, s) + % escapeSqlLiteral - escape a value for a double-quoted SQL literal + % + % S = escapeSqlLiteral(THIS_OBJ, S) + % + % Several queries below interpolate identifiers (branch_id, doc_id, + % filename) into SQL string literals delimited by double quotes, + % e.g. ['... WHERE doc_id="' document_id '"']. run_sql_query() does + % not forward bind parameters to mksqlite (it calls + % do_run_sql_query without varargin), so those values cannot be + % passed as '?' placeholders and must be escaped to prevent SQL + % injection from a crafted id/branch name. Inside a double-quoted + % token SQLite treats "" as an escaped ", so doubling the double + % quotes neutralizes any attempt to break out of the literal. This + % mirrors the DID-python _sql_escape (which doubles single quotes + % for its single-quoted literals). + s = strrep(char(s), '"', '""'); + end % escapeSqlLiteral() + function branch_ids = do_get_branch_ids(this_obj) % do_get_branch_ids - return all unique branch ids in the database % @@ -153,7 +172,7 @@ function do_add_branch(this_obj, branch_id, parent_branch_id, varargin) this_obj.insert_into_table('branches', 'branch_id,parent_id,timestamp', branch_id, parent_branch_id, tnow); % Duplicate the docs from parent branch to the newly-created branch - sqlStr = ['SELECT doc_idx FROM branch_docs WHERE branch_id="' parent_branch_id '"']; + sqlStr = ['SELECT doc_idx FROM branch_docs WHERE branch_id="' this_obj.escapeSqlLiteral(parent_branch_id) '"']; data = this_obj.run_sql_noOpen(sqlStr); if ~isempty(data) doc_idx = [data.doc_idx]; @@ -176,11 +195,11 @@ function do_delete_branch(this_obj, branch_id, varargin) if ~isempty(doc_ids) % Remove all documents from the branch_docs table % TODO: also delete records of unreferenced docs ??? - this_obj.run_sql_query(['DELETE FROM branch_docs WHERE branch_id="' branch_id '"']); + this_obj.run_sql_query(['DELETE FROM branch_docs WHERE branch_id="' this_obj.escapeSqlLiteral(branch_id) '"']); end % Now delete the branch record - this_obj.run_sql_query(['DELETE FROM branches WHERE branch_id="' branch_id '"']); + this_obj.run_sql_query(['DELETE FROM branches WHERE branch_id="' this_obj.escapeSqlLiteral(branch_id) '"']); end % do_delete_branch() function parent_branch_id = do_get_branch_parent(this_obj, branch_id, varargin) @@ -190,7 +209,7 @@ function do_delete_branch(this_obj, branch_id, varargin) % % Returns the ID of the parent branch for the specified BRANCH_ID. - sqlStr = ['SELECT parent_id FROM branches WHERE branch_id="' branch_id '"']; + sqlStr = ['SELECT parent_id FROM branches WHERE branch_id="' this_obj.escapeSqlLiteral(branch_id) '"']; data = this_obj.run_sql_query(sqlStr); if isempty(data) parent_branch_id = ''; @@ -219,7 +238,7 @@ function do_delete_branch(this_obj, branch_id, varargin) % Returns a cell array of IDs of sub-branches of the specified BRANCH_ID. % If BRANCH_ID has no sub-branches, an empty cell array is returned. - sqlStr = ['SELECT branch_id FROM branches WHERE parent_id="' branch_id '"']; + sqlStr = ['SELECT branch_id FROM branches WHERE parent_id="' this_obj.escapeSqlLiteral(branch_id) '"']; data = this_obj.run_sql_query(sqlStr); if isempty(data) branch_ids = {}; @@ -241,7 +260,7 @@ function do_delete_branch(this_obj, branch_id, varargin) if nargin > 1 && ~isempty(branch_id) sqlStr = ['SELECT docs.doc_id FROM docs,branch_docs' ... ' WHERE docs.doc_idx = branch_docs.doc_idx' ... - ' AND branch_id="' branch_id '"']; + ' AND branch_id="' this_obj.escapeSqlLiteral(branch_id) '"']; else sqlStr = 'SELECT docs.doc_id FROM docs'; end @@ -432,7 +451,7 @@ function do_add_doc(this_obj, document_obj, branch_id, options) %document_obj = did.document(doc); % Run the SQL query in the database - query_str = ['SELECT json_code FROM docs WHERE doc_id="' document_id '"']; + query_str = ['SELECT json_code FROM docs WHERE doc_id="' this_obj.escapeSqlLiteral(document_id) '"']; data = this_obj.run_sql_query(query_str); % Process missing document results @@ -500,8 +519,8 @@ function do_remove_doc(this_obj, document_id, branch_id, options) % Handle case of missing document sqlStr = ['SELECT docs.doc_idx FROM docs,branch_docs ' ... ' WHERE docs.doc_idx = branch_docs.doc_idx ' ... - ' AND branch_id="' branch_id '"' ... - ' AND doc_id="' doc_id '"']; + ' AND branch_id="' this_obj.escapeSqlLiteral(branch_id) '"' ... + ' AND doc_id="' this_obj.escapeSqlLiteral(doc_id) '"']; %doc_id = [doc_id '/' branch_id]; data = this_obj.run_sql_noOpen(sqlStr); if isempty(data) @@ -521,7 +540,7 @@ function do_remove_doc(this_obj, document_id, branch_id, options) doc_idx = data(1).doc_idx; % Remove the document from the branch_docs table - this_obj.run_sql_noOpen(['DELETE FROM branch_docs WHERE branch_id="' branch_id '" AND doc_idx=?'], doc_idx); + this_obj.run_sql_noOpen(['DELETE FROM branch_docs WHERE branch_id="' this_obj.escapeSqlLiteral(branch_id) '" AND doc_idx=?'], doc_idx); % TODO - remove all document records if no branch references remain? %{ @@ -579,10 +598,10 @@ function do_remove_doc(this_obj, document_id, branch_id, options) % Get the cached filepath to the specified document query_str = ['SELECT cached_location,orig_location,uid,type ' ... ' FROM docs,files ' ... - ' WHERE docs.doc_id="' document_id '" ' ... + ' WHERE docs.doc_id="' this_obj.escapeSqlLiteral(document_id) '" ' ... ' AND files.doc_idx=docs.doc_idx']; if nargin > 2 && ~isempty(filename) - query_str = [query_str ' AND files.filename="' filename '"']; + query_str = [query_str ' AND files.filename="' this_obj.escapeSqlLiteral(filename) '"']; else error('DID:SQLITEDB:open','The requested filename must be specified in open_doc()'); %filename = ''; % used in catch block below @@ -713,10 +732,10 @@ function tryCustomFileHandler(customFileHandler, destPath, sourcePath, file_type % Get the cached filepath to the specified document query_str = ['SELECT cached_location,orig_location,uid,type ' ... ' FROM docs,files ' ... - ' WHERE docs.doc_id="' document_id '" ' ... + ' WHERE docs.doc_id="' this_obj.escapeSqlLiteral(document_id) '" ' ... ' AND files.doc_idx=docs.doc_idx']; if nargin > 2 && ~isempty(filename) - query_str = [query_str ' AND files.filename="' filename '"']; + query_str = [query_str ' AND files.filename="' this_obj.escapeSqlLiteral(filename) '"']; else error('DID:SQLITEDB:open','The requested filename must be specified in check_exist_doc()'); end From be7462c51e065dd000f66832be9937204cbd087d Mon Sep 17 00:00:00 2001 From: audristroyer Date: Fri, 12 Jun 2026 15:55:30 -0400 Subject: [PATCH 2/6] docs: add audit remediation results for the DID-matlab branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maps the §6.1 finding addressed here (SQL-literal escaping, 6.1-3) to its commit, notes that isa (6.1-1) and the sqlite indexes (6.1-4) needed no MATLAB change (MATLAB is the correct reference / already has them; Python caught up), and records the timestamp-format DECISION (§6.1-2/§7.3-13: datenum vs epoch seconds in the same column) as a cross-client choice deliberately NOT changed here, pending joint sign-off. Reiterates author-not-run + the lockstep merge with audit/did-python-2026-06. --- docs/Audit_Remediation_Results_2026-06-12.md | 46 ++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 docs/Audit_Remediation_Results_2026-06-12.md diff --git a/docs/Audit_Remediation_Results_2026-06-12.md b/docs/Audit_Remediation_Results_2026-06-12.md new file mode 100644 index 0000000..2441c24 --- /dev/null +++ b/docs/Audit_Remediation_Results_2026-06-12.md @@ -0,0 +1,46 @@ +# DID-matlab Audit Remediation — Results (2026-06-12) + +Branch `audit/did-matlab-2026-06`, off `origin/main` (`03b0f7f`). This is the +MATLAB half of the **DID lockstep**; the Python half is +`audit/did-python-2026-06`. + +> **⚠️ Author-not-run.** Authored without a local MATLAB runtime. Please run the +> DID-matlab + symmetry test suites before merging. + +## Findings addressed (audit §6.1) + +| # | Severity | Commit | Summary | +|---|----------|--------|---------| +| 6.1-3 | Medium | `14439fb` | **SQL injection via interpolated ids.** Several `sqlitedb` queries interpolate `branch_id` / `doc_id` / `document_id` / `filename` directly into double-quoted SQL string literals, e.g. `['... WHERE doc_id="' document_id '"']`. `run_sql_query()` does not forward bind parameters to mksqlite (it calls `do_run_sql_query` without `varargin`), so those values cannot be passed as `?` placeholders; a value containing a double quote could break out of the literal and inject SQL. Added a private `escapeSqlLiteral` helper that doubles embedded double quotes (inside a double-quoted token SQLite reads `""` as an escaped `"`), mirroring DID-python's `_sql_escape`, and applied it at all 15 value-interpolation sites. The already-parameterized `run_sql_noOpen('... =?', val)` calls are unchanged. | + +### isa (6.1-1) — no MATLAB change needed + +MATLAB's `isa` (via `doc2sql` `meta.class`/`meta.superclass` + the SQL translation) is +the **correct reference**; the divergence was on the Python side, which has been brought +to parity (`audit/did-python-2026-06`). No DID-matlab isa change is required; the two are +merged together so a symmetry run stays consistent. + +### sqlite indexes (6.1-4) — already present in MATLAB + +The audit item is "Python omits MATLAB's sqlite indexes." MATLAB already creates them; +the Python side added the equivalents. No MATLAB change. + +## DECISION required — do not merge a serialization change without sign-off + +**§6.1-2 / §7.3-13: the `timestamp` column meaning diverges across languages and is NOT +changed here.** DID-matlab writes MATLAB `now` (datenum — days since year 0) into the +`docs`/`branches`/`branch_docs` `timestamp REAL` column (`sqlitedb.m` `do_add_branch`, +add-doc, add-to-branch); DID-python writes `time.time()` (Unix epoch seconds) into the +same column. Cross-client `lessthan`/`greaterthan` comparisons on `timestamp` silently +break, and the cloud backend stores the value verbatim. + +This is a **cross-client format decision**, not a one-sided patch — changing the +serialization on either side alone would break the other and any already-stored data. It +needs an explicit decision (proposed: **ISO-8601 TEXT**, or Unix epoch seconds, with a +documented one-time conversion for existing rows) made jointly for DID-python, +DID-matlab, and the cloud backend. Left untouched pending that decision. + +## Lockstep / merge + +Merge with `audit/did-python-2026-06` (the Python field-name validation is the same audit +item 6.1-3). VH-Lab fork-and-PR: a DID-matlab fork is created at review time. From 02c54b5464220995a61ed8698126bfd73baaa0a9 Mon Sep 17 00:00:00 2001 From: audristroyer Date: Fri, 12 Jun 2026 18:43:21 -0400 Subject: [PATCH 3/6] docs: correct escapeSqlLiteral site count (14, not 15) Adversarial review counted 14 escapeSqlLiteral call sites in sqlitedb.m, not 15 (the earlier count of 15 included a docstring-example occurrence that was later reverted). All vulnerable double-quoted interpolations are still covered. --- docs/Audit_Remediation_Results_2026-06-12.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/Audit_Remediation_Results_2026-06-12.md b/docs/Audit_Remediation_Results_2026-06-12.md index 2441c24..035b6fd 100644 --- a/docs/Audit_Remediation_Results_2026-06-12.md +++ b/docs/Audit_Remediation_Results_2026-06-12.md @@ -11,7 +11,7 @@ MATLAB half of the **DID lockstep**; the Python half is | # | Severity | Commit | Summary | |---|----------|--------|---------| -| 6.1-3 | Medium | `14439fb` | **SQL injection via interpolated ids.** Several `sqlitedb` queries interpolate `branch_id` / `doc_id` / `document_id` / `filename` directly into double-quoted SQL string literals, e.g. `['... WHERE doc_id="' document_id '"']`. `run_sql_query()` does not forward bind parameters to mksqlite (it calls `do_run_sql_query` without `varargin`), so those values cannot be passed as `?` placeholders; a value containing a double quote could break out of the literal and inject SQL. Added a private `escapeSqlLiteral` helper that doubles embedded double quotes (inside a double-quoted token SQLite reads `""` as an escaped `"`), mirroring DID-python's `_sql_escape`, and applied it at all 15 value-interpolation sites. The already-parameterized `run_sql_noOpen('... =?', val)` calls are unchanged. | +| 6.1-3 | Medium | `14439fb` | **SQL injection via interpolated ids.** Several `sqlitedb` queries interpolate `branch_id` / `doc_id` / `document_id` / `filename` directly into double-quoted SQL string literals, e.g. `['... WHERE doc_id="' document_id '"']`. `run_sql_query()` does not forward bind parameters to mksqlite (it calls `do_run_sql_query` without `varargin`), so those values cannot be passed as `?` placeholders; a value containing a double quote could break out of the literal and inject SQL. Added a private `escapeSqlLiteral` helper that doubles embedded double quotes (inside a double-quoted token SQLite reads `""` as an escaped `"`), mirroring DID-python's `_sql_escape`, and applied it at all 14 value-interpolation sites. The already-parameterized `run_sql_noOpen('... =?', val)` calls are unchanged. | ### isa (6.1-1) — no MATLAB change needed From 80c2a81e2586d95fb315b00f6d3aa8f83eededfc Mon Sep 17 00:00:00 2001 From: audristroyer Date: Fri, 12 Jun 2026 19:49:56 -0400 Subject: [PATCH 4/6] docs: add reviewer/next-agent context banner (part of the 9-PR 2026-06 ecosystem audit; lockstep + deferrals) --- docs/Audit_Remediation_Results_2026-06-12.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/Audit_Remediation_Results_2026-06-12.md b/docs/Audit_Remediation_Results_2026-06-12.md index 035b6fd..d84642a 100644 --- a/docs/Audit_Remediation_Results_2026-06-12.md +++ b/docs/Audit_Remediation_Results_2026-06-12.md @@ -1,5 +1,11 @@ # DID-matlab Audit Remediation — Results (2026-06-12) +> **Context for a reviewer / next agent.** One of **9 coordinated PRs** in the 2026-06 NDI +> ecosystem audit; **none are merged.** This repo's PR: **VH-Lab/DID-matlab#146** — **merge +> together with the DID-python PR VH-Lab/DID-python#23** (same audit item §6.1). +> **Author-not-run** — run the DID-matlab + symmetry suites before merge. The one deferral +> is the **`timestamp` format DECISION** (see "DECISION required" below). + Branch `audit/did-matlab-2026-06`, off `origin/main` (`03b0f7f`). This is the MATLAB half of the **DID lockstep**; the Python half is `audit/did-python-2026-06`. From 8c5c67e3acbb10241852205a8a2b51a5c6862f70 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 14 Jun 2026 21:19:23 +0000 Subject: [PATCH 5/6] ci: move codecheckToolbox into +didtools to stop shadowing matbox The bare-named tools/tasks/codecheckToolbox.m sits on the genpath'd tools/ directory and shadowed matbox.tasks.codecheckToolbox, so the check-code@v1 action's codecheckToolbox("FoldersToCheck","src") call resolved to the local zero-arg wrapper and failed with "Too many input arguments". Packaging it as didtools.codecheckToolbox removes the shadow while keeping the developer convenience. Mirrors the fix already made on the V2 branch. --- tools/+didtools/codecheckToolbox.m | 16 ++++++++++++++++ tools/tasks/codecheckToolbox.m | 4 ---- 2 files changed, 16 insertions(+), 4 deletions(-) create mode 100644 tools/+didtools/codecheckToolbox.m delete mode 100644 tools/tasks/codecheckToolbox.m diff --git a/tools/+didtools/codecheckToolbox.m b/tools/+didtools/codecheckToolbox.m new file mode 100644 index 0000000..6cc4163 --- /dev/null +++ b/tools/+didtools/codecheckToolbox.m @@ -0,0 +1,16 @@ +function codecheckToolbox() + % didtools.codecheckToolbox Developer convenience wrapper for + % matbox.tasks.codecheckToolbox: run the project code check + % against the DID-matlab repo root with badge writing disabled. + % + % Call as `didtools.codecheckToolbox` from the MATLAB prompt. + % + % This used to live at tools/tasks/codecheckToolbox.m (bare name) + % but check-code@v1 puts tools/ on the path and resolved to this + % zero-arg wrapper instead of matbox.tasks.codecheckToolbox's + % multi-arg version, breaking CI with "Too many input arguments". + % Moving it into +didtools/ keeps the convenience without + % shadowing matbox. + projectRootDirectory = didtools.projectdir(); + matbox.tasks.codecheckToolbox(projectRootDirectory, "CreateBadge", false) +end diff --git a/tools/tasks/codecheckToolbox.m b/tools/tasks/codecheckToolbox.m deleted file mode 100644 index 07d140f..0000000 --- a/tools/tasks/codecheckToolbox.m +++ /dev/null @@ -1,4 +0,0 @@ -function codecheckToolbox() - projectRootDirectory = didtools.projectdir(); - matbox.tasks.codecheckToolbox(projectRootDirectory, "CreateBadge", false) -end From 2e7a968e68394fd5934545529aa6bebce76fbe39 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 14 Jun 2026 21:31:16 +0000 Subject: [PATCH 6/6] ci: make codecheckToolbox accept args and keep badges disabled Reverts the package-move approach (which let CI fall through to matbox with CreateBadge=true and fail in createBadgeSvg: 'Python commands require a supported version of CPython'). Instead keep the bare-named wrapper that the check-code@v1 action resolves to, but accept and forward the new name-value args (FoldersToCheck/src) while forcing CreateBadge=false. Fixes both the original 'Too many input arguments' error and the CPython badge error without changing the workflow. --- tools/+didtools/codecheckToolbox.m | 16 ---------------- tools/tasks/codecheckToolbox.m | 21 +++++++++++++++++++++ 2 files changed, 21 insertions(+), 16 deletions(-) delete mode 100644 tools/+didtools/codecheckToolbox.m create mode 100644 tools/tasks/codecheckToolbox.m diff --git a/tools/+didtools/codecheckToolbox.m b/tools/+didtools/codecheckToolbox.m deleted file mode 100644 index 6cc4163..0000000 --- a/tools/+didtools/codecheckToolbox.m +++ /dev/null @@ -1,16 +0,0 @@ -function codecheckToolbox() - % didtools.codecheckToolbox Developer convenience wrapper for - % matbox.tasks.codecheckToolbox: run the project code check - % against the DID-matlab repo root with badge writing disabled. - % - % Call as `didtools.codecheckToolbox` from the MATLAB prompt. - % - % This used to live at tools/tasks/codecheckToolbox.m (bare name) - % but check-code@v1 puts tools/ on the path and resolved to this - % zero-arg wrapper instead of matbox.tasks.codecheckToolbox's - % multi-arg version, breaking CI with "Too many input arguments". - % Moving it into +didtools/ keeps the convenience without - % shadowing matbox. - projectRootDirectory = didtools.projectdir(); - matbox.tasks.codecheckToolbox(projectRootDirectory, "CreateBadge", false) -end diff --git a/tools/tasks/codecheckToolbox.m b/tools/tasks/codecheckToolbox.m new file mode 100644 index 0000000..a2dfbe2 --- /dev/null +++ b/tools/tasks/codecheckToolbox.m @@ -0,0 +1,21 @@ +function codecheckToolbox(varargin) + % codecheckToolbox Project code-check wrapper invoked by CI + % (ehennestad/matbox-actions check-code@v1). + % + % The check-code action adds tools/ to the path and calls the bare + % name, e.g. codecheckToolbox("FoldersToCheck","src"). This wrapper + % must therefore: + % (a) live at the bare name so the action's + % exist("codecheckToolbox","file") branch resolves to it + % (rather than falling through to matbox with CreateBadge=true), + % (b) accept and forward those name-value args, and + % (c) force CreateBadge=false. Badge generation goes through + % matbox.utility.createBadgeSvg, which needs a CPython install + % the CI runner does not have ("Python commands require a + % supported version of CPython"). + % + % Earlier this was a zero-arg wrapper, which broke once the action + % began passing "FoldersToCheck"/"src" ("Too many input arguments"). + projectRootDirectory = didtools.projectdir(); + matbox.tasks.codecheckToolbox(projectRootDirectory, "CreateBadge", false, varargin{:}); +end