Skip to content

fix(query): fix fp for s3_bucket_logging_disabled#7559

Merged
cx-artur-ribeiro merged 5 commits into
Checkmarx:masterfrom
cx-andre-pereira:AST-94574--FP_S3_Bucket_Logging_Disabled--terraform/aws
Jul 17, 2025
Merged

fix(query): fix fp for s3_bucket_logging_disabled#7559
cx-artur-ribeiro merged 5 commits into
Checkmarx:masterfrom
cx-andre-pereira:AST-94574--FP_S3_Bucket_Logging_Disabled--terraform/aws

Conversation

@cx-andre-pereira

@cx-andre-pereira cx-andre-pereira commented Jul 16, 2025

Copy link
Copy Markdown
Contributor

Closes #7515

Reason for Proposed Changes

  • This query must guarantee bucket logging is enabled, however it does not currently account for buckets which are themselves logging buckets in the first place.
  • Given this, logging buckets are getting wrongfully flagged. Many false positives might result from this lack of case handling, moreover enforcing this condition could easily result in recursive logging loops.

Proposed Changes

  • I have implemented a simple is_logging_target policy that ensures buckets that are referenced as logging targets for another bucket do not get flagged.
  • Test "negative4" has been added to exemplify that logging buckets will not be flagged.

Note : There could be a need for a similar implementation on "module" type bucket declaration (negative2 / positive2), but i could not replicate this false positive detection with that declaration structure.

originalPR

I submit this contribution under the Apache-2.0 license.

@cx-andre-pereira cx-andre-pereira requested a review from a team as a code owner July 16, 2025 10:59
@github-actions github-actions Bot added community Community contribution query New query feature labels Jul 16, 2025

@cx-artur-ribeiro cx-artur-ribeiro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@cx-eduardo-semanas cx-eduardo-semanas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@cx-artur-ribeiro cx-artur-ribeiro merged commit 5502a2e into Checkmarx:master Jul 17, 2025
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community Community contribution query New query feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

query(terraform): dependency loop when trying to resolve "S3 Bucket Logging Disabled"

3 participants