Skip to content

Remediate audit findings, replace vulnerable Firecrawl SDK, and harden release validation#1030

Open
jatmn wants to merge 6 commits intoGitlawb:mainfrom
hicap-oss:deps-update
Open

Remediate audit findings, replace vulnerable Firecrawl SDK, and harden release validation#1030
jatmn wants to merge 6 commits intoGitlawb:mainfrom
hicap-oss:deps-update

Conversation

@jatmn
Copy link
Copy Markdown
Collaborator

@jatmn jatmn commented May 5, 2026

Summary

This PR addresses the dependency and release issues fixed on deps-update

The main goal was to resolve the npm audit findings reported against @gitlawb/openclaude, remove the vulnerable Firecrawl SDK path, and make the release path more reliable after the npm latest mismatch reported in issue #1027.

What prompted this PR

Running npm audit against the installed package reported:

  • @anthropic-ai/sdk vulnerabilities affecting versions 0.79.0 - 0.91.0
  • axios vulnerabilities affecting versions 1.0.0 - 1.15.1
  • a vulnerable transitive path through @mendable/firecrawl-js

This branch addresses those findings in the package graph OpenClaude ships to consumers, and also hardens the release flow so a GitHub release does not succeed if npm has not actually updated latest to the expected version yet.

What changed

  • Upgraded direct dependencies:
    • @anthropic-ai/sdk from 0.81.0 to 0.94.0
    • axios from 1.15.0 to 1.16.0
    • @anthropic-ai/bedrock-sdk from 0.26.4 to 0.29.1
    • @anthropic-ai/vertex-sdk from 0.14.4 to 0.16.0
  • Regenerated bun.lock so Bun-based installs dedupe the Bedrock/Foundry/Vertex wrappers onto the patched Anthropic SDK path.
  • Removed @mendable/firecrawl-js entirely from package.json.
  • Replaced the Firecrawl SDK usage with a small in-repo fetch-based client used by:
    • src/tools/WebFetchTool/WebFetchTool.ts
    • src/tools/WebSearchTool/providers/firecrawl.ts
  • Preserved Firecrawl self-hosted support while adding behavior that is better aligned with the old integration:
    • AbortSignal support for request cancellation
    • transient 502 retry/backoff handling
    • focused unit coverage for the new client
  • Added a post-publish verification step to .github/workflows/release.yml so the release job fails if npm does not resolve both the package version and dist-tags.latest to the expected release version within the retry window.
  • Hardened Bun test isolation after release validation exposed process-global mock leakage across shared modules:
    • preserve full module surfaces when mocking shared env/provider modules
    • remove unnecessary env / envUtils mocks from tests that did not need them
    • make the Codex OAuth profile cleanup test use a fresh providerProfile module import
    • relax the Windows-only directory permission assertion in providerProfile tests

User impact

  • Consumers installing @gitlawb/openclaude no longer inherit the vulnerable Firecrawl SDK dependency path from this package.
  • OpenClaude’s bundled Firecrawl integration remains supported without the external SDK and now supports cancellation plus retry behavior for transient upstream failures.
  • Release automation is less likely to produce a GitHub release that appears complete before npm has actually updated the live latest tag.
  • The local release gate is more trustworthy on Windows because the full serialized Bun suite no longer fails due to unrelated shared-module mock leakage.

Validation

  • bun install --frozen-lockfile
  • bun test --max-concurrency=1
  • bun run smoke
  • bun run build
  • npm pack

Branch scope

This PR description covers only the branch-local work on deps-update:

  • dependency remediation
  • Firecrawl client replacement
  • npm release verification hardening
  • Bun test-harness isolation fixes discovered during release validation

jatmn added 2 commits May 5, 2026 13:13
Add a post-publish npm verification step to the release workflow so GitHub releases fail if the npm latest tag does not resolve to the expected version within the retry window.

Update dependency pins to remediate the audit findings by moving axios to 1.16.0, upgrading the Anthropic SDK to 0.94.0, and bumping the Bedrock and Vertex wrapper packages so Bun installs dedupe onto the patched SDK.

Replace the @mendable/firecrawl-js dependency with a small in-repo fetch-based Firecrawl client used by WebFetchTool and the Firecrawl web-search provider. Preserve self-hosted support, add transient 502 retry/backoff behavior, and cover the new client with focused tests.

Validation:
- bun test src/tools/firecrawl/client.test.ts src/tools/WebSearchTool/providers/firecrawl.test.ts
- bun run build
- bun run smoke
- packed-install npm audit --omit dev --json returned 0 vulnerabilities
Fix shared-module test leaks that were breaking providerProfile in the full serialized Bun suite.

- preserve full module surfaces when mocking env/provider modules
- remove unnecessary env/envUtils mocks from user/install surface tests
- use a fresh providerProfile module import for the Codex OAuth cleanup regression
- relax the Windows-only permission assertion in providerProfile tests

Validation:
- bun install --frozen-lockfile
- bun test --max-concurrency=1
- bun run smoke
- bun run build
- npm pack
@jatmn jatmn changed the title Deps update Remediate audit findings, replace vulnerable Firecrawl SDK, and harden release validation May 5, 2026
Copy link
Copy Markdown
Collaborator

@techbrewboss techbrewboss left a comment

Choose a reason for hiding this comment

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

I found one release-validation blocker while testing this PR.

Comment thread src/utils/effort.codex.test.ts Outdated
Fix the remaining cross-file Bun mock leak reported in review by expanding the persisted execa mock in src/utils/user.test.ts to include execaSync.

This keeps later imports that touch secure-storage and exec helpers from failing or hanging when bun test runs files serially after user.test.ts.

Validation:
- bun test src/utils/user.test.ts src/utils/effort.codex.test.ts
- bun test --max-concurrency=1
- bun run build
- bun run smoke
- npm pack
@jatmn jatmn marked this pull request as ready for review May 6, 2026 04:35
@techbrewboss
Copy link
Copy Markdown
Collaborator

Rereviewed latest head 232afcf. The execaSync symptom from the original comment is fixed, but the release-style cross-file failure is still present because src/utils/user.test.ts continues to leak partial ./auth.js and ./config.js mocks into later test files.

Current repros:

bun test src/utils/user.test.ts src/utils/effort.codex.test.ts
bun test src/utils/user.test.ts src/utils/openclaudeInstallSurfaces.test.ts

Current failures include:

SyntaxError: Export named 'isClaudeAISubscriber' not found in module src/utils/auth.ts
SyntaxError: Export named 'saveGlobalConfig' not found in module src/utils/config.ts

So the fix needs to address the underlying process-global partial mock leakage, not just add execaSync. Complete/pass-through mocks for the leaked modules, or restructuring the tests to avoid those global partial mocks before multi-file release validation, should close it.

Convert the auth, config, cwd, and execa mocks in src/utils/user.test.ts into pass-through mocks with targeted overrides.

This fixes the remaining Bun process-global mock leakage where later suites could fail or hang after user.test.ts because leaked partial mocks were missing exports such as auth/config helpers or execaSync.

Validation:
- bun test src/utils/user.test.ts src/utils/effort.codex.test.ts
- bun test src/utils/user.test.ts src/utils/openclaudeInstallSurfaces.test.ts
- bun test --max-concurrency=1
@jatmn
Copy link
Copy Markdown
Collaborator Author

jatmn commented May 6, 2026

Rereviewed latest head 232afcf. The execaSync symptom from the original comment is fixed, but the release-style cross-file failure is still present because src/utils/user.test.ts continues to leak partial ./auth.js and ./config.js mocks into later test files.

Current repros:

bun test src/utils/user.test.ts src/utils/effort.codex.test.ts
bun test src/utils/user.test.ts src/utils/openclaudeInstallSurfaces.test.ts

Current failures include:

SyntaxError: Export named 'isClaudeAISubscriber' not found in module src/utils/auth.ts
SyntaxError: Export named 'saveGlobalConfig' not found in module src/utils/config.ts

So the fix needs to address the underlying process-global partial mock leakage, not just add execaSync. Complete/pass-through mocks for the leaked modules, or restructuring the tests to avoid those global partial mocks before multi-file release validation, should close it.

should be fixed now

@techbrewboss
Copy link
Copy Markdown
Collaborator

Rereviewed latest head 12744d7.

Good news: the cross-file mock issue is fixed now. These repros all pass for me:

bun test src/utils/user.test.ts src/utils/effort.codex.test.ts
bun test src/utils/user.test.ts src/utils/openclaudeInstallSurfaces.test.ts
bun test --max-concurrency=1 src/utils/user.test.ts src/utils/effort.codex.test.ts src/utils/openclaudeInstallSurfaces.test.ts src/utils/providerProfile.test.ts

I also verified:

bun test src/tools/firecrawl/client.test.ts src/tools/WebSearchTool/providers/firecrawl.test.ts
bun test src/tools/WebFetchTool
bun run smoke

One remaining issue, though: bun audit now fails with a moderate advisory:

ip-address <=10.1.0
@modelcontextprotocol/sdk -> express-rate-limit -> ip-address
moderate: ip-address has XSS in Address6 HTML-emitting methods

bun pm why ip-address resolves this to ip-address@10.1.0 via express-rate-limit@8.5.0, and npm view ip-address version shows 10.2.0 is available. Since this PR is framed as audit remediation, I think it should leave bun audit clean or explicitly document why this advisory is accepted/not reachable.

Add a top-level override for ip-address and refresh bun.lock so the MCP SDK -> express-rate-limit path resolves to ip-address@10.2.0 instead of 10.1.0.

This keeps the branch's audit-remediation scope aligned with the remaining transitive advisory path without changing the direct MCP SDK pin.

Validation:
- bun pm why ip-address
- bun audit
@jatmn
Copy link
Copy Markdown
Collaborator Author

jatmn commented May 6, 2026

Rereviewed latest head 12744d7.

Good news: the cross-file mock issue is fixed now. These repros all pass for me:

bun test src/utils/user.test.ts src/utils/effort.codex.test.ts
bun test src/utils/user.test.ts src/utils/openclaudeInstallSurfaces.test.ts
bun test --max-concurrency=1 src/utils/user.test.ts src/utils/effort.codex.test.ts src/utils/openclaudeInstallSurfaces.test.ts src/utils/providerProfile.test.ts

I also verified:

bun test src/tools/firecrawl/client.test.ts src/tools/WebSearchTool/providers/firecrawl.test.ts
bun test src/tools/WebFetchTool
bun run smoke

One remaining issue, though: bun audit now fails with a moderate advisory:

ip-address <=10.1.0
@modelcontextprotocol/sdk -> express-rate-limit -> ip-address
moderate: ip-address has XSS in Address6 HTML-emitting methods

bun pm why ip-address resolves this to ip-address@10.1.0 via express-rate-limit@8.5.0, and npm view ip-address version shows 10.2.0 is available. Since this PR is framed as audit remediation, I think it should leave bun audit clean or explicitly document why this advisory is accepted/not reachable.

cant fully resolve that one, updated to more recent version but a dependent in the chain we dont control is still out of date. So audit isnt fully green.. think we should come back in a week or so and re-run this audit for possible fixes then as well.

Comment thread package.json
kevincodex1
kevincodex1 previously approved these changes May 6, 2026
Copy link
Copy Markdown
Contributor

@kevincodex1 kevincodex1 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Copy Markdown
Collaborator

@Vasanthdev2004 Vasanthdev2004 left a comment

Choose a reason for hiding this comment

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

Scope: Targeted review of current merge readiness only.

Verdict: Needs changes

This PR is currently DIRTY against main, so it cannot merge as-is and the final diff may change during conflict resolution. Please rebase/merge latest main and resolve the conflicts, then ping for a full review of the dependency, Firecrawl replacement, release workflow, and test-isolation changes.

Notes from the current state:

  • The stated scope is important and worth doing: dependency audit remediation, replacing the vulnerable Firecrawl SDK path, and release verification hardening.
  • Current GitHub checks are green, but they are from the pre-conflict head.
  • Kevin has approved, but I do not want to overclaim a full approval until the conflict-resolved head is available.

Happy to re-review once the branch is clean.

@jatmn
Copy link
Copy Markdown
Collaborator Author

jatmn commented May 7, 2026

Scope: Targeted review of current merge readiness only.

Verdict: Needs changes

This PR is currently DIRTY against main, so it cannot merge as-is and the final diff may change during conflict resolution. Please rebase/merge latest main and resolve the conflicts, then ping for a full review of the dependency, Firecrawl replacement, release workflow, and test-isolation changes.

Notes from the current state:

  • The stated scope is important and worth doing: dependency audit remediation, replacing the vulnerable Firecrawl SDK path, and release verification hardening.
  • Current GitHub checks are green, but they are from the pre-conflict head.
  • Kevin has approved, but I do not want to overclaim a full approval until the conflict-resolved head is available.

Happy to re-review once the branch is clean.

updated.

@jatmn jatmn requested a review from Vasanthdev2004 May 7, 2026 16:40
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.

4 participants