Skip to content

Commit 4c34bea

Browse files
committed
Fix code quality and security issues from audit
- Fix worker queue race condition with try-finally block - Replace non-null assertions with proper null checks - Add debug logging to silent catch blocks - Expand sensitive header filtering (add set-cookie, proxy-authorization, x-access-token, x-csrf-token, x-xsrf-token) - Remove unused sendSessionData config option - Fix repository URLs to Checkend/checkend-node - Add shutdown hook race condition protection - Add build verification and npm audit to CI
1 parent 4f7b2ca commit 4c34bea

10 files changed

Lines changed: 121 additions & 59 deletions

File tree

.github/workflows/ci.yml

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,51 @@ jobs:
3333
- name: Run tests
3434
run: npm run test
3535

36+
build:
37+
runs-on: ubuntu-latest
38+
39+
steps:
40+
- uses: actions/checkout@v4
41+
42+
- name: Use Node.js
43+
uses: actions/setup-node@v4
44+
with:
45+
node-version: 22
46+
cache: 'npm'
47+
48+
- name: Install dependencies
49+
run: npm ci
50+
51+
- name: Build
52+
run: npm run build
53+
54+
- name: Verify build output
55+
run: |
56+
test -f dist/index.js
57+
test -f dist/index.cjs
58+
test -f dist/index.d.ts
59+
test -f dist/integrations/express.js
60+
test -f dist/integrations/koa.js
61+
test -f dist/integrations/fastify.js
62+
63+
security:
64+
runs-on: ubuntu-latest
65+
66+
steps:
67+
- uses: actions/checkout@v4
68+
69+
- name: Use Node.js
70+
uses: actions/setup-node@v4
71+
with:
72+
node-version: 22
73+
cache: 'npm'
74+
75+
- name: Install dependencies
76+
run: npm ci
77+
78+
- name: Audit dependencies
79+
run: npm audit --audit-level=moderate
80+
3681
lint:
3782
runs-on: ubuntu-latest
3883

package.json

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@
66
"license": "MIT",
77
"repository": {
88
"type": "git",
9-
"url": "https://github.com/furvur/checkend-node.git"
9+
"url": "https://github.com/Checkend/checkend-node.git"
1010
},
11-
"homepage": "https://github.com/furvur/checkend-node",
11+
"homepage": "https://github.com/Checkend/checkend-node",
1212
"bugs": {
13-
"url": "https://github.com/furvur/checkend-node/issues"
13+
"url": "https://github.com/Checkend/checkend-node/issues"
1414
},
1515
"keywords": [
1616
"error",

src/configuration.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,6 @@ export class Configuration {
8282
appName?: string
8383
revision?: string
8484
sendRequestData: boolean
85-
sendSessionData: boolean
8685
sendEnvironmentData: boolean
8786
sendUserData: boolean
8887

@@ -113,7 +112,6 @@ export class Configuration {
113112
this.appName = options.appName || process.env.CHECKEND_APP_NAME
114113
this.revision = options.revision || process.env.CHECKEND_REVISION
115114
this.sendRequestData = options.sendRequestData ?? true
116-
this.sendSessionData = options.sendSessionData ?? true
117115
this.sendEnvironmentData = options.sendEnvironmentData ?? false
118116
this.sendUserData = options.sendUserData ?? true
119117
}

src/index.ts

Lines changed: 40 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -139,13 +139,13 @@ export async function reset(): Promise<void> {
139139
* Report an error to Checkend
140140
*/
141141
export function notify(error: Error, options: NotifyOptions = {}): void {
142-
if (!shouldNotify()) return
142+
if (!shouldNotify() || !config) return
143143

144144
const errorClass = error.name || 'Error'
145145
const message = error.message || 'Unknown error'
146146
const code = (error as NodeJS.ErrnoException).code
147147

148-
if (config!.shouldIgnore(errorClass, message, code)) {
148+
if (config.shouldIgnore(errorClass, message, code)) {
149149
log(`Ignoring error: ${errorClass}`)
150150
return
151151
}
@@ -154,18 +154,18 @@ export function notify(error: Error, options: NotifyOptions = {}): void {
154154

155155
// Build context with optional environment data
156156
let contextData = { ...globalContext, ...localContext, ...options.context }
157-
if (config!.sendEnvironmentData) {
157+
if (config.sendEnvironmentData) {
158158
contextData = { ...contextData, env: sanitize({ ...process.env }) }
159159
}
160160
const mergedContext = sanitize(contextData)
161161

162162
// Include user data based on config
163-
const mergedUser = config!.sendUserData
163+
const mergedUser = config.sendUserData
164164
? sanitize({ ...globalUser, ...localUser, ...options.user })
165165
: {}
166166

167167
// Include request data based on config
168-
const mergedRequest = config!.sendRequestData
168+
const mergedRequest = config.sendRequestData
169169
? sanitize({ ...localRequest, ...options.request })
170170
: {}
171171

@@ -175,10 +175,10 @@ export function notify(error: Error, options: NotifyOptions = {}): void {
175175
user: mergedUser,
176176
fingerprint: options.fingerprint,
177177
tags: options.tags,
178-
environment: config!.environment,
179-
rootPath: config!.rootPath,
180-
appName: config!.appName,
181-
revision: config!.revision,
178+
environment: config.environment,
179+
rootPath: config.rootPath,
180+
appName: config.appName,
181+
revision: config.revision,
182182
})
183183

184184
if (!runBeforeNotifyCallbacks(notice)) {
@@ -192,24 +192,24 @@ export function notify(error: Error, options: NotifyOptions = {}): void {
192192
* Report an error synchronously (returns promise)
193193
*/
194194
export async function notifySync(error: Error, options: NotifyOptions = {}): Promise<ApiResponse | null> {
195-
if (!shouldNotify()) return null
195+
if (!shouldNotify() || !config || !client) return null
196196

197197
const { context: localContext, user: localUser, request: localRequest } = getLocalStorage()
198198

199199
// Build context with optional environment data
200200
let contextData = { ...globalContext, ...localContext, ...options.context }
201-
if (config!.sendEnvironmentData) {
201+
if (config.sendEnvironmentData) {
202202
contextData = { ...contextData, env: sanitize({ ...process.env }) }
203203
}
204204
const mergedContext = sanitize(contextData)
205205

206206
// Include user data based on config
207-
const mergedUser = config!.sendUserData
207+
const mergedUser = config.sendUserData
208208
? sanitize({ ...globalUser, ...localUser, ...options.user })
209209
: {}
210210

211211
// Include request data based on config
212-
const mergedRequest = config!.sendRequestData
212+
const mergedRequest = config.sendRequestData
213213
? sanitize({ ...localRequest, ...options.request })
214214
: {}
215215

@@ -219,17 +219,17 @@ export async function notifySync(error: Error, options: NotifyOptions = {}): Pro
219219
user: mergedUser,
220220
fingerprint: options.fingerprint,
221221
tags: options.tags,
222-
environment: config!.environment,
223-
rootPath: config!.rootPath,
224-
appName: config!.appName,
225-
revision: config!.revision,
222+
environment: config.environment,
223+
rootPath: config.rootPath,
224+
appName: config.appName,
225+
revision: config.revision,
226226
})
227227

228228
if (!runBeforeNotifyCallbacks(notice)) {
229229
return null
230230
}
231231

232-
return client!.sendNotice(notice)
232+
return client.sendNotice(notice)
233233
}
234234

235235
// ========== Context Management ==========
@@ -344,25 +344,25 @@ function uninstallUnhandledRejectionHandler(): void {
344344
}
345345

346346
function handleUncaughtException(error: Error, origin: NodeJS.UncaughtExceptionOrigin): void {
347-
if (!shouldNotify()) return
347+
if (!shouldNotify() || !config) return
348348

349349
const code = (error as NodeJS.ErrnoException).code
350-
if (config!.shouldIgnore(error.name, error.message, code)) {
350+
if (config.shouldIgnore(error.name, error.message, code)) {
351351
return
352352
}
353353

354354
const notice = createNotice(error, {
355355
context: sanitize({ ...globalContext, unhandled: true, origin }),
356356
tags: ['unhandled', 'uncaughtException'],
357-
environment: config!.environment,
358-
rootPath: config!.rootPath,
359-
appName: config!.appName,
360-
revision: config!.revision,
357+
environment: config.environment,
358+
rootPath: config.rootPath,
359+
appName: config.appName,
360+
revision: config.revision,
361361
})
362362

363363
// Send synchronously for uncaught exceptions
364-
client?.sendNotice(notice).catch(() => {
365-
// Ignore errors when reporting errors
364+
client?.sendNotice(notice).catch((err) => {
365+
config?.logger.debug(`Failed to send uncaught exception notice: ${err instanceof Error ? err.message : err}`)
366366
})
367367

368368
// Call original handler if it exists
@@ -372,7 +372,7 @@ function handleUncaughtException(error: Error, origin: NodeJS.UncaughtExceptionO
372372
}
373373

374374
function handleUnhandledRejection(reason: unknown, promise: Promise<unknown>): void {
375-
if (!shouldNotify()) return
375+
if (!shouldNotify() || !config) return
376376

377377
let error: Error
378378
if (reason instanceof Error) {
@@ -386,17 +386,17 @@ function handleUnhandledRejection(reason: unknown, promise: Promise<unknown>): v
386386
}
387387

388388
const code = (error as NodeJS.ErrnoException).code
389-
if (config!.shouldIgnore(error.name, error.message, code)) {
389+
if (config.shouldIgnore(error.name, error.message, code)) {
390390
return
391391
}
392392

393393
const notice = createNotice(error, {
394394
context: sanitize({ ...globalContext, unhandled: true, rejection: true }),
395395
tags: ['unhandled', 'unhandledRejection'],
396-
environment: config!.environment,
397-
rootPath: config!.rootPath,
398-
appName: config!.appName,
399-
revision: config!.revision,
396+
environment: config.environment,
397+
rootPath: config.rootPath,
398+
appName: config.appName,
399+
revision: config.revision,
400400
})
401401

402402
// Queue for async sending
@@ -408,11 +408,15 @@ function handleUnhandledRejection(reason: unknown, promise: Promise<unknown>): v
408408
}
409409
}
410410

411+
let isShuttingDown = false
412+
411413
function installShutdownHooks(): void {
412414
if (shutdownHooksInstalled) return
413415
shutdownHooksInstalled = true
414416

415417
const shutdown = async () => {
418+
if (isShuttingDown) return
419+
isShuttingDown = true
416420
await stop()
417421
}
418422

@@ -441,7 +445,8 @@ function runBeforeNotifyCallbacks(notice: Notice): boolean {
441445
return false
442446
}
443447
} catch (e) {
444-
config.logger.warn(`beforeNotify callback failed: ${e}`)
448+
const errorMsg = e instanceof Error ? e.message : String(e)
449+
config.logger.warn(`beforeNotify callback failed: ${errorMsg}`)
445450
}
446451
}
447452

@@ -452,8 +457,8 @@ function sendNotice(notice: Notice): void {
452457
if (worker) {
453458
worker.push(notice)
454459
} else {
455-
client?.sendNotice(notice).catch(() => {
456-
// Ignore errors when reporting errors
460+
client?.sendNotice(notice).catch((err) => {
461+
config?.logger.debug(`Failed to send notice: ${err instanceof Error ? err.message : err}`)
457462
})
458463
}
459464
}

src/integrations/express.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,14 @@ import type { User, RequestInfo } from '../types'
1313
// Headers that should be filtered from request data
1414
const FILTERED_HEADERS = [
1515
'cookie',
16+
'set-cookie',
1617
'authorization',
18+
'proxy-authorization',
1719
'x-api-key',
1820
'x-auth-token',
21+
'x-access-token',
22+
'x-csrf-token',
23+
'x-xsrf-token',
1924
]
2025

2126
// Headers to exclude entirely (not useful for debugging)

src/integrations/fastify.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,14 @@ import type { User, RequestInfo } from '../types'
1313
// Headers that should be filtered from request data
1414
const FILTERED_HEADERS = [
1515
'cookie',
16+
'set-cookie',
1617
'authorization',
18+
'proxy-authorization',
1719
'x-api-key',
1820
'x-auth-token',
21+
'x-access-token',
22+
'x-csrf-token',
23+
'x-xsrf-token',
1924
]
2025

2126
// Headers to exclude entirely (not useful for debugging)

src/integrations/koa.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,14 @@ import type { User, RequestInfo } from '../types'
1313
// Headers that should be filtered from request data
1414
const FILTERED_HEADERS = [
1515
'cookie',
16+
'set-cookie',
1617
'authorization',
18+
'proxy-authorization',
1719
'x-api-key',
1820
'x-auth-token',
21+
'x-access-token',
22+
'x-csrf-token',
23+
'x-xsrf-token',
1924
]
2025

2126
// Headers to exclude entirely (not useful for debugging)

src/types.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,6 @@ export interface ConfigOptions {
154154
revision?: string
155155
/** Include request data in error reports (default: true) */
156156
sendRequestData?: boolean
157-
/** Include session data in error reports (default: true) */
158-
sendSessionData?: boolean
159157
/** Include environment variables in error reports (default: false) */
160158
sendEnvironmentData?: boolean
161159
/** Include user data in error reports (default: true) */

src/worker.ts

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -104,22 +104,28 @@ export class Worker {
104104

105105
this.processing = true
106106

107-
while (this.queue.length > 0) {
108-
const item = this.queue.shift()!
107+
try {
108+
while (this.queue.length > 0) {
109+
const item = this.queue.shift()!
109110

110-
if (item === SHUTDOWN) {
111-
break
112-
}
111+
if (item === SHUTDOWN) {
112+
break
113+
}
113114

114-
if (typeof item === 'object' && 'type' in item && item.type === FLUSH) {
115-
item.resolve()
116-
continue
117-
}
115+
if (typeof item === 'object' && 'type' in item && item.type === FLUSH) {
116+
item.resolve()
117+
continue
118+
}
118119

119-
await this.sendWithThrottle(item as NoticePayload)
120+
await this.sendWithThrottle(item as NoticePayload)
121+
}
122+
} finally {
123+
this.processing = false
124+
// Retry if queue has items (e.g., added during processing)
125+
if (this.queue.length > 0 && !this.shutdown) {
126+
this.processQueue()
127+
}
120128
}
121-
122-
this.processing = false
123129
}
124130

125131
private async sendWithThrottle(payload: NoticePayload): Promise<void> {

test/configuration.test.ts

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -133,11 +133,6 @@ describe('Configuration', () => {
133133
const config = new Configuration({ apiKey: 'test-key', sendEnvironmentData: true })
134134
expect(config.sendEnvironmentData).toBe(true)
135135
})
136-
137-
it('sets sendSessionData to true by default', () => {
138-
const config = new Configuration({ apiKey: 'test-key' })
139-
expect(config.sendSessionData).toBe(true)
140-
})
141136
})
142137

143138
describe('isValid', () => {

0 commit comments

Comments
 (0)