Skip to content

Commit

Permalink
Fix issue of copy succeeded without 'r' permission in SAS token crede…
Browse files Browse the repository at this point in the history 
…ntial (#2330)
  • Loading branch information
@EmmaZhu
EmmaZhu authored Dec 26, 2023
1 parent 9208664 commit 3f64420
Show file tree
Hide file tree
Showing 4 changed files with 110 additions and 67 deletions.
1 change: 1 addition & 0 deletions ChangeLog.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Blob:

- Fixed issue of setting blob tag should not update Blob Etag and LastModified. (issue #2327)
- Fix HTTP header parsing of `SubmitBatch()`. If a HTTP header has HTTP header delimiter (`:`) in its value, `SubmitBatch()` returns "400 One of the request inputs is not valid". For example, if `user-agent` header is `azsdk-cpp-storage-blobs/12.10.0-beta.1 (Darwin 23.1.0 arm64 Darwin Kernel Version 23.1.0: Mon Oct 9 21:28:12 PDT 2023; root:xnu-10002.41.9~6/RELEASE_ARM64_T8103)`, all `SubmitBatch()` requests are failed.
- Fixed issue of blob copying succeed without 'r' permission in source blob's SAS token credential.

## 2023.12 Version 3.29.0

Expand Down
127 changes: 63 additions & 64 deletions src/blob/handlers/BlobHandler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,39 +122,39 @@ export default class BlobHandler extends BaseHandler implements IBlobHandler {

const response: Models.BlobGetPropertiesResponse = againstMetadata
? {
statusCode: 200,
metadata: res.metadata,
eTag: res.properties.etag,
requestId: context.contextId,
version: BLOB_API_VERSION,
date: context.startTime,
clientRequestId: options.requestId,
contentLength: res.properties.contentLength,
lastModified: res.properties.lastModified
}
statusCode: 200,
metadata: res.metadata,
eTag: res.properties.etag,
requestId: context.contextId,
version: BLOB_API_VERSION,
date: context.startTime,
clientRequestId: options.requestId,
contentLength: res.properties.contentLength,
lastModified: res.properties.lastModified
}
: {
statusCode: 200,
metadata: res.metadata,
isIncrementalCopy: res.properties.incrementalCopy,
eTag: res.properties.etag,
requestId: context.contextId,
version: BLOB_API_VERSION,
date: context.startTime,
acceptRanges: "bytes",
blobCommittedBlockCount:
res.properties.blobType === Models.BlobType.AppendBlob
? res.blobCommittedBlockCount
: undefined,
isServerEncrypted: true,
clientRequestId: options.requestId,
...res.properties,
cacheControl: context.request!.getQuery("rscc") ?? res.properties.cacheControl,
contentDisposition: context.request!.getQuery("rscd") ?? res.properties.contentDisposition,
contentEncoding: context.request!.getQuery("rsce") ?? res.properties.contentEncoding,
contentLanguage: context.request!.getQuery("rscl") ?? res.properties.contentLanguage,
contentType: context.request!.getQuery("rsct") ?? res.properties.contentType,
tagCount: res.properties.tagCount,
};
statusCode: 200,
metadata: res.metadata,
isIncrementalCopy: res.properties.incrementalCopy,
eTag: res.properties.etag,
requestId: context.contextId,
version: BLOB_API_VERSION,
date: context.startTime,
acceptRanges: "bytes",
blobCommittedBlockCount:
res.properties.blobType === Models.BlobType.AppendBlob
? res.blobCommittedBlockCount
: undefined,
isServerEncrypted: true,
clientRequestId: options.requestId,
...res.properties,
cacheControl: context.request!.getQuery("rscc") ?? res.properties.cacheControl,
contentDisposition: context.request!.getQuery("rscd") ?? res.properties.contentDisposition,
contentEncoding: context.request!.getQuery("rsce") ?? res.properties.contentEncoding,
contentLanguage: context.request!.getQuery("rscl") ?? res.properties.contentLanguage,
contentType: context.request!.getQuery("rsct") ?? res.properties.contentType,
tagCount: res.properties.tagCount,
};

return response;
}
Expand Down Expand Up @@ -329,12 +329,10 @@ export default class BlobHandler extends BaseHandler implements IBlobHandler {
const metadata = convertRawHeadersToMetadata(
blobCtx.request!.getRawHeaders()
);

if (metadata != undefined)
{

if (metadata != undefined) {
Object.entries(metadata).forEach(([key, value]) => {
if (key.includes("-"))
{
if (key.includes("-")) {
throw StorageErrorFactory.getInvalidMetadata(context.contextId!);
}
});
Expand Down Expand Up @@ -664,7 +662,8 @@ export default class BlobHandler extends BaseHandler implements IBlobHandler {
throw StorageErrorFactory.getBlobNotFound(context.contextId!);
}

if (sourceAccount !== blobCtx.account) {
const sig = url.searchParams.get("sig");
if ((sourceAccount !== blobCtx.account) || (sig !== null)) {
await this.validateCopySource(copySource, sourceAccount, context);
}

Expand Down Expand Up @@ -1103,7 +1102,7 @@ export default class BlobHandler extends BaseHandler implements IBlobHandler {
tagCount: getBlobTagsCount(blob.blobTags),
isServerEncrypted: true,
clientRequestId: options.requestId,
creationTime:blob.properties.creationTime,
creationTime: blob.properties.creationTime,
blobCommittedBlockCount:
blob.properties.blobType === Models.BlobType.AppendBlob
? (blob.committedBlocksInOrder || []).length
Expand Down Expand Up @@ -1176,9 +1175,9 @@ export default class BlobHandler extends BaseHandler implements IBlobHandler {
contentLength <= 0
? []
: this.rangesManager.fillZeroRanges(blob.pageRangesInOrder, {
start: rangeStart,
end: rangeEnd
});
start: rangeStart,
end: rangeEnd
});

const bodyGetter = async () => {
return this.extentStore.readExtents(
Expand Down Expand Up @@ -1233,7 +1232,7 @@ export default class BlobHandler extends BaseHandler implements IBlobHandler {
blobContentMD5: blob.properties.contentMD5,
tagCount: getBlobTagsCount(blob.blobTags),
isServerEncrypted: true,
creationTime:blob.properties.creationTime,
creationTime: blob.properties.creationTime,
clientRequestId: options.requestId
};

Expand All @@ -1243,7 +1242,7 @@ export default class BlobHandler extends BaseHandler implements IBlobHandler {
public async query(
options: Models.BlobQueryOptionalParams,
context: Context
): Promise<Models.BlobQueryResponse>{
): Promise<Models.BlobQueryResponse> {
throw new NotImplementedError(context.contextId);
}

Expand All @@ -1266,13 +1265,13 @@ export default class BlobHandler extends BaseHandler implements IBlobHandler {
);

const response: Models.BlobGetTagsResponse = {
statusCode: 200,
blobTagSet: tags === undefined ? []: tags.blobTagSet,
requestId: context.contextId,
version: BLOB_API_VERSION,
date: context.startTime,
clientRequestId: options.requestId,
};
statusCode: 200,
blobTagSet: tags === undefined ? [] : tags.blobTagSet,
requestId: context.contextId,
version: BLOB_API_VERSION,
date: context.startTime,
clientRequestId: options.requestId,
};

return response;
}
Expand Down Expand Up @@ -1315,18 +1314,18 @@ export default class BlobHandler extends BaseHandler implements IBlobHandler {
return response;
}

private NewUriFromCopySource(copySource: string, context: Context): URL{
try{
return new URL(copySource)
}
catch
{
throw StorageErrorFactory.getInvalidHeaderValue(
context.contextId,
{
HeaderName: "x-ms-copy-source",
HeaderValue: copySource
})
}
private NewUriFromCopySource(copySource: string, context: Context): URL {
try {
return new URL(copySource)
}
catch
{
throw StorageErrorFactory.getInvalidHeaderValue(
context.contextId,
{
HeaderName: "x-ms-copy-source",
HeaderValue: copySource
})
}
}
}
44 changes: 43 additions & 1 deletion tests/blob/apis/blockblob.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import {
StorageSharedKeyCredential,
BlobServiceClient,
newPipeline
newPipeline,
BlobSASPermissions
} from "@azure/storage-blob";
import assert = require("assert");
import crypto = require("crypto");
Expand Down Expand Up @@ -548,4 +549,45 @@ describe("BlockBlobAPIs", () => {
body
);
});

it("Start copy without required permission should fail @loki @sql", async () => {
const body: string = getUniqueName("randomstring");
const expiryTime = new Date();
expiryTime.setDate(expiryTime.getDate() + 1);
await blockBlobClient.upload(body, Buffer.byteLength(body));

const sourceURLWithoutPermission = await blockBlobClient.generateSasUrl({
permissions: BlobSASPermissions.parse("w"),
expiresOn: expiryTime
});

const destBlobName: string = getUniqueName("destBlobName");
const destBlobClient = containerClient.getBlockBlobClient(destBlobName);

try {
await destBlobClient.beginCopyFromURL(sourceURLWithoutPermission);
assert.fail("Copy without required permision should fail");
}
catch (ex) {
assert.deepStrictEqual(ex.statusCode, 403);
assert.ok(ex.message.startsWith("This request is not authorized to perform this operation using this permission."));
assert.deepStrictEqual(ex.code, "CannotVerifyCopySource");
}

// Copy within the same account without SAS token should succeed.
const result = await (await destBlobClient.beginCopyFromURL(blockBlobClient.url)).pollUntilDone();
assert.ok(result.copyId);
assert.strictEqual(result.errorCode, undefined);

// Copy with 'r' permission should succeed.
const sourceURL = await blockBlobClient.generateSasUrl({
permissions: BlobSASPermissions.parse("r"),
expiresOn: expiryTime
});

const resultWithPermission = await (await destBlobClient.beginCopyFromURL(sourceURL)).pollUntilDone();
assert.ok(resultWithPermission.copyId);
assert.strictEqual(resultWithPermission.errorCode, undefined);
});

});
5 changes: 3 additions & 2 deletions tests/blob/sas.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,15 +655,16 @@ describe("Shared Access Signature (SAS) authentication", () => {
);

const containerName = getUniqueName("con");
const containerClient = serviceClientWithSAS.getContainerClient(
const containerClient = serviceClient.getContainerClient(containerName);
const containerClientWithSAS = serviceClientWithSAS.getContainerClient(
containerName
);
await containerClient.create();

const blobName1 = getUniqueName("blob");
const blobName2 = getUniqueName("blob");
const blob1 = containerClient.getBlockBlobClient(blobName1);
const blob2 = containerClient.getBlockBlobClient(blobName2);
const blob2 = containerClientWithSAS.getBlockBlobClient(blobName2);

await blob1.upload("hello", 5);

Expand Down

0 comments on commit 3f64420

Please sign in to comment.