fix: uncaught exception due to second response with digest auth #530
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve a modification to the Changes
Possibly related PRs
Poem
Tip OpenAI O1 model for chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
damiengrandi
changed the title
commit: |
| @@ -562,6 +562,8 @@ export class HttpClient extends EventEmitter { | |||
| // FIXME: merge exists cookie header | |||
| requestOptions.headers.cookie = response.headers['set-cookie'].join(';'); | |||
| } | |||
| // Ensure the previous response is consumed as we re-use the same variable | |||
| await response.body.arrayBuffer(); | |||
There was a problem hiding this comment.
@damiengrandi Please add an associated test case for this change.
There was a problem hiding this comment.
Hi, are you asking for an example to reproduce this issue? With an example URL and additional details?
There was a problem hiding this comment.
Yes, a test example to reproduce the problem.
There was a problem hiding this comment.
In some specific cases where APIs are slow to respond and DigestAuth is used, an uncaught UND_ERR_BODY_TIMEOUT exception can occur. Below is a source code example illustrating this issue:
import { request, RequestOptions } from 'urllib';
try {
const username = "someUsername";
const password = "somePassword";
const deviceHost = "123.456.789.123";
const deviceWebPort = 80;
const path = "/axis-cgi/com/ptz.cgi?camera=1&query=position";
const requestUrl = `http://${deviceHost}:${deviceWebPort}${path}`;
const requestOptions: RequestOptions = {
digestAuth: `${username}:${password}`,
method: 'GET',
timeout: [1000, 1], // 1ms to force UND_ERR_BODY_TIMEOUT to happen more often for testing purpose
};
// This line may provoke an uncaught exception in some cases
const res = await request(requestUrl, requestOptions);
const data = res.data?.toString();
console.log(data);
} catch (error) {
console.error(error);
}I’ve observed this issue when querying PTZ position from an AXIS camera. The device can be slow to respond, which makes it a good candidate for testing this issue. You can find more details on the relevant endpoint in AXIS's documentation here: https://www.axis.com/vapix-library/subjects/t10175981/section/t10036011/display?section=t10036011-t10004639
Unfortunately, due to GDPR restrictions, I can't provide a camera with dummy credentials.
However, you may reproduce the issue with other devices that use DigestAuth and are slow to respond to API requests.
There was a problem hiding this comment.
I found a way for you to reproduce in local.
Using the following PHP script to simulate slow response during the first DigestAuth request :
<?php
function sendDigestAuthChallenge() {
$realm = 'Restricted Area';
$nonce = uniqid();
header('HTTP/1.1 401 Unauthorized');
header('WWW-Authenticate: Digest realm="' . $realm . '",qop="auth",nonce="' . $nonce . '",opaque="' . md5($realm) . '"');
// Simulate slow response time
for ($i = 0; $i < 10000000; $i++) {
echo ".";
}
echo "Authentication required";
exit;
}
if (empty($_SERVER['PHP_AUTH_DIGEST'])) {
sendDigestAuthChallenge();
}
echo "OK";And using this code to test it from NodeJS:
do {
try {
console.log('HERE');
const host = 'http://localhost/test_digest/test.php';
const options: RequestOptions = {
digestAuth: 'root:root',
method: 'GET',
timeout: [3000, 1],
};
const response = await request(host, options);
console.log(response.data?.toString());
} catch (error) {
console.error(error);
}
// Wait 1 sec
await new Promise((resolve) => setTimeout(resolve, 1000));
} while (true);I got the crash very quickly:
fengmk2
changed the title
|
@damiengrandi Thanks a lot! |
[skip ci] ## [4.2.2](v4.2.1...v4.2.2) (2024-09-14) ### Bug Fixes * uncaught exception due to second response with digest auth ([#530](#530)) ([9a7833e](9a7833e))
pick from #530 Co-authored-by: Damien Grandi <damien.grandi@gmail.com>
In rare cases, an uncaught exception can happen with digest auth because of the re-use of the same variable representing the
response.Demonstration of the issue:
Can cause this crash, the exception have not been caught by the
try-catchblock:I fixed the issue by consuming the initial response body before sending a new request:
Another way to fix this could be to use a dedicated variable for each response, but the first response may need to be closed anyway to avoid potential memory issue.
This issue exists in both version 4.2 and 3.22.2 (tested).
Summary by CodeRabbit