Skip to content

Fix SSRF class URL validation gaps and harden command/file trust boundaries#4

Open
000boil wants to merge 2 commits intorohunvora:mainfrom
000boil:security/hardening-ssrf-command-injection
Open

Fix SSRF class URL validation gaps and harden command/file trust boundaries#4
000boil wants to merge 2 commits intorohunvora:mainfrom
000boil:security/hardening-ssrf-command-injection

Conversation

@000boil
Copy link
Copy Markdown

@000boil 000boil commented Mar 20, 2026

this fixes a few security holes around url intake and local execution flows.
i blocked unsafe url targets/schemes at intake (ssrf hardening) and replaced risky urlopen behavior with safer process handling, locked base url trust so tokens don’t get routed to shady hosts, restricted helper scripts to read files only from intended runtime dirs, and tightened url input handling where commandbound extraction happens.

untrusted input was crossing network/process/filesystem trust boundaries wich was not great lol cuz it lowers backend request abuse risk, unsafe exec risk, and local secret exposure risk.

ur welcome :p

Harden URL and path validation plus execution boundaries to reduce SSRF and unsafe command/file behaviors.
Copy link
Copy Markdown
Owner

@rohunvora rohunvora left a comment

Choose a reason for hiding this comment

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

Thanks for tightening the trust boundaries here. I do think there are still a few blocking issues before this should merge:

  1. scripts/security.ts: openUrlInBrowser() still uses cmd /c start on Windows. That means untrusted input is still reaching cmd.exe, so the command-injection risk is not fully removed on Windows.

  2. scripts/ensure-key.ts + scripts/security.ts: normalizeTrustedBaseUrl() now rejects documented self-hosted PASTE_TRADE_URL values and silently falls back to https://paste.trade. That is a behavior regression, and worse, it can route tokens/data to production when the user explicitly configured another host. If we block custom bases, this should fail closed, not silently rewrite to prod.

  3. scripts/extract.ts: the SSRF hardening only validates the initial URL string, but the later fetch(...) calls still follow redirects. An attacker-controlled HTTPS URL can still redirect to a loopback/private target after passing the initial check, so the SSRF fix is incomplete.

  4. scripts/security.ts: ensurePathInsideDir() uses resolvedFile.startsWith(${resolvedDir}/), which breaks on Windows because path.resolve() returns backslash-separated paths. That will reject valid nested files under the runtime source directory on Windows.

I’d be comfortable re-reviewing after those are addressed.

Fix windows browser-open execution path, fail closed on invalid base URL config, enforce safe redirect handling for article/image fetches, and make path boundary checks cross-platform.

Made-with: Cursor
@000boil
Copy link
Copy Markdown
Author

000boil commented Mar 20, 2026

thanks done

@rohunvora
Copy link
Copy Markdown
Owner

Thanks for the hardening work here. I ended up landing a broader maintainer patch for URL validation, redirect-safe fetches, non-shell browser opening, and path containment. That supersedes the intended fix here and avoids the remaining issues and regressions in this branch, so I won't be merging this one.

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