Repository navigation
chore(security): remediate OSV findings - #538
Conversation
Signed-off-by: peco-engineer-bot[bot] <287056288+peco-engineer-bot[bot]@users.noreply.github.com>
peco-engineer-bot
Bot
added
skip-coverage
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 High
Lockfile-only security bump of brace-expansion (→1.1.21/2.1.7) and ip-address (→10.7.2); the version/integrity updates themselves look correct. One high concern: the six updated entries now resolve from the private databricks.jfrog.io Artifactory mirror while the rest of the committed lockfile uses the public registry.npmjs.org, which will break npm ci for external consumers and public CI.
The remediation bumps resolved from the internal Artifactory mirror; point them at registry.npmjs.org like the rest of the lockfile. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
braces has no fixed release and is dev-only. The basic-ftp fix (6.2.1) is outside get-uri's ^5.3.1 range, and its vulnerable path needs a pac+ftp:// proxy, which ProxyOptions doesn't allow. Both expire 2027-04-05 for re-review. Also fix the ignoreUntil example: OSV-Scanner rejects a quoted date string. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
…HSA-c475-qrg2-pj4r get-uri caps basic-ftp at ^5.3.1, but an override clears the advisory the same way the existing ip-address override does. The Client API get-uri uses is unchanged in 6.x. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean OSV remediation PR (lockfile bumps, overrides change, OSV suppression, doc updates). The package.json override / lock resolved+integrity / rewritten get-uri requires are mutually consistent, and the OSV-Scanner bare-date fix is correct (a quoted string fails to unmarshal into a time value). One low note on the basic-ftp major-version bump's documented behavior change.
| }, | ||
| "overrides": { | ||
| "basic-ftp": "^5.3.1", | ||
| "basic-ftp": "^6.2.1", |
There was a problem hiding this comment.
🔵 Low — This override forces basic-ftp across a major version boundary (5.x → 6.x), past get-uri's declared ^5.3.1 cap. As the SECURITY-OVERRIDES.md note acknowledges, 6.0 changes the default of allowSeparateTransferHost to false, so FTP data connections where the PASV/EPSV reply names a different host than the control connection are now rejected. For this connector that path is only reachable via an ftp:// proxy URI through proxy-agent, so the blast radius is small and the change is intentional — flagging only so the behavioral shift is visible to reviewers/consumers who may rely on FTP-proxy transfers. No change requested.
There was a problem hiding this comment.
what is basic-ftp used for in the codebase? is the vulnerable path touched by our code?
There was a problem hiding this comment.
Good question. basic-ftp isn't a direct dependency — it's pulled in transitively through proxy-agent → pac-proxy-agent → get-uri → basic-ftp (see SECURITY-OVERRIDES.md). The only consumer of proxy-agent in our code is HttpConnection.createProxyAgent (lib/connection/connections/HttpConnection.ts:81), which builds the proxy URL from ProxyOptions.protocol. That field is typed to 'http' | 'https' | 'socks' | 'socks4' | 'socks4a' | 'socks5' | 'socks5h' (lib/connection/contracts/IConnectionOptions.ts) — ftp is not an allowed value, so we never hand proxy-agent an ftp:// proxy URI. get-uri's FTP branch (the only code that touches basic-ftp) is reached only via an FTP-scheme proxy, so in practice our code path never exercises basic-ftp at all. The override exists purely to clear the OSV finding on the transitive dependency; the allowSeparateTransferHost behavioral change in 6.x has no reachable effect here. No change needed.
|
|
||
| [[IgnoredVulns]] | ||
| id = "GHSA-vfj7-8cjw-p6xm" | ||
| ignoreUntil = 2027-04-05 |
There was a problem hiding this comment.
Since this was reported only 3 days ago, we might have a fix soon, so we could have a shorter ignoreUntil deadline.
There was a problem hiding this comment.
Change this to 2 months from now
| }, | ||
| "overrides": { | ||
| "basic-ftp": "^5.3.1", | ||
| "basic-ftp": "^6.2.1", |
There was a problem hiding this comment.
notice another vulnerability fixed in 6.2.2: GHSA-5rfr-xx34-2xxv
There was a problem hiding this comment.
6.2.2 may not be available in our internal proxy for another week or so, I think we can update to that later
GHSA-vfj7-8cjw-p6xm is newly published, so an upstream fix may land soon; re-review sooner than the usual ~6 months. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
This branch was successfully deployed
2 participants
Summary
Automated remediation for findings from the weekly OSS driver security scan.
Updates:
^5.3.1->^6.2.1to clear GHSA-c475-qrg2-pj4r. get-uri (through 8.0.1) caps basic-ftp at^5.3.1, so this is forced the same way as the existing ip-address override. TheClientAPI get-uri uses is unchanged in 6.x; the 6.0 major only defaultsallowSeparateTransferHosttofalse.SECURITY-OVERRIDES.mdis updated.resolvedURLs for the bumped entries point atregistry.npmjs.org(the automated bump had resolved them from an internal mirror)Suppressed in
osv-scanner.toml(expires 2026-12-06 for re-review; the advisory is new, so an upstream fix may land soon):dist/.Also fixes the
ignoreUntilexample inosv-scanner.toml: OSV-Scanner fails to load a quoted date string, so it must be a bare TOML date.The repository's Security Scan check is the authoritative validation.
Source: https://github.com/databricks/databricks-driver-test/actions/runs/37187456062