Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Storage Emulator crash on iOS auth error for resumable uploads #4210

Merged
merged 6 commits into from
Mar 1, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
- Updates reserved environment variables for CF3 to include 'EVENTARC_CLOUD_EVENT_SOURCE' (#4196).
- Fixes arg order for `firebase emulators:start --only storage` (#4195).
- Fixes iOS auth for resumable uploads in Storage Emulator (#4184).
- Fixes Storage Emulator crash on iOS auth error for resumable uploads (#4210).
- Fixes bug where environment variable for gen 2 functions weren't updated on deploy (#4209).
- Fixes an issue in the storage emulator where a file upload would trigger functions with a metadata update handler (#4213).
138 changes: 138 additions & 0 deletions scripts/storage-emulator-integration/tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1340,6 +1340,144 @@ describe("Storage emulator", () => {
"X-Goog-Upload-Command": "upload, finalize",
})
.expect(200);

await supertest(STORAGE_EMULATOR_HOST)
.get(`/v0/b/${storageBucket}/o/test_upload.jpg`)
.set({ Authorization: "Bearer owner" })
.expect(200);
});

it("should return 403 when resumable upload is unauthenticated", async () => {
const uploadURL = await supertest(STORAGE_EMULATOR_HOST)
.post(
`/v0/b/${storageBucket}/o/test_upload.jpg?uploadType=resumable&name=test_upload.jpg`
)
.set({
// Authorization missing
"X-Goog-Upload-Protocol": "resumable",
"X-Goog-Upload-Command": "start",
})
.expect(200)
.then((res) => new URL(res.header["x-goog-upload-url"]));

await supertest(STORAGE_EMULATOR_HOST)
.put(uploadURL.pathname + uploadURL.search)
.set({
"X-Goog-Upload-Protocol": "resumable",
"X-Goog-Upload-Command": "upload, finalize",
})
.expect(403);
});

describe("cancels upload", () => {
it("should cancel upload successfully", async () => {
const uploadURL = await supertest(STORAGE_EMULATOR_HOST)
.post(
`/v0/b/${storageBucket}/o/test_upload.jpg?uploadType=resumable&name=test_upload.jpg`
)
.set({
Authorization: "Bearer owner",
"X-Goog-Upload-Protocol": "resumable",
"X-Goog-Upload-Command": "start",
})
.expect(200)
.then((res) => new URL(res.header["x-goog-upload-url"]));

await supertest(STORAGE_EMULATOR_HOST)
.put(uploadURL.pathname + uploadURL.search)
.set({
"X-Goog-Upload-Protocol": "resumable",
"X-Goog-Upload-Command": "cancel",
})
.expect(200);
tohhsinpei marked this conversation as resolved.
Show resolved Hide resolved

await supertest(STORAGE_EMULATOR_HOST)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add an assertion in this test case or another that ensures the querying for the upload after a cancel denotes that the upload was cancelled?

go/unified-upload-protocols#ended-uploads

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure we're doing this now actually 😅 But I would fix this in another PR.

.get(`/v0/b/${storageBucket}/o/test_upload.jpg`)
.set({ Authorization: "Bearer owner" })
.expect(404);
});

it("should return 200 when cancelling already cancelled upload", async () => {
const uploadURL = await supertest(STORAGE_EMULATOR_HOST)
.post(
`/v0/b/${storageBucket}/o/test_upload.jpg?uploadType=resumable&name=test_upload.jpg`
)
.set({
Authorization: "Bearer owner",
"X-Goog-Upload-Protocol": "resumable",
"X-Goog-Upload-Command": "start",
})
.expect(200)
.then((res) => new URL(res.header["x-goog-upload-url"]));

await supertest(STORAGE_EMULATOR_HOST)
.put(uploadURL.pathname + uploadURL.search)
.set({
"X-Goog-Upload-Protocol": "resumable",
"X-Goog-Upload-Command": "cancel",
})
.expect(200);

await supertest(STORAGE_EMULATOR_HOST)
.put(uploadURL.pathname + uploadURL.search)
.set({
"X-Goog-Upload-Protocol": "resumable",
"X-Goog-Upload-Command": "cancel",
})
.expect(200);
});

it("should return 400 when cancelling finalized resumable upload", async () => {
const uploadURL = await supertest(STORAGE_EMULATOR_HOST)
.post(
`/v0/b/${storageBucket}/o/test_upload.jpg?uploadType=resumable&name=test_upload.jpg`
)
.set({
Authorization: "Bearer owner",
"X-Goog-Upload-Protocol": "resumable",
"X-Goog-Upload-Command": "start",
})
.expect(200)
.then((res) => new URL(res.header["x-goog-upload-url"]));

await supertest(STORAGE_EMULATOR_HOST)
.put(uploadURL.pathname + uploadURL.search)
.set({
"X-Goog-Upload-Protocol": "resumable",
"X-Goog-Upload-Command": "upload, finalize",
})
.expect(200);

await supertest(STORAGE_EMULATOR_HOST)
.put(uploadURL.pathname + uploadURL.search)
.set({
"X-Goog-Upload-Protocol": "resumable",
"X-Goog-Upload-Command": "cancel",
})
.expect(400);
});

it("should return 404 when cancelling non-existent upload", async () => {
const uploadURL = await supertest(STORAGE_EMULATOR_HOST)
.post(
`/v0/b/${storageBucket}/o/test_upload.jpg?uploadType=resumable&name=test_upload.jpg`
)
.set({
Authorization: "Bearer owner",
"X-Goog-Upload-Protocol": "resumable",
"X-Goog-Upload-Command": "start",
})
.expect(200)
.then((res) => new URL(res.header["x-goog-upload-url"]));

await supertest(STORAGE_EMULATOR_HOST)
.put(uploadURL.pathname + uploadURL.search.replace(/(upload_id=).*?(&)/, "$1foo$2"))
.set({
"X-Goog-Upload-Protocol": "resumable",
"X-Goog-Upload-Command": "cancel",
})
.expect(404);
});
});
});

Expand Down
27 changes: 14 additions & 13 deletions src/emulator/storage/apis/firebase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -493,12 +493,13 @@ export function createFirebaseEndpoints(emulator: StorageEmulator): Router {
}

if (uploadCommand == "cancel") {
const upload = storageLayer.cancelUpload(uploadId);
if (!upload) {
res.sendStatus(400);
return;
const upload = storageLayer.queryUpload(uploadId);
if (upload) {
const cancelled = storageLayer.cancelUpload(upload);
res.sendStatus(cancelled ? 200 : 400);
} else {
res.sendStatus(404);
}
res.sendStatus(200);
return;
}

Expand Down Expand Up @@ -530,14 +531,11 @@ export function createFirebaseEndpoints(emulator: StorageEmulator): Router {
}

if (uploadCommand.includes("finalize")) {
const finalizedUpload = storageLayer.finalizeUpload(uploadId);
if (!finalizedUpload) {
upload = storageLayer.queryUpload(uploadId);
if (!upload) {
res.sendStatus(400);
return;
}
upload = finalizedUpload.upload;

res.header("x-goog-upload-status", "final");

// For resumable uploads, we check auth on finalization in case of byte-dependant rules
if (
Expand All @@ -548,7 +546,7 @@ export function createFirebaseEndpoints(emulator: StorageEmulator): Router {
path: operationPath,
authorization: upload.authorization,
file: {
after: storageLayer.getMetadata(req.params.bucketId, name)?.asRulesResource(),
after: storageLayer.createMetadata(upload).asRulesResource(),
},
}))
) {
Expand All @@ -561,12 +559,15 @@ export function createFirebaseEndpoints(emulator: StorageEmulator): Router {
});
}

const md = finalizedUpload.file.metadata;
res.header("x-goog-upload-status", "final");
const uploadedFile = storageLayer.finalizeUpload(upload);

const md = uploadedFile.metadata;
if (md.downloadTokens.length == 0) {
tohhsinpei marked this conversation as resolved.
Show resolved Hide resolved
md.addDownloadToken();
}

res.json(new OutgoingFirebaseMetadata(finalizedUpload.file.metadata));
res.json(new OutgoingFirebaseMetadata(uploadedFile.metadata));
} else if (!upload) {
res.sendStatus(400);
return;
Expand Down
12 changes: 3 additions & 9 deletions src/emulator/storage/apis/gcloud.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,20 +123,14 @@ export function createCloudEndpoints(emulator: StorageEmulator): Router {
});
});

let upload = storageLayer.uploadBytes(uploadId, req.body);

const upload = storageLayer.uploadBytes(uploadId, req.body);
if (!upload) {
res.sendStatus(400);
return;
}

const finalizedUpload = storageLayer.finalizeUpload(uploadId);
if (!finalizedUpload) {
res.sendStatus(400);
return;
}
upload = finalizedUpload.upload;
res.status(200).json(new CloudStorageObjectMetadata(finalizedUpload.file.metadata)).send();
const uploadedFile = storageLayer.finalizeUpload(upload);
res.status(200).json(new CloudStorageObjectMetadata(uploadedFile.metadata)).send();
});

gcloudStorageAPI.post("/b/:bucketId/o/:objectId/acl", (req, res) => {
Expand Down
77 changes: 44 additions & 33 deletions src/emulator/storage/files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,6 @@ export enum UploadStatus {
FINISHED,
}

export type FinalizedUpload = {
upload: ResumableUpload;
file: StoredFile;
};

export class StorageLayer {
private _files!: Map<string, StoredFile>;
private _uploads!: Map<string, ResumableUpload>;
Expand Down Expand Up @@ -173,6 +168,27 @@ export class StorageLayer {
return;
}

/**
* Generates metadata for an uploaded file. Generally, this should only be used for finalized
* uploads, unless needed for security rule checks.
* @param upload The upload corresponding to the file for which to generate metadata.
* @returns Metadata for uploaded file.
*/
public createMetadata(upload: ResumableUpload): StoredFileMetadata {
const bytes = this._persistence.readBytes(upload.fileLocation, upload.currentBytesUploaded);
return new StoredFileMetadata(
{
name: upload.objectId,
bucket: upload.bucketId,
contentType: "",
contentEncoding: upload.metadata.contentEncoding,
customMetadata: upload.metadata.metadata,
},
this._cloudFunctions,
bytes
);
}

public getBytes(
bucket: string,
object: string,
Expand Down Expand Up @@ -216,13 +232,18 @@ export class StorageLayer {
return this._uploads.get(uploadId);
}

public cancelUpload(uploadId: string): ResumableUpload | undefined {
const upload = this._uploads.get(uploadId);
if (!upload) {
return undefined;
/**
* Deletes partially uploaded file from persistence layer and updates its status. Cancelling
* an upload is idempotent.
* @param upload The upload to be cancelled.
* @returns Whether the upload was cancelled (i.e. its initial status was ACTIVE or CANCELLED).
*/
public cancelUpload(upload: ResumableUpload): boolean {
if (upload.status === UploadStatus.ACTIVE) {
this._persistence.deleteFile(upload.fileLocation);
upload.status = UploadStatus.CANCELLED;
}
upload.status = UploadStatus.CANCELLED;
this._persistence.deleteFile(upload.fileLocation);
return upload.status === UploadStatus.CANCELLED;
tohhsinpei marked this conversation as resolved.
Show resolved Hide resolved
}

public uploadBytes(uploadId: string, bytes: Buffer): ResumableUpload | undefined {
Expand Down Expand Up @@ -266,36 +287,26 @@ export class StorageLayer {
return this._persistence.deleteAll();
}

public finalizeUpload(uploadId: string): FinalizedUpload | undefined {
const upload = this._uploads.get(uploadId);

if (!upload) {
return undefined;
}

/**
* Stores the uploaded file with generated metadata and triggers Object Finalize Cloud Functions.
* @param upload The upload to finalize.
* @returns The stored file.
*/
public finalizeUpload(upload: ResumableUpload): StoredFile {
upload.status = UploadStatus.FINISHED;

const metadata = this.createMetadata(upload);
const filePath = this.path(upload.bucketId, upload.objectId);
const file = new StoredFile(metadata, filePath);

const bytes = this._persistence.readBytes(upload.fileLocation, upload.currentBytesUploaded);
const finalMetadata = new StoredFileMetadata(
{
name: upload.objectId,
bucket: upload.bucketId,
contentType: "",
contentEncoding: upload.metadata.contentEncoding,
customMetadata: upload.metadata.metadata,
},
this._cloudFunctions,
bytes
);
const file = new StoredFile(finalMetadata, filePath);
this._files.set(filePath, file);

this._persistence.deleteFile(filePath, true);
this._persistence.deleteFile(filePath, /* failSilently = */ true);
this._persistence.renameFile(upload.fileLocation, filePath);

this._cloudFunctions.dispatch("finalize", new CloudStorageObjectMetadata(file.metadata));
return { upload: upload, file: file };

return file;
}

public oneShotUpload(
Expand Down
27 changes: 27 additions & 0 deletions src/test/emulators/storage/files.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { expect } from "chai";
import { StoredFileMetadata } from "../../../emulator/storage/metadata";
import { StorageCloudFunctions } from "../../../emulator/storage/cloudFunctions";
import { StorageLayer } from "../../../emulator/storage/files";

describe("files", () => {
it("can serialize and deserialize metadata", () => {
Expand All @@ -23,4 +24,30 @@ describe("files", () => {
const deserialized = StoredFileMetadata.fromJSON(json, cf);
expect(deserialized).to.deep.equal(metadata);
});

it("should store file in memory when upload is finalized", () => {
const storageLayer = new StorageLayer("project");
const bytesToWrite = "Hello, World!";

const upload = storageLayer.startUpload("bucket", "object", "mime/type", {
contentType: "mime/type",
});
storageLayer.uploadBytes(upload.uploadId, Buffer.from(bytesToWrite));
storageLayer.finalizeUpload(upload);
tohhsinpei marked this conversation as resolved.
Show resolved Hide resolved

expect(storageLayer.getBytes("bucket", "object")?.includes(bytesToWrite));
expect(storageLayer.getMetadata("bucket", "object")?.size).equals(bytesToWrite.length);
});

it("should delete file from persistence layer when upload is cancelled", () => {
const storageLayer = new StorageLayer("project");

const upload = storageLayer.startUpload("bucket", "object", "mime/type", {
contentType: "mime/type",
});
storageLayer.uploadBytes(upload.uploadId, Buffer.alloc(0));
storageLayer.cancelUpload(upload);

expect(storageLayer.getMetadata("bucket", "object")).to.equal(undefined);
});
});