fix: validate relay URL before calling load() and soften unhandledRejection handler
All checks were successful
CI / checks (pull_request) Successful in 34s
All checks were successful
CI / checks (pull_request) Successful in 34s
POST /notify/:id accepted an attacker-supplied relay URL with a non-ws://
scheme (e.g. http://…). The URL was passed straight to @welshman/net's
load(), whose internal batcher throws 'Invalid relay url' asynchronously
(via setTimeout). That error escaped the route's try/catch as an unhandled
rejection, triggering process.exit(1) in src/index.ts — the entire server
died. Any registered subscriber could crash the service repeatedly.
Two fixes applied (either alone breaks the attack):
1. Validate relay scheme in the route handler before calling load()
(src/server.ts). Uses isRelayUrl() from @welshman/util, which accepts
only wss:// and ws:// schemes. Returns 400 immediately for invalid
URLs, preventing the bad URL from ever reaching the batcher.
2. Soften process.on('unhandledRejection') in src/index.ts to log and
continue instead of calling process.exit(1). Defence in depth: if any
other async error escapes a try/catch, the server stays up.
Test: test/notify-non-wss-relay-crash.test.ts — starts an ephemeral
server, creates a confirmed subscription, POSTs with http://127.0.0.1:9,
expects 400 with an error message.
This commit is contained in:
parent
8235513822
commit
0a5b7ad14d
3 changed files with 96 additions and 2 deletions
|
|
@ -7,7 +7,9 @@ import { registerSubscription } from './worker/index.js'
|
||||||
|
|
||||||
process.on('unhandledRejection', (error: Error) => {
|
process.on('unhandledRejection', (error: Error) => {
|
||||||
console.error('Unhandled rejection:', error.stack)
|
console.error('Unhandled rejection:', error.stack)
|
||||||
process.exit(1)
|
// Do not process.exit(1) — an async rejection from a library's internal
|
||||||
|
// timer (e.g. @welshman/net's batcher) would let an attacker crash the
|
||||||
|
// entire server with a single malformed request. Log and continue.
|
||||||
})
|
})
|
||||||
|
|
||||||
process.on('uncaughtException', (error: Error) => {
|
process.on('uncaughtException', (error: Error) => {
|
||||||
|
|
@ -15,6 +17,7 @@ process.on('uncaughtException', (error: Error) => {
|
||||||
process.exit(1)
|
process.exit(1)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
|
||||||
migrate().then(async () => {
|
migrate().then(async () => {
|
||||||
server.listen(PORT, () => {
|
server.listen(PORT, () => {
|
||||||
console.log('Running on port', PORT)
|
console.log('Running on port', PORT)
|
||||||
|
|
|
||||||
|
|
@ -6,7 +6,7 @@ import { render } from './templates.js'
|
||||||
import { confirmSubscriptionAction, unsubscribeAction, registerSubscription, ActionError } from './actions.js'
|
import { confirmSubscriptionAction, unsubscribeAction, registerSubscription, ActionError } from './actions.js'
|
||||||
import { getSubscriptionById, insertEvent, getSubscriptionByKey, getSubscriptionByPubkey } from './database.js'
|
import { getSubscriptionById, insertEvent, getSubscriptionByKey, getSubscriptionByPubkey } from './database.js'
|
||||||
import { load } from '@welshman/net'
|
import { load } from '@welshman/net'
|
||||||
import { getIdFilters } from '@welshman/util'
|
import { getIdFilters, isRelayUrl } from '@welshman/util'
|
||||||
import crypto from 'crypto'
|
import crypto from 'crypto'
|
||||||
import { verifyEvent } from 'nostr-tools/pure'
|
import { verifyEvent } from 'nostr-tools/pure'
|
||||||
|
|
||||||
|
|
@ -263,6 +263,15 @@ addRoute('post', '/notify/:id', async (req: Request, res: Response) => {
|
||||||
return res.status(400).json({ error: 'id and relay are required' })
|
return res.status(400).json({ error: 'id and relay are required' })
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Reject non-ws:// relay URLs. load() from @welshman/net throws
|
||||||
|
// Invalid relay url asynchronously inside a batcher timer, which
|
||||||
|
// escapes the route's try/catch and becomes an unhandledRejection
|
||||||
|
// that would crash the server. Validate early to avoid calling
|
||||||
|
// load() with an unsupported scheme.
|
||||||
|
if (!isRelayUrl(relay)) {
|
||||||
|
return res.status(400).json({ error: 'Invalid relay URL. Only wss:// or ws:// relays are supported.' })
|
||||||
|
}
|
||||||
|
|
||||||
const sub = await getSubscriptionById(req.params.id)
|
const sub = await getSubscriptionById(req.params.id)
|
||||||
|
|
||||||
if (!sub) {
|
if (!sub) {
|
||||||
|
|
|
||||||
82
test/notify-non-wss-relay-crash.test.ts
Normal file
82
test/notify-non-wss-relay-crash.test.ts
Normal file
|
|
@ -0,0 +1,82 @@
|
||||||
|
// POST /notify with non-wss relay URL crashes the server (remote DoS)
|
||||||
|
//
|
||||||
|
// Bug: When POST /notify/:id receives a relay URL with a non-wss scheme
|
||||||
|
// (e.g. http://…), the handler calls `load()` from @welshman/net. Inside
|
||||||
|
// `load`, the batcher schedules an async `_execute` via setTimeout(200ms).
|
||||||
|
// When `getAdapter` throws `Invalid relay url`, the error escapes as an
|
||||||
|
// unhandledPromiseRejection because the batcher's `_execute` async function
|
||||||
|
// is called from setTimeout with no `.catch()`. The global
|
||||||
|
// `process.on('unhandledRejection')` handler in `src/index.ts` then calls
|
||||||
|
// `process.exit(1)`, killing the entire server.
|
||||||
|
//
|
||||||
|
// Fix applied (2 of 3 fixes):
|
||||||
|
// 1. Validate relay scheme before calling load() — the route handler now
|
||||||
|
// checks isRelayUrl() and returns 400 for non-ws:// schemes.
|
||||||
|
// 2. Don't process.exit(1) on unhandledRejection — log and continue
|
||||||
|
// (defence in depth for any other async edge-case).
|
||||||
|
|
||||||
|
import { describe, it, expect, beforeAll, afterAll } from 'vitest'
|
||||||
|
import * as db from '../src/database.js'
|
||||||
|
import { server } from '../src/server.js'
|
||||||
|
import { createServer, type Server } from 'http'
|
||||||
|
|
||||||
|
// Partially mock @welshman/net so that `load()` returns an empty array,
|
||||||
|
// preventing real relay connections during the test, while preserving all
|
||||||
|
// other exports from the library.
|
||||||
|
vi.mock('@welshman/net', async (importOriginal) => {
|
||||||
|
const actual = await importOriginal()
|
||||||
|
return {
|
||||||
|
...(actual as Record<string, unknown>),
|
||||||
|
load: vi.fn().mockResolvedValue([]),
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
describe('notify_non_wss_relay', () => {
|
||||||
|
let httpServer: Server
|
||||||
|
let baseUrl: string
|
||||||
|
let subId: string
|
||||||
|
|
||||||
|
beforeAll(async () => {
|
||||||
|
await db.migrate()
|
||||||
|
|
||||||
|
// Create and confirm a subscription we can use for the notify call
|
||||||
|
const pubkey = 'nws-test-pk-' + Date.now()
|
||||||
|
const email = 'nws-test-' + Date.now() + '@example.com'
|
||||||
|
const sub = await db.insertSubscription(pubkey, email, 'daily')
|
||||||
|
const confirmed = await db.confirmSubscription(sub.key)
|
||||||
|
subId = confirmed.id
|
||||||
|
|
||||||
|
// Start the express server on a random available port
|
||||||
|
await new Promise<void>((resolve) => {
|
||||||
|
httpServer = createServer(server)
|
||||||
|
httpServer.listen(0, () => {
|
||||||
|
const addr = httpServer.address()
|
||||||
|
if (addr && typeof addr === 'object') {
|
||||||
|
baseUrl = `http://localhost:${addr.port}`
|
||||||
|
}
|
||||||
|
resolve()
|
||||||
|
})
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
afterAll(async () => {
|
||||||
|
httpServer?.close()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('rejects non-wss relay URL with 400 instead of crashing the server', async () => {
|
||||||
|
const res = await fetch(`${baseUrl}/notify/${subId}`, {
|
||||||
|
method: 'POST',
|
||||||
|
headers: { 'Content-Type': 'application/json' },
|
||||||
|
body: JSON.stringify({
|
||||||
|
id: 'nonexistent-' + Date.now(),
|
||||||
|
relay: 'http://127.0.0.1:9',
|
||||||
|
}),
|
||||||
|
})
|
||||||
|
|
||||||
|
expect(res.status).toBe(400)
|
||||||
|
|
||||||
|
const body = await res.json()
|
||||||
|
expect(body).toHaveProperty('error')
|
||||||
|
expect(body.error).toMatch(/Invalid relay/)
|
||||||
|
})
|
||||||
|
})
|
||||||
Loading…
Reference in a new issue