Skip to content

Conversation

Napalys
Copy link
Contributor

@Napalys Napalys commented Jul 28, 2025

This PR adds modeling for v2 and v3 AWS SDK clients:

  • client-dynamodb
  • client-s3
  • client-athena
  • client-rds-data

Important notes:

@Napalys Napalys marked this pull request as ready for review July 30, 2025 07:50
@Copilot Copilot AI review requested due to automatic review settings July 30, 2025 07:50
@Napalys Napalys requested a review from a team as a code owner July 30, 2025 07:50
Copilot

This comment was marked as outdated.

@Napalys Napalys changed the title Js: Modeling of dynamodb JS: Modeling of aws-sdk clients* Jul 30, 2025
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

I haven't finished reviewing, but two comments so far:

  • You don't have any tests for the sources you have added. Can you add a quick use of each of those APIs and see if the existing database access test will capture them?
  • Would it be easier to put typeModel at the top of the model.yml file? I found that more intuitive, so I could look to the top to see what things are defined to mean.

@owen-mc
Copy link
Contributor

owen-mc commented Sep 3, 2025

Oh, and you have merge conflicts now I see.

@Copilot Copilot AI review requested due to automatic review settings September 4, 2025 09:51
Copilot

This comment was marked as outdated.

@Napalys Napalys requested review from Copilot and owen-mc September 4, 2025 10:33
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds comprehensive modeling for AWS SDK clients (both v2 and v3) to support SQL injection detection. The modeling includes support for DynamoDB, S3, Athena, and RDS Data Service clients and their respective query execution methods.

Key changes include:

  • Addition of SQL injection sinks and sources for AWS SDK v2 and v3 clients
  • Test cases demonstrating SQL injection vulnerabilities across different AWS services
  • Support for both callback and promise-based patterns in v2 SDK

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
aws-sdk.model.yml Defines type models, sinks, summaries, and sources for AWS SDK clients and commands
rds-client.js Test cases for RDS Data Service SQL injection vulnerabilities
dynamodb.js Test cases for DynamoDB PartiQL SQL injection vulnerabilities
clients3.js Test cases for S3 SQL injection via SelectObjectContent
athena.js Test cases for Athena SQL injection via query execution and storage commands
aws.js XSS test cases for AWS SDK response data
aws-db.js Additional XSS test cases for database response data
Various .expected files Updated test expectations with new SQL injection and XSS alerts
2025-07-28-dynamodb.md Change note documenting the new AWS SDK support

Comment on lines 6 to 9
- ["AthenaClientV2", "aws-sdk", "Member[Athena]"]
- ["S3ClientV2", "aws-sdk", "Member[S3]"]
- ["RDSDataClientV2", "aws-sdk", "Member[RDSDataService]"]
- ["DynamoDBClientV2", "aws-sdk", "Member[DynamoDB]"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- ["AthenaClientV2", "aws-sdk", "Member[Athena]"]
- ["S3ClientV2", "aws-sdk", "Member[S3]"]
- ["RDSDataClientV2", "aws-sdk", "Member[RDSDataService]"]
- ["DynamoDBClientV2", "aws-sdk", "Member[DynamoDB]"]
- ["aws-sdk.Athena", "aws-sdk", "Member[Athena]"]
- ["aws-sdk.S3", "aws-sdk", "Member[S3]"]
- ["aws-sdk.RDSDataService", "aws-sdk", "Member[RDSDataService]"]
- ["aws-sdk.DynamoDB", "aws-sdk", "Member[DynamoDB]"]

Types should ideally be named after the correspinding TypeScript type if there is one, using the format <package>.<typename>.. This means we automatically match them up with type annotations and the names are also namespaced so different models easily clash with each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Napalys Napalys requested a review from asgerf September 16, 2025 13:59
@Napalys Napalys merged commit e1d27f3 into github:main Sep 17, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants