fix(appsec): suppress informational responses after blocking requests#9290
Merged
Conversation
The repo's .mocharc.js enables `allowUncaught`. When a before/after hook throws synchronously, mocha's Runnable.run re-throws the error instead of invoking the hook callback. Our Hook.prototype.run wrapper caught that throw and re-threw it, relying on mocha's uncaughtException handler to record the failure. That path is fragile: when a preceding hook is async, the throw escapes in a promise-continuation context, the fixture process dies before the JSON reporter emits anything, and the failure is silently lost — the run appears to skip/pass instead of failing. Route the caught error through the hook callback (`fn(err)`) instead, which is the same path mocha uses without allowUncaught. This triggers `self.fail(hook, err)` deterministically regardless of async ordering, while preserving suppression of teardown errors when an earlier runnable already failed. Add coverage for both the async-before-all ordering and after-all suppression under `--allow-uncaught`. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
Overall package sizeSelf size: 6.71 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.3.1 | 122.62 kB | 438.86 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 429732a | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-07-13 18:44:26 Comparing candidate commit 429732a in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2313 metrics, 45 unstable metrics.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9290 +/- ##
==========================================
+ Coverage 96.57% 96.60% +0.03%
==========================================
Files 918 919 +1
Lines 121564 121691 +127
Branches 20380 20759 +379
==========================================
+ Hits 117399 117563 +164
+ Misses 4165 4128 -37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The IAST sourcemaps integration test's `before` hook compiles a TS fixture with `npx tsc`. Under TypeScript 6.x this failed for two reasons, so the hook threw: - TS5107: `moduleResolution: "node"` (node10) is now a hard deprecation error. Silenced with `"ignoreDeprecations": "6.0"` (keeps emit identical). - TS2591: `process`/`crypto` globals unresolved. TS 6.x no longer auto-includes `@types` from grandparent `node_modules`, and the sandbox installs deps at its root while tsc runs two levels down. Adding `"types": ["node"]` resolves `@types/node` via normal module walk-up. This hook failure was previously swallowed under mocha's allowUncaught, which also killed the mocha process and silently skipped every appsec integration spec loaded after it (RASP, multer, standalone-asm, etc.). The companion mocha-hooks fix surfaces the hook error instead; this commit makes the compile actually succeed so the whole appsec suite runs green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05b6ea2e88
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
CarlesDD
reviewed
Jul 13, 2026
CarlesDD
left a comment
CarlesDD
left a comment
Contributor
There was a problem hiding this comment.
LGTM from AppSec. Fix is correct, no unintended header logic gets triggered. Left one non-blocking naming suggestion inline.
Comment on lines
+224
to
+240
| function wrapInformationalResponse (originalMethod) { | ||
| return function wrappedInformationalResponse (...args) { | ||
| if (!startSetHeaderCh.hasSubscribers) { | ||
| return Reflect.apply(originalMethod, this, args) | ||
| } | ||
|
|
||
| const abortController = new AbortController() | ||
| startSetHeaderCh.publish({ res: this, abortController }) | ||
|
|
||
| if (abortController.signal.aborted) { | ||
| return | ||
| } | ||
|
|
||
| return Reflect.apply(originalMethod, this, args) | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Reusing set-header:start channel for informational responses (1xx) is misleading, they're not header mutations. Consider a dedicated channel (datadog:http:server:informational-response:start for example) with its own subscribe in appsec/index.js as set-header channel already has.
BridgeAR
reviewed
Jul 13, 2026
BridgeAR
left a comment
BridgeAR
left a comment
Member
There was a problem hiding this comment.
This seems to not mainly be about our test setup anymore but more about the AppSec fix. Ideally, the appsec one would land before the test fix. But I think it is fine as long as we declare it as AppSec fix in one go
| } catch (err) { | ||
| if (shouldSuppress(this)) return fn() | ||
| throw err | ||
| return this.callback(shouldSuppress(this) ? undefined : err) |
Member
There was a problem hiding this comment.
Suggested change
| return this.callback(shouldSuppress(this) ? undefined : err) | |
| return this.callback(shouldSuppress(this) ? undefined : Runnable.toValueOrError(err)) |
Runnable must be imported from mocha above. With this change, we will also handle falsy values correct, even undefined :)
pabloerhard
changed the title
BridgeAR
approved these changes
Jul 13, 2026
Member
|
Please make sure to write a proper description when landing :) |
dd-octo-sts Bot
pushed a commit
that referenced
this pull request
Jul 14, 2026
…#9290) * test(mocha-hooks): stop swallowing hook errors under allowUncaught The repo's .mocharc.js enables `allowUncaught`. When a before/after hook throws synchronously, mocha's Runnable.run re-throws the error instead of invoking the hook callback. Our Hook.prototype.run wrapper caught that throw and re-threw it, relying on mocha's uncaughtException handler to record the failure. That path is fragile: when a preceding hook is async, the throw escapes in a promise-continuation context, the fixture process dies before the JSON reporter emits anything, and the failure is silently lost — the run appears to skip/pass instead of failing. Route the caught error through the hook callback (`fn(err)`) instead, which is the same path mocha uses without allowUncaught. This triggers `self.fail(hook, err)` deterministically regardless of async ordering, while preserving suppression of teardown errors when an earlier runnable already failed. Add coverage for both the async-before-all ordering and after-all suppression under `--allow-uncaught`. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * test(iast): fix tsc build in IAST sourcemaps integration test The IAST sourcemaps integration test's `before` hook compiles a TS fixture with `npx tsc`. Under TypeScript 6.x this failed for two reasons, so the hook threw: - TS5107: `moduleResolution: "node"` (node10) is now a hard deprecation error. Silenced with `"ignoreDeprecations": "6.0"` (keeps emit identical). - TS2591: `process`/`crypto` globals unresolved. TS 6.x no longer auto-includes `@types` from grandparent `node_modules`, and the sandbox installs deps at its root while tsc runs two levels down. Adding `"types": ["node"]` resolves `@types/node` via normal module walk-up. This hook failure was previously swallowed under mocha's allowUncaught, which also killed the mocha process and silently skipped every appsec integration spec loaded after it (RASP, multer, standalone-asm, etc.). The companion mocha-hooks fix surfaces the hook error instead; this commit makes the compile actually succeed so the whole appsec suite runs green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * fix http tests in node 26 * clear timeout with callbacks * rename dc channel * handle falsy values --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
Closed
dd-octo-sts Bot
pushed a commit
that referenced
this pull request
Jul 14, 2026
…#9290) * test(mocha-hooks): stop swallowing hook errors under allowUncaught The repo's .mocharc.js enables `allowUncaught`. When a before/after hook throws synchronously, mocha's Runnable.run re-throws the error instead of invoking the hook callback. Our Hook.prototype.run wrapper caught that throw and re-threw it, relying on mocha's uncaughtException handler to record the failure. That path is fragile: when a preceding hook is async, the throw escapes in a promise-continuation context, the fixture process dies before the JSON reporter emits anything, and the failure is silently lost — the run appears to skip/pass instead of failing. Route the caught error through the hook callback (`fn(err)`) instead, which is the same path mocha uses without allowUncaught. This triggers `self.fail(hook, err)` deterministically regardless of async ordering, while preserving suppression of teardown errors when an earlier runnable already failed. Add coverage for both the async-before-all ordering and after-all suppression under `--allow-uncaught`. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * test(iast): fix tsc build in IAST sourcemaps integration test The IAST sourcemaps integration test's `before` hook compiles a TS fixture with `npx tsc`. Under TypeScript 6.x this failed for two reasons, so the hook threw: - TS5107: `moduleResolution: "node"` (node10) is now a hard deprecation error. Silenced with `"ignoreDeprecations": "6.0"` (keeps emit identical). - TS2591: `process`/`crypto` globals unresolved. TS 6.x no longer auto-includes `@types` from grandparent `node_modules`, and the sandbox installs deps at its root while tsc runs two levels down. Adding `"types": ["node"]` resolves `@types/node` via normal module walk-up. This hook failure was previously swallowed under mocha's allowUncaught, which also killed the mocha process and silently skipped every appsec integration spec loaded after it (RASP, multer, standalone-asm, etc.). The companion mocha-hooks fix surfaces the hook error instead; this commit makes the compile actually succeed so the whole appsec suite runs green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * fix http tests in node 26 * clear timeout with callbacks * rename dc channel * handle falsy values --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
Closed
juan-fernandez
pushed a commit
that referenced
this pull request
Jul 14, 2026
…#9290) * test(mocha-hooks): stop swallowing hook errors under allowUncaught The repo's .mocharc.js enables `allowUncaught`. When a before/after hook throws synchronously, mocha's Runnable.run re-throws the error instead of invoking the hook callback. Our Hook.prototype.run wrapper caught that throw and re-threw it, relying on mocha's uncaughtException handler to record the failure. That path is fragile: when a preceding hook is async, the throw escapes in a promise-continuation context, the fixture process dies before the JSON reporter emits anything, and the failure is silently lost — the run appears to skip/pass instead of failing. Route the caught error through the hook callback (`fn(err)`) instead, which is the same path mocha uses without allowUncaught. This triggers `self.fail(hook, err)` deterministically regardless of async ordering, while preserving suppression of teardown errors when an earlier runnable already failed. Add coverage for both the async-before-all ordering and after-all suppression under `--allow-uncaught`. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * test(iast): fix tsc build in IAST sourcemaps integration test The IAST sourcemaps integration test's `before` hook compiles a TS fixture with `npx tsc`. Under TypeScript 6.x this failed for two reasons, so the hook threw: - TS5107: `moduleResolution: "node"` (node10) is now a hard deprecation error. Silenced with `"ignoreDeprecations": "6.0"` (keeps emit identical). - TS2591: `process`/`crypto` globals unresolved. TS 6.x no longer auto-includes `@types` from grandparent `node_modules`, and the sandbox installs deps at its root while tsc runs two levels down. Adding `"types": ["node"]` resolves `@types/node` via normal module walk-up. This hook failure was previously swallowed under mocha's allowUncaught, which also killed the mocha process and silently skipped every appsec integration spec loaded after it (RASP, multer, standalone-asm, etc.). The companion mocha-hooks fix surfaces the hook error instead; this commit makes the compile actually succeed so the whole appsec suite runs green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * fix http tests in node 26 * clear timeout with callbacks * rename dc channel * handle falsy values --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
juan-fernandez
pushed a commit
that referenced
this pull request
Jul 14, 2026
…#9290) * test(mocha-hooks): stop swallowing hook errors under allowUncaught The repo's .mocharc.js enables `allowUncaught`. When a before/after hook throws synchronously, mocha's Runnable.run re-throws the error instead of invoking the hook callback. Our Hook.prototype.run wrapper caught that throw and re-threw it, relying on mocha's uncaughtException handler to record the failure. That path is fragile: when a preceding hook is async, the throw escapes in a promise-continuation context, the fixture process dies before the JSON reporter emits anything, and the failure is silently lost — the run appears to skip/pass instead of failing. Route the caught error through the hook callback (`fn(err)`) instead, which is the same path mocha uses without allowUncaught. This triggers `self.fail(hook, err)` deterministically regardless of async ordering, while preserving suppression of teardown errors when an earlier runnable already failed. Add coverage for both the async-before-all ordering and after-all suppression under `--allow-uncaught`. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * test(iast): fix tsc build in IAST sourcemaps integration test The IAST sourcemaps integration test's `before` hook compiles a TS fixture with `npx tsc`. Under TypeScript 6.x this failed for two reasons, so the hook threw: - TS5107: `moduleResolution: "node"` (node10) is now a hard deprecation error. Silenced with `"ignoreDeprecations": "6.0"` (keeps emit identical). - TS2591: `process`/`crypto` globals unresolved. TS 6.x no longer auto-includes `@types` from grandparent `node_modules`, and the sandbox installs deps at its root while tsc runs two levels down. Adding `"types": ["node"]` resolves `@types/node` via normal module walk-up. This hook failure was previously swallowed under mocha's allowUncaught, which also killed the mocha process and silently skipped every appsec integration spec loaded after it (RASP, multer, standalone-asm, etc.). The companion mocha-hooks fix surfaces the hook error instead; this commit makes the compile actually succeed so the whole appsec suite runs green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * fix http tests in node 26 * clear timeout with callbacks * rename dc channel * handle falsy values --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
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
3 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.
What does this PR do?
Fixes AppSec request blocking so applications cannot emit an HTTP informational response after AppSec has already blocked the request.
The HTTP server instrumentation now wraps
ServerResponse.writeContinue(),writeProcessing(), andwriteEarlyHints()and publishes a dedicated informational-response diagnostic channel. AppSec subscribes to that channel and aborts the operation when the response is already blocked; otherwise, the original Node.js method runs unchanged.This PR also includes test-infrastructure fixes needed for these tests to fail correctly if necessary:
allowUncaught, preserving timeout cleanup and normalizing falsy thrown values.Motivation
Once AppSec has produced a blocking response, later calls to Node.js informational-response APIs must not write additional status lines to the socket. These methods were not covered by the existing response-operation guard, so application code could still call them after a request had been blocked.
When
allowUncaughtenabled, synchronous hook failures could escape through Mocha's uncaught-exception path, terminate a fixture before its reporter output was complete, and make later AppSec tests appear skipped or successful. Routing those failures through Mocha's runnable callback makes the suite report the original failure deterministically.Additional Notes
--allow-uncaught.semver-patchproduction fix.