Finish the audit follow-ups and restore unrestricted agent SQL - #206
Merged
Conversation
The audit changes were written but never compiled or run to completion. This brings them to a green state and applies the product decisions taken since. Completed: - Add the missing `using ModelContextProtocol` in DbAdminMcpTools — two McpException references failed to compile, which blocked the whole solution. - Name the protected surface in ExternalAgentSqlPolicy.RejectionMessage. 43 tests across Api and Mcp asserted that the refusal says what it protects; the constant was generic, so every one of them failed. Product decisions: - Drop the Admin gate on global variables in the SCOrch import. Operators are trusted automation authors who may already run script under the service identity, so the gate split Orchestrator migrations into two passes without removing a capability. The Operator test now guards the opposite contract. - Gate main on CI unconditionally (`push: [main]`). The duplicate run against a merge commit is accepted deliberately; the alternative left direct pushes and release cuts resting on a manual workflow_dispatch. CI hygiene: - Let VSTest name TRX files. Every test project wrote the same test_results.trx into one results directory, so the uploaded artifact held only the last one. - Apply the documented -assemblyfilters in the coverage report step, so the gated number stops counting NodePilot.TestCommons and NodePilot.LoadTests as production code. - Lower the lint ceiling from 13 to the measured 11. Verified: solution builds; Ai 504, Api 2438, Cli 496, Mcp 192, Engine 1831, Data 291 backend tests green; tsc -b clean; 174 frontend tests across the 9 touched spec files green. Remaining audit items (workflow-mutation service, DB-side custom-activity invariants, WinRM credential rotation, N+1 batching, real integration boundaries, UI modularisation) are recorded in docs/roadmap.md.
The audit introduced ExternalAgentSqlPolicy, which hid Workflows,
WorkflowVersions and both custom-activity tables from schema discovery and
rejected any SQL mentioning them — for the AI knowledge source and the MCP
server alike. That is reverted: the surface is a product decision, and the
owner wants the assistant to answer from SQL without exceptions.
The restriction bought little. Both surfaces are Admin-only, and a global
Admin reads the same rows through the forensic DbAdmin view anyway, so the
policy mainly made inventory questions ("which workflows exist") unanswerable
while the data stayed one click away. get_workflow_definition remains the more
convenient path when secret redaction is wanted.
Deliberately kept, because they are unrelated to workflow tables and close real
holes in the shared read guard for the genuinely secret columns:
- dynamic XML exporters (query_to_xml/table_to_xml/database_to_xml), which
take their target as a string and so defeat identifier-based checks;
- PostgreSQL U&"..." escaped identifiers, which spell a protected column name
without ever writing it.
SqlStatementInspector stays for the same reason — DbAdminReadOnlySqlGuard now
builds on it. SqlKnowledgeReader and DbAdminMcpTools are back at their previous
state; docs/ai-features.md and docs/mcp-server.md record why the tables are
deliberately not blocked.
Verified: solution builds; Api 2400 and Mcp 180 tests green (the delta to the
previous commit is exactly the removed policy tests).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+171
to
+176
| foreach (var node in nodes) | ||
| { | ||
| case JsonObject obj: | ||
| { | ||
| var result = new JsonObject(); | ||
| var isHeadersObject = string.Equals(parentName, "headers", StringComparison.OrdinalIgnoreCase); | ||
| foreach (var (name, value) in obj) | ||
| result[name] = value is null ? null : Walk(value, name, isHeadersObject, protector); | ||
| return result; | ||
| } | ||
| case JsonArray arr: | ||
| { | ||
| var result = new JsonArray(); | ||
| foreach (var item in arr) | ||
| result.Add(item is null ? null : Walk(item, parentName, isHttpHeaderValue, protector)); | ||
| return result; | ||
| } | ||
| case JsonValue val when val.TryGetValue(out string? s) && s is not null: | ||
| { | ||
| if (!NodePilot.Core.WorkflowDefinitions.WorkflowSecretKeys.IsSecretValue(parentName, s, isHttpHeaderValue)) | ||
| return JsonValue.Create(s); | ||
| if (node is not JsonObject nodeObject || nodeObject["data"] is not JsonObject data) continue; | ||
| RemapNodeReference(data, "targetMachineId", resolveMachine, unresolved); | ||
| RemapNodeReference(data, "credentialId", resolveCredential, unresolved); | ||
| } |
Comment on lines
+66
to
+69
| v.WorkflowId.CompareTo(workflowCursor) > 0 | ||
| || (v.WorkflowId == workflowCursor | ||
| && (v.Version > lastVersion | ||
| || (v.Version == lastVersion && v.Id.CompareTo(lastId) > 0)))); |
Comment on lines
+133
to
+140
| if (proactive) | ||
| { | ||
| if (WasRecentlyRotated(current, _timeProvider.GetUtcNow()) | ||
| || IsInTransientFailureCooldown(current.Token)) | ||
| { | ||
| return current; | ||
| } | ||
| } |
Comment on lines
+71
to
+73
| var tempPath = Path.Combine( | ||
| directory, | ||
| $".{Path.GetFileName(canonicalPath)}.{Environment.ProcessId}.{Guid.NewGuid():N}.tmp"); |
Comment on lines
+33
to
+37
| foreach (var token in Tokenize(sql)) | ||
| { | ||
| if ((includeQuoted || !token.Quoted) && identifiers.Contains(token.Value)) | ||
| return token.Value; | ||
| } |
Comment on lines
+124
to
+125
| var databasePath = Path.Combine( | ||
| Path.GetTempPath(), $"nodepilot-version-rotation-{Guid.NewGuid():N}.db"); |
| public sealed class ClientSessionFileCoordinatorTests : IDisposable | ||
| { | ||
| private readonly string _dir = Directory.CreateDirectory( | ||
| Path.Combine(Path.GetTempPath(), "np-session-lock-" + Guid.NewGuid().ToString("N"))).FullName; |
|
|
||
| public void Dispose() | ||
| { | ||
| try { Directory.Delete(_dir, recursive: true); } catch { /* best-effort cleanup */ } |
| [Fact] | ||
| public async Task EquivalentSessionPathAndServerOrigin_ShareCancellableLock() | ||
| { | ||
| var canonicalPath = Path.Combine(_dir, "session-default.dat"); |
| public async Task EquivalentSessionPathAndServerOrigin_ShareCancellableLock() | ||
| { | ||
| var canonicalPath = Path.Combine(_dir, "session-default.dat"); | ||
| var equivalentPath = Path.Combine(_dir, "nested", "..", "session-default.dat"); |
Sev7eNup
added a commit
that referenced
this pull request
Aug 16, 2026
README.md and CLAUDE.md both said CI has "deliberately no push trigger" and ran only on PRs. PR #206 added push: [main] to ci.yml but left both prose blocks unchanged, making them contradict the actual workflow config. - README.md: replace the "deliberately no push trigger" sentence with a statement that CI runs on every PR and on every push to main. - CLAUDE.md: update "zweifach" (twice) to "dreifach" (three times) and expand the ci.yml clause to mention push-to-main alongside PRs. Co-authored-by: Claude <noreply@anthropic.com>
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
2 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.
Completes the security-audit follow-up work that was written but never compiled
or run to a green state, then applies two product decisions taken while
reviewing it.
Brought to a working state
DbAdminMcpToolsdid not compile. TwoMcpExceptionreferences weremissing
using ModelContextProtocol, which broke the whole solution build.the SQL refusal names what is protected, while the message was generic.
Product decisions
audit gated this on Admin. An Operator is a trusted automation author who can
already run script under the service identity, so the gate split Orchestrator
migrations into two passes without removing a capability. The Operator test
now guards the opposite contract.
ExternalAgentSqlPolicyhid
Workflows,WorkflowVersionsand both custom-activity tables from schemadiscovery and rejected any SQL mentioning them. Both surfaces are Admin-only and
a global Admin reads the same rows through the forensic DbAdmin view anyway, so
the policy mainly made inventory questions ("which workflows exist")
unanswerable. Reverted for the AI knowledge source and the MCP server.
Two guards from that work are kept, because they are unrelated to workflow
tables and close real holes for the genuinely secret columns: dynamic XML
exporters (
query_to_xmland friends), which take their target as a string andso defeat identifier-based checks, and PostgreSQL
U&"..."escaped identifiers,which spell a protected column name without ever writing it.
CI hygiene
mainunconditionally (push: [main]). The duplicate run against a mergecommit is accepted deliberately; the alternative left direct pushes and release
cuts resting on a manual
workflow_dispatch.test_results.trxinto one results directory, so the uploaded artifact held only the last one.
-assemblyfiltersin the coverage step, so the gatednumber stops counting
NodePilot.TestCommonsandNodePilot.LoadTestsasproduction code.
Verification
Solution builds. Backend green: Ai 504, Api 2400, Cli 496, Mcp 180, Engine 1831,
Data 291.
tsc -bclean, 174 frontend tests across the 9 touched spec files green.The remaining audit items (workflow-mutation service, DB-side custom-activity
invariants, WinRM credential rotation, N+1 batching, real integration boundaries,
UI modularisation, xUnit1051) are recorded in
docs/roadmap.mdwith triggers.🤖 Generated with Claude Code