Skip to content

Commit 361df8d

Browse files
Merge pull request #24 from VH-Lab/claude/charming-pasteur-8r1m9v
Audit remediation: isa parity + SQL field-name hardening + indexes + LICENSE (lockstep with DID-matlab #148)
2 parents 1b1491f + 1dd9904 commit 361df8d

6 files changed

Lines changed: 309 additions & 28 deletions

File tree

‎.github/workflows/symmetry.yml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,13 @@ jobs:
1111
symmetry:
1212
name: MATLAB <-> Python symmetry tests
1313
runs-on: ubuntu-latest
14+
# matbox.installRequirements() (and the MATLAB test steps) call the GitHub
15+
# API to resolve dependency commit ids. Without a token those calls are
16+
# unauthenticated (60 req/hr per runner IP) and intermittently fail with
17+
# "GitHub API rate limit exceeded". Expose the auto-provided token so the
18+
# calls are authenticated (5000 req/hr).
19+
env:
20+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
1421
steps:
1522
- name: Check out DID-python
1623
uses: actions/checkout@v4

‎LICENSE‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Data Data Interface(https://github.com/VH-Lab/DID-python) is licensed under [CC BY-NC-SA 4.0](http://creativecommons.org/licenses/by-nc-sa/4.0/?ref=chooser-v1).
2+
3+
Contact [Brandeis University Office of Technology Licensing](https://www.brandeis.edu/innovation/what-we-do/office-of-technology-licensing.html) for commercial licensing.
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
# DID-python Audit Remediation — Results (2026-06-12)
2+
3+
> **Context for a reviewer / next agent.** One of **9 coordinated PRs** in the 2026-06 NDI
4+
> ecosystem audit; **none are merged.** This repo's PR: **VH-Lab/DID-python#23** — **merge
5+
> together with the DID-matlab PR VH-Lab/DID-matlab#146** (same audit item §6.1; isa +
6+
> SQL hardening land in lockstep). The one deferral is the **`timestamp` format DECISION**
7+
> (see "DECISION required" below).
8+
9+
Branch `audit/did-python-2026-06`, off `origin/main` (`1b1491f`). This is the
10+
Python half of the **DID lockstep** (DID-python + DID-matlab change together so a
11+
cross-language symmetry run stays consistent); the MATLAB half is
12+
`audit/did-matlab-2026-06`.
13+
14+
## Findings addressed (audit §6.1)
15+
16+
| # | Severity | Commit | Summary |
17+
|---|----------|--------|---------|
18+
| 6.1-1 | Critical | `76ae1bb` | **isa operator parity.** The brute-force `field_search` isa used a `param1 in a` heuristic plus an exact `class_name` check, so it (a) missed superclass membership — a probe document did not match `isa('element')` — and (b) spuriously matched a class name that was merely an incidental top-level field of an unrelated document. Same query, different result set per language. The brute-force isa now derives the class and superclass names from the **same** `did.implementations.doc2sql` helpers the SQL path uses, so both paths agree and both follow MATLAB's `isa(X)` = (X is the class) OR (X is a superclass) semantics. |
19+
| 6.1-3 | Medium | `76ae1bb` | **SQL field-name hardening.** The query field name was interpolated into the SQL text (`fields.field_name = '<field>'`, and into `LIKE` patterns, so it cannot be a bound parameter). It is now restricted to `^[A-Za-z0-9_.]+$`; an out-of-charset field name returns `None` and the caller falls back to the injection-free brute-force search. |
20+
| 6.1-4 | Low | `4ae15de` | **Missing sqlite indexes.** `doc_data` (one row per field per document, the largest table) had no index, so every search did a full-scan nested-loop join. Adds `doc_data(value)` (the index the DID-MATLAB reference already has), plus the more targeted `doc_data(field_idx, value)` and `doc_data(doc_idx)`. Created `IF NOT EXISTS` on every open, so databases downloaded before this fix benefit on next use. |
21+
| 6.1-8 | Medium | (this commit) | **No LICENSE.** Added `LICENSE` (CC BY-NC-SA 4.0) matching the DID-matlab counterpart. |
22+
23+
### Correction to the audit's 6.1-1 wording
24+
25+
The audit said the SQL isa path matched `meta.class`/`meta.superclass` — "fields that
26+
never exist in stored docs." In fact `doc2sql.py` **does** produce `meta.class` and
27+
`meta.superclass` (mirroring MATLAB `doc2sql.m`), so the SQL isa path was already
28+
correct; the real divergence was the brute-force path, fixed above. The "never exist"
29+
observation applies only to a **MATLAB-written `ndi.db`** where the `doc_data` search
30+
cache is left unpopulated (0 rows) — a separate issue (the search cache is a derived
31+
index; authoritative content is `docs.json_code`), not an isa field-name bug.
32+
33+
## DECISION required — do not merge a serialization change without sign-off
34+
35+
**§6.1-2 / §7.3-13: the `timestamp` column meaning diverges across languages and is
36+
NOT changed here.** DID-matlab writes MATLAB `now` (datenum — days since year 0) into
37+
the `docs`/`branches`/`branch_docs` `timestamp REAL` column; DID-python writes
38+
`time.time()` (Unix epoch seconds) into the same column. A document written by one
39+
client and compared with `lessthan`/`greaterthan` on `timestamp` by the other silently
40+
gives wrong results, and the cloud backend stores the value verbatim (it has no opinion).
41+
42+
This is a **cross-client format decision**, not a bug to patch on one side — changing
43+
the serialization on either side alone would break the other and any already-stored
44+
data. It needs an explicit decision (proposed: **ISO-8601 TEXT**, or Unix epoch seconds,
45+
with a documented one-time conversion/migration for existing rows) made jointly for
46+
DID-python, DID-matlab, and the cloud backend. Left untouched pending that decision.
47+
48+
## Validation
49+
50+
`PYTHONPATH=src python -m pytest tests/ --ignore=tests/symmetry` = **62 passed** (was
51+
55; +7 from the new `tests/test_isa_parity.py`), black + ruff clean. The new tests cover
52+
isa across own-class / superclass-descendant / root-superclass / unrelated, an
53+
incidental-field trap, SQL-vs-brute-force agreement, and a field-name injection attempt
54+
that does not inject.
55+
56+
## Lockstep / merge
57+
58+
Merge with `audit/did-matlab-2026-06` (the MATLAB SQL-injection escaping is the same
59+
audit item 6.1-3). The isa change brings Python to parity with MATLAB (MATLAB's isa was
60+
already the correct reference; no MATLAB isa change was needed).

‎src/did/datastructures.py‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -277,11 +277,23 @@ def field_search(a, search_struct):
277277
b = True
278278
break
279279
elif op_lower == "isa":
280-
# param1 = class name
281-
if param1 in a:
282-
b = True
283-
elif "document_class" in a and a["document_class"].get("class_name") == param1:
284-
b = True
280+
# isa(param1): true if param1 is the document's class OR one of its
281+
# superclasses. Mirror did.implementations.doc2sql -- the same
282+
# derivation the SQL isa path matches via meta.class / meta.superclass --
283+
# so the brute-force and SQL paths agree and both follow MATLAB's
284+
# semantics. The class is document_class.class_name (or the DID-python
285+
# 'classname'); the superclasses are the bare names of
286+
# document_class.superclasses[].definition (path + extension stripped).
287+
# The previous "param1 in a" heuristic missed superclass membership
288+
# (e.g. a probe document did not match isa('element')).
289+
from .implementations.doc2sql import _get_class_name, _get_superclass_str
290+
291+
class_name = _get_class_name(a)
292+
superclass_str = _get_superclass_str(a)
293+
superclasses = (
294+
[s for s in superclass_str.split(", ") if s] if superclass_str else []
295+
)
296+
b = (param1 == class_name) or (param1 in superclasses)
285297
else:
286298
raise ValueError(f"Unknown search operation: {operation}")
287299

‎src/did/implementations/sqlitedb.py‎

Lines changed: 66 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,38 @@ def _open_db(self):
3939

4040
if is_new:
4141
self._create_db_tables()
42+
# Always ensure the search-critical indexes exist — including on
43+
# databases created before this fix (idempotent).
44+
self._ensure_indexes()
45+
46+
def _ensure_indexes(self):
47+
"""Create the indexes the document search depends on, if missing.
48+
49+
The search is a 4-table join over docs/branch_docs/doc_data/fields
50+
that filters on ``doc_data.field_idx`` + ``doc_data.value`` and joins
51+
on ``doc_data.doc_idx``. ``doc_data`` (one row per field per doc — by
52+
far the largest table) had NO index, so every search did a full-scan
53+
nested-loop join — pathologically slow on large datasets (a single
54+
``getprobes`` took ~70 s on a 115 MB cloud dataset).
55+
56+
This restores the indexing the DID-MATLAB reference already has
57+
(sqlitedb.m creates ``doc_data(value)``) — the Python port dropped
58+
it — and adds ``doc_data(field_idx, value)`` (more targeted for the
59+
actual field+value search) and ``doc_data(doc_idx)`` (the doc join).
60+
Run on every open (``IF NOT EXISTS``) so databases downloaded before
61+
this fix benefit on next use. (docs.doc_id and fields.field_name are
62+
already covered by UNIQUE constraints.)
63+
"""
64+
cursor = self.dbid.cursor()
65+
cursor.execute('CREATE INDEX IF NOT EXISTS "doc_data_value" ON doc_data(value)')
66+
cursor.execute(
67+
'CREATE INDEX IF NOT EXISTS "doc_data_field_value" '
68+
"ON doc_data(field_idx, value)"
69+
)
70+
cursor.execute(
71+
'CREATE INDEX IF NOT EXISTS "doc_data_doc_idx" ON doc_data(doc_idx)'
72+
)
73+
self.dbid.commit()
4274

4375
def _close_db(self):
4476
if self.dbid:
@@ -425,6 +457,15 @@ def _query_struct_to_sql_str(self, search_struct):
425457
op = op[1:]
426458
op_lower = op.lower()
427459

460+
# The query field name is interpolated into the SQL text below (e.g.
461+
# fields.field_name = '<field>'); it cannot be a bound '?' parameter
462+
# because it also appears in LIKE patterns. Restrict it to the
463+
# dotted-identifier charset so a crafted field name cannot break out of
464+
# the quoting. An out-of-charset field returns None here and the caller
465+
# falls back to the (injection-free) brute-force search.
466+
if field and not _re.fullmatch(r"[A-Za-z0-9_.]+", field):
467+
return None
468+
428469
if op_lower == "exact_string":
429470
return f"fields.field_name = '{field}' AND doc_data.value = '{_sql_escape(param1)}'"
430471

@@ -549,30 +590,32 @@ def get_docs(self, document_ids, branch_id=None, OnMissing="error", **kwargs):
549590
if not document_ids:
550591
return [] if not is_single else None
551592

552-
# Filter by branch if requested
553-
if branch_id is not None:
554-
branch_doc_ids = set(self.get_doc_ids(branch_id))
555-
requested = []
556-
for doc_id in document_ids:
557-
if doc_id in branch_doc_ids:
558-
requested.append(doc_id)
559-
elif OnMissing == "error":
560-
raise ValueError(
561-
f"Document {doc_id} not found in branch {branch_id}"
562-
)
563-
elif OnMissing == "warn":
564-
print(f"Warning: Document {doc_id} not found in branch {branch_id}")
565-
document_ids = requested
566-
567-
if not document_ids:
568-
return [] if not is_single else None
569-
570-
# Single SELECT ... WHERE doc_id IN (?, ?, ...)
593+
# Fetch the requested docs in ONE indexed query, restricting to the
594+
# branch via a JOIN.
595+
#
596+
# PERF: the previous code fetched ALL of the branch's doc_ids
597+
# (``set(self.get_doc_ids(branch_id))``, O(total_docs)) and filtered
598+
# the small ``document_ids`` list in Python. NDI's ``epochtable``
599+
# calls ``get_docs`` once per epoch, so that was O(epochs x
600+
# total_docs) — a single ``getprobes`` took minutes on a large cloud
601+
# dataset (the live NDI spike-sort hung here). The branch JOIN +
602+
# ``doc_id IN (...)`` is O(len(document_ids)) using the branch_docs
603+
# and docs indexes. Branch membership is enforced by the JOIN; docs
604+
# not in the branch simply aren't returned and are handled by the
605+
# OnMissing pass below (same behaviour as before).
571606
placeholders = ",".join("?" for _ in document_ids)
572-
rows = self.do_run_sql_query(
573-
f"SELECT doc_id, json_code FROM docs WHERE doc_id IN ({placeholders})",
574-
tuple(document_ids),
575-
)
607+
if branch_id is not None:
608+
rows = self.do_run_sql_query(
609+
f"SELECT d.doc_id, d.json_code FROM docs d "
610+
f"JOIN branch_docs bd ON d.doc_idx = bd.doc_idx "
611+
f"WHERE bd.branch_id = ? AND d.doc_id IN ({placeholders})",
612+
(branch_id, *document_ids),
613+
)
614+
else:
615+
rows = self.do_run_sql_query(
616+
f"SELECT doc_id, json_code FROM docs WHERE doc_id IN ({placeholders})",
617+
tuple(document_ids),
618+
)
576619

577620
# Build lookup dict
578621
doc_map = {}

‎tests/test_isa_parity.py‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
"""isa-operator parity: the brute-force (field_search) and SQL search paths must
2+
agree, and both must follow MATLAB's isa semantics -- a document is isa(X) iff X
3+
is its class OR one of its superclasses (matched by bare name, as produced by
4+
did.implementations.doc2sql's meta.class / meta.superclass).
5+
6+
Regression for the DID-python isa divergence (audit 6.1-1): the old brute-force
7+
field_search isa used a ``param1 in a`` heuristic plus an exact class_name
8+
check, so it (a) missed superclass membership -- a probe did not match
9+
isa('element') -- and (b) spuriously matched a class name that happened to be an
10+
incidental top-level field of an unrelated document. Same query, different
11+
result set per language. The SQL path already matched MATLAB (meta.class /
12+
meta.superclass); this test pins both paths to the same, correct answer.
13+
"""
14+
15+
import os
16+
import unittest
17+
18+
from did.document import Document
19+
from did.implementations.sqlitedb import SQLiteDB
20+
from did.query import Query
21+
from did.datastructures import field_search
22+
23+
24+
def _doc(class_name, superclasses, fields=None):
25+
props = {
26+
"document_class": {
27+
"definition": f"$NDIDOCUMENTPATH/{class_name}.json",
28+
"class_name": class_name,
29+
"class_version": 1,
30+
"property_list_name": class_name,
31+
"superclasses": [
32+
{"definition": f"$NDIDOCUMENTPATH/{s}.json"} for s in superclasses
33+
],
34+
},
35+
"base": {
36+
"id": f"id_{class_name}",
37+
"name": class_name,
38+
"datestamp": "2026-06-12T00:00:00",
39+
},
40+
}
41+
if fields:
42+
props.update(fields)
43+
return Document(props)
44+
45+
46+
class TestIsaParity(unittest.TestCase):
47+
DB = "test_isa_parity.sqlite"
48+
49+
@classmethod
50+
def setUpClass(cls):
51+
if os.path.exists(cls.DB):
52+
os.remove(cls.DB)
53+
cls.db = SQLiteDB(cls.DB)
54+
cls.db.add_branch("a")
55+
# probe isa {probe, element, base}; element isa {element, base};
56+
# session isa {session, base}. 'thing' is class thing with superclass
57+
# base only, but carries an incidental top-level 'element' field -- it
58+
# must NOT match isa('element') (the old heuristic wrongly would).
59+
specs = [
60+
("probe", ["element", "base"], {"probe": {"name": "p"}}),
61+
("element", ["base"], {"element": {"name": "e"}}),
62+
("session", ["base"], {"session": {"name": "s"}}),
63+
("thing", ["base"], {"element": {"note": "incidental, not a superclass"}}),
64+
]
65+
cls.by_class = {}
66+
cls.docs = []
67+
for class_name, superclasses, fields in specs:
68+
d = _doc(class_name, superclasses, fields)
69+
cls.by_class[class_name] = d
70+
cls.docs.append(d)
71+
cls.db._do_add_doc(d, "a")
72+
73+
@classmethod
74+
def tearDownClass(cls):
75+
cls.db._close_db()
76+
if os.path.exists(cls.DB):
77+
os.remove(cls.DB)
78+
79+
def _brute(self, class_name):
80+
ss = Query("", "isa", class_name).to_search_structure()
81+
return sorted(
82+
d.id() for d in self.docs if field_search(d.document_properties, ss)
83+
)
84+
85+
def _sql(self, class_name):
86+
return sorted(self.db.search(Query("", "isa", class_name), branch_id="a"))
87+
88+
def _expect(self, *class_names):
89+
return sorted(self.by_class[c].id() for c in class_names)
90+
91+
def test_isa_own_class(self):
92+
self.assertEqual(self._sql("probe"), self._expect("probe"))
93+
self.assertEqual(self._brute("probe"), self._expect("probe"))
94+
95+
def test_isa_superclass_matches_descendant(self):
96+
# isa(element) -> the probe (element is its superclass) AND element.
97+
# Must NOT include 'thing' despite its incidental 'element' field.
98+
self.assertEqual(self._sql("element"), self._expect("probe", "element"))
99+
self.assertEqual(self._brute("element"), self._expect("probe", "element"))
100+
101+
def test_isa_root_superclass_matches_all(self):
102+
self.assertEqual(
103+
self._sql("base"), self._expect("probe", "element", "session", "thing")
104+
)
105+
self.assertEqual(
106+
self._brute("base"), self._expect("probe", "element", "session", "thing")
107+
)
108+
109+
def test_isa_unrelated_matches_nothing(self):
110+
self.assertEqual(self._sql("nonexistent"), [])
111+
self.assertEqual(self._brute("nonexistent"), [])
112+
113+
def test_sql_and_brute_force_agree(self):
114+
for c in ["probe", "element", "base", "session", "thing", "nonexistent"]:
115+
self.assertEqual(
116+
self._sql(c), self._brute(c), f"isa({c}) SQL vs brute-force mismatch"
117+
)
118+
119+
120+
class TestSqlFieldNameValidation(unittest.TestCase):
121+
"""A query field name with SQL metacharacters must not be interpolated into
122+
SQL; the leaf falls back to the injection-free brute-force path (returns the
123+
correct empty set here rather than raising or injecting)."""
124+
125+
DB = "test_isa_fieldname.sqlite"
126+
127+
@classmethod
128+
def setUpClass(cls):
129+
if os.path.exists(cls.DB):
130+
os.remove(cls.DB)
131+
cls.db = SQLiteDB(cls.DB)
132+
cls.db.add_branch("a")
133+
cls.db._do_add_doc(_doc("probe", ["base"], {"probe": {"name": "p"}}), "a")
134+
135+
@classmethod
136+
def tearDownClass(cls):
137+
cls.db._close_db()
138+
if os.path.exists(cls.DB):
139+
os.remove(cls.DB)
140+
141+
def test_malicious_field_name_does_not_inject(self):
142+
# Classic injection attempt as the field name.
143+
evil = "base.id' OR '1'='1"
144+
# Must not raise, must not return everything via injection.
145+
result = self.db.search(Query(evil, "exact_string", "anything"), branch_id="a")
146+
self.assertEqual(result, [])
147+
148+
def test_legitimate_dotted_field_still_works(self):
149+
result = self.db.search(
150+
Query("base.name", "exact_string", "probe"), branch_id="a"
151+
)
152+
self.assertEqual(result, ["id_probe"])
153+
154+
155+
if __name__ == "__main__":
156+
unittest.main()

0 commit comments

Comments
 (0)