Skip to content

feat(secret-serialization): Add skill for secrets leaked via serialization - #173

Open
geoffg-sentry wants to merge 2 commits into
mainfrom
feat/secret-serialization-skill
Open

geoffg-sentry wants to merge 2 commits into
mainfrom
feat/secret-serialization-skill

Conversation

@geoffg-sentry

@geoffg-sentry geoffg-sentry commented Sep 25, 2026 •

Copy link
Copy Markdown

Adds secret-serialization, a review skill for credentials that leak through generated serialization: dataclass repr and asdict, attrs, pydantic model_dump, JSON.stringify, util.inspect, structured logs, and tracing spans.

These leaks need two changes that each look safe alone. One stores a credential on a type that serializes itself, for example a dataclass field without field(repr=False). The other serializes whole objects or every kwarg into telemetry, for example str(value) over tool-call kwargs written to a span attribute. They are usually written months apart, so diff-only review passes both. The skill reports either side on its own and searches the whole repository for the other side before choosing severity. It does not downgrade a finding because one half predates the diff.

Direct logging of a secret stays with security-review. This skill is opt-in, and repos add it through Warden with remote = "getsentry/skills".

Evaluation

Run through Warden (pi runtime, openrouter/x-ai/grok-4.5, effort high) against the internal repository where the original leak happened:

  • Incident diff (credential fields added to dataclass RPC clients; the str(value) span sink was already on the default branch): high findings on both fields in two runs. Each finding traced tool kwarg, then tracing decorator, then str(value), then span attribute.
  • Same diff with field(repr=False): no findings.
  • Unrelated diff adding seven *_tokens usage-count fields: no findings.

Details and open gaps are in SOURCES.md. Known noise: call-site hunks that pass the credential into a constructor produce duplicate findings for the same field.

allowed-tools must be space-delimited for Warden. With Read, Grep, Glob, Warden drops Read, and Grep,, so the agent could not read files or search code, and the incident only produced medium findings. Other skills in this repo that use the comma form, such as gha-security-review, likely have the same problem under Warden.

geoffg-sentry and others added 2 commits September 25, 2026 16:53
…ation

Add a review skill for credentials that leak through generated
serialization: dataclass repr and asdict, attrs, pydantic model_dump,
JSON.stringify, util.inspect, logs, and tracing spans.

The leak usually needs a credential field on an auto-serializing type
and a sink that stringifies whole objects or every kwarg. Each half
looks safe in its own diff, so the skill reports either side alone and
searches the repository for the other side.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Warden splits allowed-tools on spaces and drops unknown tokens, so
"Read, Grep, Glob" left the agent with only Glob (find and ls). Without
read and grep it could not trace sinks and capped findings at medium.

Record the Warden evaluation runs: the incident diff now yields high
findings with a full sink trace, and two negative diffs yield none.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@geoffg-sentry
geoffg-sentry marked this pull request as ready for review September 25, 2026 23:56

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 831d092. Configure here.


Accept as exclusion:

- Field-level flags that remove the field from generated output: `dataclasses.field(repr=False)`, `attrs.field(repr=False)`, pydantic `Field(exclude=True)`, `enumerable: false`, JavaScript `#private` fields.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pydantic exclusions treated as complete

High Severity

Field(exclude=True) is treated as a complete exclusion, but it only drops a field from model_dump and response serialization. Pydantic __repr__ still embeds the raw credential, so the str(value) span path this skill targets still leaks. The Python table lists Field(repr=False) the same way even though model_dump still includes it.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 831d092. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant