fix(ses-actions): condition should use SourceAccount by Lilja · Pull Request #34081 · aws/aws-cdk

Lilja · 2025-04-09T09:01:11Z

Issue

Reason for this change

Restricts the SES to come through the SourceAccount in question.

This will change does not affect bucket policy and ses rule action race condition, reported in #30143 and introduced in #29833 and reverted in #30375. That PR introduced the rule set name into the bucket policy, which added a dependency to the policy to the rule set(while the rule set requires that the policy is created first). Doing this change made a circular dependency between the two resources.

Description of changes

Simply use SourceAccount instead of Referer.

Describe any new or updated permissions being added

n/a

Description of how you validated changes

n/a

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

aws-cdk-automation

(This review is outdated)

codecov · 2025-04-09T09:10:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.00%. Comparing base (74cbe27) to head (445af73).
Report is 34 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #34081      +/-   ##
==========================================
+ Coverage   83.98%   84.00%   +0.01%     
==========================================
  Files         120      121       +1     
  Lines        6976     6984       +8     
  Branches     1178     1179       +1     
==========================================
+ Hits         5859     5867       +8     
  Misses       1005     1005              
  Partials      112      112

Flag	Coverage Δ
suite.unit	`84.00% <ø> (+0.01%)`	⬆️

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

Components	Coverage Δ
packages/aws-cdk	`∅ <ø> (∅)`
packages/aws-cdk-lib/core	`84.00% <ø> (+0.01%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Lilja-at-funnel · 2025-04-14T07:23:23Z

Clarification Request please update your assessment

QuantumNeuralCoder · 2025-04-15T20:37:10Z

packages/aws-cdk-lib/aws-ses-actions/lib/s3.ts

      conditions: {
        StringEquals: {
-          'aws:Referer': cdk.Aws.ACCOUNT_ID,
+          'aws:SourceAccount': cdk.Aws.ACCOUNT_ID,


Breaking change!

QuantumNeuralCoder · 2025-04-15T20:37:21Z

packages/aws-cdk-lib/aws-ses-actions/test/actions.test.ts

          Condition: {
            StringEquals: {
-              'aws:Referer': {
+              'aws:SourceAccount': {


Breaking change!

QuantumNeuralCoder · 2025-04-15T20:39:00Z

...-integ/test/aws-ses-actions/test/integ.actions.js.snapshot/aws-cdk-ses-receipt.template.json

       "Condition": {
        "StringEquals": {
-         "aws:Referer": {
+         "aws:SourceAccount": {


Breaking change! Can you please help us understand this? is the Referer deprecated now?
We do not allow breaking changes in stable modules unless its a changed necessitated by the underlying service.

I would argue that the existing Referer header is a bug and it should use the SourceAccount property instead. We have an ongoing confused deputy problem because of this and I'm sure the correct behaviour would be to use the SourceAccount property.

Also, if you take a look at the changes made in https://github.com/aws/aws-cdk/pull/29833/files#diff-824a0448a198a71c623bc8daaae829a1cd482701129ebd395fca0ee665e96ffaR102-R112, it was merged without any consideration that it would be a breaking change.

Makes sense. Can you add a section to the README with a couple of lines explaining this.

Restricts the SES to come through the SourceAccount in question. This will change does not affect bucket policy and ses rule action race condition, reported in aws#30143 and introduced in aws#29833 and reverted in aws#30375. That PR introduced the rule set name into the bucket policy, which added a dependency to the policy to the rule set(while the rule set requires that the policy is created first). Doing this change made a circular dependency between the two resources. This change solves aws#29811

QuantumNeuralCoder

LGTM. Ship it.

QuantumNeuralCoder · 2025-05-12T18:38:56Z

@Mergifyio requeue

mergify · 2025-05-12T18:39:03Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

Lilja · 2025-05-15T11:16:08Z

Hi @paulhcsun, any reason for this pull request not to be merged? (Judging by the label assigned to this PR)

samson-keung · 2025-05-16T22:02:37Z

Hi @paulhcsun, any reason for this pull request not to be merged? (Judging by the label assigned to this PR)

Hi @Lilja, it was because we wanted to evaluate if this change will break existing CDK applications. After some research, I guess at some point in the past, some SES example code out there uses aws:Referer (e.g. I found aws/serverless-application-model#1037 mentioning that). From my testing, both aws:Referer and aws:SourceAccount will work, meaning SES is able to put objects into the bucket. It seems to be me the value for these 2 keys are always the same. This leads me to the following question:

You mentioned that you are experiencing the confused deputy problem and this change will fix it. Do you mind explaining further in what scenario you believe the confused deputy problem will occur? I suspect we may need to look into adding the aws: SourceArn to solve your problem completely.
[Note: I wanted to ask to make sure your problem is solved. This is not a blocker to merging this PR and I will ensure we merge this PR while I look forward to your response]

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

mergify · 2025-05-20T23:02:51Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2025-05-21T00:12:47Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing

To prevent automatic closure:

Resume work on the PR
OR request an exemption by adding a comment containing 'Exemption Request' with justification e.x "Exemption Request: "
OR request clarification by adding a comment containing 'Clarification Request' with a question e.x "Clarification Request: "

This PR will automatically close in 14 days if no action is taken.

aws-cdk-automation · 2025-05-21T18:13:15Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 0cb5d7c
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2025-05-21T18:13:21Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2025-05-21T18:13:31Z

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.

aws-cdk-automation requested a review from a team April 9, 2025 09:01

github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Apr 9, 2025

aws-cdk-automation previously requested changes Apr 9, 2025

View reviewed changes

Lilja force-pushed the fix/permissions-to-wide-29811 branch from f50a82b to 8d4cbdb Compare April 9, 2025 11:38

Lilja mentioned this pull request Apr 9, 2025

aws_ses: SES ReceiptRuleSet S3 action grants too wide permissions #29811

Open

aws-cdk-automation added pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Apr 14, 2025

shikha372 assigned QuantumNeuralCoder Apr 15, 2025

QuantumNeuralCoder reviewed Apr 15, 2025

View reviewed changes

QuantumNeuralCoder suggested changes Apr 15, 2025

View reviewed changes

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

shikha372 assigned paulhcsun Apr 28, 2025

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

Monthly PR metrics report #34324

Closed

Lilja force-pushed the fix/permissions-to-wide-29811 branch from 445af73 to c843674 Compare May 7, 2025 11:11

QuantumNeuralCoder approved these changes May 9, 2025

View reviewed changes

QuantumNeuralCoder added 2 commits May 9, 2025 15:27

Merge branch 'main' into fix/permissions-to-wide-29811

d72cf0a

Merge branch 'main' into fix/permissions-to-wide-29811

d10d00f

QuantumNeuralCoder approved these changes May 12, 2025

View reviewed changes

Merge branch 'main' into fix/permissions-to-wide-29811

f089155

paulhcsun added the pr/do-not-merge This PR should not be merged at this time. label May 13, 2025

samson-keung removed the pr/do-not-merge This PR should not be merged at this time. label May 16, 2025

Merge branch 'main' into fix/permissions-to-wide-29811

a8b24ca

samson-keung added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run labels May 20, 2025

mergify bot added 2 commits May 21, 2025 15:13

Merge branch 'main' into fix/permissions-to-wide-29811

82f60aa

Merge branch 'main' into fix/permissions-to-wide-29811

0cb5d7c

mergify bot merged commit f68b947 into aws:main May 21, 2025
15 of 16 checks passed

github-actions bot locked as resolved and limited conversation to collaborators May 21, 2025

aws unlocked this conversation May 21, 2025

Lilja deleted the fix/permissions-to-wide-29811 branch May 22, 2025 13:28

Conversation

Lilja commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Lilja-at-funnel commented Apr 14, 2025

Uh oh!

QuantumNeuralCoder Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

QuantumNeuralCoder Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

QuantumNeuralCoder Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

Lilja Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

QuantumNeuralCoder May 5, 2025

Choose a reason for hiding this comment

Uh oh!

QuantumNeuralCoder left a comment

Choose a reason for hiding this comment

Uh oh!

QuantumNeuralCoder commented May 12, 2025

Uh oh!

mergify bot commented May 12, 2025

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

Uh oh!

Lilja commented May 15, 2025

Uh oh!

samson-keung commented May 16, 2025

Uh oh!

mergify bot commented May 20, 2025

Uh oh!

aws-cdk-automation commented May 21, 2025

Uh oh!

aws-cdk-automation commented May 21, 2025

AWS CodeBuild CI Report

Uh oh!

Uh oh!

mergify bot commented May 21, 2025

Uh oh!

github-actions bot commented May 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Lilja commented Apr 9, 2025 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

codecov bot commented Apr 9, 2025 •

edited

Loading