Skip to content

fix: add URL validation in index.js (CWE-918) - #3578

Open
anupamme wants to merge 8 commits into
slurpcode:mainfrom
anupamme:fix-repo-slurp-cwe-918-worker-ssrf-hardening
Open

anupamme wants to merge 8 commits into
slurpcode:mainfrom
anupamme:fix-repo-slurp-cwe-918-worker-ssrf-hardening

Conversation

@anupamme

@anupamme anupamme commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

The Cloudflare Worker implements a request proxy pattern without URL validation or allowlisting. While currently using a hardcoded URL, the architectural pattern creates a security gap that could be exploited if modified to accept dynamic URLs, allowing attackers to redirect requests to internal infrastructure. The affected code is workers.dev/preston/index.js:1. This change is the fix I would apply.

Reference: CWE-918

What changed

  • workers.dev/preston/index.js: the upstream is a fixed constant (TARGET_URL) that is never derived from the incoming request (URL, headers or body), with a comment documenting that invariant. The earlier new URL(...).toString() reparse was removed, as it was not an allowlist check.
  • workers.dev/preston/index.test.js: new regression test (Node's built-in node:test, no new dependencies).
  • workers.dev/preston/package.json: npm test now runs node --test.

Verification

cd workers.dev/preston && npm test passes (4/4).

Regression test

The test loads the real worker script in a vm context with stubbed addEventListener and fetch, dispatches requests carrying internal addresses in the query string, path, X-Forward-Url/Host headers and POST body, and asserts the only URL fetched is the fixed upstream.

Test (workers.dev/preston/index.test.js)
const attacks = [
  new Request("https://preston.example.workers.dev/?url=http://169.254.169.254/latest/meta-data/"),
  new Request("https://preston.example.workers.dev/http://localhost:8080/admin"),
  new Request("https://preston.example.workers.dev/", {
    headers: { "X-Forward-Url": "http://127.0.0.1/", Host: "10.0.0.1" },
  }),
  new Request("https://preston.example.workers.dev/", {
    method: "POST",
    body: "http://192.168.0.1/",
  }),
]

for (const request of attacks) {
  test(`only fetches the fixed upstream for ${request.method} ${request.url}`, () => {
    const { handler, fetched } = loadWorker()
    let responded
    handler({ request, respondWith: r => (responded = r) })
    return Promise.resolve(responded).then(() => {
      assert.deepStrictEqual(fetched, [TARGET_URL])
    })
  })
}

Automated security fix by OrbisAI Security

🤖 Generated with Claude Code

Automated security fix generated by OrbisAI Security

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The implementation does not enforce URL allowlisting or validate request-provided destinations.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Low severity

Open (1)
What changed in this PR

Attempts to mitigate SSRF risk in the Cloudflare Worker proxy by centralizing and parsing its target URL.

Changes:

  • Adds an ALLOWED_TARGET_URL constant.
  • Fetches the serialized target URL.
  • Does not implement actual allowlisting or request-input validation.
File Description
workers.dev/​preston/​index.js Defines and fetches the static proxy destination.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread workers.dev/preston/index.js Outdated
addEventListener("fetch", event => {
return event.respondWith(
fetch("https://prestonhunter.github.io/resume/")
fetch(new URL(ALLOWED_TARGET_URL).toString())

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, the reparse was a no-op. Fixed in 1cc8d26 by keeping the destination strictly fixed: TARGET_URL is a constant never derived from the request, and the new URL(...).toString() wrapper is gone. Added workers.dev/preston/index.test.js (node:test, no new deps), which loads the actual worker with stubbed addEventListener/fetch, sends requests with internal addresses in the query, path, X-Forward-Url/Host headers and body, and asserts the only fetch is to the fixed upstream. The PR description now shows this test instead of the old handleRequest one.

@BaseMax

BaseMax commented Nov 4, 2026

Copy link
Copy Markdown
Member

cc @anupamme check reviews please.

Drop the no-op new URL(...).toString() reparse, which was not an
allowlist. The upstream is a constant never derived from the request,
and a node:test suite verifies that request URL, headers and body
cannot change the fetch destination.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@pull-request-size pull-request-size Bot added size/M and removed size/XS labels Nov 4, 2026
Hound's ESLint parser predates ES2017 and rejects `async () =>`.
Return the promise from the test callback instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread workers.dev/preston/index.test.js Outdated
]

for (const request of attacks) {
test(`only fetches the fixed upstream for ${request.method} ${request.url}`, async () => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c464281. Hound's parser rejects ES2017 async arrows (the plain arrows above parsed fine), so the test callback now returns the promise instead of using async/await. The file parses at ES2016 and npm test still passes (4/4).

Hound lints with its own config, which lacks Node globals, so it
flagged require and __dirname as no-undef.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@@ -0,0 +1,49 @@
const test = require("node:test")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bb9c0fd: added /* eslint-env node */ so require/__dirname are recognized as Node globals.

@@ -0,0 +1,49 @@
const test = require("node:test")
const assert = require("node:assert")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bb9c0fd: added /* eslint-env node */ so require/__dirname are recognized as Node globals.

@@ -0,0 +1,49 @@
const test = require("node:test")
const assert = require("node:assert")
const fs = require("node:fs")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bb9c0fd: added /* eslint-env node */ so require/__dirname are recognized as Node globals.

@anupamme

anupamme commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

cc @anupamme check reviews please.

Responded. Pls review.

@BaseMax

BaseMax commented Nov 4, 2026

Copy link
Copy Markdown
Member

Thanks - Super. @jbampton please check.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants