Repository navigation
Add regression tests for DAP debugger secret masking - #4579
Merged
Merged
Conversation
Member
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
salmanmkc
approved these changes
Aug 3, 2026
rentziass
enabled auto-merge (squash)
August 3, 2026 08:44
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$