Skip to content

feat: block unsafe npm package installs#3152

Merged
wwwillchen merged 15 commits intomainfrom
socket-firewall-package-installs
Apr 7, 2026
Merged

feat: block unsafe npm package installs#3152
wwwillchen merged 15 commits intomainfrom
socket-firewall-package-installs

Conversation

@keppo-bot
Copy link
Copy Markdown
Contributor

@keppo-bot keppo-bot bot commented Apr 7, 2026

Summary

  • add a default-on Experiments setting to block unsafe npm packages with Socket Firewall
  • wrap shared add-dependency installs in sfw when available and bootstrap sfw via npm install -g sfw when it is missing
  • surface firewall bootstrap warnings through build-mode approvals and local-agent add_dependency flows, with tests for the new setting and install path

Test plan

  • npm run fmt && npm run lint:fix && npm run ts
  • npm test

🤖 Generated with Claude Code


Open with Devin

@keppo-bot keppo-bot bot requested a review from a team April 7, 2026 00:47
github-advanced-security[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 22 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

devin-ai-integration[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@github-actions github-actions bot added the needs-human:review-issue ai agent flagged an issue that requires human review label Apr 7, 2026
chatgpt-codex-connector[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@keppo-bot keppo-bot bot removed the needs-human:review-issue ai agent flagged an issue that requires human review label Apr 7, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

@github-actions github-actions bot added the needs-human:review-issue ai agent flagged an issue that requires human review label Apr 7, 2026
chatgpt-codex-connector[bot]

This comment was marked as resolved.

wwwillchen and others added 2 commits April 6, 2026 21:01
- preserve socket firewall warnings across error paths
- fall back to direct package installs when sfw has runtime failures
- restore Windows shell execution for npm-style commands

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 490a6e7477

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +138 to +140
warningMessages.push(SOCKET_FIREWALL_FALLBACK_WARNING_MESSAGE);
({ succeeded, installResults, lastError } = await runAddDependencyCommands(
buildAddDependencyCommands(packages, false),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Delay fallback warning until plain install actually succeeds

This branch pushes SOCKET_FIREWALL_FALLBACK_WARNING_MESSAGE before the unwrapped pnpm/npm retry runs, so when the fallback commands also fail the user still gets a warning that says Dyad installed the packages without firewall checks. In that failure scenario the warning is factually wrong and can send users debugging the wrong issue; only emit this warning after the fallback succeeds (or use a different message for fallback failure).

Useful? React with 👍 / 👎.

@keppo-bot keppo-bot bot removed the needs-human:review-issue ai agent flagged an issue that requires human review label Apr 7, 2026
const updatedContent = message.content.replace(
new RegExp(
`<dyad-add-dependency packages="${packages.join(" ")}">[^<]*</dyad-add-dependency>`,
`<dyad-add-dependency packages="(?:${buildPackagesAttrPattern(packages)})">[\\s\\S]*?</dyad-add-dependency>`,
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 89796cad72

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +115 to 119
const packageManager = await detectPreferredPackageManager();
let { succeeded, installResults, lastError } = await runAddDependencyCommand(
buildAddDependencyCommand(packages, packageManager, useSocketFirewall),
appPath,
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Retry npm when pnpm add fails

This change now selects a single package manager based only on whether pnpm exists and executes one install path, so when pnpm is present but pnpm add exits non-zero (for example, repo-specific pnpm failures), the function throws instead of retrying with npm. The previous behavior explicitly attempted npm after a pnpm failure, so this is a regression that can make dependency installs fail on environments that happen to have pnpm installed.

Useful? React with 👍 / 👎.

@github-actions github-actions bot added the needs-human:review-issue ai agent flagged an issue that requires human review label Apr 7, 2026
@keppo-bot keppo-bot bot removed the needs-human:review-issue ai agent flagged an issue that requires human review label Apr 7, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 10df01b0cd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

src/main.ts Outdated
// Cleanup old media files to reclaim disk space
cleanupOldMediaFiles();

await applyRemoteSettingsDefaultsIfNeeded();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid blocking startup on remote settings fetch

onReady() now awaits applyRemoteSettingsDefaultsIfNeeded() before createWindow(), so app launch is gated on the remote desktop-config request. Because that fetch uses a 5,000 ms timeout, users on slow/offline networks can see a multi-second cold-start delay before any window appears; running this fetch asynchronously (or after first window creation) would avoid this startup regression while still applying defaults.

Useful? React with 👍 / 👎.

@github-actions github-actions bot added the needs-human:review-issue ai agent flagged an issue that requires human review label Apr 7, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 30c0c7296e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

}) {
const packageStr = packages.join(" ");
}): Promise<ExecuteAddDependencyResult> {
const settings = readEffectiveSettings();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Read effective settings asynchronously for dependency installs

executeAddDependency uses the synchronous readEffectiveSettings() path, which only consults getCachedRemoteDesktopConfig() and then defaults blockUnsafeNpmPackages to true when the cache is missing/expired. With an unset local value and a remote default of false, this makes install behavior flip back to firewall-on after cache expiry (or before any async refresh), so package installs can unexpectedly start failing in the same session. Please use the async effective-settings read here (or otherwise persist/refresh the last remote default) so remote defaults stay consistent.

Useful? React with 👍 / 👎.

Comment on lines +75 to +77
remoteDesktopConfigCache = {
config: null,
expiresAt: Date.now() + FAILURE_CACHE_TTL_MS,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve last known remote config on fetch failures

On fetch errors this branch overwrites the cache with config: null, which discards any previously successful remote defaults. For users without a local override, a transient network failure then changes effective behavior (via fallback to true) until the next successful fetch, causing settings-driven install policy to flap. Keeping the last good config while applying a shorter retry TTL would avoid these unintended flips.

Useful? React with 👍 / 👎.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2026

🎭 Playwright Test Results

❌ Some tests failed

OS Passed Failed Flaky Skipped
🍎 macOS 406 2 4 131
🪟 Windows 406 11 4 131

Summary: 812 passed, 13 failed, 8 flaky, 262 skipped

Failed Tests

🍎 macOS

  • queued_message.spec.ts > editing queued message restores attachments and selected components
    • Error: expect(locator).toBeVisible() failed
  • queued_message.spec.ts > canceling queued message edit clears restored components
    • Error: expect(locator).toBeVisible() failed

🪟 Windows

Show all 11 failures
  • context_manage.spec.ts > manage context - default
    • Error: expect(locator).toMatchAriaSnapshot(expected) failed
  • context_manage.spec.ts > manage context - smart context
    • Error: expect(locator).toMatchAriaSnapshot(expected) failed
  • context_manage.spec.ts > manage context - smart context - auto-includes only
    • Error: expect(locator).toMatchAriaSnapshot(expected) failed
  • context_manage.spec.ts > manage context - exclude paths
    • Error: expect(locator).toMatchAriaSnapshot(expected) failed
  • context_manage.spec.ts > manage context - exclude paths with smart context
    • Error: expect(locator).toMatchAriaSnapshot(expected) failed
  • github.spec.ts > should connect to GitHub using device flow
    • Error: expect(locator).toBeVisible() failed
  • github.spec.ts > create and sync to new repo
    • Error: expect(locator).toHaveClass(expected) failed
  • github.spec.ts > create and sync to new repo - custom branch
    • TimeoutError: locator.click: Timeout 30000ms exceeded.
  • github.spec.ts > create and sync to existing repo
    • Error: expect(locator).toMatchAriaSnapshot(expected) failed
  • github.spec.ts > create and sync to existing repo - custom branch
    • Error: expect(locator).toMatchAriaSnapshot(expected) failed
  • github.spec.ts > github clear integration settings
    • Error: expect(locator).toBeVisible() failed

📋 Re-run Failing Tests (macOS)

Copy and paste to re-run all failing spec files locally:

npm run e2e \
  e2e-tests/queued_message.spec.ts

⚠️ Flaky Tests

🍎 macOS

  • chat_input.spec.ts > send button disabled during pending proposal - reject (passed after 1 retry)
  • context_limit_banner.spec.ts > context limit banner shows 'running out' when near context limit (passed after 1 retry)
  • debugging_logs.spec.ts > clear logs button clears all logs (passed after 1 retry)
  • setup_flow.spec.ts > Setup Flow > setup banner shows correct state when node.js is installed (passed after 1 retry)

🪟 Windows

  • edit_code.spec.ts > edit code (passed after 2 retries)
  • git_collaboration.spec.ts > Git Collaboration > should create, switch, rename, merge, and delete branches (passed after 1 retry)
  • setup_flow.spec.ts > Setup Flow > setup banner shows correct state when node.js is installed (passed after 1 retry)
  • setup_flow.spec.ts > Setup Flow > node.js install flow (passed after 2 retries)

📊 View full report

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 190ce65df6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

createTypedHandler(settingsContracts.setUserSettings, async (_, settings) => {
writeSettings(settings);
return readSettings();
return readEffectiveSettings();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid awaiting remote config in settings write handler

setUserSettings now returns readEffectiveSettings(), which performs a remote desktop-config fetch (with a 5s timeout on cache miss) before resolving. In offline/slow-network scenarios this makes every settings toggle wait on that network call, so controlled switches can appear stuck and setting updates feel unresponsive even though local persistence is synchronous. Returning locally written settings (or using only cached remote defaults) would prevent this user-visible latency regression.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 75bbf0c7bd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +205 to +208
export async function readEffectiveSettings(): Promise<UserSettings> {
const settings = readSettings();
const remoteConfig = await getRemoteDesktopConfig();
return resolveEffectiveSettings(settings, remoteConfig);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Short-circuit remote fetch when local setting is explicit

readEffectiveSettings() always awaits getRemoteDesktopConfig() even when settings.blockUnsafeNpmPackages is already explicitly set, so operations that call it (including dependency installs and settings reads/writes) are unnecessarily coupled to a network request with a 5s timeout. In offline/slow-network scenarios this adds avoidable latency despite the local value already taking precedence; returning early when the local boolean is present avoids these stalls without changing behavior.

Useful? React with 👍 / 👎.

@wwwillchen wwwillchen merged commit 2ff6fb1 into main Apr 7, 2026
16 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human:review-issue ai agent flagged an issue that requires human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants