fix(fix): validate target directory and detect misplaced IDs#1227
fix(fix): validate target directory and detect misplaced IDs#1227John-David Dalton (jdalton) merged 4 commits intomainfrom
Conversation
Backport of v1.x #1199 to main. Two quality-of-life fixes for \`socket fix\` that surface clear error messages instead of the confusing API error the user hits today: 1. **Misplaced vulnerability ID**: if the first positional arg looks like a GHSA, CVE, or PURL, we treat it as a directory path and eventually fail with "Need at least one file to be uploaded" from the API. Now we detect the pattern early, fail fast with a "Did you mean: socket fix --id <that value>" hint. 2. **Nonexistent target directory**: previously the directory wasn't validated upfront, so we'd happily resolve it, glob zero files, upload nothing, and the API would reject with the same confusing "Need at least one file to be uploaded" message. Now we \`existsSync()\` the resolved path and fail with "Target directory does not exist: <path>". Tests: * Reworked the existing "should support custom cwd argument" test to pass \`process.cwd()\` (guaranteed to exist) now that we validate the dir. * Added "should fail fast when target directory does not exist". * Added "should fail fast when a GHSA is passed as a positional arg".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Addresses both Cursor bugbot findings on #1227: 1. **[Medium] Misplaced-ID check was behind getDefaultOrgSlug()**. If a user had no API token configured, `getDefaultOrgSlug()` would fail first and the helpful "looks like a vulnerability identifier" / "Target directory does not exist" messages never reached them. Moved both validation blocks ahead of the org-slug resolution — they only depend on `cli.input[0]`, no token required. 2. **[Low] Case-insensitive detection with verbatim echo**. The regex matched `ghsa-abcd-…` but `handle-fix.mts` matches the GHSA/CVE prefix case-sensitively, so the suggested `socket fix --id ghsa-…` would fail downstream with "Unsupported ID format". Now we normalize GHSA/CVE to uppercase in the suggestion while keeping PURLs unchanged (they're intentionally lowercase). Added tests: - uppercase normalization of a lowercase GHSA in the suggestion. - validation short-circuits before `getDefaultOrgSlug` for both the misplaced-ID path and the missing-target-dir path.
|
Both Cursor bugbot findings addressed in e6a91f7: [Medium] Misplaced-ID check runs after org-slug network call — moved both validation blocks (misplaced ID + missing target dir) to fire before [Low] Case-insensitive detection suggests command that fails downstream — Added three tests covering the uppercase normalization and the two "runs before org-slug resolution" paths. |
|
Cursor (@cursor) review |
Cursor bugbot caught a second-order bug in my earlier case fix on #1227. The previous commit uppercased the *entire* rawInput, but handle-fix.mts's format validator is GHSA_FORMAT_REGEXP = /^GHSA-[a-z0-9]{4}-[a-z0-9]{4}-[a-z0-9]{4}$/ which requires the body segments to be lowercase. A user typing `ghsa-abcd-efgh-ijkl` would have been told to run `socket fix --id GHSA-ABCD-EFGH-IJKL`, and that command would fail downstream with "Invalid GHSA format". Fix: only uppercase the prefix. * GHSA: `'GHSA-' + body.toLowerCase()` * CVE: `'CVE-' + rest` (body is digits + dashes, case-free) * PURL: leave verbatim. Added tests covering all three branches.
|
Good catch from Cursor (@cursor) — the prior fix uppercased the whole GHSA ID, but Fixed in 3997897: only uppercase the prefix.
Added tests for all three branches plus the PURL passthrough. |
|
Cursor (@cursor) review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3997897. Configure here.
Tightens the cmd-fix tests introduced with the input-validation work:
* Consolidates four separate ID-detection tests into an `it.each`
table that exercises the real case matrix against handle-fix.mts's
downstream format regexes:
- canonical / lowercase / mixed-case GHSA
- canonical / lowercase CVE
- PURL (unchanged)
Each row asserts both the "looks like a vulnerability identifier"
message and the exact downstream-valid `--id <suggestion>` form.
Previously a mixed-case GHSA and the canonical CVE/GHSA cases
weren't covered, so a future regression to the case-normalization
logic could slip by.
* Splits the directory-validation tests into a dedicated
`describe('target directory validation', ...)` block and adds a
happy-path assertion ("lets a real directory flow through to
handleFix") so we can tell the difference between "validation
passed" and "validation silently skipped".
* Folded the redundant "should support custom cwd argument" test
into the happy-path assertion — same setup, one test instead of
two with overlapping mocks.
* Order-of-operations assertions (that validation runs before
`getDefaultOrgSlug`) now live next to the related bail tests
instead of floating at the end of the outer describe.
Net: 6 cases in the ID-detection table (up from 3 hardcoded), plus
3 directory-validation tests including the happy path. 47 tests total.
|
Cursor (@cursor) review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e7a1bb1. Configure here.
John-David Dalton (jdalton)
deleted the
backport/v1x-fix-input-validation
branch
2 participants
Summary
Backport of v1.x #1199 to main. Two quality-of-life fixes for `socket fix` that surface clear error messages instead of the confusing API error users hit today.
Misplaced vulnerability ID
Before:
```
$ socket fix GHSA-xxxx-xxxx-xxxx
✖ Socket API error: Need at least one file to be uploaded
```
After:
```
$ socket fix GHSA-xxxx-xxxx-xxxx
✖ "GHSA-xxxx-xxxx-xxxx" looks like a vulnerability identifier, not a directory path.
Did you mean: socket fix --id GHSA-xxxx-xxxx-xxxx
```
Triggered on first positional arg matching `/^GHSA-/`, `/^CVE-/`, or `pkg:…`.
Nonexistent target directory
Before: silently resolve to absolute, glob zero files, API rejects with the same confusing "Need at least one file to be uploaded".
After:
```
$ socket fix ./does-not-exist
✖ Target directory does not exist: /absolute/path/to/does-not-exist
```
Tests
Test plan
Note
Low Risk
Low risk: adds early CLI input validation and clearer error messages, with corresponding unit tests; main risk is minor behavior change for users previously relying on implicit path handling.
Overview
Improves
socket fixUX by detecting GHSA/CVE/PURL values mistakenly passed as the positional CWD and exiting with a helpful--idsuggestion instead of proceeding to a confusing upload error.Adds fast-fail validation that the target directory exists (before org-slug/API token resolution) to surface a clear local error and avoid the API’s generic “Need at least one file to be uploaded” failure. Unit tests are updated/expanded to cover the new validation and ordering guarantees.
Reviewed by Cursor Bugbot for commit e7a1bb1. Configure here.