Skip to content

Fix SiteSucker proxy configuration updates - #785

Open
sfeko wants to merge 1 commit into
xpf0000:masterfrom
sfeko:fix/site-06-proxy-config
Open

Fix SiteSucker proxy configuration updates#785
sfeko wants to merge 1 commit into
xpf0000:masterfrom
sfeko:fix/site-06-proxy-config

Conversation

@sfeko

@sfeko sfeko commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • copy and validate the complete SiteSucker runtime configuration, including proxy
  • unwrap the persisted commonSetup payload before applying live settings updates
  • use the validated directory and window count when starting a task
  • align the renderer SiteSuckerSetup type and defaults with the main-process configuration
  • add focused regression coverage for valid, cleared, and invalid settings

Root cause

Config.update() updated only page limits, timeout, and media sizes, so the proxy entered in settings never reached Config.proxy. In addition, the live settings-save path passed the outer { commonSetup } persistence object to a runtime method that expects the inner configuration.

Impact

Saved proxy settings now reach Electron session proxy configuration and the HTTP/HTTPS agents both when a task starts and when settings are changed while the SiteSucker runtime is loaded. Clearing the proxy also clears the active runtime value. Invalid numeric settings fall back to the existing safe defaults.

Validation

  • npx --yes tsx@4.20.3 scripts/site-sucker-config-test.ts
  • strict TypeScript check of src/main/ui/SiteSucker/Config.ts
  • targeted esbuild bundles of the SiteSucker runtime, IPC handler, and renderer store
  • npx --yes prettier@3.6.1 --check src/main/core/IPCHandler.ts src/main/ui/SiteSucker/Config.ts src/main/ui/SiteSucker/index.ts src/render/components/Tools/SiteSucker/store.ts scripts/site-sucker-config-test.ts
  • git diff --check

The full repository test suite was not run because dependencies are not installed in the current checkout.

@xpf0000

xpf0000 commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Thanks for the PR. The configuration changes are heading in the right direction, but I don’t think this is
ready to merge yet.

There are several blocking runtime issues:

  1. PageTask.updateConfig() discards the promises returned by PageTaskItem.updateConfig(). SiteSucker.show()
    then starts PageTask.run() immediately, so the first loadURL() may execute before session.setProxy() has
    completed. The proxy is therefore not guaranteed to be active for the initial requests.

  2. The SiteSucker windows do not use a dedicated Electron session partition. Calling
    webContents.session.setProxy() modifies the shared default session used by the main and tray windows. The
    proxy is also not restored when SiteSucker is destroyed, so it may continue affecting the rest of the
    application.

  3. The proxy value is only trimmed, not validated. Values such as abc are accepted by Config, but hpagent
    throws TypeError: Invalid URL. Because the configuration promises are not awaited or caught, IPC reports
    success while the Electron session and Axios agents can end up with inconsistent proxy settings.

There is also a security concern: HttpsProxyAgent is created with rejectUnauthorized: false, which disables
TLS certificate verification for proxied HTTPS downloads.

Suggested changes:

  • Make PageTask.updateConfig() return and await Promise.all(...).
  • Await proxy configuration before starting page and link tasks.
  • Propagate configuration failures back through the IPC response.
  • Use a dedicated SiteSucker session partition and reliably reset its proxy.
  • Validate and normalize the proxy URL before applying it.
  • Avoid disabling TLS certificate verification.
  • Add runtime tests covering ordering, invalid proxies, session isolation, and cleanup.

The new test passes, as do TypeScript, ESLint, Prettier, the existing test:main-lazy suite, and the targeted
bundle. However, site-sucker-config-test.ts is not currently registered in package.json, and the PR has no CI/
check runs.

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.

2 participants