fix(s3): default BlockPublicAccess class properties to true (under feature flag) #33001

manan-pancholi · 2025-01-17T22:57:28Z

Issue # aws-s3: blockPublicAccess has a counterintuitive behaviour #32811

Reason for this change

S3 Bucket construct when initialized without specifying the blockPublicAccess property results in all the members of class BlockPublicAccess set to true. However if some properties are set to false during initialization, then all remaining properties are also set to false. Reason for this is because the unspecified properties are treated as undefined, which evaluates to false. This causes an inconsistent behavior and leads to confusion.

Description of changes

Inside the constructor of BlockPublicAccess class in aws-s3/lib/bucket.ts, we use the nullish coalescing operator ?? to check if the properties are not explicitly set by the customer. If they are not, then we set them to true. This will create a consistent default for those properties to true.

Description of how you validated changes

Added a unit test to validate the changes.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

… consistent behavior

aws-cdk-automation

The pull request linter fails with the following errors:

❌ The title of the pull request should omit 'aws-' from the name of modified packages. Use 's3' instead of 'aws-s3'.

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

lpizzinidev

Thanks 👍
A couple of notes:

I think the PR should be classified as a fix and an integration test should be created/updated to cover the scenario.
More importantly, I assume this could change the visibility of deployed buckets without users noticing. If so, a feature flag is needed.

manan-pancholi · 2025-01-21T17:21:15Z

Thanks @lpizzinidev for reviewing the change. I'll make the suggested changes and send out a revision.

…o-true

lpizzinidev

Thanks for the adjustment 👍 Left comments for minor syntax adjustments.

packages/aws-cdk-lib/cx-api/lib/features.ts

packages/aws-cdk-lib/cx-api/README.md

packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md

Thanks for the suggestion. Co-authored-by: Luca Pizzini <[email protected]>

Co-authored-by: Luca Pizzini <[email protected]>

aaythapa · 2025-01-29T22:33:24Z

packages/aws-cdk-lib/aws-s3/lib/bucket.ts

-    this.blockPublicPolicy = options.blockPublicPolicy;
-    this.ignorePublicAcls = options.ignorePublicAcls;
-    this.restrictPublicBuckets = options.restrictPublicBuckets;
+    this.blockPublicAcls = options.blockPublicAcls ?? true;


nit the docstrings for these don't have a default it looks like

/** * Whether to block public ACLs * * @default true * @see https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html#access-control-block-public-access-options */ readonly blockPublicAcls?: boolean;

@aaythapa - Could you please elaborate on your comment? I'm unable to understand the suggested change.

aws-cdk-automation · 2025-01-30T00:12:13Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

gracelu0

Thanks for working on this!

gracelu0 · 2025-01-30T00:17:47Z

packages/aws-cdk-lib/aws-s3/lib/bucket.ts

@@ -1093,10 +1093,10 @@ export class BlockPublicAccess {
  public restrictPublicBuckets: boolean | undefined;

  constructor(options: BlockPublicAccessOptions) {


These new defaults should be set depending on the feature flag, e.g.:

constructor(options: BlockPublicAccessOptions) { const defaultToTrue = FeatureFlags.of(this).isEnabled(cxapi.S3_BUCKET_DEFAULT_BLOCK_PUBLIC_ACCESS_PROPERTIES_TO_TRUE); this.blockPublicAcls = defaultToTrue ? options.blockPublicAcls ?? true : options.blockPublicAcls; this.blockPublicPolicy = defaultToTrue ? options.blockPublicPolicy ?? true : options.blockPublicPolicy; this.ignorePublicAcls = defaultToTrue ? options.ignorePublicAcls ?? true : options.ignorePublicAcls; this.restrictPublicBuckets = defaultToTrue ? options.restrictPublicBuckets ?? true : options.restrictPublicBuckets; }

Good catch, will update in the next commit.

@gracelu0 - FeatureFlags.of(this) - I could not use this because the current context, i.e. class BlockPublicAccess doesn't implement IConstruct. In my latest commit, I've used a workaround where in I declare a top level variable to represent the feature flag, modify it inside the Bucket constructor and then use its value inside BlockPublicAccess class constructor. I know it doesn't look neat, but could not think of a better way. Let me know if you have a better suggestion.

gracelu0 · 2025-01-30T01:08:35Z

packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-block-public-access.ts

+  }),
+});
+
+new IntegTest(app, 'cdk-integ-s3-bucket-block-public-access', {


could we add an assertion to this test to check that the BucketWithPartialBlockAccess sets the unspecified properties to true? From docs it seems like

PUT Bucket ACL and PUT Object ACL calls fail if the specified ACL is public. PUT Object calls fail if the request includes a public ACL. PUT Bucket calls fail if the request includes a public ACL.

codecov · 2025-01-30T01:28:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.83%. Comparing base (bc82f57) to head (25436a4).
Report is 17 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #33001   +/-   ##
=======================================
  Coverage   80.83%   80.83%           
=======================================
  Files         236      236           
  Lines       14253    14253           
  Branches     2490     2490           
=======================================
  Hits        11522    11522           
  Misses       2446     2446           
  Partials      285      285

Flag	Coverage Δ
suite.unit	`80.83% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`79.57% <ø> (ø)`
packages/aws-cdk-lib/core	`82.20% <ø> (ø)`

aws-cdk-automation · 2025-02-02T00:14:06Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2025-02-03T00:12:48Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2025-02-04T00:11:53Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2025-02-05T00:13:00Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2025-02-06T00:13:14Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Pull request has been modified.

QuantumNeuralCoder · 2025-02-07T22:17:13Z

packages/aws-cdk-lib/aws-s3/test/bucket.test.ts

@@ -970,6 +970,34 @@ describe('bucket', () => {
    });
  });

+  test('unspecified blockPublicAccess properties should default to true', () => {


The test is failing since this does not take into account the feature flag. Please refactor to something like this.
Note that you will need to create a new bucket since re-using an already created bucket in tests above will fail the test and is not a true test of this feature.

test('featureFlag @aws-cdk/aws-s3:blockPublicAccessPropertiesDefaultToTrue unspecified blockPublicAccess properties should default to true', () => { // GIVEN const app = new cdk.App({ context: { '@aws-cdk/aws-s3:blockPublicAccessPropertiesDefaultToTrue': true, }, }); // WHEN const stack = new cdk.Stack(app); new s3.Bucket(stack, 'MyBucketNewDefaults', { blockPublicAccess: new s3.BlockPublicAccess({ blockPublicPolicy: false, restrictPublicBuckets: false, }), }); // THEN Template.fromStack(stack).templateMatches({ 'Resources': { 'MyBucketNewDefaultsC1A67BCD': { 'Type': 'AWS::S3::Bucket', 'Properties': { 'PublicAccessBlockConfiguration': { 'BlockPublicAcls': true, 'BlockPublicPolicy': false, 'IgnorePublicAcls': true, 'RestrictPublicBuckets': false, }, }, 'DeletionPolicy': 'Retain', 'UpdateReplacePolicy': 'Retain', }, }, }); });

QuantumNeuralCoder · 2025-02-07T22:21:26Z

packages/aws-cdk-lib/aws-s3/lib/bucket.ts

-    this.blockPublicPolicy = options.blockPublicPolicy;
-    this.ignorePublicAcls = options.ignorePublicAcls;
-    this.restrictPublicBuckets = options.restrictPublicBuckets;
+    const defaultToTrue = s3BucketDefaultBlockPublicAccessPropertiesToTrueFeatureFlag;


This implementation has a problem as revealed when you run the test.
For a test code like below, note the order of call flow of the constructors.

new s3.BlockPublicAccess

is called before

new s3.Bucket()

This leads to defaultToTrue being false regardless of the value in the feature flag.

new s3.Bucket(stack, 'MyBucketNewDefaults', { blockPublicAccess: new s3.BlockPublicAccess({ blockPublicPolicy: false, restrictPublicBuckets: false, }), });

Thanks for reviewing and pointing this out - I need to update the implementation for feature flag, will send out an updated revision.

QuantumNeuralCoder

Please note the changes requested. Summarily, the implementation has issues and the test doesnt pass. Please take a look.

Pull request has been modified.

QuantumNeuralCoder · 2025-02-19T00:31:40Z

packages/aws-cdk-lib/cx-api/lib/features.ts

+      When the property value is not specified, then all the 4 member properties (\`blockPublicAcls\`,
+      \`ignorePublicAcls\`, \`blockPublicPolicy\` and \`restrictPublicBuckets\`) will default to \`true\`. However, in
+      cases where selected properties are explicitly set to \`false\`, the remaining properties for which no value
+      was specified will also default to \`false\`.


default to `true` I believe...

QuantumNeuralCoder · 2025-02-19T00:33:44Z

packages/aws-cdk-lib/aws-s3/test/bucket.test.ts

@@ -970,6 +970,34 @@ describe('bucket', () => {
    });
  });

+  test('unspecified blockPublicAccess properties should default to true', () => {
+    const stack = new cdk.Stack();
+    new s3.Bucket(stack, 'MyBucket', {


Can we create a new bucket please. MyBucket is already being created as a part of other tests and the test might become flaky in future.

I also dont see the feature flag being set in the test. does this work?

QuantumNeuralCoder · 2025-02-19T00:44:48Z

packages/aws-cdk-lib/aws-s3/lib/bucket.ts

@@ -2188,6 +2190,10 @@ export class Bucket extends BucketBase {
    // Enhanced CDK Analytics Telemetry
    addConstructMetadata(this, props);

+    // Set the S3_BUCKET_DEFAULT_BLOCK_PUBLIC_ACCESS_PROPERTIES_TO_TRUE feature flag
+    s3BucketDefaultBlockPublicAccessPropertiesToTrueFeatureFlag = FeatureFlags.of(this)
+      .isEnabled(cxapi.S3_BUCKET_DEFAULT_BLOCK_PUBLIC_ACCESS_PROPERTIES_TO_TRUE) ?? false;


??false is not required as the api guarantees the return value.

aws-cdk-automation · 2025-02-24T00:11:34Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Pull request has been modified.

aws-cdk-automation · 2025-02-24T18:02:07Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: c69b118
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

manan-pancholi · 2025-02-24T19:38:10Z

The proposed logic in this PR will not work with a corresponding feature flag. To access the feature flag setting, we need access to the scope, however we cannot get a scope instance inside the BlockPublicAccess class. The fix for the original issue must be implemented in a different way, and I suspect will not be as straightforward as we originally anticipated. Considering this, I am closing this request. I will create a new one if I get enough bandwidth to work on this again.

github-actions · 2025-02-24T19:38:24Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

chore(aws-s3): default BlockPublicAccess class properties to true for…

e7776ff

… consistent behavior

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Jan 17, 2025

aws-cdk-automation requested a review from a team January 17, 2025 22:57

aws-cdk-automation previously requested changes Jan 17, 2025

View reviewed changes

manan-pancholi changed the title ~~chore(aws-s3): default BlockPublicAccess class properties to true~~ chore(s3): default BlockPublicAccess class properties to true Jan 17, 2025

Merge branch 'main' into default-block-public-access-properties-to-true

ee421d3

manan-pancholi requested a review from aws-cdk-automation January 17, 2025 23:04

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 17, 2025

lpizzinidev suggested changes Jan 18, 2025

View reviewed changes

manan-pancholi added 2 commits January 21, 2025 18:11

Add integ tests

d965576

Merge branch 'aws:main' into default-block-public-access-properties-t…

c75abfc

…o-true

manan-pancholi changed the title ~~chore(s3): default BlockPublicAccess class properties to true~~ fix(s3): default BlockPublicAccess class properties to true Jan 22, 2025

manan-pancholi changed the title ~~fix(s3): default BlockPublicAccess class properties to true~~ fix(s3): default BlockPublicAccess class properties to true (under feature flag) Jan 22, 2025

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 22, 2025

manan-pancholi added 2 commits January 21, 2025 19:04

Add feature flag for the bug fix

41e1cb0

Merge branch 'main' into default-block-public-access-properties-to-true

1b61d31

manan-pancholi requested a review from lpizzinidev January 22, 2025 01:27

lpizzinidev suggested changes Jan 22, 2025

View reviewed changes

packages/aws-cdk-lib/cx-api/lib/features.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/cx-api/README.md Outdated Show resolved Hide resolved

packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md Outdated Show resolved Hide resolved

manan-pancholi and others added 4 commits January 22, 2025 10:41

Update packages/aws-cdk-lib/cx-api/lib/features.ts

74287fe

Thanks for the suggestion. Co-authored-by: Luca Pizzini <[email protected]>

Update packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md

c42e87e

Co-authored-by: Luca Pizzini <[email protected]>

Merge branch 'main' into default-block-public-access-properties-to-true

b7313a5

Update packages/aws-cdk-lib/cx-api/README.md

33d386f

Co-authored-by: Luca Pizzini <[email protected]>

lpizzinidev approved these changes Jan 23, 2025

View reviewed changes

aaythapa reviewed Jan 29, 2025

View reviewed changes

gracelu0 previously requested changes Jan 30, 2025

View reviewed changes

github-actions bot mentioned this pull request Feb 1, 2025

Monthly PR metrics report #33266

Closed

Merge branch 'main' into default-block-public-access-properties-to-true

2f735b9

Enable feature flagging based on @gracelu0's suggestion

25436a4

QuantumNeuralCoder self-assigned this Feb 6, 2025

QuantumNeuralCoder reviewed Feb 7, 2025

View reviewed changes

QuantumNeuralCoder previously requested changes Feb 7, 2025

View reviewed changes

Merge branch 'main' into default-block-public-access-properties-to-true

f170fd5

QuantumNeuralCoder mentioned this pull request Feb 11, 2025

fix(lambda): updating addToRolePolicy to avoid circular dependency potential (under feature flag) #33291

Merged

1 task

Merge branch 'main' into default-block-public-access-properties-to-true

7e62563

QuantumNeuralCoder previously requested changes Feb 19, 2025

View reviewed changes

Merge branch 'main' into default-block-public-access-properties-to-true

c69b118

manan-pancholi closed this Feb 24, 2025

manan-pancholi deleted the default-block-public-access-properties-to-true branch February 24, 2025 19:38

github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(s3): default BlockPublicAccess class properties to true (under feature flag) #33001

fix(s3): default BlockPublicAccess class properties to true (under feature flag) #33001

manan-pancholi commented Jan 17, 2025

aws-cdk-automation left a comment

lpizzinidev left a comment

manan-pancholi commented Jan 21, 2025

lpizzinidev left a comment

aaythapa Jan 29, 2025

manan-pancholi Feb 6, 2025

aws-cdk-automation commented Jan 30, 2025

gracelu0 left a comment

gracelu0 Jan 30, 2025

manan-pancholi Feb 6, 2025

manan-pancholi Feb 6, 2025

gracelu0 Jan 30, 2025

codecov bot commented Jan 30, 2025 •

edited

Loading

aws-cdk-automation commented Feb 2, 2025

aws-cdk-automation commented Feb 3, 2025

aws-cdk-automation commented Feb 4, 2025

aws-cdk-automation commented Feb 5, 2025

aws-cdk-automation commented Feb 6, 2025

QuantumNeuralCoder Feb 7, 2025

QuantumNeuralCoder Feb 7, 2025

manan-pancholi Feb 9, 2025

QuantumNeuralCoder left a comment

QuantumNeuralCoder Feb 19, 2025

QuantumNeuralCoder Feb 19, 2025

QuantumNeuralCoder Feb 19, 2025

QuantumNeuralCoder Feb 19, 2025

aws-cdk-automation commented Feb 24, 2025

aws-cdk-automation commented Feb 24, 2025

manan-pancholi commented Feb 24, 2025

github-actions bot commented Feb 24, 2025

		@@ -1093,10 +1093,10 @@ export class BlockPublicAccess {
		public restrictPublicBuckets: boolean \| undefined;

		constructor(options: BlockPublicAccessOptions) {

fix(s3): default BlockPublicAccess class properties to true (under feature flag) #33001

fix(s3): default BlockPublicAccess class properties to true (under feature flag) #33001

Conversation

manan-pancholi commented Jan 17, 2025

Issue # aws-s3: blockPublicAccess has a counterintuitive behaviour #32811

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

manan-pancholi commented Jan 21, 2025

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 30, 2025

gracelu0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2025 • edited Loading

Codecov Report

aws-cdk-automation commented Feb 2, 2025

aws-cdk-automation commented Feb 3, 2025

aws-cdk-automation commented Feb 4, 2025

aws-cdk-automation commented Feb 5, 2025

aws-cdk-automation commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuantumNeuralCoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 24, 2025

aws-cdk-automation commented Feb 24, 2025

AWS CodeBuild CI Report

manan-pancholi commented Feb 24, 2025

github-actions bot commented Feb 24, 2025

codecov bot commented Jan 30, 2025 •

edited

Loading