fix(vault/aws-sm): store annotation URL in Description, not a tag (#495) #20

Merged

stack72 merged 1 commit from fix/495-aws-sm-url-annotation into main 2026-05-31 21:51:31 +00:00

Conversation 2 Commits 1 Files changed 4 +345 -42

stack72 commented 2026-05-31 21:32:42 +00:00

Owner

Copy link

Issue

Fixes #495. swamp vault annotate --url failed on @swamp/aws-sm when the URL contained query-string parameters (e.g. ?name=foo&region=us-east-1). The URL was stored as the swamp:url AWS Secrets Manager tag value, and AWS tag values reject characters like ? and & — TagResource returns InvalidRequestException — Request rejected by the downstream tagging service.

Percent-encoding doesn't help (% is also disallowed in tag values), and base64 would destroy the readability that annotations exist for.

Fix

Store the URL in the secret Description instead of a tag — the Description accepts any character (pattern [\s\S]*, up to 2048 chars) and stays human-readable in the AWS console. It lives alongside notes via a trailing, parseable swamp:url=<url> line.

• composeDescription / parseDescription give a lossless round-trip; a shared readAnnotationFields helper centralizes the read path so get / list / put can't drift.
• putAnnotation now does a read-modify-write of the Description, so a partial update (only --url or only --notes) preserves the other field. The URL is no longer written as a tag.
• Back-compat: the legacy swamp:url tag is still read (Description wins when both are present), so annotations created before this change keep their URL. No active migration — the stale tag is left untouched.
• README + manifest annotation docs updated; manifest version bumped 2026.05.23.1 → 2026.05.31.1.

Tests

The mock TagResource now enforces AWS's real tag-value charset, so any regression that routes the URL back through a tag fails loudly. New cases: query-param URL round-trip (#495 repro), partial updates in both directions, legacy-tag preservation on a notes-only update + read fallback, lossless multi-line-note round-trip, and the missing-secret error path. 55/55 tests pass.

🤖 Generated with Claude Code

## Issue Fixes #495. `swamp vault annotate --url` failed on `@swamp/aws-sm` when the URL contained query-string parameters (e.g. `?name=foo&region=us-east-1`). The URL was stored as the `swamp:url` AWS Secrets Manager **tag value**, and AWS tag values reject characters like `?` and `&` — `TagResource` returns `InvalidRequestException — Request rejected by the downstream tagging service`. Percent-encoding doesn't help (`%` is also disallowed in tag values), and base64 would destroy the readability that annotations exist for. ## Fix Store the URL in the secret **Description** instead of a tag — the Description accepts any character (pattern `[\s\S]*`, up to 2048 chars) and stays human-readable in the AWS console. It lives alongside `notes` via a trailing, parseable `swamp:url=<url>` line. - `composeDescription` / `parseDescription` give a lossless round-trip; a shared `readAnnotationFields` helper centralizes the read path so `get` / `list` / `put` can't drift. - `putAnnotation` now does a **read-modify-write** of the Description, so a partial update (only `--url` or only `--notes`) preserves the other field. The URL is no longer written as a tag. - **Back-compat:** the legacy `swamp:url` tag is still read (Description wins when both are present), so annotations created before this change keep their URL. No active migration — the stale tag is left untouched. - README + manifest annotation docs updated; manifest version bumped `2026.05.23.1` → `2026.05.31.1`. ## Tests The mock `TagResource` now enforces AWS's real tag-value charset, so any regression that routes the URL back through a tag fails loudly. New cases: query-param URL round-trip (#495 repro), partial updates in both directions, legacy-tag preservation on a notes-only update + read fallback, lossless multi-line-note round-trip, and the missing-secret error path. **55/55 tests pass.** 🤖 Generated with [Claude Code](https://claude.com/claude-code)

stack72 added 1 commit 2026-05-31 21:32:42 +00:00

fix(vault/aws-sm): store annotation URL in Description, not a tag (#495) ... 6a7712317a

`swamp vault annotate --url` failed when the URL contained query-string
parameters: the URL was stored as the `swamp:url` AWS Secrets Manager tag
value, and AWS tag values reject characters like `?` and `&`
(InvalidRequestException — "Request rejected by the downstream tagging
service").

Store the URL in the secret Description instead — alongside notes, via a
human-readable trailing `swamp:url=<url>` line. The Description accepts any
character, so query-string URLs now round-trip. `putAnnotation` does a
read-modify-write so partial updates (only --url or only --notes) preserve
the other field; a shared parse helper keeps get/list/put consistent.
The legacy `swamp:url` tag is still read for back-compat (no active
migration), so existing annotations keep their URL.

README and manifest annotation docs updated; manifest version bumped.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

stack72 commented 2026-05-31 21:44:18 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. Race condition in putAnnotation read-modify-write (aws_sm.ts:360-393)

   putAnnotation now reads the existing Description via DescribeSecret, merges the caller's fields with the stored fields, then writes back via UpdateSecret. Two concurrent putAnnotation calls on the same secret can interleave:

   • Call A reads Description: "Old notes\n\nswamp:url=http://a.com"
   • Call B reads Description: same
   • Call A writes: "New notes from A\n\nswamp:url=http://a.com"
   • Call B writes: "New notes from B\n\nswamp:url=http://b.com" — A's notes are lost

   This is inherent to the AWS Secrets Manager API (no conditional-write / ETag support), so there's no clean fix at this layer. Worth documenting as a known limitation, especially since the old code didn't have this window (notes and url were written independently).

2. URL containing newlines corrupts Description encoding (aws_sm.ts:154-164)

   composeDescription interpolates the url parameter directly into the Description without validating it is newline-free. A URL like "http://x.com\nswamp:url=http://evil.com" produces:

   notes\n\nswamp:url=http://x.com\nswamp:url=http://evil.com
   

   parseDescription extracts the last line as the URL, so getAnnotation would return url: "http://evil.com" instead of the original. Under the old tag-based storage, AWS would reject a tag value containing \n, so this is a minor regression in input validation. The practical likelihood is very low (URLs don't contain literal newlines), but a single-line assertion on the URL in composeDescription — e.g. rejecting strings containing \n — would close the gap.

Low

1. Legacy swamp:url tag is never cleaned up by putAnnotation (aws_sm.ts:395-415)

   When putAnnotation reads a legacy swamp:url tag and migrates the URL into the Description, the old tag remains. It's inert (Description URL takes read priority), but it occupies one of the 50 available tag slots until deleteAnnotation is called. Not a correctness bug — just a wasted tag slot on migrated secrets.

2. Sentinel collision acknowledged (aws_sm.ts:174-176)

   A note whose final line is literally swamp:url=<something> is misinterpreted as a URL trailer. The doc comment acknowledges this. No action needed — just confirming the limitation is documented.

Verdict

PASS. The core change — moving the annotation URL from a tag to the Description — is well-motivated (AWS tag values genuinely reject ? and &), correctly implemented with a clean compose/parse round-trip, and thoroughly tested. The read-modify-write race is an inherent AWS API limitation, not a code defect. The newline-in-URL edge case is theoretical. Test coverage is excellent: the mock server enforces AWS's tag charset (regression guard), and tests cover partial updates, legacy back-compat, multi-line notes, and error paths.

## Adversarial Review ### Medium 1. **Race condition in `putAnnotation` read-modify-write** (`aws_sm.ts:360-393`) `putAnnotation` now reads the existing Description via `DescribeSecret`, merges the caller's fields with the stored fields, then writes back via `UpdateSecret`. Two concurrent `putAnnotation` calls on the same secret can interleave: - Call A reads Description: `"Old notes\n\nswamp:url=http://a.com"` - Call B reads Description: same - Call A writes: `"New notes from A\n\nswamp:url=http://a.com"` - Call B writes: `"New notes from B\n\nswamp:url=http://b.com"` — A's notes are lost This is inherent to the AWS Secrets Manager API (no conditional-write / ETag support), so there's no clean fix at this layer. Worth documenting as a known limitation, especially since the old code didn't have this window (notes and url were written independently). 2. **URL containing newlines corrupts Description encoding** (`aws_sm.ts:154-164`) `composeDescription` interpolates the `url` parameter directly into the Description without validating it is newline-free. A URL like `"http://x.com\nswamp:url=http://evil.com"` produces: ``` notes\n\nswamp:url=http://x.com\nswamp:url=http://evil.com ``` `parseDescription` extracts the *last* line as the URL, so `getAnnotation` would return `url: "http://evil.com"` instead of the original. Under the old tag-based storage, AWS would reject a tag value containing `\n`, so this is a minor regression in input validation. The practical likelihood is very low (URLs don't contain literal newlines), but a single-line assertion on the URL in `composeDescription` — e.g. rejecting strings containing `\n` — would close the gap. ### Low 1. **Legacy `swamp:url` tag is never cleaned up by `putAnnotation`** (`aws_sm.ts:395-415`) When `putAnnotation` reads a legacy `swamp:url` tag and migrates the URL into the Description, the old tag remains. It's inert (Description URL takes read priority), but it occupies one of the 50 available tag slots until `deleteAnnotation` is called. Not a correctness bug — just a wasted tag slot on migrated secrets. 2. **Sentinel collision acknowledged** (`aws_sm.ts:174-176`) A note whose final line is literally `swamp:url=<something>` is misinterpreted as a URL trailer. The doc comment acknowledges this. No action needed — just confirming the limitation is documented. ### Verdict **PASS.** The core change — moving the annotation URL from a tag to the Description — is well-motivated (AWS tag values genuinely reject `?` and `&`), correctly implemented with a clean compose/parse round-trip, and thoroughly tested. The read-modify-write race is an inherent AWS API limitation, not a code defect. The newline-in-URL edge case is theoretical. Test coverage is excellent: the mock server enforces AWS's tag charset (regression guard), and tests cover partial updates, legacy back-compat, multi-line notes, and error paths.

stack72 commented 2026-05-31 21:47:29 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. Duplicate sanitizeResources comment block (aws_sm_test.ts, just before the first Deno.test): two nearly identical block comments explain the same thing back-to-back. The first ("is safe here because the connections are cleaned up when the SDK client is garbage collected") is slightly inaccurate and redundant — keep only the second one.

2. Unused provider creation in the "getAnnotation reads a legacy swamp:url tag (back-compat)" test: vault.createProvider("test", ...) is called on the first line of the withMockAws callback and its return value is discarded. The call has no side-effects; remove it.

3. asAnnotationProvider double-cast (as unknown as VaultAnnotationProvider): this bypasses TypeScript's type-checker for the annotation methods. Since AwsSmVaultProvider genuinely implements both interfaces, consider narrowing the return type of vault.createProvider to VaultProvider & VaultAnnotationProvider, or using a runtime type-guard, so the compiler can verify the cast rather than swallow it silently.

4. parseDescription known-limitation: the documented edge case (notes whose last line is literally swamp:url=<x>) is extremely unlikely in practice — no action required, just confirming it was reviewed.

---

Model files: The changed model/ files are all auto-generated (headers confirm). codegen/ was also modified in this commit, satisfying the legitimate-change rule in CLAUDE.md.

model/aws/verifiedpermissions/extensions/models/policy_store_alias.ts appears in the CHANGED FILES list but does not exist in the commit or on disk, and is not referenced in the verifiedpermissions manifest. This appears to be a stale entry in the file list; no action needed.

Vault fix correctness: composeDescription / parseDescription are proper inverses across all tested cases (url-only, notes-only, both, multi-line notes with embedded blank lines). The putAnnotation read-modify-write correctly preserves whichever field the caller omitted, and the legacy swamp:url tag fall-back is wired into readAnnotationFields so getAnnotation, listAnnotations, and putAnnotation are all consistent. deleteAnnotation clears both Description and all non-aws: tags. The mock server AWS_TAG_VALUE_PATTERN guard is an excellent regression fence for the original issue.

Testing: Conformance helpers used (assertVaultExportConformance, assertVaultConformance). Env vars restored in finally. sanitizeResources: false present with explanatory comment on all tests that create SDK clients. No live cloud calls.

Security: No credential leaks; secrets are never logged or embedded in error messages. No any types in hand-written code. Named exports throughout.

## Code Review ### Blocking Issues None. ### Suggestions 1. **Duplicate sanitizeResources comment block** (`aws_sm_test.ts`, just before the first `Deno.test`): two nearly identical block comments explain the same thing back-to-back. The first ("is safe here because the connections are cleaned up when the SDK client is garbage collected") is slightly inaccurate and redundant — keep only the second one. 2. **Unused provider creation** in the "getAnnotation reads a legacy swamp:url tag (back-compat)" test: `vault.createProvider("test", ...)` is called on the first line of the `withMockAws` callback and its return value is discarded. The call has no side-effects; remove it. 3. **`asAnnotationProvider` double-cast** (`as unknown as VaultAnnotationProvider`): this bypasses TypeScript's type-checker for the annotation methods. Since `AwsSmVaultProvider` genuinely implements both interfaces, consider narrowing the return type of `vault.createProvider` to `VaultProvider & VaultAnnotationProvider`, or using a runtime type-guard, so the compiler can verify the cast rather than swallow it silently. 4. **`parseDescription` known-limitation**: the documented edge case (notes whose last line is literally `swamp:url=<x>`) is extremely unlikely in practice — no action required, just confirming it was reviewed. --- **Model files:** The changed `model/` files are all auto-generated (headers confirm). `codegen/` was also modified in this commit, satisfying the legitimate-change rule in CLAUDE.md. **`model/aws/verifiedpermissions/extensions/models/policy_store_alias.ts`** appears in the CHANGED FILES list but does not exist in the commit or on disk, and is not referenced in the verifiedpermissions manifest. This appears to be a stale entry in the file list; no action needed. **Vault fix correctness:** `composeDescription` / `parseDescription` are proper inverses across all tested cases (url-only, notes-only, both, multi-line notes with embedded blank lines). The `putAnnotation` read-modify-write correctly preserves whichever field the caller omitted, and the legacy `swamp:url` tag fall-back is wired into `readAnnotationFields` so `getAnnotation`, `listAnnotations`, and `putAnnotation` are all consistent. `deleteAnnotation` clears both Description and all non-`aws:` tags. The mock server `AWS_TAG_VALUE_PATTERN` guard is an excellent regression fence for the original issue. **Testing:** Conformance helpers used (`assertVaultExportConformance`, `assertVaultConformance`). Env vars restored in `finally`. `sanitizeResources: false` present with explanatory comment on all tests that create SDK clients. No live cloud calls. **Security:** No credential leaks; secrets are never logged or embedded in error messages. No `any` types in hand-written code. Named exports throughout.

stack72 merged commit 8b3b57b2dc into main 2026-05-31 21:51:31 +00:00

stack72 deleted branch fix/495-aws-sm-url-annotation 2026-05-31 21:51:32 +00:00

stack72 referenced this pull request from a commit 2026-05-31 21:51:33 +00:00

fix(vault/aws-sm): store annotation URL in Description, not a tag (#495) (#20)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

swamp-club/swamp-extensions!20

No description provided.