Audit remediation (cloud/security/perf/symmetry) + review refinements#828
Merged
Conversation
…t nvpairs()
ndi.cloud.sync.enum.SyncMode/execute forwarded SyncOptions to the selected
sync function with syncOptions.nvpairs() — a method that does not exist on
SyncOptions (the accessor is toCell()) — and without {:} expansion. Every
dataset sync routed through ndi.cloud.syncDataset therefore errored at
dispatch before doing any work.
Fix: call toCell() (the documented name-value accessor) and expand the
returned 1-by-2N cell with {:} so the sync function receives name-value
arguments, matching the arguments block 'opts.?ndi.cloud.sync.SyncOptions'
in downloadNew/uploadNew/mirror*/twoWaySync.
The existing DownloadNewTest/UploadNewTest/... call the sync functions
directly and require a live cloud dataset, so they never covered the
SyncMode.execute forwarding contract. Adds an offline SyncModeDispatchTest
that exercises exactly it (toCell shape, {:} expansion into a name-value
arguments block, round-trip reconstruction, and that every SyncMode maps to
a real sync function).
Authored without a local MATLAB runtime; needs MATLAB to validate/run.
…istent /finalize
ndi.cloud.api.url mapped 'finalize_compute_session' to
POST /compute/{sessionId}/finalize, but the backend exposes no /finalize
route — the session-finalization action is POST /compute/{sessionId}/advance
(compute.router.ts; the advance controller stamps finalizedAt when advancing
past the last stage). FinalizeSession.m therefore 404'd on every call.
Fix: point the mapping at /advance. The user-facing finalizeSession wrapper
and the 'finalize_compute_session' key are unchanged (the implementation
class passes the key through ndi.cloud.api.url, so only the path template
changes). Matches the NDI-python cloud client (fix/cloud-api-contract).
Authored without a local MATLAB runtime; needs MATLAB to validate/run.
…uirements succeeds
The cross-language symmetry workflow (Test Cross-Language Symmetry, driven from
NDI-python) runs:
symDir = fullfile(pwd, 'tests', '+ndi', '+symmetry');
matbox.installRequirements(symDir);
matbox.installRequirements reads requirements.txt from that exact directory,
but the folder had none, so Stage 1 (MATLAB makeArtifacts) died before any
symmetry test ran — which is why the whole workflow has been red and the four
existing namespaces (util/file/dataset/session) have never actually executed.
Adds the missing requirements.txt at the symmetry dir, mirroring
tests/requirements.txt (the symmetry makeArtifacts build real sessions,
datasets, and navigators, so they need the full NDI runtime dependency stack;
the workflow only addpaths NDI's own src, not its deps).
Caveat (no local MATLAB to validate): this unblocks Stage 1 installRequirements;
whether the four existing namespaces' makeArtifacts then pass is unverifiable
here, since the job has always died before reaching them. The first green MATLAB
run may surface parity work to do.
Authored without a local MATLAB runtime; needs MATLAB to validate/run.
convertoldnsd2ndi interpolated the caller's PATHNAME unquoted into ten find/sed shell commands run via system(). A path containing spaces broke the commands, and a crafted path (e.g. '/tmp/; rm -rf /') injected arbitrary shell commands. Harden: require PATHNAME to be an existing folder (mustBeFolder), reject embedded double quotes, and double-quote the interpolated path in every shell string. The mustBeFolder guard alone defeats metacharacter injection (an injected command string is not a real directory); quoting additionally fixes legitimate paths with spaces. The MATLAB-side movefile/genpath loop is unchanged (no shell involved there). Authored without a local MATLAB runtime; needs MATLAB to validate/run.
…hmod 600) The AES secrets backend wrote NDI_Cloud_Secrets.json (AES-encrypted passwords) and NDI_Cloud_Profiles.json (account emails/UIDs) to prefdir with default umask permissions — typically world-readable on POSIX systems. Other local users could read the ciphertext (and the machine-derived AES key is weak), so the password ciphertext was effectively exposed. Add a best-effort restrictToOwner helper that sets POSIX 600 (owner read/write only) via java.nio.file after each write, consistent with the file's existing Java-based crypto. No-op on Windows; failures are non-fatal (secrets remain AES-encrypted regardless). Matches the NDI-python 0600 secrets/profile hardening (PR fix/security-packaging-ci). Authored without a local MATLAB runtime; needs MATLAB to validate/run.
…access
Several code paths built a property-access string from a name and ran it
through eval(), so a crafted property name could execute arbitrary MATLAB:
* ndi.document constructor + setproperties: eval(['document_properties.'
name '=value']) for each caller-supplied name/value pair.
* ndi.element: eval() of the document-stored element.direct flag.
* ndi.validate (x2) + ndi.calculator: eval(strcat('...document_properties.',
NAME)) where NAME is a document-derived property/class name.
Replace each with code that cannot execute the name:
* New ndi.document.assignPropertyPath(s, path, value): validates every
dotted segment with isvarname and assigns via subsasgn.
* New ndi.fun.getfieldpath(s, path): same validation, reads via getfield.
* ndi.element parses element.direct as an explicit boolean (1/0/true/false
or numeric) instead of eval.
Behaviour is preserved for every legitimate dotted field-name path (the
documented name/value contract); only inputs that were never valid field
paths — i.e. injection attempts — now raise instead of executing. The
callers' existing try/catch wrappers keep their original error messages.
Authored without a local MATLAB runtime; needs MATLAB to validate/run.
ndi.document.readblankdefinition read and jsondecode'd a class definition from disk on every call, and recursed once per superclass — so creating a document re-parsed its whole definition hierarchy every time. On a dataset import this is thousands of redundant disk reads + JSON parses. The function is a pure function of the location string (the second 's' arg and 's_is_empty' are dead — 's' is overwritten by the decoded JSON), so cache the parsed result in a persistent containers.Map keyed on the location string. MATLAB structs are copy-on-write, so callers that mutate the returned definition cannot corrupt the cache. Pass '--clear-cache' to reset the memo (for tests that swap definition files on disk). Mirrors the NDI-python read_blank_definition memoization (PR6). Authored without a local MATLAB runtime; needs MATLAB to validate/run.
Supply-chain hardening for the workflows (several run with live NDI cloud credentials): every action is pinned to a full commit SHA with a # vN comment (so Dependabot can still bump them) instead of a movable major tag, which an upstream account compromise could repoint at malicious code. SHAs for the actions shared with the NDI-python workflows reuse those already in use there (checkout, setup-matlab, run-command, setup-python, matbox-actions @v1); the rest (upload-artifact, codecov, action-junit-report, publish-unit-test-result) were resolved to their current v-tag commits. Also removes the dead push trigger in test-cloud-api.yml on the add-cloud-api-testing branch (the branch no longer exists; the workflow keeps its daily schedule + manual dispatch). Authored without a local MATLAB/Actions runtime; needs CI to validate.
…, for_deletion/)
Deletes files with zero inbound references anywhere in src/, tests/, or
tools/:
* +cloud/authenticateOriginal.m (superseded by authenticate.m)
* +cloud/+api/+auth/loginOriginal.m, logoutOriginal.m (superseded by
login.m / logout.m)
* +cloud/+upload/for_deletion/ (update_cloud_metadata{,_struct}.m — the
directory is named for deletion)
Verified no symbol references each name outside the files themselves. Left
the +setup/+daq/+system/deprecating/ directory in place despite its name: its
lab-setup functions (vhlab, marderlab, etc.) are still referenced by setup,
test, and conversion code.
Authored without a local MATLAB runtime; needs MATLAB to validate/run.
…I build
Hardens the ndi-validator-java build definition (the JSON-schema validator
MATLAB loads via javaaddpath):
* everit-org/json-schema now resolves from Maven Central
(com.github.erosb:everit-json-schema:1.14.6) instead of jitpack, which
builds artifacts on demand and is not a stable, auditable registry. The
Java package stays org.everit.json.schema.*, so no source imports change.
* JUnit moved from the 5.7.0-M1 milestone to the 5.12.2 release, using the
junit-jupiter aggregator (+ pinned maven-surefire-plugin 3.5.6) so the
tests actually run under 'mvn package'.
* Pin transitive Jackson via the jackson-bom (2.18.4) and pin the
maven-compiler-plugin (3.13.0); drop the now-unused jitpack <repositories>.
* maven-shade-plugin (3.6.0) + <finalName>ndi-validator-java</finalName>
bundle the runtime deps into one fat JAR with the exact name the runtime
loads, reproducing the committed binary from source.
* New build-validator-jar.yml workflow builds + tests the JAR on changes and
uploads it as an artifact (SHA-pinned actions).
IMPORTANT — author-not-built (no Maven/JDK in this environment):
* The committed jar/ndi-validator-java.jar still embeds the OLD
everit-1.12.1-from-jitpack. This pom change does NOT harden the runtime
until the JAR is rebuilt (via the new workflow or 'mvn package') and the
fresh target/ndi-validator-java.jar replaces the committed copy. That
re-commit is intentionally left to someone with the toolchain.
* The everit 1.14.6 API compatibility with Validator.java and the exact
shade transformer set must be confirmed by a real build; the CI job is the
intended validation.
Needs Maven/JDK (CI) to validate/run.
…space)
Mirrors the NDI-python tests/symmetry/{make,read}_artifacts/time/test_time_convert
+ _time_scenario.py (item 28) so the syncgraph time_convert contract is checked
across languages, completing the symmetry coverage (util/file/dataset/session
already existed; time was the gap).
* ndi.symmetry.time.scenario - the shared SCENARIO (one referent, two
multi-clock epochs) + 7 CASES, all same-referent, plus runCases() which
drives the real ndi.time.syncgraph/time_convert and records out_time/
out_epoch/msg (errors recorded as data, like the Python run_cases).
* ndi.symmetry.time.scenarioReferent - a minimal ndi.epoch.epochset & ndi.ido
whose epochtable is built from the scenario, the MATLAB analogue of the
Python _SpecRef.
* makeArtifacts/time/timeConvert - writes timeConvertCases.json (same layout
+ null/NaN encoding as the Python artifact).
* readArtifacts/time/timeConvert - self-consistency re-run + MATLAB-vs-Python
comparison.
SAFETY: makeArtifacts writes the artifact ONLY if every case converts cleanly;
otherwise it marks itself Incomplete (assumption failure, not a test failure)
and writes nothing, so the Python read side skips the time comparison exactly
as when the artifact is absent. The four existing namespaces and the rest of
the suite are therefore unaffected by this addition until it is validated.
⚠️ AUTHORED WITHOUT A MATLAB RUNTIME — needs MATLAB to validate. The
unverified points are the synthetic referent integration (ndi.epoch.epochset &
ndi.ido compose; == compares by id; epochtable epoch_clock/t0_t1 cell format;
the referent's live ndi.session) and JSON null/NaN parity with the Python
artifact. Until a MATLAB run confirms the cases convert, this contributes a
skipped (Incomplete) time namespace, not a failure.
…type.ne typo
Addresses findings from an adversarial review of this branch:
* document.m readblankdefinition: move the DID-path-constants
initialization (updateDIDConstants) BEFORE the memo lookup so a cache hit
can never skip it. It worked before only because the first call is always
a cache miss; doing it unconditionally is the correct, idempotent guard.
* profile.m restrictToOwner: verify the POSIX permissions actually took
effect (getPosixFilePermissions == rw-------) and warn on failure instead
of swallowing every exception silently, so a failed chmod can't quietly
leave a secrets file world-readable. UnsupportedOperationException (no
POSIX view on the platform) stays a silent no-op.
* clocktype.m ne(): the second parameter was misnamed (ndi_cock_obj_b) while
the body used ndi_clocktype_obj_b, so the ~= operator errored on an
undefined variable for every call. Fix the name and compare type strings
with strcmp (the inverse of eq), matching eq()'s implementation. This is a
pre-existing bug surfaced by the review.
(The review's two 'critical' subsasgn claims against assignPropertyPath were
checked and rejected: struct('type',{'.','.'},'subs',{p1,p2}) does build a
1-by-N substruct array, and subsasgn auto-creates intermediate structs.)
Authored without a local MATLAB runtime; needs MATLAB to validate/run.
Companion review/audit-trail doc (mirrors the cloud-node manuals/reviews/ convention): maps each audit finding to its commit, records the adversarial review outcome, and lists the deliberately-deferred items (JWT-off-env lockstep, requirements SHA-pin, GenBank/LFS, bulk error-IDs) plus the rebuild-the-JAR caveat. Reiterates the author-not-run status.
…6 ecosystem audit; lockstep + deferrals)
ndi.validate (and the com.ndi/everit JAR it loads via checkJavaPath) is not on any runtime path: nothing in src/ or tests/ constructs ndi.validate, ndi.document.validate() is a stub that returns 1, and live document validation is handled by DID-matlab when documents are added to the database. The validator was legacy. Removes: - src/ndi/+ndi/validate.m - src/ndi/java/ndi-validator-java/ (jar, source-code/pom.xml, docs, tutorials) - .github/workflows/build-validator-jar.yml This supersedes the audit PR's M3 (pom jitpack -> Maven Central) and makes the deferred 'rebuild the validator JAR' blocker and the GenBank-TSV/LFS item moot: there is no validator to build or harden. Unrelated validate functions (ndi.cloud.sync.validate, ndi.document.validate stub) are kept.
The two M2 eval-replacement helpers are general-purpose, so place them in the ndi.util utility package rather than as a static method on ndi.document (assignPropertyPath) and in ndi.fun (getfieldpath). Updates the call sites in ndi.document (constructor + setproperties) and ndi.calculator. Error identifiers are namespaced ndi:util:* accordingly. Behaviour is unchanged.
The NDI cloud API (swagger) exposes POST /compute/{sessionId}/advance
("advance a compute session to the next stage") and has no /finalize
route. The client wrapper was named finalize but, after M0, pointed at
/advance -- a confusing mismatch. Rename to match the API contract:
- url endpoint: finalize_compute_session -> advance_compute_session
- ndi.cloud.api.implementation.compute.FinalizeSession -> AdvanceSession
- ndi.cloud.api.compute.finalizeSession -> advanceSession
Finalization is the backend side effect of advancing past the last stage;
there is no separate finalize operation. No in-repo callers; the old call
never worked (404) so there is no working behavior to preserve. NDI-python
client should rename to match for cross-language parity.
tests/requirements.txt and tests/+ndi/+symmetry/requirements.txt were byte-identical. tests/requirements.txt was read only by this repo's own test-symmetry.yml; the NDI-python-driven cross-language symmetry workflow reads tests/+ndi/+symmetry/requirements.txt. Point our test-symmetry.yml at the +symmetry copy (co-located with the symmetry tests) and delete the duplicate, so there is a single source of truth for symmetry dependencies. Top-level requirements.txt (the run-tests superset) is unchanged.
…ference) (a) ndi.time.syncgraph/time_convert: the same-referent, different-clocktype path read epoch_clock with '==' (j1/j2 = find(epoch_clock==clock)) while epoch_clock is canonically a CELL array (per epochset docs, the readers, and line 664 of this same function which uses cellfun). On a cell array '==' errors, so converting between two clocks of the same referent threw. Use the same cellfun(@(x) eq(x,...)) form as the global-clock path. This path was latent (cross-referent conversions go through the syncgraph), surfaced by the +time symmetry scenario. (b) time symmetry now asserts instead of skipping. scenario.expected() encodes the MATLAB-authoritative correct outputs; makeArtifacts verifies every case converts AND equals expected (was assumeTrue -> Incomplete, which silently masked the bug above) and always writes the artifact. Python must match these reference values.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return | ||
| end | ||
|
|
||
| s_is_empty = 0; |
|
|
||
| s_is_empty = 0; | ||
| if nargin<2 | ||
| s_is_empty = 1; |
| s_is_empty = 0; | ||
| if nargin<2 | ||
| s_is_empty = 1; | ||
| s = vlt.data.emptystruct; |
| 'Invalid property name "%s".', char(propertyPath)); | ||
| end | ||
| end | ||
| value = getfield(s, parts{:}); %#ok<GFLD> |
| matches = []; | ||
| for i=1:numel(docs) | ||
| try, input_param = eval(['docs{i}.document_properties.' property_list_name '.input_parameters;']); catch, input_param = []; end | ||
| try, input_param = ndi.util.getfieldpath(docs{i}.document_properties, [property_list_name '.input_parameters']); catch, input_param = []; end |
…stall failures run-tests, test-symmetry, and test-cloud-prod all failed during matbox dependency setup: matbox resolves each git dependency via an unauthenticated webread to api.github.com, which gets rate-limited/throttled when the jobs install in parallel (surfaced as 'connection ... timed out after 5.000 seconds' resolving vhlab-toolbox-matlab@master). Add a job-level GITHUB_TOKEN so the API calls are authenticated (5000 req/hr), matching the fix already applied on the DID-python symmetry workflow. CI-only; no runtime change.
Contributor
Test Results600 tests +3 598 ✅ +1 5m 20s ⏱️ +7s For more details on these failures, see this check. Results for commit 7a34241. ± Comparison against base commit 84418b9. ♻️ This comment has been updated with latest results. |
The de-dup deleted tests/requirements.txt, but it is also read by testToolboxNoCloud() -> matbox.installRequirements(fullfile(projectRootDir, 'tests')) in the run-tests and cloud workflows, which then failed with 'No requirements file was found'. Restore tests/requirements.txt and point test-symmetry.yml back at tests/. Full de-dup isn't safe from the MATLAB side: testToolboxNoCloud reads tests/ and the NDI-python cross-language workflow reads tests/+ndi/+symmetry/, so both files coexist as before. The GITHUB_TOKEN auth fix is unaffected.
ndi.time.syncgraph/time_convert compares referents with '==' (the same-referent fast path, line 677). scenarioReferent extends ndi.epoch.epochset & ndi.ido but is not an ndi.element, and none of those superclasses defines eq (ndi.element defines its own at element.m:508), so the synthetic referent had no '==' at all -> 'Operator == is not supported for ndi.symmetry.time.scenarioReferent', making every scenario conversion fall through to empty. Add an explicit id-based eq/ne mirroring ndi.element's identity semantics. This was the root cause of the empty time_convert results for scenario cases 4-7 (the epochtable itself was well-formed).
…enarioReferent
The +time symmetry scenario built a bespoke ndi.epoch.epochset subclass
(scenarioReferent) to stand in for a referent. That class re-implemented the
referent contract but is not an ndi.element and inherited no eq, so
ndi.time.syncgraph/time_convert's 'referent == referent_out' threw
('Operator == is not supported'), making every conversion fall through to
empty (scenario cases 4-7). Rather than patch a fake referent, build a real
ndi.element.timeseries and register the scenario epochs with multi-clock
addepoch (verified on MATLAB: epochtable + conversions correct, e.g. ep1
t=10 dev_local -> 110 exp_global, exp_global 105 -> 105). Deletes
scenarioReferent.m entirely. The shared scenario spec (scenarioStruct) and
the Python _SpecRef are unchanged, so cross-language artifacts still match.
makeArtifacts.time now passes (real-element referent), but
readArtifacts.time/testMatlabArtifactsReproduce failed comparing recorded
(JSON round-tripped) vs fresh results: an empty msg is '' (0x0 char) when
loaded from JSON but "" (empty string) in-memory, and txt() mapped those
differently (isempty('') true -> <null>, isempty("") false -> ''). Convert
to string first and treat missing/empty-string/empty all as one token, so
empty values compare equal regardless of representation. out_time/out_epoch
already matched; this was purely the msg normalization.
stevewds
approved these changes
Jun 15, 2026
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
5 participants
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.
Repo-side branch of the NDI-matlab audit work originally proposed in the fork PR #827 (
audriB/NDI-matlab:audit/ndi-matlab-2026-06), brought into aVH-Labbranch so it can run through CI and be edited directly. Part of the 2026-06 NDI ecosystem audit; the DID halves merged as VH-Lab/DID-matlab#148 and VH-Lab/DID-python#24.Audit fixes (from #827)
finalize_compute_sessionpointed at a nonexistent/compute/{id}/finalize(404). Renamed toadvanceto match the cloud API (swagger exposes onlyPOST /compute/{sessionId}/advance; there is nofinalize) — see refinements below.SyncMode.executecalled a nonexistentSyncOptions.nvpairs()→ nowtoCell(){:}; adds an offlineSyncModeDispatchTest(existing sync tests are live-cloud and call the sync functions directly, so they never covered the dispatch path).convertoldnsd2ndi(mustBeFolder+ quoting); chmod-600 (verify+warn) on the cloud secrets/profile files; removal ofeval()code-injection vectors indocument/element/calculator, replaced with safe helpers (relocated tondi.util, see below).ndi.document.readblankdefinition(persistent cache; DID-const init kept before the cache lookup;--clear-cacheescape hatch); SHA-pinned every GitHub Action across all workflows (with# vNcomments for Dependabot) and dropped a deadadd-cloud-api-testingtrigger.*Original.mand+cloud/+upload/for_deletion/(zero inbound refs; live versions retained). The+setup/+daq/+system/deprecating/dir is intentionally kept.requirements.txt(de-duped, see below) and the+timetime_convertsymmetry artifacts.ndi.time.clocktype/neparameter typo (~=errored on every call).Refinements made during review
pom.xml:ndi.validate+src/ndi/java/ndi-validator-java/+build-validator-jar.yml. It was off the runtime path (nothing constructsndi.validate; live validation is via DID-matlab), so there was nothing to harden or rebuild.finalize→advancerename: the wrapper now matches the API (advanceSession/AdvanceSession/advance_compute_session) rather than keeping a misleadingfinalizename pointed at/advance.ndi.util:assignPropertyPathandgetfieldpathmoved out ofndi.document(static)/ndi.funinto thendi.utilutility package; call sites updated. (Verified the identifier validation preserves normal nesteda.b.cassignment for all of NDI's call sites.)requirements.txtde-dup:tests/requirements.txtwas identical totests/+ndi/+symmetry/requirements.txt; consolidated onto the latter (where the NDI-python cross-language workflow reads) and repointed ourtest-symmetry.ymlthere.time_convertbug fix + symmetry assert: the same-referent / different-clock path read the cell-arrayepoch_clockwith==(would error) instead ofcellfun(@(x) eq(x,...))like the global-clock path; fixed. The+timesymmetry test now asserts MATLAB produces the expected reference outputs (scenario.expected()) instead of silently skipping on error — MATLAB leads, Python follows.Deferred (not changed; flagged in
manuals/reviews/Audit_Remediation_Results_2026-06-12.md)JWT-off-environment-variables (lockstep with NDI-python),
requirements.txtSHA-pinning (needs matbox verification), GenBank vocabulary dedup → git-LFS, bulkerror()-identifier rewrite.Testing notes
Author-not-run for the cloud pieces (M0/M1 and the rename need live cloud — only
test-cloud-prod/test-cloud-apiexercise them, notrun-tests). Thetime_convertfix and symmetry assertions are validated byrun-tests/test-symmetry. NDI-python symmetry follow-ups (computeadvancerename;time_convertMATLAB-as-reference) will be filed separately.https://claude.ai/code/session_01Nn5vG7DR3RqvBAGwd94JKt
Generated by Claude Code