-
Notifications
You must be signed in to change notification settings - Fork 632
Description
Checkboxes for prior research
- I've gone through Developer Guide and API reference
- I've checked AWS Forums and StackOverflow.
- I've searched for previous similar issues and didn't find any solution.
Describe the bug
The byteLength()
function in @aws-sdk/lib-storage
incorrectly identifies Express.js Request
objects as file streams due to an overly broad check for the path
property. This causes lstatSync()
to be called on the request's URL path (e.g., /
), resulting in incorrect byte length calculations and upload failures.
The byteLength()
function incorrectly treating a Request
object as a file system object has been in the library for 5+ years: https://github.com/aws/aws-sdk-js-v3/pull/1547/files#diff-400008ea4066bc75db4aeaaaeacd8992a5a24b989de4d3173b2352ec7e44a10a
The part size validation error was added 2 weeks ago: https://github.com/aws/aws-sdk-js-v3/pull/7363/files#diff-12fa2b5136167522f27fddb10706f410daffd1fef7a7ac5a5cac5b5a273fc588
Regression Issue
- Select this option if this issue appears to be a regression.
SDK version number
@aws-sdk/lib-storage@3.899.0+
Which JavaScript Runtime is this issue in?
Node.js
Details of the browser/Node.js/ReactNative version
v22.20.0
Reproduction Steps
const express = require("express");
const { Upload } = require("@aws-sdk/lib-storage");
const { S3Client } = require("@aws-sdk/client-s3");
const app = express();
const port = 3000;
const s3Client = new S3Client({ region: "us-east-1" });
app.post("/", async (req, res) => {
const upload = new Upload({
client: s3Client,
params: {
Bucket: "my-bucket",
Key: "test-file",
Body: req, // Express Request object - has req.path property
},
});
await upload.done(); // Fails with "Expected 1 part(s) but uploaded X part(s)"
res.send("Upload complete");
});
// Start the server
app.listen(port, () => {
console.log(`Server listening at http://localhost:${port}`);
});
Create a dummy test file, must be greater than 5 MB to trigger multi-part code paths:
dd if=/dev/random of=./test-data bs=1024 count=10000
Then try to upload the file:
curl -X POST --data-binary @test-data http://localhost:3000/
Observed Behavior
When passing an Express Request
object (which extends http.IncomingMessage
and stream.Readable
) directly to the Upload
class, the SDK:
- Checks if
input.path
exists as a string - Assumes this indicates a file stream
- Calls
lstatSync(input.path)
whereinput.path
is actually the HTTP request path - Gets the file system size of that path (e.g., 4096 bytes for
/
on Linux) - Expects that many bytes but receives the actual stream content
- Throws:
Error: Expected 1 part(s) but uploaded 12 part(s)
Expected Behavior
The SDK should correctly identify stream types and not confuse HTTP request objects with file streams. Express Request
objects are valid Readable
streams and should be handled as such.
Possible Solution
In byteLength.ts
, the check for file streams is too broad:
} else if (typeof input.path === 'string') {
// file read stream with path.
try {
return lstatSync(input.path).size;
} catch (_error: any) {
return undefined;
}
}
This check assumes only file streams have a path
property, but Express Request
objects also have a path
property containing the URL path (e.g., /
, /api/upload
, etc.).
The file stream detection should be more specific. Options include:
- Check for multiple file stream properties:
} else if (typeof input.path === 'string' && typeof input.fd !== 'undefined') {
// More likely to be a file stream with both path and file descriptor
- Use instanceof check:
} else if (input instanceof fs.ReadStream) {
// Definitely a file stream
- Check for Node.js stream-specific properties:
} else if (typeof input.path === 'string' &&
(input.constructor?.name === 'ReadStream' ||
input.constructor?.name === 'FileReadStream')) {
- Add exclusion for HTTP-related objects:
} else if (typeof input.path === 'string' &&
!input.headers && !input.method && !input.url) {
// Has path but not HTTP request properties
Additional Information/Context
Workaround
Until fixed, developers can work around this by:
// Option 1: Remove the path property
const { path, ...streamWithoutPath } = req;
upload({ Body: streamWithoutPath });
// Option 2: Pipe through a PassThrough stream
const { PassThrough } = require('stream');
const safeStream = new PassThrough();
req.pipe(safeStream);
upload({ Body: safeStream });
Additional Issue
Beyond the incorrect stream detection, the use of lstatSync()
in a library designed for async operations is problematic:
- Blocks event loop: Synchronous filesystem operations block the entire Node.js process
- Performance degradation: In high-concurrency scenarios (common with S3 uploads), this becomes a bottleneck
- Against Node.js best practices: Node.js explicitly recommends avoiding sync operations in server code
- Inconsistent with SDK design: The AWS SDK v3 is built on promises/async, but uses sync I/O here
This is particularly concerning for a core AWS SDK used in production systems handling potentially thousands of concurrent uploads. The synchronous call could cause request timeouts and degraded performance under load.