MemorySmith External Audit — Final Council Review

Council Composition: 5 Specialist Chairs + 2 Anonymous Peer Reviewers + Chairman Process: 2-round independent review with per-chair peer validation
Date: 2026-06-17 Source audit: Data/Pages/audits/external-deep-research-audit-20260617.md Verdict Authority: Two complete review rounds; all Round 1 open questions closed by Round 2 forensic verification

Executive Summary

The external audit identified directionally important problems (missing antiforgery, OAuth bootstrap weakness, missing HSTS, hardcoded paths) but surrounded those real findings with factual errors, mislabelled severities, references to code that does not exist, and at least one outright fabricated CVE chain. Round 2 forensic verification confirmed two CRITICAL issues (leaked production secrets in version control; unauthenticated first-user-becomes-Admin OAuth bootstrap), four HIGH issues (universal antiforgery gap including a CSRF-to-shell vector; anonymous Editor escalation via OpenLocalEditor + null-loopback; live MemoryIndex concurrency race; OAuth ClientSecret rotation hygiene), and several MEDIUM/LOW items.

The audit itself is graded C-minus: useful as a checklist trigger, unsafe as a primary source. Roughly one-third of its specific technical claims are false, but the genuine findings it surfaced are severe enough that the document warranted council review.

Part I: Verdicts on Original Audit Claims

Tally: 5 CONFIRMED, 7 REFUTED, 1 PARTIAL, 2 CONFIRMED-as-positive.

Part II: Final Finding Register

All findings — from the original audit (where confirmed) plus council discoveries — ordered by severity.

CRITICAL

C-1. Production credentials committed to public repository

First found by: Chairs A and B independently. Locations: - artifacts/MemorySmith.App/appsettings.LocalOverrides.json — committed 80002eb4 on 2026-05-30; public 18+ days. - .vscode/mcp.json — committed 2840bd23 on 2026-05-27; second independent exposure.

Exposed secrets: - MemorySmith:ApiKey = DLxQJGounPrxit_ITQlcHBBbXLAbaryl-oduvmQMphw — the shared secret gating /api/* and /mcp when AllowRemoteApi=true. Grants Viewer/Editor access, NOT Admin. - GitHub OAuth ClientId = Ov23liSOQ3m2pthdQv0w — real post-2023 Ov23li prefix format. - GitHub OAuth ClientSecret = 9d0b9498af3f872b7cb1289b175498da2ffccdb8 — 40-hex format consistent with real GitHub OAuth secrets.

Aggravating factors: .gitignore lists both source paths but files were force-added despite ignore rules. No rotation visible in git history. The audit missed this entirely.

C-2. First OAuth user automatically becomes Admin (unauthenticated bootstrap)

First found by: Chair A. Location: GitHubOAuthCallbackHandler.cs:115 — first GitHub-authenticated user is elevated to Admin with zero gating (no IP allowlist, no bootstrap token, no email allowlist). Reachability: OAuth challenge endpoints (/api/auth/challenge?scheme=GitHub) are NOT loopback-gated. Any HTTPS caller can initiate the OAuth flow. Composability with C-1: The leaked OAuth ClientSecret in C-1 amplifies this. An attacker who can intercept the OAuth callback against a server that has never been bootstrapped can plausibly claim Admin.

HIGH

H-1. Universal CSRF gap including CSRF-to-shell vector

First found by: Chair A (Round 1 observation), confirmed exhaustively by Chair A Round 2. Scope: Zero antiforgery enforcement across all 11 controllers and 27+ POST/PUT/DELETE endpoints including: - SourceLinksController.Open — can be CSRF'd to invoke the OS default-app handler (CSRF-to-shell) - AdminController.AssignRole / RemoveRole — CSRF privilege assignment - MemoriesController, PagesController, TasksController — full data mutation - MaintenanceAgentController.RunNow / RunWeekly — trigger background jobs Mitigating factor for remote: AllowRemoteApi=false default blocks remote callers at transport layer. Still HIGH because malicious local pages and localhost callbacks can cross-origin-POST.

H-2. Anonymous Editor escalation via OpenLocalEditor + null-loopback

First found by: Chair A. Composition: OpenLocalEditorCompatibility=true (default in appsettings.json and MemorySmithOptions.cs) + IsLoopback(null) returns true in MemorySmithRequestGuardMiddleware.cs. When an upstream proxy strips RemoteIpAddress to null (mTLS, unix-socket, misconfigured reverse proxy), anonymous callers receive Editor-level authority — sufficient to write memories, write pages, trigger maintenance, use chat with agent writes. Important bound: RequiresAuthenticatedAdmin early-out is real and prevents this from escalating to Admin. This is an Editor-escalation only.

H-3. Live concurrency race in MemoryIndex

First found by: Chair C. Confirmed by Chair A Round 2. Location: MemorySmith.Core/Indexing/MemoryIndex.cs — three plain Dictionary<,> fields, no locks, no ConcurrentDictionary. Registered as singleton (AddSingleton<MemoryIndex>()). Concurrent write paths verified: - MemoryApplicationService.cs:549 — Add from HTTP POST /memories - MemoryApplicationService.cs:588-589 — Remove + Add from HTTP PUT /memories/{id} - MemoryApplicationService.cs:624 — Remove from HTTP DELETE - MemoryApplicationService.cs:677-678 — Remove + Add from touch/UsageCount (very frequent) - MemoryMaintenanceTasks.RunIndexRebuildAsync() — Clear() then full re-Add from snapshot, called periodically by MemoryMaintenanceService : BackgroundService

Worst pattern: Rebuild() does Clear() then refills from a pre-captured snapshot. Any web-request Add() between Clear() and the refill is silently lost. Current impact bounded by: Search currently uses _store.LoadAll(), not the index. Impact escalates to CRITICAL the moment search is promoted to consume the index.

H-4. OAuth ClientSecret rotation hygiene

First found by: Council (composes with C-1). Issue: A leaked OAuth ClientSecret participates in GitHub's token exchange flow. Rotation requires both a new secret in GitHub and deployment update. Not yet rotated per git history.

MEDIUM

M-1. Missing HSTS

No app.UseHsts() and no Strict-Transport-Security header emitted. Leaves SSL-strip and downgrade attacks in scope for any HTTPS deployment. Trivial one-line fix.

M-2. Server: Kestrel banner exposed

Information disclosure of server technology. Set AddServerHeader = false in Kestrel options.

M-3. FixedTimeEquals length-leak before constant-time compare

Locations: MemorySmithRequestGuardMiddleware.cs:96-104 AND SecurityServices.cs:344-356 (both copies). Pattern: actualBytes.Length == expectedBytes.Length && CryptographicOperations.FixedTimeEquals(...) — C# && short-circuits, leaking whether lengths match via response timing. Practical impact: LOW. The configured ApiKey is 32+ bytes of entropy; length-disclosure does not reduce keyspace meaningfully. Why M not L: The code reads as "we used FixedTimeEquals therefore we're constant-time" when it isn't. Every subsequent reviewer will reflag it. Fix is trivial: pad both sides to a common length before comparing.

M-4. Hardcoded CWD-relative path fallbacks

Path.Combine("..", "Data", ...) used as fallback throughout hosting setup (MemorySmithStorageSetup.cs, MemorySmithSecuritySetup.cs, MemorySmithContentEndpoints.cs, TrainingOptions.cs, MaintenanceAgentServices.cs). Configurable overrides exist but fallbacks depend on working directory at process launch — known to have caused Windows Service runtime issues in prior deployments (per in-repo audit history).

M-5. Bootstrap Serilog has no retainedFileCountLimit

Program.cs:23-26 early-startup logger writes without retention policy. Small but real disk-fill window during repeated crash-before-host-build cycles.

M-6. No coverage threshold gate

CI publishes Cobertura + HTML per push but sets no minimum. Coverage can regress without breaking a build. No coverage badge in README.

M-7. No packages.lock.json

Dependency graph is not locked. SourceGear.sqlite3 floor of >= 3.50.4.5 currently resolves above all known CVE fix points, but a future NuGet snapshot could pull a regressed transitive version.

M-8. ONNX model provenance has no integrity verification

Models pulled from HuggingFace via PowerShell+Python install script. No hash check, no lockfile, no signature verification. Models are gitignored — no audit trail of which version is in use. Acceptable for single-user local tool; unacceptable for any shared/production deployment.

LOW / IMPROVEMENT

L-1. TSK-0280 (shared WebApplicationFactory) unresolved

7+ distinct factory setups duplicated across integration tests. Maintenance and test-isolation cost. Pre-existing tracked task.

L-2. Bridge CLI reaches /mcp without credentials under default config

ApiKey=null default + AnonymousAccess=Viewer means any local process can call read-only MCP tools without authenticating. Intentional design for local-first; flag for remote-enabled deployments.

L-3. PowerShell CI data-validation scripts were not mentioned in the audit

Test-TaskRecords.ps1, Test-MemoryRecords.ps1, Test-PageLinks.ps1, Test-PagePathLiterals.ps1 all run as hard-stopping gates before the build step (all use $ErrorActionPreference = 'Stop'). These represent a test layer the audit missed when claiming coverage was "unknown."

Part III: Audit Quality Assessment

Overall Grade: C-minus

Rationale. The audit successfully triggered review of two genuinely critical issues (C-1: leaked credentials, C-2: OAuth bootstrap) and a cluster of valid medium items (HSTS, server header, hardcoded paths, CSP verification). Those finds alone justify the document's existence.

However, the audit also: - Fabricated a CVE chain (SQLite vulnerabilities that do not apply to the bundled version, with the recommendation to switch to the less safe package). - Invented technical details (WebSocket bridge, Lucene full-text search, ONNX model location) that do not exist in the codebase. - Cited wrong NuGet versions (4+ major versions stale for a .NET 10 project). - Misread the antiforgery state — claimed "partial protection exists" when there is in fact no protection (an error that understated the real severity). - Dismissed coverage as "unknown" without looking at the CI workflow that publishes coverage on every push.

Pattern diagnosis: A checklist-driven review with limited code-reading discipline. The auditor correctly identified the classes of risk to look for but verified against templates rather than against actual source files. The Bridge LOC fabrication (720 lines reported as ~3K), the wrong ONNX path, and the wrong NuGet versions together suggest portions of the report were generated from project descriptions or templates rather than from direct file inspection.

Reuse policy: Treat findings from this audit as directional hints requiring independent verification. Do not carry specific version numbers, file paths, or CVE references forward without re-checking them. The directional HSTS / server-header / secrets-hygiene signals are accurate and worth keeping.

Part IV: Recommended Actions

P0 — Must do before next deployment (within 24 hours)

1. Rotate all credentials from C-1. Generate a new MemorySmith:ApiKey. Generate a new GitHub OAuth ClientSecret. Update all deployments. Treat both as compromised.
2. Purge secrets from git history. Use git filter-repo or BFG against artifacts/MemorySmith.App/appsettings.LocalOverrides.json and .vscode/mcp.json. Force-push. Add those paths to .gitignore (already listed; fix the force-add workflow).
3. Gate OAuth first-user-is-Admin bootstrap (C-2). Replace the implicit promotion at GitHubOAuthCallbackHandler.cs:115 with: (a) bootstrap token required, or (b) email/username allowlist, or (c) loopback-only first-login. Any gating mechanism closes this.
4. Add global antiforgery enforcement (H-1). Apply [AutoValidateAntiforgeryToken] globally in Program.cs or per-controller. Highest urgency on SourceLinksController.Open (OS shell vector) and AdminController (role assignment).

P1 — This sprint

1. Fix MemoryIndex thread safety (H-3). Swap internal Dictionary<,> for ConcurrentDictionary<,> and rewrite Rebuild() to swap atomically (build a new dictionary, then swap the reference rather than Clear()-then-refill).
2. Close the null-loopback escalation (H-2). Either fail-closed on RemoteIpAddress=null (treat null as non-loopback), or require explicit opt-in configuration.
3. Add HSTS and disable the Server banner (M-1, M-2). One-line fixes each.
4. Fix FixedTimeEquals in both copies (M-3). Pad to common length before calling CryptographicOperations.FixedTimeEquals.

P2 — This month

1. Adopt packages.lock.json (M-7). Lock the dependency graph; re-verify SQLite version under the lock.
2. Add coverage threshold gate to CI (M-6). Pick a current-baseline floor; add badge to README.
3. Add hash verification to ONNX install script (M-8). SHA-256 check on downloaded model files; fail install on mismatch.
4. Resolve TSK-0280 (L-1). Consolidate integration test factory.
5. Write a SECURITY.md. Document the local-first threat model, the AnonymousAccess defaults, OAuth bootstrap procedure post-fix, and the rotation procedure for API key and OAuth credentials.

P3 — Backlog

1. Bootstrap Serilog retention (M-5). Add retainedFileCountLimit to the early-startup logger.
2. Replace hardcoded CWD path fallbacks (M-4). Use AppContext.BaseDirectory or an explicit MemorySmith:DataPath configuration requirement at startup.
3. Add regression tests for each closed P0/P1 finding. One test per resolved vulnerability demonstrating the fix.

Part V: Round 2 Open Questions — Closure Record

Part VI: Council Dissent Record

Dissent A — FixedTimeEquals severity (M-3): Chair A argued LOW (32-byte key, length disclosure does not reduce keyspace on real hardware); Peer Reviewer B argued MEDIUM (the code reads as constant-time when it isn't — future reviewers will repeatedly flag it). Resolution: MEDIUM with LOW practical-impact note preserved.

Dissent B — Audit grade (C vs. D): Peer Reviewer A argued D (fabricated CVE chain, invented Bridge architecture, wrong NuGet versions). Peer Reviewer B argued C (did correctly surface three genuinely critical issues). Resolution: C-minus, with explicit instruction that the audit is directional-hint-quality, not primary-source-quality.

Dissent C — MemoryIndex race severity (HIGH vs. CRITICAL): Chair A (Concurrency view) argued CRITICAL (data-loss pattern is live). Chair C (Storage view) argued HIGH (search doesn't yet consume the index; impact bounded today). Resolution: HIGH with explicit note that severity escalates to CRITICAL the moment search is promoted to consult the index. P1 priority unchanged.

Dissent D — OAuth Chain 2 severity (HIGH vs. CRITICAL): Chair A argued CRITICAL (zero-gating first-Admin path + leaked ClientSecret = internet-reachable Admin takeover on un-bootstrapped server). Peer Reviewer A argued HIGH (conditional — requires un-bootstrapped target). Resolution: CRITICAL (C-2) — composability with C-1 (leaked secret) was the deciding factor. The combination of no bootstrap gating + leaked OAuth credentials exceeds the "conditional" threshold for CRITICAL.

Meta-dissent (Peer Review): Both anonymous peer reviewers independently observed that the council itself in Round 1 accepted the audit's "no antiforgery anywhere" claim before Round 2 verified the exhaustive controller sweep. The lesson — apply the same skepticism to council output that the council applied to the audit — is recorded for future reviews.

Report produced by 5-chair council with 2 anonymous peer reviewers and Chairman synthesis across 2 rounds. Commit this document to Data/Pages/council/external-audit-council-review-20260617.md.