diff --git a/ChangeLog.md b/ChangeLog.md index 900ec37a2..2a34fb168 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -27,6 +27,7 @@ Blob: - Fixed service- and container-level Filter Blobs requests failing when the optional `where` query parameter is omitted. - Fixed blob operations hanging when a client disconnects before the operation queue processes the request. (issue #2575) - Implement `PutBlobFromUrl` (`Put Blob From URL`), which previously returned 501. The source is fetched over loopback, as `PutBlockFromURL` already does, so that SAS authentication and the `x-ms-source-if-*` conditions are enforced by the existing download path. Standard blob properties are copied from the source unless `x-ms-copy-source-blob-properties` is false, request blob content headers override them either way, request metadata replaces the source's rather than adding to it, and `x-ms-copy-source-tag-option: COPY` reads the source's tags over that same authorized path. An `x-ms-source-content-md5`, `x-ms-blob-content-md5`, `Content-MD5`, or `x-ms-content-crc64` header is checked against the copied content, and the response reports the MD5 and CRC64 of that content. A SAS needs Create or Write to create the blob, Write to overwrite it, and Tag as well when the request sets tags with `x-ms-tags` or copies the source's. As with `CopyBlobFromURL`, only sources on the same Azurite instance are supported. +- Fixed `Acquire Lease` accepting a request without `x-ms-lease-duration`. The lease was stored with an invalid expiry, and every later request for the blob or container returned `500`. Acquire now returns `400 MissingRequiredHeader`, and the lease state builders reject a duration that is missing or not an integer before any lease state is persisted. (issue #2855) Queue: diff --git a/src/blob/errors/StorageErrorFactory.ts b/src/blob/errors/StorageErrorFactory.ts index 71ac5013b..a5e3b7807 100644 --- a/src/blob/errors/StorageErrorFactory.ts +++ b/src/blob/errors/StorageErrorFactory.ts @@ -282,6 +282,27 @@ export default class StorageErrorFactory { ); } + public static getMissingRequiredHeader( + contextID: string = DefaultID, + headerName?: string + ): StorageError { + const additionalMessages: { + [key: string]: string; + } = {}; + + if (headerName) { + additionalMessages.HeaderName = headerName; + } + + return new StorageError( + 400, + "MissingRequiredHeader", + "A required HTTP header wasn't specified for the request.", + contextID, + additionalMessages + ); + } + public static getInvalidLeaseBreakPeriod( contextID: string = DefaultID ): StorageError { diff --git a/src/blob/handlers/BlobHandler.ts b/src/blob/handlers/BlobHandler.ts index acbbe5c23..f42d1d05a 100644 --- a/src/blob/handlers/BlobHandler.ts +++ b/src/blob/handlers/BlobHandler.ts @@ -398,6 +398,15 @@ export default class BlobHandler extends BaseHandler implements IBlobHandler { ): Promise { validateProposedLeaseId(options.proposedLeaseId, context.contextId); + // x-ms-lease-duration is mandatory for acquire; without it the lease would + // be persisted with no usable expiry and every later request would fail. + if (options.duration === undefined) { + throw StorageErrorFactory.getMissingRequiredHeader( + context.contextId, + HeaderConstants.X_MS_LEASE_DURATION + ); + } + const blobCtx = new BlobStorageContext(context); const account = blobCtx.account!; const container = blobCtx.container!; diff --git a/src/blob/handlers/ContainerHandler.ts b/src/blob/handlers/ContainerHandler.ts index 48f699304..6b2bd43c9 100644 --- a/src/blob/handlers/ContainerHandler.ts +++ b/src/blob/handlers/ContainerHandler.ts @@ -6,16 +6,18 @@ import IExtentStore from "../../common/persistence/IExtentStore"; import { convertRawHeadersToMetadata, newEtag } from "../../common/utils/utils"; import BlobStorageContext from "../context/BlobStorageContext"; import NotImplementedError from "../errors/NotImplementedError"; +import StorageErrorFactory from "../errors/StorageErrorFactory"; import * as Models from "../generated/artifacts/models"; import Context from "../generated/Context"; import IContainerHandler from "../generated/handlers/IContainerHandler"; import IBlobMetadataStore from "../persistence/IBlobMetadataStore"; import { BLOB_API_VERSION, + DEFAULT_LIST_BLOBS_MAX_RESULTS, EMULATOR_ACCOUNT_KIND, - EMULATOR_ACCOUNT_SKUNAME + EMULATOR_ACCOUNT_SKUNAME, + HeaderConstants } from "../utils/constants"; -import { DEFAULT_LIST_BLOBS_MAX_RESULTS } from "../utils/constants"; import { getBlobTagsCount, removeQuotationFromListBlobEtag, @@ -431,6 +433,14 @@ export default class ContainerHandler extends BaseHandler ): Promise { validateProposedLeaseId(options.proposedLeaseId, context.contextId); + // x-ms-lease-duration is mandatory for acquire, the same as for blobs. + if (options.duration === undefined) { + throw StorageErrorFactory.getMissingRequiredHeader( + context.contextId, + HeaderConstants.X_MS_LEASE_DURATION + ); + } + const blobCtx = new BlobStorageContext(context); const accountName = blobCtx.account!; const containerName = blobCtx.container!; diff --git a/src/blob/lease/LeaseAvailableState.ts b/src/blob/lease/LeaseAvailableState.ts index 7943ea7f1..eec22c7ff 100644 --- a/src/blob/lease/LeaseAvailableState.ts +++ b/src/blob/lease/LeaseAvailableState.ts @@ -97,9 +97,7 @@ export default class LeaseAvailableState extends LeaseStateBase { } public acquire(duration: number, proposedLeaseId: string = ""): ILeaseState { - if ((duration < 15 || duration > 60) && duration !== -1) { - throw StorageErrorFactory.getInvalidLeaseDuration(this.context.contextId); - } + this.validateLeaseDuration(duration); // TODO: Validate proposedLeaseId follows GUID format diff --git a/src/blob/lease/LeaseBrokenState.ts b/src/blob/lease/LeaseBrokenState.ts index 5dd748164..8a79db5e0 100644 --- a/src/blob/lease/LeaseBrokenState.ts +++ b/src/blob/lease/LeaseBrokenState.ts @@ -137,9 +137,7 @@ export default class LeaseBrokenState extends LeaseStateBase { } public acquire(duration: number, proposedLeaseId: string = ""): ILeaseState { - if ((duration < 15 || duration > 60) && duration !== -1) { - throw StorageErrorFactory.getInvalidLeaseDuration(this.context.contextId); - } + this.validateLeaseDuration(duration); // TODO: Validate proposedLeaseId follows GUID format diff --git a/src/blob/lease/LeaseExpiredState.ts b/src/blob/lease/LeaseExpiredState.ts index 5baa90fc0..8c7642c99 100644 --- a/src/blob/lease/LeaseExpiredState.ts +++ b/src/blob/lease/LeaseExpiredState.ts @@ -144,9 +144,7 @@ export default class LeaseExpiredState extends LeaseStateBase { } public acquire(duration: number, proposedLeaseId: string = ""): ILeaseState { - if ((duration < 15 || duration > 60) && duration !== -1) { - throw StorageErrorFactory.getInvalidLeaseDuration(this.context.contextId); - } + this.validateLeaseDuration(duration); // TODO: Validate proposedLeaseId follows GUID format diff --git a/src/blob/lease/LeaseLeasedState.ts b/src/blob/lease/LeaseLeasedState.ts index 242e592e8..aacef4ea6 100644 --- a/src/blob/lease/LeaseLeasedState.ts +++ b/src/blob/lease/LeaseLeasedState.ts @@ -91,9 +91,7 @@ export default class LeaseLeasedState extends LeaseStateBase { throw StorageErrorFactory.getLeaseAlreadyPresent(this.context.contextId); } - if ((duration < 15 || duration > 60) && duration !== -1) { - throw StorageErrorFactory.getInvalidLeaseDuration(this.context.contextId); - } + this.validateLeaseDuration(duration); // TODO: Validate proposedLeaseId follows GUID format diff --git a/src/blob/lease/LeaseStateBase.ts b/src/blob/lease/LeaseStateBase.ts index 6f92ae40a..f1ebd0e46 100644 --- a/src/blob/lease/LeaseStateBase.ts +++ b/src/blob/lease/LeaseStateBase.ts @@ -1,3 +1,4 @@ +import StorageErrorFactory from "../errors/StorageErrorFactory"; import Context from "../generated/Context"; import { ILease, @@ -19,6 +20,21 @@ export default abstract class LeaseStateBase implements ILeaseState { public abstract renew(leaseId: string): ILeaseState; public abstract change(leaseId: string, proposedLeaseId: string): ILeaseState; public abstract release(leaseId: string): ILeaseState; + /** + * Azure accepts a fixed lease duration between 15 and 60 seconds, or -1 for + * an infinite lease. A missing x-ms-lease-duration parses as undefined and + * used to slip through the comparison below, persisting a lease whose expiry + * time was invalid and making every later request fail. + */ + protected validateLeaseDuration(duration: number): void { + if ( + !Number.isInteger(duration) || + ((duration < 15 || duration > 60) && duration !== -1) + ) { + throw StorageErrorFactory.getInvalidLeaseDuration(this.context.contextId); + } + } + public sync(syncer: ILeaseSyncer): T { return syncer.sync(this.lease); } diff --git a/src/blob/utils/constants.ts b/src/blob/utils/constants.ts index 12d0ab34c..77f6d254e 100644 --- a/src/blob/utils/constants.ts +++ b/src/blob/utils/constants.ts @@ -67,6 +67,7 @@ export const HeaderConstants = { X_MS_ENCRYPTION_KEY: "x-ms-encryption-key", X_MS_ENCRYPTION_KEY_SHA256: "x-ms-encryption-key-sha256", X_MS_ENCRYPTION_ALGORITHM: "x-ms-encryption-algorithm", + X_MS_LEASE_DURATION: "x-ms-lease-duration", PREFIX_FOR_STORAGE: "x-ms-", RANGE: "Range", USER_AGENT: "User-Agent", diff --git a/tests/blob/apis/blob.test.ts b/tests/blob/apis/blob.test.ts index 902fc7503..1e7568862 100644 --- a/tests/blob/apis/blob.test.ts +++ b/tests/blob/apis/blob.test.ts @@ -887,6 +887,36 @@ describe("BlobAPIs", () => { } }); + it("acquireLease_without_duration_is_rejected_and_leaves_the_blob_usable @loki @sql", async () => { + // x-ms-lease-duration is mandatory for acquire; the SDK omits the header + // when the duration is undefined. + let err; + try { + await blobLeaseClient.acquireLease(undefined as any); + } catch (error) { + err = error; + } + assert.deepStrictEqual((err as any).statusCode, 400); + assert.deepStrictEqual((err as any).code, "MissingRequiredHeader"); + + // A rejected acquire must not persist a half built lease, which used to + // make every later request against the blob fail with 500. + const properties = await blobClient.getProperties(); + assert.equal(properties.leaseStatus, "unlocked"); + assert.equal(properties.contentLength, content.length); + const downloaded = await blobClient.download(0); + assert.equal(await bodyToString(downloaded, content.length), content); + const listed = ( + await containerClient.listBlobsFlat().byPage().next() + ).value; + assert.equal(listed.segment.blobItems!.length, 1); + + // A well formed acquire still succeeds afterwards. + const acquired = await blobLeaseClient.acquireLease(15); + assert.ok(acquired.leaseId); + await blobClient.getBlobLeaseClient(acquired.leaseId!).releaseLease(); + }); + it("acquireLease_available_proposedLeaseId_fixed @loki @sql", async () => { const guid = "ca761232ed4211cebacd00aa0057b223"; const duration = 30; diff --git a/tests/blob/apis/container.test.ts b/tests/blob/apis/container.test.ts index 418102417..180335884 100644 --- a/tests/blob/apis/container.test.ts +++ b/tests/blob/apis/container.test.ts @@ -429,6 +429,30 @@ describe("ContainerAPIs", () => { } }); + it("acquireLease_without_duration_is_rejected_and_leaves_the_container_usable @loki @sql", async () => { + let err; + try { + await blobLeaseClient.acquireLease(undefined as any); + } catch (error) { + err = error; + } + assert.deepStrictEqual((err as any).statusCode, 400); + assert.deepStrictEqual((err as any).code, "MissingRequiredHeader"); + + // The container must stay usable, and a rejected acquire must not persist + // a lease with no usable expiry. + const properties = await containerClient.getProperties(); + assert.equal(properties.leaseStatus, "unlocked"); + const listed = ( + await containerClient.listBlobsFlat().byPage().next() + ).value; + assert.deepStrictEqual(listed.segment.blobItems!.length, 0); + + const acquired = await blobLeaseClient.acquireLease(15); + assert.ok(acquired.leaseId); + await containerClient.getBlobLeaseClient(acquired.leaseId!).releaseLease(); + }); + it("acquireLease_available_proposedLeaseId_fixed @loki @sql", async () => { const guid = "ca761232-ed42-11ce-bacd-00aa0057b223"; const duration = 30;