Add regression tests for DAP debugger secret masking - #4579
Open
rentziass wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds L0 regression coverage to ensure all user-visible strings emitted over the DAP debugger channel continue to be passed through the runner SecretMasker, preventing future refactors from accidentally reintroducing secret leakage via DAP output.
Changes:
- Added end-to-end L0 tests for
DapReplExecutorsinks (stdout, stderr, exception/error result masking, and${{ }}expansion masking), including aFakeStepHostto simulate process output events without launching a process. - Added L0 socket-based tests for
DapDebuggersinks (catch-all error responses,threadsjob label, andstoppedstep description masking). - Documented the intentional exemption: REPL command echo is unmasked (user’s own input), and added a test to pin verbatim + truncated echo behavior.
Show a summary per file
| File | Description |
|---|---|
| src/Test/L0/Worker/DapReplExecutorL0.cs | Adds FakeStepHost and new REPL masking/echo regression tests. |
| src/Test/L0/Worker/DapDebuggerL0.cs | Adds DapDebugger masking regression tests over a real socket. |
| src/Runner.Worker/Dap/DapReplExecutor.cs | Adds an explanatory comment for why REPL console echo is intentionally unmasked. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
Comment on lines
+1703
to
+1706
| var response = await ReadDapMessageAsync(stream, TimeSpan.FromSeconds(5)); | ||
| Assert.Contains("\"success\":false", response); | ||
| Assert.Contains("***", response); | ||
| Assert.DoesNotContain(secret, response, StringComparison.Ordinal); |
| [Fact] | ||
| [Trait("Level", "L0")] | ||
| [Trait("Category", "Worker")] | ||
| public void BuildEnvironment_MasksNothingButExpandsSecretValuesForExecution() |
The DAP transport is a secret-carrying channel that bypasses the job
log's masking, so every user-visible string a DAP producer relays is run
through the runner's SecretMasker at the point of construction. That
behavior was correct but unpinned: nothing failed if a sink lost its
mask call.
Add L0 regression tests covering each previously untested sink:
DapReplExecutor stdout, stderr, the EvaluateResponseBody error path,
and expression expansion
DapDebugger the HandleMessageAsync catch-all error response,
the threads response job label, and the stopped
event step description
Each test was verified to fail when its mask call is removed.
The one REPL sink deliberately left unmasked is the console echo of the
script the user just typed, which only reflects their own input back to
the session that sent it. Cover its truncation behavior instead, so the
gap reads as a decision rather than an oversight.
DapReplExecutorL0 gains a FakeStepHost so the output pipeline can be
exercised without launching a process, replaying canned stdout/stderr
through the same events a real process raises.
Refs github/actions-runtime#5586 (security review finding #13).
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4fd6981-5088-4e99-9cc2-d1b16bf3c44a
rentziass
force-pushed
the
rentziass-dap-masking-regression-tests
branch
from
July 28, 2026 13:51
ffbee36 to
4ad1d89
Compare
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
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.
Why
The DAP transport is a secret-carrying channel that bypasses the job log's masking, so every user-visible string a DAP producer relays has to go through the runner's
SecretMaskerat the point of construction. (The raw protocol JSON deliberately isn't masked - masking it would corrupt envelope fields liketype/command/seqif a secret collided with them. See the comment onSendMessageInternal.)That behavior was already correct, but it was unpinned: several sinks had no test at all, so a future refactor could silently drop a mask call and nothing would go red. The welcome message and
DapVariableProviderwere covered; the REPL and the rest ofDapDebuggerwere not.What
Adds L0 regression tests for each previously untested sink:
DapReplExecutorDapReplExecutorDapReplExecutorEvaluateResponseBodyerror path (step host throws)DapReplExecutor${{ }}expression expansionDapDebuggerHandleMessageAsynccatch-all error responseDapDebuggerthreadsresponse job labelDapDebuggerstoppedevent step descriptionEvery one of these was verified to fail when its mask call is removed, so none of them are vacuous.
DapReplExecutorL0gains aFakeStepHostthat replays canned stdout/stderr through the sameOutputDataReceived/ErrorDataReceivedevents a real process raises. This lets the full REPL output pipeline run end to end without launching a process. TheDapDebuggertests go over a real socket using the harness already inDapDebuggerL0.The
HandleMessageAsynctest is the one bit of cleverness worth flagging: it triggers the catch-all by sending a non-integerframeId, because Newtonsoft embeds the offending value in its exception message. That gives a realistic case where attacker-influenced input ends up in an error string.The one deliberate gap
DapReplExecutorechoes$ <shell> <script>to the console unmasked. The security review stated the only unmasked REPL path was a constant exit-code string; that isn't quite right, this echo is unmasked too.Leaving it that way is the right call: it reflects the user's own input back to the same session that just typed it, so masking removes the secret from nowhere it wasn't already, and it makes the console actively worse to use (you'd type
run("curl -H 'token: abc123'")and seetoken: ***echoed back while trying to work out what you actually ran).So instead of masking it, this PR covers the echo's real behavior:
ExecuteRunCommand_EchoesScriptVerbatimAndTruncatesLongScriptspins verbatim output plus truncation at 80 chars, which was previously untested. The test region comment is scoped to output the REPL relays from elsewhere and names the echo as excluded, so the gap reads as a decision rather than an oversight.Worth correcting the record with the reviewers on this point rather than letting the "only a constant string is unmasked" claim stand.
Note on finding #18
The linked issue also asks for reconnect tests: different-token reconnect fails, same-token resumes without state leak. The first half is not implementable in this repo. The runner performs no token validation anywhere - it hosts the tunnel with
HostTokenandWebSocketDapBridgeaccepts whatever the relay hands it. The single-subject ACL is a Dev Tunnels property configured by actions-run-service, so that assertion belongs there or in a true integration test. Also worth noting for whoever picks it up:HandleSetBreakpointsis a no-op that returns an empty array, so there is no breakpoint state that could leak today.Testing
Full L0 suite: 1113 passed. The single failure (
ProcessExtensionL0.SuccessReadProcessEnv) is a pre-existing local environment issue - it needs_layout/externals/node20, which isn't built in this worktree, and it is unaffected by this change. All 141Dap*tests pass.