Skip to content

fix(security): reject userinfo in is_url_safe to prevent SSRF bypass (#97)#98

Merged
HackingRepo merged 3 commits into
HackingRepo:mainfrom
andrewmkhoury:fix/ssrf-userinfo-bypass
Jun 2, 2026
Merged

fix(security): reject userinfo in is_url_safe to prevent SSRF bypass (#97)#98
HackingRepo merged 3 commits into
HackingRepo:mainfrom
andrewmkhoury:fix/ssrf-userinfo-bypass

Conversation

@andrewmkhoury
Copy link
Copy Markdown
Contributor

Summary

Fixes #97 — SSRF bypass via userinfo stripping in is_url_safe (v1.0.3).

Root Cause

is_url_safe called remove_at_symbol_in_string on the raw URL before new URL() parsed it. This stripped the @ separator between userinfo and host, corrupting the hostname:

http://evil.com@127.0.0.1/
  → remove_at_symbol → http://evil.com127.0.0.1/
  → hostname = "evil.com127.0.0.1"  ← NXDOMAIN, passes as safe

Every internal IPv4 range, AWS IMDS (169.254.169.254), and IPv6 addresses via userinfo all bypassed the guard.

Fix

  • Removed remove_at_symbol_in_string from is_url_safe
  • Added an explicit reject for any URL where parsed.username !== "" or parsed.password !== "" after new URL() construction
  • Also fixed tsconfig.json (rootDir, types: ["node"], ignoreDeprecations) so the project builds cleanly

Verification

Tested against 15 vectors — all pass:

Vector Result
http://evil.com@127.0.0.1/ BLOCKED ✓
http://evil.com@10.0.0.1/ BLOCKED ✓
http://evil.com@192.168.1.1/ BLOCKED ✓
http://evil.com@169.254.169.254/ BLOCKED ✓
http://evil.com@[::1]/ BLOCKED ✓
http://[::1]/ (bare IPv6 loopback) BLOCKED ✓
http://[fc00::1]/ (ULA) BLOCKED ✓
http://[fe80::1]/ (link-local) BLOCKED ✓
http://[::ffff:127.0.0.1]/ (IPv4-mapped) BLOCKED ✓
http://[::ffff:169.254.169.254]/ (IPv4-mapped IMDS) BLOCKED ✓
http://[64:ff9b:1::1]/ (NAT64 RFC8215) BLOCKED ✓
http://[5f00::1]/ (SRv6 SID RFC9602) BLOCKED ✓
http://[fec0::1]/ (site-local) BLOCKED ✓
https://www.adobe.com/ ALLOWED ✓
https://8.8.8.8/ ALLOWED ✓

Stripping '@' from the raw URL string before parsing corrupts the
host/userinfo boundary, allowing payloads like
http://evil.com@127.0.0.1/ to pass all internal-IP checks.

Fix: remove remove_at_symbol_in_string from is_url_safe and instead
reject any URL whose parsed .username or .password is non-empty.
Also fix tsconfig.json so the project builds cleanly with TS 6.

Fixes HackingRepo#97
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Fix SSRF bypass via userinfo in is_url_safe validation

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Removes userinfo stripping before URL parsing to prevent SSRF bypass
• Adds explicit rejection of URLs with username or password fields
• Fixes tsconfig.json for TypeScript 6 compatibility
Diagram
flowchart LR
  A["Raw URL with userinfo<br/>http://evil.com@127.0.0.1/"] --> B["Parse with new URL"]
  B --> C["Check parsed.username<br/>and parsed.password"]
  C --> D["Reject if non-empty"]
  D --> E["SSRF blocked ✓"]

Loading

Grey Divider

File Changes

1. src/helpers.ts 🐞 Bug fix +5/-1

Reject userinfo in URL safety validation

• Removed remove_at_symbol_in_string call that corrupted hostname parsing
• Added explicit check to reject URLs with non-empty parsed.username or parsed.password
• Added clarifying comment explaining userinfo rejection as SSRF prevention

src/helpers.ts


2. tsconfig.json ⚙️ Configuration changes +4/-1

Update TypeScript configuration for v6 compatibility

• Added rootDir configuration pointing to src directory
• Added ignoreDeprecations set to "6.0" for TypeScript 6 compatibility
• Added types array with "node" to resolve type definitions

tsconfig.json


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 29, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)

Grey Divider


Action required

1. Fix not in runtime build ✓ Resolved 🐞 Bug ⛨ Security
Description
The PR updates only src/helpers.ts, but package.json entrypoints load dist/utils.js which re-exports
is_url_safe from dist/helpers.js where the old '@' stripping logic still exists. This means
consumers will still execute the vulnerable dist implementation and the SSRF bypass is not actually
fixed in the shipped API.
Code

src/helpers.ts[R508-519]

Evidence
The package entrypoints are wired to dist/, and the current dist/helpers.js still contains the
old vulnerable is_url_safe implementation, so the fix in src/ is not what consumers will
execute.

package.json[22-36]
dist/utils.js[2-21]
dist/helpers.js[425-456]
src/helpers.ts[504-545]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`is_url_safe` was fixed in `src/helpers.ts`, but the package runtime entrypoints reference `dist/` and those compiled files still contain the pre-fix logic (including `remove_at_symbol_in_string` inside `is_url_safe`). As a result, the published/consumed API continues to use the vulnerable implementation.
## Issue Context
- `package.json` points `main`/`exports` to `dist/utils.js`.
- `dist/utils.js` re-exports `is_url_safe` from `dist/helpers.js`.
- `dist/helpers.js` still strips `@` before parsing and lacks the new userinfo rejection.
- There is no `prepare`/`prepublishOnly` script shown that would guarantee `dist/` is regenerated on install/publish.
## Fix Focus Areas
- package.json[22-36]
- dist/utils.js[2-21]
- dist/helpers.js[425-456]
- src/helpers.ts[504-545]
## What to change
1. Regenerate `dist/` from `src/` (run `npm run build`) and commit the updated `dist/helpers.js` / `dist/helpers.d.ts` / `dist/utils.js` / `dist/utils.d.ts` so the shipped entrypoints contain the fix.
2. Alternatively (or additionally), add a `prepare` (and/or `prepublishOnly`) script to run `tsc` so `dist/` is always rebuilt for published artifacts.
3. Ensure the distributed `types` entry also points to the generated declarations that actually exist (e.g., `dist/utils.d.ts`) if you keep shipping compiled output as the main entrypoint.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Redirect check mutates URL ✓ Resolved 🐞 Bug ≡ Correctness
Description
After removing remove_at_symbol_in_string from is_url_safe, the redirect-check path still strips
all '@' in is_redirect_safe before performing requests. When DSSRF_CHECK_REDIRECTS=1, redirect
safety is evaluated for a different URL than the one validated whenever '@' appears in the
path/query, making the safety decision incorrect.
Code

src/helpers.ts[R508-522]

Evidence
The code shows is_url_safe no longer strips '@' and passes u into is_redirect_safe, while
is_redirect_safe still strips all '@' from the URL string before making requests, guaranteeing a
mismatch whenever '@' appears outside userinfo.

src/helpers.ts[451-456]
src/helpers.ts[504-540]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`is_url_safe()` no longer removes `@` characters, but when redirect checking is enabled it calls `is_redirect_safe(u)`. `is_redirect_safe()` still calls `remove_at_symbol_in_string()` on the full URL string, which changes the URL (including path/query), so the redirect chain being checked may not correspond to the actual URL the caller will request.
## Issue Context
- `is_url_safe` passes the original normalized URL into `is_redirect_safe`.
- `is_redirect_safe` removes all `@` characters before parsing and performing HEAD requests.
- This mismatch was introduced by the PR removing `remove_at_symbol_in_string` only from `is_url_safe`.
## Fix Focus Areas
- src/helpers.ts[451-486]
- src/helpers.ts[504-540]
## What to change
1. Remove `normalized = remove_at_symbol_in_string(normalized);` from `is_redirect_safe`.
2. Add the same userinfo rejection used in `is_url_safe`:
- after `new URL(...)`, reject if `parsed.username !== "" || parsed.password !== ""`.
3. Apply the userinfo rejection for each redirect target as well (after `current = new URL(loc, ...)`), so userinfo-based redirect URLs are consistently blocked without mutating `@` in paths/queries.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@codacy-production
Copy link
Copy Markdown

codacy-production Bot commented May 29, 2026

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

Comment thread src/helpers.ts Outdated
Comment thread src/helpers.ts Outdated
- Commit rebuilt dist/ so the shipped entrypoints contain the fix
  (dist/helpers.js was still carrying the old vulnerable is_url_safe)
- Remove remove_at_symbol_in_string from is_redirect_safe (same root
  cause as is_url_safe — strips '@' before parsing, corrupting host)
- Add userinfo rejection (username/password check) to is_redirect_safe
  for both the initial URL and each redirect target

Addresses review comments on HackingRepo#97
All proto, userinfo, and hostname validation is now in one place:
is_parsed_url_safe(). Both is_url_safe and is_redirect_safe delegate
to it, removing the repeated username/password checks and making
redirect-target validation consistent with the primary check.

Also removes the now-dead post-parse IPv4 normalization branch —
WHATWG URL already normalizes the hostname before we inspect it, so
normalize_ipv4 on parsed.hostname was a no-op.
@HackingRepo
Copy link
Copy Markdown
Owner

HackingRepo commented May 29, 2026

we not able to reproduce that @andrewmkhoury

relunsec@relunsec:~/software/dssrf$ cat index.js
const { is_url_safe } = require('dssrf');

(async () => {
  const cases = [
    'http://evil.com@127.0.0.1/',
    'http://evil.com@10.0.0.1/',
    'http://evil.com@192.168.1.1/',
    'http://evil.com@169.254.169.254/',   // AWS IMDS
    'http://evil.com@[::1]/',
  ];
  for (const url of cases) {
    const safe = await is_url_safe(url);
    console.log(`${safe ? 'BYPASSED ✗' : 'blocked  ✓'}  ${url}`);
  }
})();
relunsec@relunsec:~/software/dssrf$ node index.js 
blocked    http://evil.com@127.0.0.1/
blocked    http://evil.com@10.0.0.1/
blocked    http://evil.com@192.168.1.1/
blocked    http://evil.com@169.254.169.254/
blocked    http://evil.com@[::1]/
relunsec@relunsec:~/software/dssrf$ 

Everything blocked

I installed dssrf via npm install dssrf

@HackingRepo
Copy link
Copy Markdown
Owner

pong @andrewmkhoury , see my comments

Copy link
Copy Markdown
Owner

@HackingRepo HackingRepo left a comment

Choose a reason for hiding this comment

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

see my comments, we not able to reproduce that

@HackingRepo
Copy link
Copy Markdown
Owner

HackingRepo commented May 30, 2026

pong @andrewmkhoury, please address what i said, i will merge you improvements in the next version but not a security vuln, since i see it blocked

@HackingRepo HackingRepo self-requested a review May 30, 2026 19:52
@andrewmkhoury
Copy link
Copy Markdown
Contributor Author

There is a confirmed bypass in published 1.0.3. The remove_at_symbol_in_string call strips @ before URL parsing, corrupting userinfo@host into a single token. When the result is a valid public IP, is_url_safe returns true while an HTTP client hits the original private address:

http://1@10.0.0.1/  →  strip @  →  http://110.0.0.1/  →  110.0.0.1 is public  →  is_url_safe returns true

Verified against the published package:

const { is_url_safe } = require('dssrf'); // 1.0.3
await is_url_safe('http://1@10.0.0.1/'); // true  ← BYPASSED
await is_url_safe('http://2@10.0.0.1/'); // true  ← BYPASSED

Your test used evil.com@127.0.0.1 which strips to evil.com127.0.0.1 — an invalid URL that throws in new URL(), so it gets blocked incidentally. Inputs where the concatenation produces a valid public-looking IP slip through.

This PR fixes it by checking parsed.username !== "" after new URL(), before the IP check is reached. Verified:

const { is_url_safe } = require('./dist/utils'); // this PR
await is_url_safe('http://1@10.0.0.1/'); // false ✓
await is_url_safe('http://2@10.0.0.1/'); // false ✓

Re: Snyk failure — the job log shows SNYK_TOKEN: (empty). Snyk exits with 401, never writes snyk-results.sarif, and the upload step fails on the missing file. Not related to this PR's changes.

@HackingRepo
Copy link
Copy Markdown
Owner

ok now i able to reproduce it, thank's for that @andrewmkhoury, in another please do'nt post security issues here

@HackingRepo HackingRepo added the hardening PRs that harden security of dssrf, they are different to security vulns label Jun 2, 2026
@HackingRepo HackingRepo merged commit 9211f91 into HackingRepo:main Jun 2, 2026
15 of 16 checks passed
@HackingRepo
Copy link
Copy Markdown
Owner

ok will be also the hardening integrated to npm dssrf also in new version, do'nt worry

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

Labels

hardening PRs that harden security of dssrf, they are different to security vulns

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: SSRF bypass via userinfo stripping in is_url_safe (1.0.3)

2 participants