Scope external-trigger keys and contain the auth boundary - #203
Merged
Conversation
A repo-wide clone scan plus a semantic pass found ~85 verified places where the same logic existed more than once. This removes them, mostly by routing callers through a helper or base type that already existed. Backend: - Five retention services shared a byte-identical ExecuteAsync loop; they now derive from LeaderGatedRetentionService. The broad catch stays in each service's RunIterationAsync, one level below the host-fatal boundary. - fileOperation/folderOperation share FileSystemOperationActivityBase; the marker-envelope PostProcess preamble is now one helper used by seven activities. - The AES-GCM envelope (version byte + nonce + ciphertext + tag) had two independent implementations; both now use SecretEnvelope. Header validation stays outside the metrics scope, as before. - Crypto call/latency instrumentation collapses into DataMetrics.MeasureCrypto. - The ExecRow projection, the long-running/queued-long collectors, the NotificationDispatcher send tail, CsvField, the trigger manual.* extraction and the folder-tree helpers each exist once now. - ScimProvisioningService now forwards audits through AuditEventForwarder instead of hand-rolling the ECS scope. This fixes a real inconsistency: it hard-coded event.category=iam where the shared classifier returns "configuration" for SCIM_GROUP_* codes. Clients: - ApiException, the response plumbing and the config.json read side move to NodePilot.Core.Clients, shared by the CLI and the MCP server. The DTO copies stay duplicated on purpose (ApiDtoParityTests). Frontend: - The AI chat message mutators and the thread picker were duplicated between the designer panel and the global chat page; both now share one module. - LoggingTelemetrySection carried a private fork of useSectionForm. The fork had drifted and was the better version (it re-sends the mapped payload after a 412), so that behaviour moved into the shared hook before the fork was removed. - useDashboardFeed/useOperationsFeed, the type-the-phrase confirm dialog and the node context menu now use shared implementations. Dead code: three unused DbErrorClassifier predicates, a deprecated cloneableConfigKeys alias, four unused frontend exports, two orphaned stress scripts and an unused npm dependency. Test doubles: FakeSmtpServer, CapturingLogger, StubHttpClientFactory, StubGlobalVariableStore and FakeEngine each existed in two places; they now live in TestCommons or a shared Helpers file. ActivityConfigReferenceTests learned to follow an executor's base class and helpers, so config keys that moved into shared types stay verified instead of being exempted. Net effect: 1407 fewer lines of code. Full backend suites (5486 tests) and the frontend suite (2596 tests) pass; the solution builds clean.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+99
to
+102
| window.dispatchEvent(new StorageEvent('storage', { | ||
| key: AUTH_BOUNDARY_STORAGE_KEY, | ||
| newValue: JSON.stringify(remote), | ||
| })); |
Comment on lines
+297
to
+300
| window.dispatchEvent(new StorageEvent('storage', { | ||
| key: AUTH_BOUNDARY_STORAGE_KEY, | ||
| newValue: JSON.stringify(remoteIdentity), | ||
| })); |
Comment on lines
+333
to
+342
| window.dispatchEvent(new StorageEvent('storage', { | ||
| key: AUTH_BOUNDARY_STORAGE_KEY, | ||
| newValue: JSON.stringify({ | ||
| version: 1, | ||
| type: 'identity', | ||
| userId: 'u-b', | ||
| sourceId: 'another-tab', | ||
| eventId: 'newer-login-before-stale-failure', | ||
| } satisfies AuthBoundaryEvent), | ||
| })); |
Comment on lines
+408
to
+411
| window.dispatchEvent(new StorageEvent('storage', { | ||
| key: AUTH_BOUNDARY_STORAGE_KEY, | ||
| newValue: JSON.stringify(remoteEvent), | ||
| })); |
Comment on lines
+452
to
+455
| window.dispatchEvent(new StorageEvent('storage', { | ||
| key: AUTH_BOUNDARY_STORAGE_KEY, | ||
| newValue: JSON.stringify(started), | ||
| })); |
Comment on lines
+542
to
+545
| window.dispatchEvent(new StorageEvent('storage', { | ||
| key: AUTH_BOUNDARY_STORAGE_KEY, | ||
| newValue: JSON.stringify(unauthorized), | ||
| })); |
Comment on lines
+569
to
+577
| window.dispatchEvent(new StorageEvent('storage', { | ||
| key: AUTH_BOUNDARY_STORAGE_KEY, | ||
| newValue: JSON.stringify({ | ||
| version: 1, | ||
| type: 'cookie-changed', | ||
| sourceId: 'another-tab', | ||
| eventId: 'stale-login-set-cookie', | ||
| } satisfies AuthBoundaryEvent), | ||
| })); |
Comment on lines
+599
to
+602
| const dispatch = (event: AuthBoundaryEvent) => window.dispatchEvent(new StorageEvent('storage', { | ||
| key: AUTH_BOUNDARY_STORAGE_KEY, | ||
| newValue: JSON.stringify(event), | ||
| })); |
Comment on lines
+657
to
+660
| window.dispatchEvent(new StorageEvent('storage', { | ||
| key: AUTH_BOUNDARY_STORAGE_KEY, | ||
| newValue: JSON.stringify(identityEvent), | ||
| })); |
Comment on lines
+674
to
+677
| window.dispatchEvent(new StorageEvent('storage', { | ||
| key: AUTH_BOUNDARY_STORAGE_KEY, | ||
| newValue: JSON.stringify(logoutStarted), | ||
| })); |
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Folding the three field pickers onto one PickerPopover passed the whole useSearchablePicker() result through as a prop. Inside the component that object is opaque, so the React Compiler treats every read on it as a ref access during render and eslint fails the build with seven "Cannot access refs during render" errors. Destructure once at the top of the component instead. The refs are then forwarded as plain values, which is what the pre-dedup code did and what AnchoredPickerPopover expects.
Sev7eNup
force-pushed
the
security/harden-auth-boundary-and-trigger-scopes
branch
from
August 14, 2026 18:56
2888900 to
2091772
Compare
Both armed-trigger blackout tests split their evaluator verdict on "now + 30 seconds", while the fixture's cron fires at the top of the coming hour. Whenever the suite runs in the last half minute of an hour the cutoff lands past the fire time and the split inverts, so the flagged case reports no window and the not-flagged case reports one. CI hit exactly that at 19:00:20Z. Derive the cutoff from the hour boundary instead and place it midway, so it is strictly between "now" and the fire time at every wall-clock moment.
A repo-wide sweep looked for code that a smaller solution would cover just as well. These twelve findings survived an adversarial check for hidden consumers, test doubles and documented conventions. Engine - ReturnDataActivity: drop the refcounted per-execution semaphore registry. It guarded a single atomic ExecuteUpdateAsync, and the determinism it implied is explicitly not promised (last-write-wins). - WorkflowDbWriteMetrics: SaveChangesMeasuredAsync now delegates to ExecuteMeasuredAsync instead of repeating its body; the two error catches collapse into one that keeps the cancelled/failure split. - BaseRemoteActivity: delete the unreachable four-argument constructor and make _configuration non-nullable. All thirteen subclasses already passed a configuration, so the null-config branches in StartProgramActivity, PowerManagementActivity and WaitForConditionActivity were dead - and they silently permitted useShellExecute and skipped the path guards. Api - CmTraceFormatter: replace the hand-rolled in-place substitution loop with StringBuilder.Replace. - Observability: drop four DTO records that were never constructed and three response fields that were always empty, plus their TypeScript mirrors. Mcp - DefinitionDiff: replace the custom canonicalising serializer and its JsonEquals(a, b) => a == b wrapper with JsonNode.DeepEquals. Ai - LlmConfiguredProxy: drop the six-field cache record and its SequenceEqual comparison. It avoided constructing a WebProxy on a path capped at 20 requests per minute. Frontend - NodeLibrary: remove the 23-entry Tailwind colour table that shadowed the generated --act-* tokens without a dark variant, and read the shared activity visual instead. Six catalog types that the table missed are now coloured correctly. - configClone: drop the skip-key registry for a provably empty set. - SystemSettingsPage: remove the unreachable comingSoon branch and replace the nine-way || chain and ternary with a lookup record. Tooling - dev-reset.ps1: remove the SkipTests switch that defaulted to true and therefore disabled its own block. - Delete the MockRemoteSession test helper; its single caller now builds the mock inline like the other fifteen activity test files.
SystemSettingsPage had no vitest file, so the LABEL_KEYS record that replaced the nine-way || chain went in unguarded. It doubles as the whitelist for the ?section= deep link, which makes the mapping worth pinning: one tab per sub-tab, every label translated (a missing key would render as "adminSettings:subTabX"), each section reachable by deep link, an unknown value falling back to integrations, and a click writing tab=system§ion=<id> back into the query string.
Backend
- External trigger: X-Api-Key is matched against SHA-256 hashes under
ExternalTrigger:Keys:<id>, and every entry carries a GUID-only
AllowedWorkflowIds list. The whole Keys map now comes atomically from
the highest-priority provider that declares it, so `Keys: {}` revokes
everything below it instead of merging with it; scope arrays are
provider-atomic the same way, which makes `[]` deny-all rather than
inherit-all. The legacy ApiKey is inert without its own list.
Idempotency is domain-separated by canonical integration id, key
fingerprint and workflow, and only the digest reaches the database.
- Remote activities inject a target-path guard into the generated script
(TargetPathGuardScript), so filePath/workingDirectory are checked
against the configured roots on the executing host, including a
reparse-point check.
- LLM: a plaintext loopback endpoint is never handed to a proxy - with
one configured, "localhost" would resolve on the proxy machine and the
unencrypted prompt would leave the boundary the endpoint guard
promised.
Frontend
- An explicit auth boundary (security/authBoundary.ts,
sensitiveBrowserState.ts) plus a ProtectedRoute keyed on the boundary
epoch. React can batch a clear/accept pair into a single render during
an A-to-B identity switch, so the key forces the protected subtree to
unmount: component-local SQL and results drop, and AbortControllers
for AI/SSE consumers run their cleanup.
Docs are synced across README, CLAUDE.md, E2ETests.md, docs/, the
deployment templates and the docs-ui corpus.
Sev7eNup
force-pushed
the
security/harden-auth-boundary-and-trigger-scopes
branch
from
August 14, 2026 19:09
2091772 to
5de0b99
Compare
This was referenced Aug 15, 2026
Sev7eNup
added a commit
that referenced
this pull request
Aug 15, 2026
docs: sync docs-ui proxy loopback bypass after #203
Sev7eNup
added a commit
that referenced
this pull request
Aug 15, 2026
- The remote-side path guard (#205): PR #203 made every path-taking remote activity repeat the AllowedRoots + reparse-point check inside the WinRM target's own PowerShell context, which inverts the previous "the API cannot resolve the remote host's reparse map" statement. The docs site carried the new behaviour; claude-reference did not. - OpenTelemetry:RedactHostnames (#200): the default flipped to true, so after an upgrade host.name disappears from OTLP and service.instance.id stops being hostname:pid. siem-logging.md described it; the README config table and the hardening-flags table did not, and neither did the docs site. - LeaderOnlyAttribute (#200): the middleware now checks endpoint metadata before its path heuristics, which is how a semantically mutating GET (webhook ingress) gets fenced on a follower. Named in enterprise-features.md and the HA page so the next such endpoint gets the attribute. - av-exclusions (#184): PR #183 added a detailed A.1 row for %TEMP%\nodepilot-artifact-* without touching the older maintenance- window row, leaving the same path listed twice with contradictory priorities — and the old row said "Installation" although the updater has used that path since Update-NodePilot.ps1 gained artifact staging. The row now matches A.1 and points at it. Closes #205 Closes #200 Closes #184
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.
Backend
X-Api-Keyis matched against SHA-256 hashes underExternalTrigger:Keys:<id>, and every entry carries a GUID-onlyAllowedWorkflowIdslist.The whole
Keysmap now comes atomically from the highest-priority provider that declares it,so
Keys: {}revokes everything below it instead of merging with it; scope arrays areprovider-atomic the same way, which makes
[]deny-all rather than inherit-all. The legacyApiKeyis inert without its own list. Idempotency is domain-separated by canonicalintegration id, key fingerprint and workflow, and only the digest reaches the database.
(
TargetPathGuardScript), sofilePath/workingDirectoryare validated against theconfigured roots on the executing host, including a reparse-point check.
configured,
localhostwould resolve on the proxy machine and the unencrypted prompt wouldleave the boundary the endpoint guard promised.
Frontend
security/authBoundary.ts,sensitiveBrowserState.ts) plus aProtectedRoutekeyed on the boundary epoch. React can batch a clear/accept pair into asingle render during an A-to-B identity switch, so the key forces the protected subtree to
unmount: component-local SQL and results drop, and
AbortControllers for AI/SSE consumersrun their cleanup.
Docs are synced across README, CLAUDE.md, E2ETests.md,
docs/, the deployment templates andthe docs-ui corpus.
Size
7736 added lines sounds large; 54 % of it is test code (4192 lines across 39 test files).
Product source is 3377 lines over 68 files.
Verification
Full backend suite (5612 tests, 6 projects) and full frontend unit suite (2657 tests, 202
files) pass; solution builds with 0 errors;
tsc --noEmitclean. Playwright E2E is left to CI.Stacked on #201 and #202 — merge those first.