|
| 1 | +--- |
| 2 | +name: fix-security-vulnerability |
| 3 | +description: Analyze and propose fixes for Dependabot security alerts |
| 4 | +argument-hint: <dependabot-alert-url> |
| 5 | +--- |
| 6 | + |
| 7 | +# Fix Security Vulnerability Skill |
| 8 | + |
| 9 | +Analyze Dependabot security alerts and propose fixes. **Does NOT auto-commit** - always presents analysis first and waits for user approval. |
| 10 | + |
| 11 | +## Input |
| 12 | + |
| 13 | +- Dependabot URL: `https://github.com/getsentry/sentry-javascript/security/dependabot/1046` |
| 14 | +- Or just the alert number: `1046` |
| 15 | + |
| 16 | +## Workflow |
| 17 | + |
| 18 | +### Step 1: Fetch Vulnerability Details |
| 19 | + |
| 20 | +```bash |
| 21 | +gh api repos/getsentry/sentry-javascript/dependabot/alerts/<alert-number> |
| 22 | +``` |
| 23 | + |
| 24 | +Extract: package name, vulnerable/patched versions, CVE ID, severity, description. |
| 25 | + |
| 26 | +### Step 2: Analyze Dependency Tree |
| 27 | + |
| 28 | +```bash |
| 29 | +yarn why <package-name> |
| 30 | +``` |
| 31 | + |
| 32 | +Determine if it's a direct or transitive dependency, and whether it's production or dev. |
| 33 | + |
| 34 | +### Step 3: Determine Fix Strategy |
| 35 | + |
| 36 | +#### Check for version-specific test packages |
| 37 | + |
| 38 | +Many packages in `dev-packages/e2e-tests/test-applications/` intentionally pin specific versions: |
| 39 | + |
| 40 | +- `nextjs-13` - Tests Next.js 13.x, should NOT bump to 14 |
| 41 | +- `remix-2` - Tests Remix 2.x specifically |
| 42 | + |
| 43 | +**Do NOT bump these.** Recommend dismissing the alert with an explanation. |
| 44 | + |
| 45 | +#### For other dependencies |
| 46 | + |
| 47 | +| Type | Action | |
| 48 | +| --------------------- | ----------------------------------- | |
| 49 | +| Patch bump available | Preferred - lowest risk | |
| 50 | +| Minor bump needed | Usually safe | |
| 51 | +| Major bump needed | Analyze breaking changes first | |
| 52 | +| Transitive dependency | Bump the parent package (see below) | |
| 53 | + |
| 54 | +### Step 3a: Transitive Dependencies |
| 55 | + |
| 56 | +If the vulnerable package is pulled in by another package: |
| 57 | + |
| 58 | +**1. Identify and check the parent:** |
| 59 | + |
| 60 | +```bash |
| 61 | +yarn why <vulnerable-package> |
| 62 | +npm view <parent-package>@latest dependencies.<vulnerable-package> |
| 63 | +``` |
| 64 | + |
| 65 | +**2. Fix approach:** |
| 66 | + |
| 67 | +| Scenario | Action | |
| 68 | +| --------------------------------- | ------------------------------- | |
| 69 | +| Parent has newer version with fix | **Bump the parent** | |
| 70 | +| Parent hasn't released fix | Wait, or open an issue upstream | |
| 71 | +| We control the parent | Fix in parent package first | |
| 72 | + |
| 73 | +**AVOID RESOLUTIONS.** Using `resolutions` to force a transitive dependency version is risky - it can break the parent package silently. Only consider resolutions if: |
| 74 | + |
| 75 | +- No upstream fix exists AND it's a production-critical vulnerability |
| 76 | +- The forced version is a patch/minor bump (not major) |
| 77 | +- You've manually verified compatibility |
| 78 | + |
| 79 | +In most cases, it's better to wait for an upstream fix or accept the risk for dev-only dependencies than to use resolutions. |
| 80 | + |
| 81 | +### Step 4: Present Analysis |
| 82 | + |
| 83 | +Present findings and **wait for user approval** before making changes: |
| 84 | + |
| 85 | +``` |
| 86 | +## Security Vulnerability Analysis |
| 87 | +
|
| 88 | +**Package:** <name> | **Severity:** <severity> | **CVE:** <id> |
| 89 | +**Vulnerable:** <range> | **Patched:** <version> |
| 90 | +
|
| 91 | +### Dependency Chain |
| 92 | +<yarn why output> |
| 93 | +
|
| 94 | +### Recommendation |
| 95 | +<One of: Safe to bump / Version-specific test - do not bump / Bump parent package> |
| 96 | +
|
| 97 | +### Proposed Fix |
| 98 | +1. Update <file>: "<package>": "<new-version>" |
| 99 | +2. yarn install && yarn dedupe-deps:fix |
| 100 | +3. Verify with: yarn why <package> |
| 101 | +
|
| 102 | +Proceed? |
| 103 | +``` |
| 104 | + |
| 105 | +### Step 5: Apply Fix (After Approval) |
| 106 | + |
| 107 | +```bash |
| 108 | +# 1. Edit package.json |
| 109 | +# 2. Update lockfile |
| 110 | +yarn install |
| 111 | +# 3. Deduplicate |
| 112 | +yarn dedupe-deps:fix |
| 113 | +# 4. Verify |
| 114 | +yarn dedupe-deps:check |
| 115 | +yarn why <package> |
| 116 | +# 5. Show changes |
| 117 | +git diff |
| 118 | +``` |
| 119 | + |
| 120 | +**Do NOT commit** - let the user review first. |
| 121 | + |
| 122 | +### Step 5 (Alternative): Dismiss Alert |
| 123 | + |
| 124 | +For alerts that should not be fixed (e.g., version-specific test packages), offer to dismiss instead. |
| 125 | + |
| 126 | +**Always get user approval first.** Present the dismissal option: |
| 127 | + |
| 128 | +``` |
| 129 | +This alert should be dismissed rather than fixed because: |
| 130 | +- <reason: version-specific test / dev-only acceptable risk / etc.> |
| 131 | +
|
| 132 | +Dismiss with reason: <suggested reason> |
| 133 | +Comment: "<suggested comment>" |
| 134 | +
|
| 135 | +Proceed with dismissal? |
| 136 | +``` |
| 137 | + |
| 138 | +**After user approval**, dismiss via GitHub API: |
| 139 | + |
| 140 | +```bash |
| 141 | +gh api --method PATCH repos/getsentry/sentry-javascript/dependabot/alerts/<number> \ |
| 142 | + -f state=dismissed \ |
| 143 | + -f dismissed_reason=<reason> \ |
| 144 | + -f dismissed_comment="<comment>" |
| 145 | +``` |
| 146 | + |
| 147 | +**Dismissal reasons:** |
| 148 | + |
| 149 | +| Reason | When to use | |
| 150 | +| ---------------- | -------------------------------------------- | |
| 151 | +| `tolerable_risk` | Dev-only dependency, risk accepted | |
| 152 | +| `no_bandwidth` | Will fix later, not urgent | |
| 153 | +| `inaccurate` | False positive, not actually vulnerable | |
| 154 | +| `not_used` | Vulnerable code path is not used in our code | |
| 155 | + |
| 156 | +## Commands Reference |
| 157 | + |
| 158 | +| Command | Purpose | |
| 159 | +| ------------------------------------------------------------------------------------------------- | ---------------------------- | |
| 160 | +| `yarn why <pkg>` | Show dependency tree | |
| 161 | +| `yarn dedupe-deps:fix` | Fix duplicates in yarn.lock | |
| 162 | +| `yarn dedupe-deps:check` | Verify no duplicate issues | |
| 163 | +| `gh api repos/getsentry/sentry-javascript/dependabot/alerts/<n>` | Fetch alert | |
| 164 | +| `gh api --method PATCH .../dependabot/alerts/<n> -f state=dismissed -f dismissed_reason=<reason>` | Dismiss alert | |
| 165 | +| `npm view <pkg>@latest dependencies.<dep>` | Check transitive dep version | |
| 166 | + |
| 167 | +## Examples |
| 168 | + |
| 169 | +### Dev dependency - safe to bump |
| 170 | + |
| 171 | +``` |
| 172 | +Package: mongoose |
| 173 | +Location: dev-packages/node-integration-tests/package.json |
| 174 | +Type: Dev dependency (tests OTel instrumentation) |
| 175 | +
|
| 176 | +Recommendation: Safe to bump 5.x → 6.x |
| 177 | +- Not version-specific, just tests instrumentation works |
| 178 | +- OTel instrumentation supports mongoose 5.x-8.x |
| 179 | +``` |
| 180 | + |
| 181 | +### Version-specific test - dismiss instead |
| 182 | + |
| 183 | +``` |
| 184 | +Package: next |
| 185 | +Location: dev-packages/e2e-tests/test-applications/nextjs-13/package.json |
| 186 | +
|
| 187 | +Recommendation: DISMISS (do not bump) |
| 188 | +This app specifically tests Next.js 13 compatibility. |
| 189 | +Vulnerability only affects CI, not shipped code. |
| 190 | +
|
| 191 | +Proposed dismissal: |
| 192 | + Reason: tolerable_risk |
| 193 | + Comment: "Version-specific E2E test for Next.js 13 - intentionally pinned" |
| 194 | +
|
| 195 | +Proceed with dismissal? |
| 196 | +``` |
| 197 | + |
| 198 | +### Transitive dependency - bump parent |
| 199 | + |
| 200 | +``` |
| 201 | +Package: vulnerable-lib@1.9.0 (needs >=2.0.1) |
| 202 | +Chain: @sentry/node → @otel/instrumentation-foo@0.45.0 → vulnerable-lib |
| 203 | +
|
| 204 | +Check: npm view @otel/instrumentation-foo@latest dependencies.vulnerable-lib |
| 205 | +Result: "^2.0.1" ✓ |
| 206 | +
|
| 207 | +Recommendation: Bump @otel/instrumentation-foo 0.45.0 → 0.47.0 |
| 208 | +This pulls in the patched vulnerable-lib automatically. |
| 209 | +``` |
| 210 | + |
| 211 | +### Transitive dependency - no fix available |
| 212 | + |
| 213 | +``` |
| 214 | +Package: deep-lib@2.9.0 (needs >=3.0.0) |
| 215 | +Chain: @sentry/node → parent-pkg → middleware → deep-lib |
| 216 | +
|
| 217 | +No upstream fix available yet. Options: |
| 218 | +1. Wait for upstream fix (preferred) |
| 219 | +2. Accept risk if dev-only |
| 220 | +3. Consider alternative package if production-critical |
| 221 | +
|
| 222 | +AVOID using resolutions unless absolutely necessary. |
| 223 | +``` |
| 224 | + |
| 225 | +## Important Notes |
| 226 | + |
| 227 | +- **Never auto-commit** - Always wait for user review |
| 228 | +- **Version-specific tests should not be bumped** - They exist to test specific versions |
| 229 | +- **Dev vs Prod matters** - Dev-only vulnerabilities are lower priority |
| 230 | +- **Bump parents, not transitive deps** - If A depends on vulnerable B, bump A |
| 231 | +- **Avoid resolutions** - They bypass the parent's dependency constraints and can cause subtle breakage |
| 232 | +- **Always verify** - Run `yarn why <pkg>` after fixing to confirm the patched version is installed |
0 commit comments