fix(aws-sm): deleteAnnotation only removes swamp-owned tags (#1406) #143
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/1406-delete-annotation-preserves-foreign-tags"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
deleteAnnotationused!tag.Key.startsWith("aws:")to select tags for removal, which caught ALL non-AWS-managed tags — including user-set tags likeenv=prodorteam=infra.tag.Key.startsWith("swamp:")so only swamp-owned tags (theswamp:prefix namespace) are removed. Foreign tags are preserved.Test plan
deno check extensions/vaults/*.ts— passesdeno lint extensions/vaults/— passesdeno fmt --check extensions/vaults/— passesdeno test --allow-net --allow-env --allow-sys --allow-read extensions/vaults/— 61/61 passdeno install --frozen— lockfile cleanCloses #1406
The tag filter in deleteAnnotation used `!tag.Key.startsWith("aws:")` which removed all non-AWS-managed tags, including user-set tags like env=prod or team=infra. Changed the condition to `tag.Key.startsWith("swamp:")` so only swamp-owned tags are removed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>Code Review
Blocking Issues
None.
Summary
This PR fixes issue #1406:
deleteAnnotationwas incorrectly deleting all non-aws: tags, including user-owned tags like env=prod, instead of only removing swamp-owned tags.The fix in
aws_sm.ts(deleteAnnotation tag-filter loop):The old condition matched every user-defined tag by checking
!tag.Key.startsWith("aws:"). The new conditiontag.Key.startsWith("swamp:")precisely scopes removal to the swamp namespace. The first clause of the old condition (tag.Key === LEGACY_SWAMP_URL_TAG_KEY) was also redundant, sinceswamp:urlalready fails theaws:prefix check.Test changes are correct and complete:
labelsfield from its annotation setup, appropriate since the test now focuses on description/url clearing, not label deletion.Manifest version bump: 2026.07.26.1 to 2026.07.27.1 is a correct CalVer bump.
Suggestions
aws_sm_test.tslines 315-322 contain two nearly-identicalsanitizeResourcescomment blocks back-to-back. Pre-existing issue, not introduced here, but worth a follow-on cleanup.Adversarial Review
Medium
putAnnotationwrites label tags withoutswamp:prefix, sodeleteAnnotationcan no longer clean them up —aws_sm.ts:518-523/aws_sm.ts:589putAnnotationwrites user-supplied labels as raw AWS tags with no namespace prefix:After this PR,
deleteAnnotationonly removes tags matchingtag.Key.startsWith("swamp:"). Tags written byputAnnotation(likeenv=prod) don't match that prefix, so they survive deletion.Breaking example:
getAnnotationreturns a non-null annotation afterdeleteAnnotationbecause the orphanedenvtag is read back as a label. This breaks the reasonable expectation that delete-then-get returns null.Note: the original test
"deleteAnnotation removes all annotation data"includedlabels: { env: "prod" }and assertedgetAnnotationreturnednull. That test was renamed and the labels were removed (line 684), masking this behavioral gap rather than testing it.Suggested fix: Either prefix labels on write (e.g.,
swamp:label:${key}) sodeleteAnnotationcan identify them, or widen thedeleteAnnotationfilter to also remove tags that were written byputAnnotation. This is a design-level concern that may warrant a follow-up issue.readAnnotationFieldstreats ALL non-aws:tags as labels, butdeleteAnnotationonly removesswamp:-prefixed tags —aws_sm.ts:234vsaws_sm.ts:589The read path (
readAnnotationFields) and the delete path now disagree on what constitutes a "swamp-owned" tag. Read says: everything that isn'taws:*or the legacyswamp:urlis a label. Delete says: onlyswamp:*tags are mine.This means any non-
aws:-prefixed, non-swamp:-prefixed tag on an AWS secret (whether written by swamp or by the user directly) will appear as a label ingetAnnotation/listAnnotations, even afterdeleteAnnotation. If this is the intended behavior,readAnnotationFieldsshould also be narrowed to only reportswamp:-prefixed tags as labels to maintain consistency.Low
Existing test weakened rather than adapted —
aws_sm_test.ts:674The test
"deleteAnnotation removes all annotation data"was the only test exercising the full roundtrip: putAnnotation-with-labels → deleteAnnotation → getAnnotation-returns-null. It was renamed to"deleteAnnotation clears notes and url"and had its labels removed. A replacement test that exercises the labels-survive-delete scenario through the fullgetAnnotationAPI (not just raw metadata inspection) would strengthen confidence in the new behavior.Verdict
PASS — The core change is correct and well-motivated:
deleteAnnotationshould not destroy user-created AWS tags. The new filter (swamp:prefix) is a clean improvement over the old overly-broad filter. The medium findings describe a pre-existing design gap (unprefixed label writes vs. prefixed-only deletes) that this PR exposes but does not introduce. The gap should be tracked as a follow-up issue but does not block this merge.Code Review
Blocking Issues
None.
Suggestions
deleteAnnotationuses a string literal instead ofSWAMP_TAG_PREFIX(aws_sm.ts, the changedif (tag.Key.startsWith("swamp:"))line): the PR addsSWAMP_TAG_PREFIX = "swamp:"specifically to centralize this string, butdeleteAnnotationuses the literal directly. If the prefix ever changes, this line would need a separate update. Considertag.Key.startsWith(SWAMP_TAG_PREFIX)for consistency.Mixed bare+prefixed label edge case is undocumented at the interface level:
readAnnotationFieldssilently drops bare tags when anyswamp:-prefixed label exists. The code comment explains the intent, but the behavior (partial loss of labels on a mixed-state secret) is not surfaced in theVaultAnnotationProviderinterface docs. Since this scenario requires a secret in a transitional state that shouldn't arise from normal usage, it's a low-stakes gap — but a note in the interface JSDoc or a test asserting the "prefixed wins" tie-break would make the invariant explicit.The core fix is correct: scoping
deleteAnnotationtoswamp:tags only (previously it removed all non-aws:tags, nuking customer-owned tags) and namespacing written labels underswamp:with a backward-compatible read path for bare tags. The new tests — including the regression guard for issue #1406 and the prefix round-trip — provide solid coverage.Adversarial Review
Medium
Label key "url" silently collides with legacy swamp:url tag -- data corruption
aws_sm.ts:528-533 / aws_sm.ts:235-236
putAnnotation writes labels as swamp:key tags. A label with key "url" produces tag key swamp:url, which is LEGACY_SWAMP_URL_TAG_KEY. In readAnnotationFields, the tag.Key === LEGACY_SWAMP_URL_TAG_KEY check (line 235) fires before the startsWith(SWAMP_TAG_PREFIX) check (line 237), so the tag is consumed as a legacy URL -- never as a label.
Breaking example: call putAnnotation with labels containing url: "not-a-url". This writes tag swamp:url = "not-a-url". On getAnnotation: if no URL exists in Description, annotation.url becomes "not-a-url" and labels is empty (label silently reinterpreted as annotation URL). If a URL already exists in Description, the tag is ignored entirely (label silently dropped).
Suggested fix: Validate that label keys do not collide with reserved suffixes (reject "url" as a label key in putAnnotation), or move the legacy swamp:url check to only trigger when no swamp:-prefixed labels exist -- mirroring the prefixed vs bare strategy already used for bare tags.
deleteAnnotation cannot clean up bare tags from the pre-prefix era -- annotation resurrects
aws_sm.ts:597-603
deleteAnnotation only removes swamp:* tags. Labels written before this PR were stored as bare tags (e.g., env=prod). After deleteAnnotation: description is cleared, swamp:* tags removed, but bare tags survive (by design -- they are indistinguishable from user-created tags).
On the next getAnnotation, prefixed is empty so the code falls through to bare, and the old labels reappear. The annotation appears to be "undeleted."
Breaking example: A secret annotated with old code has bare tags env=prod, team=infra plus swamp:url=https://... and notes in description. After deleteAnnotation, swamp:* tags are removed and description is cleared -- but bare tags env and team survive. The next getAnnotation reads them as labels (back-compat path) and returns a non-null annotation.
Note: the original test "deleteAnnotation removes all annotation data" (line 674) was weakened -- it was renamed to "deleteAnnotation clears notes and url" and labels was removed from the fixture, which masks this scenario.
Suggested fix: This is an inherent limitation of the prefix migration, but the PR should either: (a) document this gap explicitly in a code comment on the back-compat path, or (b) add a migration helper that rewrites bare tags to prefixed tags so deleteAnnotation can clean them up after migration.
Low
putAnnotation accumulates stale prefixed tags (pre-existing, not introduced by this PR)
aws_sm.ts:526-534
putAnnotation only adds/overwrites tags for the labels provided. It never removes tags for labels that existed previously but are absent from the new annotation. After writing labels env and team, then writing only env, the swamp:team tag persists. This is pre-existing behavior unchanged by the PR, but it interacts with the new prefix: stale swamp: tags accumulate and cannot be cleared without calling deleteAnnotation.
Verdict
PASS -- The core logic is sound: the prefix namespace cleanly separates swamp-owned tags from user tags, back-compat reads are handled, and tests cover the new write/read/delete paths. The "url" label collision (Medium 1) and bare-tag resurrection after delete (Medium 2) are real edge cases but unlikely to hit users immediately -- neither rises to a blocking severity. Both should be tracked for follow-up.