fix: validate simulator bucket configuration - #152
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the B2Simulator’s fidelity to the real B2 API by enforcing bucket-configuration validation during bucket create/update, honoring listBuckets request filters, and enforcing optimistic-concurrency revision guards on update.
Changes:
- Added simulator-side validation for
corsRules,lifecycleRules,replicationConfiguration, anddefaultRetention. - Updated simulator bucket endpoints to run the new validators and to honor
listBucketsfilters (bucketId,bucketName,bucketTypes). - Added/expanded unit + fidelity tests to cover the new validation and revision-conflict behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/simulator/validation.ts | Adds new bucket-configuration validators (CORS, lifecycle, replication, retention) and shared helper validators. |
| src/simulator/validation.test.ts | Adds focused unit tests for the new validation helpers. |
| src/simulator/index.ts | Enforces validation during create/update; implements listBuckets filter behavior; adds updateBucket revision guard conflict behavior. |
| src/simulator/fidelity.test.ts | Adds integration-style simulator fidelity tests for bucket config validation, listBuckets filtering, and revision guard conflicts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/simulator/index.ts:2936
b2_list_bucketssupports the special filterbucketTypes: ["all"]to mean “no type filtering”. Even ifvalidateBucketTypesallows it, the current.includes(bucket.bucketType)filter will return an empty list for["all"]. Consider normalizing["all"]to an undefined filter before applying the predicate.
.filter((bucket) => req.bucketId === undefined || bucket.bucketId === req.bucketId)
.filter((bucket) => req.bucketName === undefined || bucket.bucketName === req.bucketName)
.filter(
(bucket) => req.bucketTypes === undefined || req.bucketTypes.includes(bucket.bucketType),
)
src/simulator/validation.ts:449
validateBucketTypesis missing documentedb2_list_bucketssemantics: Backblaze treats an emptybucketTypesarray as a bad request, and allows the special value["all"](but rejects using "all" alongside other types). The current implementation accepts empty arrays and rejects "all", so the simulator can diverge from real B2 behavior.
export function validateBucketTypes(bucketTypes: unknown): ValidationError | null {
if (bucketTypes === undefined) return null
if (!Array.isArray(bucketTypes)) {
return { code: 'bad_request', message: 'bucketTypes must be an array' }
}
src/types/bucket.ts:38
- Making
LifecycleRule.daysFromHidingToDeleting/daysFromUploadingToHidingoptional changes their read-side type to includeundefined, which is a potentially breaking public API change for SDK consumers. If this is intentional (to match real B2 responses/requests), it should be called out in the release notes; if not, consider normalizing rules to always include explicitnullfields (or introducing a separate “input” type) to avoid widening the response type.
/** Rule that automatically hides or deletes files after a specified number of days. */
export interface LifecycleRule {
/** Days after hiding before automatic deletion, or null to never auto-delete hidden files. */
readonly daysFromHidingToDeleting?: number | null
/** Days after upload before automatic hiding, or null to never auto-hide. */
readonly daysFromUploadingToHiding?: number | null
/** Days after starting before automatic cancellation of unfinished large files, or null to never auto-cancel. */
readonly daysFromStartingToCancelingUnfinishedLargeFiles?: number | null
src/simulator/index.ts:1350
- When
requiresAccountLevelBucketAccesstriggers, the error currently usesbucketScopeRequiredMessage()("bucket scope is required"), but the actual constraint is that bucket-scoped keys are not allowed for this operation. This message is misleading for callers trying to understand why bucket creation is denied.
This issue also appears on line 2932 of the same file.
if (grant.bucketIds !== null) {
if (scope?.requiresAccountLevelBucketAccess === true) {
return this.error(403, 'unauthorized', grant.bucketScopeRequiredMessage())
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/simulator/index.ts:475
file_lock_not_enabledis returned with a different message here than inrequireFileLockEnabled()(which uses "Bucket does not have file lock enabled"). Keeping the error message consistent for the same error code makes simulator behavior easier to assert against and reduces confusion for SDK consumers.
if (
requestRecord(fields.defaultRetention)?.['mode'] !== BucketRetentionMode.None &&
!options.objectLockEnabled
) {
return {
code: 'file_lock_not_enabled',
message: 'Bucket must have Object Lock enabled to set defaultRetention',
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/simulator/index.ts:456
- validateBucketConfigurationFields passes bucketInfo straight into validateBucketInfo without checking that it’s a plain object. If a caller sends bucketInfo: null (or any non-object), Object.entries() in validateBucketInfo will throw (or non-objects may be coerced), causing the simulator to crash instead of returning a structured 400 error.
if (fields.bucketInfo !== undefined) {
const infoError = validateBucketInfo(fields.bucketInfo as Record<string, string>)
if (infoError) return infoError
}
src/simulator/index.ts:1581
- Optional chaining in destination bucket replication lookup doesn’t protect the indexed access. If asReplicationDestination is null/undefined,
...?.sourceToDestinationKeyMapping[sourceApplicationKeyId]will still attempt[sourceApplicationKeyId]on undefined and throw. Use optional chaining on the index access (or split into a temporary mapping variable).
const destinationBucket = this.buckets.get(destinationBucketId)
const destinationApplicationKeyId =
destinationBucket?.info.replicationConfiguration.asReplicationDestination
?.sourceToDestinationKeyMapping[sourceApplicationKeyId]
if (destinationApplicationKeyId === undefined) {
Quality Keeper — Advisory ReviewSource:
Quality Keeper assessed the repository, executed checks, and collected results, covering 14 quality expectations. Result
CoveredQuality Keeper found acceptable evidence for one or more expectations in these Testing Types.
Needs attentionQuality Keeper does not yet have acceptable evidence for these Testing Types. Some have partial checks that need to run, be fixed, or be connected. Others need new proof or seeded tests. In both cases, Quality Keeper cannot count them as covered yet.
Quality Keeper changes no Evidence State. Pending, missing, executed-but-not-exercised, and review items are not passing, and seeded scaffolds are next actions, not evidence, until applied and re-evaluated. Full breakdown (14 expectations)
New to these terms? Start with the Quality Keeper Standards Wiki to learn what each testing type means, why it applies, and what counts as evidence. |
Summary
Linked issue
Closes #22
Tests
Follow-up notes