fix(secrets): store SSM scalar secrets without JSON quotes#2797
Conversation
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2797 +/- ##
==========================================
+ Coverage 81.78% 81.80% +0.01%
==========================================
Files 1793 1793
Lines 173025 173037 +12
==========================================
+ Hits 141507 141551 +44
+ Misses 23707 23678 -29
+ Partials 7811 7808 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSSM secret storage now preserves raw string values while retaining JSON encoding for structured values, with expanded round-trip tests and corrected prefix documentation. Terraform and OpenTofu tests now resolve and isolate compatible executables per test. ChangesAWS SSM secret storage
Terraform-compatible test isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SecretCaller
participant SSMStore
participant AWS_SSM
SecretCaller->>SSMStore: Set secret value
SSMStore->>AWS_SSM: PutParameter with encoded value
SecretCaller->>SSMStore: Get or GetKey
SSMStore->>AWS_SSM: GetParameter
AWS_SSM-->>SSMStore: Stored parameter value
SSMStore-->>SecretCaller: Decoded value
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/store/aws_ssm_param_store_test.go (1)
111-142: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a raw-string case containing JSON-sensitive characters.
The current test only uses
"test-value". Add a table case containing quotes, backslashes, and/or a newline, then assert the exactPutParameterInput.Value; this protects the promised byte-preserving behavior from future escaping regressions.As per coding guidelines, new features should have comprehensive behavior-focused tests.
🤖 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 `@pkg/store/aws_ssm_param_store_test.go` around lines 111 - 142, Extend the table-driven tests around the secret_string_is_stored_raw case with a raw string containing JSON-sensitive characters such as quotes, backslashes, or a newline. Configure the mock PutParameter expectation to assert the exact unchanged Value while retaining SecureString and overwrite settings, verifying byte-preserving behavior separately from secret_structured_value_is_json_encoded.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@pkg/store/aws_ssm_param_store_test.go`:
- Around line 111-142: Extend the table-driven tests around the
secret_string_is_stored_raw case with a raw string containing JSON-sensitive
characters such as quotes, backslashes, or a newline. Configure the mock
PutParameter expectation to assert the exact unchanged Value while retaining
SecureString and overwrite settings, verifying byte-preserving behavior
separately from secret_structured_value_is_json_encoded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 276197b0-fa23-445e-85b8-ceca66df8d8c
📒 Files selected for processing (3)
pkg/store/aws_ssm_param_store.gopkg/store/aws_ssm_param_store_test.gowebsite/docs/cli/configuration/stores.mdx
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/exec/yaml_func_terraform_output_test.go`:
- Around line 182-187: Update isolateTerraformTestBinary and its failure-path
error messages to use tool-neutral wording such as “Terraform-compatible
executable,” rather than “OpenTofu executable.” Ensure all reported failures in
this helper accurately cover both Terraform and OpenTofu without changing the
existing behavior.
- Around line 210-217: In TestIsolateTerraformTestBinary, replace assert.NoError
with require.NoError for both os.Executable and exec.LookPath results so
executable discovery failures stop the test immediately before subsequent
operations.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bb5be992-252e-4c3b-8e5d-bef9ae5c3812
📒 Files selected for processing (2)
internal/exec/terraform_test.gointernal/exec/yaml_func_terraform_output_test.go
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
CodeRabbit (@coderabbitai) review please |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/store/aws_ssm_param_store.go`:
- Around line 291-329: Document the raw-value/JSON ambiguity in the secret read
paths that use SSMStore.decodeParameterValue, covering !secret and atmos secret
get/env/exec plus the relevant store documentation. State that Set writes
strings raw, but Get/GetKey decode JSON objects, arrays, and quoted strings, so
secrets containing JSON blobs or literal surrounding quotes may not return
byte-exactly; note that distinguishing these cases requires a type marker.
- Around line 279-289: Update SSMStore.encodeParameterValue so secret string
values bypass JSON encoding only when non-empty; route an empty secret string
through json.Marshal to produce the JSON representation "\"\"". Preserve the
existing raw-value behavior for non-empty secret strings and existing
serialization error handling.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 94357095-fa41-46b7-bb57-fa89d6c47f07
📒 Files selected for processing (2)
pkg/store/aws_ssm_param_store.gopkg/store/aws_ssm_param_store_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/store/aws_ssm_param_store_test.go
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.224.1-rc.1. |
2 participants
what
secret: trueAWS SSM stores without JSON quoting.why
SecureStringvalue rather than a JSON-wrapped scalar.references
atmos secret(aws/ssm store) JSON-encodes scalar secrets, so string values are stored double-quoted and break external consumers #2795Summary by CodeRabbit
Bug Fixes
nulltext) without unwanted formatting or type changes.GetKey).Documentation
prefixmust start with/.secret: trueread/write behavior and related decoding ambiguity.Tests
terraformandtofu.