Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
- Fixed an issue where some MCP tools would error with "Invalid input: expected record, received array". (#10437)
- feat(emulator): resumable upload API now supports multiple chunk uploads
- feat(emulator): resumable upload API now supports cancelling an upload
160 changes: 149 additions & 11 deletions src/emulator/storage/apis/gcloud.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { EmulatorLogger } from "../../emulatorLogger";
import { GetObjectResponse, ListObjectsResponse } from "../files";
import type { Request, Response } from "express";
import { parseObjectUploadMultipartRequest } from "../multipart";
import { Upload, UploadNotActiveError } from "../upload";
import { NotCancellableError, Upload, UploadNotActiveError, UploadStatus } from "../upload";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The UploadPreviouslyFinalizedError should be imported to ensure it can be correctly handled in the catch block of the resumable upload endpoint.

Suggested change
import { NotCancellableError, Upload, UploadNotActiveError, UploadStatus } from "../upload";
import { NotCancellableError, Upload, UploadNotActiveError, UploadStatus, UploadPreviouslyFinalizedError } from "../upload";

import { ForbiddenError, NotFoundError } from "../errors";
import { reqBodyToBuffer } from "../../shared/request";
import type { Query } from "express-serve-static-core";
Expand Down Expand Up @@ -183,34 +183,172 @@ export function createCloudEndpoints(emulator: StorageEmulator): Router {

gcloudStorageAPI.put("/upload/storage/v1/b/:bucketId/o", async (req, res) => {
if (!req.query.upload_id) {
res.sendStatus(400);
return;
return res.sendStatus(404);
}

async function handleStatusCheck(upload: Upload, contentRange: string) {
// Extract the numeric total from bytes */total or bytes start-end/total forms.
const totalMatch = contentRange && /\/(\d+)$/.exec(contentRange);
const declaredTotal = totalMatch ? parseInt(totalMatch[1], 10) : undefined;

if (upload.status === UploadStatus.FINISHED) {
let getObjectResponse: GetObjectResponse;
try {
getObjectResponse = await adminStorageLayer.getObject({
bucketId: upload.bucketId,
decodedObjectId: upload.objectId,
});
} catch (err) {
if (err instanceof NotFoundError) {
return res.sendStatus(404);
}
if (err instanceof ForbiddenError) {
return res.sendStatus(403);
}
throw err;
}
return res.status(200).json(new CloudStorageObjectMetadata(getObjectResponse.metadata));
}

if (upload.status !== UploadStatus.ACTIVE) {
return res.sendStatus(400);
}

// if the declared total matches the bytes received so far, the client is signalling that
// the upload is complete (i.e. streaming upload where total size wasn't known upfront).
if (declaredTotal !== undefined && upload.size === declaredTotal) {
const metadata = await finalizeUpload(upload.id);
return res.status(200).json(new CloudStorageObjectMetadata(metadata));
}
Comment on lines +219 to +222

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If declaredTotal is provided in a status check and it is less than the current upload.size, it indicates an inconsistency. GCS would typically return a 400 error in such cases. The current logic only handles the case where they are equal.

Suggested change
if (declaredTotal !== undefined && upload.size === declaredTotal) {
const metadata = await finalizeUpload(upload.id);
return res.status(200).json(new CloudStorageObjectMetadata(metadata));
}
if (declaredTotal !== undefined) {
if (upload.size === declaredTotal) {
const metadata = await finalizeUpload(upload.id);
return res.status(200).json(new CloudStorageObjectMetadata(metadata));
} else if (upload.size > declaredTotal) {
return res.status(400).send("The total size of the object is inconsistent with the bytes already received.");
}
}


if (upload.size === 0) {
return res.status(308).send();
}

res.setHeader("Range", `bytes=0-${upload.size - 1}`);
return res.status(308).send();
}

async function handleChunkedUpload(upload: Upload, contentRange: string, data: Buffer) {
const rangeMatch = /^bytes (\d+)-(\d+)(?:\/(\d+|\*))?$/.exec(contentRange);
if (!rangeMatch) {
return res.status(400).send("Failed to parse Content-Range header.");
}

const rangeStart = parseInt(rangeMatch[1], 10);
const rangeEnd = parseInt(rangeMatch[2], 10);
const rangeTotal =
rangeMatch[3] && rangeMatch[3] !== "*" ? parseInt(rangeMatch[3], 10) : undefined;

if (rangeEnd < rangeStart || (rangeTotal !== undefined && rangeTotal < rangeEnd + 1)) {
return res.status(400).send("Failed to parse Content-Range header.");
}

if (rangeStart !== upload.size) {
return res
.status(400)
.send(`Invalid chunk position. The next chunk should start at offset ${upload.size}.`);
}
Comment on lines +247 to +251

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation returns a 400 error if the rangeStart does not exactly match the current upload.size. However, per GCS documentation, if a chunk has already been received (i.e., rangeStart < upload.size), the server should return a 308 status code with the current range, rather than an error. This is important for handling client retries.

      if (rangeStart < upload.size) {
        res.setHeader("Range", `bytes=0-${upload.size - 1}`);
        return res.status(308).send();
      }

      if (rangeStart > upload.size) {
        return res
          .status(400)
          .send(`Invalid chunk position. The next chunk should start at offset ${upload.size}.`);
      }


const expectedChunkSize = rangeEnd - rangeStart + 1;

if (data.byteLength !== expectedChunkSize) {
const message = `Invalid request. There were ${data.byteLength} byte(s) in the request body. There should have been ${expectedChunkSize} byte(s) (starting at offset ${rangeStart} and ending at offset ${rangeEnd}) according to the Content-Range header.`;
return res.status(400).send(message);
}

/**
* When the client uses an unknown total (`*` or omitted), we treat a chunk whose
* size is not a multiple of 256 KiB as the final chunk. Objects whose size is
* an exact multiple of 256 KiB must end with an explicit total in Content-Range.
* @see https://docs.cloud.google.com/storage/docs/performing-resumable-uploads#chunked-upload
*/
const isComplete =
typeof rangeTotal === "number"
? rangeEnd + 1 === rangeTotal
: data.byteLength % (256 * 1024) !== 0;
Comment thread
jakeisonline marked this conversation as resolved.

if (!isComplete && data.byteLength % (256 * 1024) !== 0) {
return res
.status(400)
.send("Chunk size must be a multiple of 256 KiB unless it is the last chunk.");
}

const updatedUpload = uploadService.continueResumableUpload(upload.id, data);

if (isComplete) {
const metadata = await finalizeUpload(upload.id);
return res.status(200).json(new CloudStorageObjectMetadata(metadata));
}

res.setHeader("Range", `bytes=0-${updatedUpload.size - 1}`);
return res.status(308).send();
}

async function finalizeUpload(uploadId: string) {
const finalizedUpload = uploadService.finalizeResumableUpload(uploadId);
return await adminStorageLayer.uploadObject(finalizedUpload);
}

const uploadId = req.query.upload_id.toString();
let upload: Upload;
try {
uploadService.continueResumableUpload(uploadId, await reqBodyToBuffer(req));
upload = uploadService.finalizeResumableUpload(uploadId);
upload = uploadService.getResumableUpload(req.query.upload_id.toString());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid using .toString() directly on req.query parameters without validation, as they can be arrays if the parameter is repeated in the URL. It's safer to ensure it's a string first.

Suggested change
upload = uploadService.getResumableUpload(req.query.upload_id.toString());
const uploadId = req.query.upload_id;
if (typeof uploadId !== "string") {
return res.sendStatus(400);
}
upload = uploadService.getResumableUpload(uploadId);


if (upload.status === UploadStatus.CANCELLED) {
return res.sendStatus(499);
}

const contentLength = req.headers["content-length"];
const contentRange = req.headers["content-range"] ?? "";

// Status check request
// @see https://docs.cloud.google.com/storage/docs/performing-resumable-uploads#status-check
if (contentLength === "0") {
return await handleStatusCheck(upload, contentRange);
}

if (contentLength && contentLength !== "0") {
const data = await reqBodyToBuffer(req);

// Multiple chunk upload
if (contentRange) {
return await handleChunkedUpload(upload, contentRange, data);
} else {
// Single chunk upload
uploadService.continueResumableUpload(upload.id, data);
const metadata = await finalizeUpload(upload.id);
return res.status(200).json(new CloudStorageObjectMetadata(metadata));
}
}
return res.status(400).send();
} catch (err) {
if (err instanceof NotFoundError) {
return res.sendStatus(404);
} else if (err instanceof ForbiddenError) {
return res.sendStatus(403);
} else if (err instanceof UploadNotActiveError) {
return res.sendStatus(400);
}
Comment on lines 329 to 331

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The UploadPreviouslyFinalizedError is not handled in the catch block. While the status is checked earlier, a race condition or logic error could lead to this being thrown, resulting in a 500 response. It should be handled, likely with a 400 status code.

Suggested change
} else if (err instanceof UploadNotActiveError) {
return res.sendStatus(400);
}
} else if (err instanceof UploadNotActiveError || err instanceof UploadPreviouslyFinalizedError) {
return res.sendStatus(400);

throw err;
}
});
Comment thread
jakeisonline marked this conversation as resolved.

gcloudStorageAPI.delete("/upload/storage/v1/b/:bucketId/o", async (req, res) => {
if (!req.query.upload_id) {
return res.sendStatus(405);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Returning a 405 Method Not Allowed for a missing upload_id query parameter seems incorrect. 405 indicates that the DELETE method is not supported for the resource, which isn't the case here. The issue is a malformed request due to a missing required parameter.

A 400 Bad Request would be a more appropriate status code. This would also make it more consistent with general REST API best practices.

Suggested change
return res.sendStatus(405);
return res.sendStatus(400);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this comment, but the live service will respond with a 405 if you supply it with no or invalid params when sending a DELETE. Given this is an emulator, I felt it appropriate to follow the live service's responses rather than general REST API best practices.

Happy to be challenged and adjust though.

}

let metadata: StoredFileMetadata;
try {
metadata = await adminStorageLayer.uploadObject(upload);
uploadService.cancelResumableUpload(req.query.upload_id.toString());
return res.sendStatus(499);
} catch (err) {
if (err instanceof ForbiddenError) {
return res.sendStatus(403);
if (err instanceof NotFoundError) {
return res.sendStatus(404);
} else if (err instanceof NotCancellableError) {
return res.sendStatus(405);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

GCS returns a 400 error if you attempt to cancel an upload that has already been completed. Returning 405 (Method Not Allowed) is less accurate here.

      } else if (err instanceof NotCancellableError) {
        return res.sendStatus(400);

}
throw err;
}
return res.json(new CloudStorageObjectMetadata(metadata));
});

gcloudStorageAPI.post("/b/:bucketId/o/:objectId/acl", async (req, res) => {
Expand Down