Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
232 changes: 232 additions & 0 deletions .claude/skills/fix-security-vulnerability/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,232 @@
---
name: fix-security-vulnerability
description: Analyze and propose fixes for Dependabot security alerts
argument-hint: <dependabot-alert-url>
---

# Fix Security Vulnerability Skill

Analyze Dependabot security alerts and propose fixes. **Does NOT auto-commit** - always presents analysis first and waits for user approval.

## Input

- Dependabot URL: `https://github.com/getsentry/sentry-javascript/security/dependabot/1046`
- Or just the alert number: `1046`

## Workflow

### Step 1: Fetch Vulnerability Details

```bash
gh api repos/getsentry/sentry-javascript/dependabot/alerts/<alert-number>
```

Extract: package name, vulnerable/patched versions, CVE ID, severity, description.

### Step 2: Analyze Dependency Tree

```bash
yarn why <package-name>
```

Determine if it's a direct or transitive dependency, and whether it's production or dev.

### Step 3: Determine Fix Strategy

#### Check for version-specific test packages

Many packages in `dev-packages/e2e-tests/test-applications/` intentionally pin specific versions:

- `nextjs-13` - Tests Next.js 13.x, should NOT bump to 14
- `remix-2` - Tests Remix 2.x specifically

**Do NOT bump these.** Recommend dismissing the alert with an explanation.
Comment thread
chargome marked this conversation as resolved.

#### For other dependencies

| Type | Action |
| --------------------- | ----------------------------------- |
| Patch bump available | Preferred - lowest risk |
| Minor bump needed | Usually safe |
| Major bump needed | Analyze breaking changes first |
| Transitive dependency | Bump the parent package (see below) |

### Step 3a: Transitive Dependencies

If the vulnerable package is pulled in by another package:

**1. Identify and check the parent:**

```bash
yarn why <vulnerable-package>
npm view <parent-package>@latest dependencies.<vulnerable-package>
```

**2. Fix approach:**

| Scenario | Action |
| --------------------------------- | ------------------------------- |
| Parent has newer version with fix | **Bump the parent** |
| Parent hasn't released fix | Wait, or open an issue upstream |
| We control the parent | Fix in parent package first |

**AVOID RESOLUTIONS.** Using `resolutions` to force a transitive dependency version is risky - it can break the parent package silently. Only consider resolutions if:

- No upstream fix exists AND it's a production-critical vulnerability
- The forced version is a patch/minor bump (not major)
- You've manually verified compatibility

In most cases, it's better to wait for an upstream fix or accept the risk for dev-only dependencies than to use resolutions.

### Step 4: Present Analysis

Present findings and **wait for user approval** before making changes:

```
## Security Vulnerability Analysis

**Package:** <name> | **Severity:** <severity> | **CVE:** <id>
**Vulnerable:** <range> | **Patched:** <version>

### Dependency Chain
<yarn why output>

### Recommendation
<One of: Safe to bump / Version-specific test - do not bump / Bump parent package>

### Proposed Fix
1. Update <file>: "<package>": "<new-version>"
2. yarn install && yarn dedupe-deps:fix
3. Verify with: yarn why <package>

Proceed?
```

### Step 5: Apply Fix (After Approval)

```bash
# 1. Edit package.json
# 2. Update lockfile
yarn install
# 3. Deduplicate
yarn dedupe-deps:fix
# 4. Verify
yarn dedupe-deps:check
yarn why <package>
# 5. Show changes
git diff
```

**Do NOT commit** - let the user review first.

### Step 5 (Alternative): Dismiss Alert

For alerts that should not be fixed (e.g., version-specific test packages), offer to dismiss instead.

**Always get user approval first.** Present the dismissal option:

```
This alert should be dismissed rather than fixed because:
- <reason: version-specific test / dev-only acceptable risk / etc.>

Dismiss with reason: <suggested reason>
Comment: "<suggested comment>"

Proceed with dismissal?
```

**After user approval**, dismiss via GitHub API:

```bash
gh api --method PATCH repos/getsentry/sentry-javascript/dependabot/alerts/<number> \
-f state=dismissed \
-f dismissed_reason=<reason> \
-f dismissed_comment="<comment>"
```

**Dismissal reasons:**

| Reason | When to use |
| ---------------- | -------------------------------------------- |
| `tolerable_risk` | Dev-only dependency, risk accepted |
| `no_bandwidth` | Will fix later, not urgent |
| `inaccurate` | False positive, not actually vulnerable |
| `not_used` | Vulnerable code path is not used in our code |

## Commands Reference

| Command | Purpose |
| ------------------------------------------------------------------------------------------------- | ---------------------------- |
| `yarn why <pkg>` | Show dependency tree |
| `yarn dedupe-deps:fix` | Fix duplicates in yarn.lock |
| `yarn dedupe-deps:check` | Verify no duplicate issues |
| `gh api repos/getsentry/sentry-javascript/dependabot/alerts/<n>` | Fetch alert |
| `gh api --method PATCH .../dependabot/alerts/<n> -f state=dismissed -f dismissed_reason=<reason>` | Dismiss alert |
| `npm view <pkg>@latest dependencies.<dep>` | Check transitive dep version |

## Examples

### Dev dependency - safe to bump

```
Package: mongoose
Location: dev-packages/node-integration-tests/package.json
Type: Dev dependency (tests OTel instrumentation)

Recommendation: Safe to bump 5.x → 6.x
- Not version-specific, just tests instrumentation works
- OTel instrumentation supports mongoose 5.x-8.x
```

### Version-specific test - dismiss instead

```
Package: next
Location: dev-packages/e2e-tests/test-applications/nextjs-13/package.json

Recommendation: DISMISS (do not bump)
This app specifically tests Next.js 13 compatibility.
Vulnerability only affects CI, not shipped code.

Proposed dismissal:
Reason: tolerable_risk
Comment: "Version-specific E2E test for Next.js 13 - intentionally pinned"

Proceed with dismissal?
```

### Transitive dependency - bump parent

```
Package: vulnerable-lib@1.9.0 (needs >=2.0.1)
Chain: @sentry/node → @otel/instrumentation-foo@0.45.0 → vulnerable-lib

Check: npm view @otel/instrumentation-foo@latest dependencies.vulnerable-lib
Result: "^2.0.1" ✓

Recommendation: Bump @otel/instrumentation-foo 0.45.0 → 0.47.0
This pulls in the patched vulnerable-lib automatically.
```

### Transitive dependency - no fix available

```
Package: deep-lib@2.9.0 (needs >=3.0.0)
Chain: @sentry/node → parent-pkg → middleware → deep-lib

No upstream fix available yet. Options:
1. Wait for upstream fix (preferred)
2. Accept risk if dev-only
3. Consider alternative package if production-critical

AVOID using resolutions unless absolutely necessary.
```

## Important Notes

- **Never auto-commit** - Always wait for user review
- **Version-specific tests should not be bumped** - They exist to test specific versions
- **Dev vs Prod matters** - Dev-only vulnerabilities are lower priority
- **Bump parents, not transitive deps** - If A depends on vulnerable B, bump A
- **Avoid resolutions** - They bypass the parent's dependency constraints and can cause subtle breakage
- **Always verify** - Run `yarn why <pkg>` after fixing to confirm the patched version is installed
Loading