Fix: [New Check]: Detect secrets in AWS Batch job definitions#11836
Closed
Deep070203 wants to merge 7 commits into
Closed
Fix: [New Check]: Detect secrets in AWS Batch job definitions#11836Deep070203 wants to merge 7 commits into
Deep070203 wants to merge 7 commits into
Conversation
- Materialize app payloads upfront to ensure line_context_by_app is fully populated upon scan failure - Add test case verifying scanner error behaves correctly (reporting MANUAL instead of defaulting to PASS)
- Add AWS CLI update-app command to amplify_app_no_secrets_in_environment metadata
github-actions
Bot
added
provider/aws
📝 WalkthroughWalkthroughAdds AWS Amplify and AWS Batch service support, two new secret-scanning checks, matching tests, Amplify IAM read permissions, and a changelog entry. ChangesAmplify secrets-scanning check
Batch secrets-scanning check
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
has-conflicts
Contributor
|
✅ No Conflicts No conflict markers, and the branch merges cleanly into its base. |
github-actions
Bot
removed
the
has-conflicts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tests/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/__init__.py (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove test-folder
__init__.py.Same violation as
tests/providers/aws/services/amplify/__init__.py— path instructions state test folders must not contain__init__.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/__init__.py` at line 1, Remove the test-folder __init__.py file from the amplify_app_no_secrets_in_environment test package, since test directories must not contain package initializers. Delete the __init__.py under the amplify_app_no_secrets_in_environment path and keep the rest of the test module structure unchanged; this matches the same cleanup needed for the parent amplify test package.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@prowler/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/amplify_app_no_secrets_in_environment.py`:
- Around line 83-87: The add_line helper currently accepts value as str but
explicitly handles None, so its signature should match the actual contract.
Update add_line in amplify_app_no_secrets_in_environment.py to accept an
Optional[str] value, keeping the early return for None and preserving the
existing lines/json.dumps/line_context behavior so callers can pass missing
values without a type mismatch.
In
`@prowler/providers/aws/services/batch/batch_job_definition_no_secrets/batch_job_definition_no_secrets.py`:
- Around line 15-78: Add a Google-style docstring to the `execute` method in
`batch_job_definition_no_secrets.py`. Document that `execute()` scans Batch job
definitions for secrets, returns a list of `Check_Report_AWS`, and note the
behavior around `detect_secrets_scan_batch`, `SecretsScanError`, and the
MANUAL/PASS/FAIL report outcomes. Keep the docstring concise and place it
directly above the `execute(self) -> list[Check_Report_AWS]` definition.
- Line 81: The helper _build_job_definition_payload currently annotates its
return type but leaves the jd parameter untyped. Add an explicit type hint for
jd (for example, the appropriate JobDefinition model/type used in this module)
so the function signature is consistent and supports IDE/type-checking; update
only the parameter annotation and keep the existing return annotation unchanged.
In
`@tests/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/amplify_app_no_secrets_in_environment_test.py`:
- Line 52: The Amplify test fixture contains a fake JWT-like string that still
triggers TruffleHog in diff scans, so update the test setup to avoid scanning
this value by adding an explicit allowlist/exception for the fixture or
relocating the JWT fixture outside the scanned test path. Make the change in the
Amplify no-secrets test around the db_pass fixture so the secret-scan gate no
longer flags these intended test tokens.
---
Duplicate comments:
In
`@tests/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/__init__.py`:
- Line 1: Remove the test-folder __init__.py file from the
amplify_app_no_secrets_in_environment test package, since test directories must
not contain package initializers. Delete the __init__.py under the
amplify_app_no_secrets_in_environment path and keep the rest of the test module
structure unchanged; this matches the same cleanup needed for the parent amplify
test package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2c06e703-85b0-4e95-9774-bf16d5b2b2c2
📒 Files selected for processing (23)
permissions/prowler-additions-policy.jsonpermissions/templates/cloudformation/prowler-scan-role.ymlprowler/CHANGELOG.mdprowler/providers/aws/services/amplify/__init__.pyprowler/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/__init__.pyprowler/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/amplify_app_no_secrets_in_environment.metadata.jsonprowler/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/amplify_app_no_secrets_in_environment.pyprowler/providers/aws/services/amplify/amplify_client.pyprowler/providers/aws/services/amplify/amplify_service.pyprowler/providers/aws/services/batch/__init__.pyprowler/providers/aws/services/batch/batch_client.pyprowler/providers/aws/services/batch/batch_job_definition_no_secrets/__init__.pyprowler/providers/aws/services/batch/batch_job_definition_no_secrets/batch_job_definition_no_secrets.metadata.jsonprowler/providers/aws/services/batch/batch_job_definition_no_secrets/batch_job_definition_no_secrets.pyprowler/providers/aws/services/batch/batch_service.pytests/providers/aws/services/amplify/__init__.pytests/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/__init__.pytests/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/amplify_app_no_secrets_in_environment_test.pytests/providers/aws/services/amplify/amplify_service_test.pytests/providers/aws/services/batch/__init__.pytests/providers/aws/services/batch/batch_job_definition_no_secrets/__init__.pytests/providers/aws/services/batch/batch_job_definition_no_secrets/batch_job_definition_no_secrets_test.pytests/providers/aws/services/batch/batch_service_test.py
Comment on lines
+83
to
+87
| def add_line(context: str, value: str) -> None: | ||
| if value is None: | ||
| return | ||
| lines.append(json.dumps({context: value})) | ||
| line_context[len(lines)] = context |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Type hint doesn't match the None-handling contract.
value: str declares required str, but the function explicitly early-returns on None (line 84), meaning callers may legitimately pass None. Should be Optional[str].
♻️ Proposed fix
+from typing import Optional
+
...
- def add_line(context: str, value: str) -> None:
+ def add_line(context: str, value: Optional[str]) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def add_line(context: str, value: str) -> None: | |
| if value is None: | |
| return | |
| lines.append(json.dumps({context: value})) | |
| line_context[len(lines)] = context | |
| def add_line(context: str, value: Optional[str]) -> None: | |
| if value is None: | |
| return | |
| lines.append(json.dumps({context: value})) | |
| line_context[len(lines)] = context |
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 85-85: use jsonify instead of json.dumps for JSON output
Context: json.dumps({context: value})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@prowler/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/amplify_app_no_secrets_in_environment.py`
around lines 83 - 87, The add_line helper currently accepts value as str but
explicitly handles None, so its signature should match the actual contract.
Update add_line in amplify_app_no_secrets_in_environment.py to accept an
Optional[str] value, keeping the early return for None and preserving the
existing lines/json.dumps/line_context behavior so callers can pass missing
values without a type mismatch.
| return findings | ||
|
|
||
|
|
||
| def _build_job_definition_payload(jd) -> tuple[str, dict[int, str]]: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Add a type hint for the jd parameter.
The function's return type is annotated but jd is not; consider annotating it (e.g., jd: JobDefinition) for consistency and IDE/type-checker support.
🧹 Proposed fix
-def _build_job_definition_payload(jd) -> tuple[str, dict[int, str]]:
+def _build_job_definition_payload(jd: "JobDefinition") -> tuple[str, dict[int, str]]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _build_job_definition_payload(jd) -> tuple[str, dict[int, str]]: | |
| def _build_job_definition_payload(jd: "JobDefinition") -> tuple[str, dict[int, str]]: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@prowler/providers/aws/services/batch/batch_job_definition_no_secrets/batch_job_definition_no_secrets.py`
at line 81, The helper _build_job_definition_payload currently annotates its
return type but leaves the jd parameter untyped. Add an explicit type hint for
jd (for example, the appropriate JobDefinition model/type used in this module)
so the function signature is consistent and supports IDE/type-checking; update
only the parameter annotation and keep the existing return annotation unchanged.
- Add Google-style docstring to batch_job_definition_no_secrets execute method - Fix batch_job_definition_no_secrets_test to mock provider during resolution
Author
|
I have added Batch Job secrets check as well here. You can disregard #11825 pull request and check this PR. It has both the changes |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/amplify_app_no_secrets_in_environment_test.py (1)
112-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the extra provider mock in
test_app_scan_error_marks_manual—_execute_checkalready patchesProvider.get_global_providerwithset_mocked_aws_provider([AWS_REGION_US_EAST_1]), so the local provider setup here is redundant. Keeping only thedetect_secrets_scan_batchfailure stub would make the test easier to read.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/amplify_app_no_secrets_in_environment_test.py` around lines 112 - 142, The test test_app_scan_error_marks_manual has a redundant Provider.get_global_provider patch because _execute_check already uses set_mocked_aws_provider([AWS_REGION_US_EAST_1]) to provide the AWS provider. Remove the extra provider mock from the local patch block and keep only the detect_secrets_scan_batch side effect setup so the test focuses on the scan failure path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@tests/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/amplify_app_no_secrets_in_environment_test.py`:
- Around line 112-142: The test test_app_scan_error_marks_manual has a redundant
Provider.get_global_provider patch because _execute_check already uses
set_mocked_aws_provider([AWS_REGION_US_EAST_1]) to provide the AWS provider.
Remove the extra provider mock from the local patch block and keep only the
detect_secrets_scan_batch side effect setup so the test focuses on the scan
failure path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 85178456-4daa-4d2e-90bc-bb804c2df167
📒 Files selected for processing (1)
tests/providers/aws/services/amplify/amplify_app_no_secrets_in_environment/amplify_app_no_secrets_in_environment_test.py
Author
|
Closed it since I don't want to add both checks in one PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Fixes #11817, #11816
Description
This PR introduces comprehensive security auditing support for AWS Amplify and AWS Batch services within the Prowler SDK, including new checks targeting hardcoded secrets and workload misconfigurations.
1. AWS Amplify Service Integration
amplify:ListAppsandamplify:ListBranches).amplify_app_no_secrets_in_environmentcheck.buildspec.yml) utilizing Prowler's shared secret-scanning helper.2. AWS Batch Service Integration
batch_job_definition_security_hardeningcheck to evaluate container and job definitions for security risks.3. Policy & Infrastructure-as-Code Updates
permissions/prowler-additions-policy.jsonto include the minimal read-only permissions required for cross-account scans.Steps to review
Checklist
Community Checklist
SDK/CLI
amplify:ListAppsandamplify:ListBranches,batch:DescribeJobDefinitions,batch:DescribeJobQueues,batch:ListJobDefinitionsto the scan role additionsUI
API
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Summary by CodeRabbit
Summary by CodeRabbit