Skip to content

fix(engine): deep-copy and freeze the default command-authorization policy - #10137

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
shin-core:fix/command-auth-default-array-aliasing-9998
Jul 31, 2026
Merged

fix(engine): deep-copy and freeze the default command-authorization policy#10137
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
shin-core:fix/command-auth-default-array-aliasing-9998

Conversation

@shin-core

Copy link
Copy Markdown
Contributor

What & why

normalizeCommandAuthorizationPolicy's two exit paths disagreed about who owns the returned object.

The non-record path deep-copies via clonePolicy (copies default and every command's role array). The record path did not:

const commands: Record<string, CommandAuthorizationRole[]> = { ...DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands };

That shallow spread makes every value the same array instance held by the module-level DEFAULT_COMMAND_AUTHORIZATION_POLICY (a plain exported const, not frozen). Only commands the caller explicitly overrode get a fresh array; every un-overridden command keeps the shared reference. So this held today:

normalizeCommandAuthorizationPolicy({}).policy.commands.review    === DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands.review  // true (aliased)
normalizeCommandAuthorizationPolicy(null).policy.commands.review  === DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands.review  // false (deep-copied)

The function is reached with a record on the live settings-write path and on every command-authorization read, so the aliased arrays are handed to real callers. DEFAULT_COMMAND_AUTHORIZATION_POLICY is the security vocabulary for the whole command surface — a single push through a returned policy would widen that command for every repo handled by the same isolate, permanently, with no config change and no audit trail. No current caller mutates them, which is why this is a latent correctness defect rather than a live incident.

The fix (both halves required)

  • Deep-copy in the normalizer. The record path seeds commands from clonePolicy(DEFAULT_COMMAND_AUTHORIZATION_POLICY).commands — the same deep copy the non-record exit already returns — so every returned role array is freshly allocated on every input, including un-overridden commands.
  • Freeze the default at runtime. DEFAULT_COMMAND_AUTHORIZATION_POLICY, its default array, its commands record, and every role array inside it are Object.freezed, so a future aliasing regression fails loudly (a strict-mode TypeError on the offending push) instead of silently corrupting the vocabulary.

Unchanged: the resolved role lists for every input ({} still produces a commands deep-equal to the default and a deep-equal default), every warning string, the /^[a-z][a-z-]{0,63}$/ key validation, the maintainer-only clamp, and every evaluateCommandAuthorization decision.

Tests

  • Engine (packages/loopover-engine/test/command-authorization.test.ts, new): {} returns role arrays notStrictEqual to the default's yet deepEqual; the two exit paths agree (null and an override both return non-aliased arrays for an un-overridden command like pause); mutating a returned array doesn't change the frozen default; and the preserved behaviour (commandAuthorizationAllowedRoles(null, "generate-tests") is ["maintainer"], a COLLABORATOR is unauthorized for generate-tests).
  • Root (test/unit/command-authorization-engine.test.ts, the vitest mirror that imports the engine src for codecov): the same non-aliasing + freeze assertions across {}, null, and an override.
  • All new assertions fail on main and pass with the fix.

Validation

  • Diff coverage on packages/loopover-engine/src/settings/command-authorization.ts is 100% line and branch (the freeze loop and the deep-copy seed). Engine lines credited via the root-vitest upload; the added test is also in packages/loopover-engine/test/** for the dual-upload union.
  • npm run typecheck clean for these files; npm run engine-parity:drift-check passes; the engine's own node --test suite (928 tests) is green; npm run dead-exports:check clean; both existing command-authorization suites still pass.
  • git diff --check clean; no schema/migration/generated-artifact change.

Closes #9998

…olicy

normalizeCommandAuthorizationPolicy's two exit paths disagreed about ownership of
the returned object. The non-record path deep-copies via clonePolicy; the record
path seeded commands with a SHALLOW spread of DEFAULT_COMMAND_AUTHORIZATION_POLICY,
so every un-overridden command's role array was the SAME instance the module-level
default holds. Only explicitly-overridden commands got a fresh array. A caller that
pushed a role through a returned policy would widen that command for every repo in
the same isolate, permanently, with no config change and no audit trail -- the
default is the security vocabulary for the whole command surface.

Seed the record path from clonePolicy too, so every returned role array is freshly
allocated on every input, and freeze DEFAULT_COMMAND_AUTHORIZATION_POLICY (object,
default array, commands record, and every role array) so a future aliasing
regression fails loudly instead of silently corrupting the vocabulary. Values,
warnings, key validation, the maintainer-only clamp, and every
evaluateCommandAuthorization decision are unchanged.

Closes JSONbored#9998
@shin-core
shin-core requested a review from JSONbored as a code owner July 31, 2026 08:50
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-31 09:18:19 UTC

3 files · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This fixes a real aliasing bug: the record path in normalizeCommandAuthorizationPolicy previously shallow-spread DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands, so un-overridden command role arrays were the same instance as the module-level default, and mutating a returned array would have silently corrupted the shared default. The fix correctly reuses clonePolicy for a deep copy on the record path (matching the non-record exit) and additionally freezes the default object at every level as defense-in-depth. The change is well-scoped, matches its stated intent, and is backed by both new unit tests (Node test runner + vitest) that verify non-aliasing and frozen-state behavior.

Nits — 5 non-blocking
  • nit: the freeze loop at command-authorization.ts:47-52 runs at module load time; if this module is ever imported before all commands are populated (unlikely given the current single object literal) the freeze would be incomplete — worth a comment noting the ordering dependency is intentional.
  • nit: normalizeCommandRoleList (unchanged) still reads DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands[commandName] directly for fallback roles and spreads it into a new array — this is fine since the source is now frozen but read-only, but worth double-checking no other call site anywhere in the codebase does a raw `Object.assign` or mutation against DEFAULT_COMMAND_AUTHORIZATION_POLICY that would now throw in strict mode where it silently succeeded before.
  • nit: the PR title claims this is a security fix without a linked exploited-in-production incident — the description itself says 'no current caller mutates them,' so this is correctly framed as latent/defensive rather than a live vulnerability, which is good, but the freeze half of the fix is arguably nice-to-have hardening rather than the core fix.
  • Consider whether clonePolicy(DEFAULT_COMMAND_AUTHORIZATION_POLICY).commands recomputation on every call to normalizeCommandAuthorizationPolicy(record) is worth the minor allocation overhead versus caching a single frozen deep clone — negligible at this call volume, so not blocking.
  • The four freeze calls at file scope could be consolidated into a small deepFreeze helper for readability, though the current explicit form is arguably clearer for future maintainers.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9998
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 78 registered-repo PR(s), 60 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor shin-core; Gittensor profile; 78 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The record path now seeds commands from clonePolicy(DEFAULT_COMMAND_AUTHORIZATION_POLICY).commands instead of a shallow spread, and DEFAULT_COMMAND_AUTHORIZATION_POLICY plus its default array, commands record, and every role array are frozen at module load, matching the issue's required pattern exactly. Tests assert non-aliasing across inputs, deep-equality preservation, frozen defaults, and uncha

Review context
  • Author: shin-core
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: TypeScript, JavaScript, Solidity, Dart, Python, CSS, PHP, Rust
  • Official Gittensor activity: 78 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 31, 2026
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.05%. Comparing base (1d2b142) to head (49588c2).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10137      +/-   ##
==========================================
+ Coverage   91.99%   92.05%   +0.05%     
==========================================
  Files         931      931              
  Lines      113994   114008      +14     
  Branches    27523    27534      +11     
==========================================
+ Hits       104871   104945      +74     
+ Misses       7823     7759      -64     
- Partials     1300     1304       +4     
Flag Coverage Δ
backend 95.67% <100.00%> (-0.01%) ⬇️
engine 73.27% <100.00%> (+0.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...pover-engine/src/settings/command-authorization.ts 86.97% <100.00%> (+24.37%) ⬆️

... and 1 file with indirect coverage changes

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 8fe0f4d into JSONbored:main Jul 31, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

engine(settings): normalizeCommandAuthorizationPolicy hands out the module-level DEFAULT command role arrays by reference

1 participant