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.

Sign up for GitHub

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 issue of copy succeeded without 'r' permission in SAS token credential #2330

Merged
merged 1 commit into from
Dec 26, 2023
Merged

Fix issue of copy succeeded without 'r' permission in SAS token credential #2330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions ChangeLog.md
View file
Open in desktop
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
Open in desktop
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)) {
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, this is the only product by propose.
There are so many coding format changes, and not easy to get all real product code change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is the only place I changed for product code. Other are all changed by prettier tool .

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
Open in desktop
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
Open in desktop
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
Loading