Skip to content

Conversation

@Dobrunia
Copy link
Member

Problem

When beforeSend returns a non-object (e.g. true, undefined, or omits return), that value was used as payload and sent to the collector. The backend then stored it as-is (e.g. payload: true in MongoDB), which led to:

  • GraphQL: Cannot return null for non-nullable field EventPayload.title
  • Frontend: TypeError: can't access property "event", l is null on project overview

Common mistakes:

  • beforeSend: (e) => true (meant “allow”, but overwrites payload)
  • beforeSend: (e) => { console.log(e); } (no return → undefined as payload)

Solution

Validate the return value of beforeSend and only use it when it’s a valid event object:

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the JavaScript SDK hook handling so beforeSend (and similarly beforeBreadcrumb) can’t accidentally cause invalid payloads/breadcrumbs to be sent/stored when the hook returns an unexpected value.

Changes:

  • Add runtime validators (isValidEventPayload, isValidBreadcrumb) and apply them to hook results.
  • Update hook typings/docs to allow void return (keep original) and use false to explicitly drop events/breadcrumbs.
  • Add/adjust examples and docs to demonstrate the new hook behavior.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
packages/javascript/src/utils/validation.ts Adds runtime validation helpers for event payloads and breadcrumbs.
packages/javascript/src/types/hawk-initial-settings.ts Updates beforeSend typing/docs to include void return behavior.
packages/javascript/src/catcher.ts Validates beforeSend return value before overwriting the outgoing payload; adds warnings for invalid returns.
packages/javascript/src/addons/breadcrumbs.ts Validates beforeBreadcrumb return value; updates hook behavior/docs and warnings.
packages/javascript/package.json Bumps package version to 3.2.16.
packages/javascript/example/index.html Updates example to use false (instead of null) for discarding breadcrumbs.
packages/javascript/example/hooks-tests.html Adds a manual test page for hook scenarios.
packages/javascript/example/before-send-tests.js Adds manual test script to exercise hook return cases.
packages/javascript/README.md Updates public docs for hook return semantics and examples.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

return false;
}

if (payload.backtrace !== undefined && !Array.isArray(payload.backtrace)) {
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

isValidEventPayload treats backtrace: null as invalid because it only allows an array when the field is present. In this SDK, HawkJavaScriptEvent['backtrace'] is BacktraceFrame[] | null, so a payload returned from beforeSend that keeps backtrace as null will be rejected and silently ignored. Update the check to allow null (and/or align the runtime validation with HawkJavaScriptEvent’s actual shape).

Suggested change
if (payload.backtrace !== undefined && !Array.isArray(payload.backtrace)) {
if (payload.backtrace !== undefined && payload.backtrace !== null && !Array.isArray(payload.backtrace)) {

Copilot uses AI. Check for mistakes.
* @param breadcrumb - value to validate
*/
export function isValidBreadcrumb(breadcrumb: unknown): breadcrumb is Breadcrumb {
if (typeof breadcrumb !== 'object' || breadcrumb === null || Array.isArray(breadcrumb)) {
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The JSDoc says the breadcrumb must be a plain object, but isValidBreadcrumb currently accepts any non-null, non-array object (e.g. new Date()), which doesn’t match the documented behavior. Either tighten the check (e.g. reuse isPlainObject) or adjust the comment to reflect what’s actually validated.

Suggested change
if (typeof breadcrumb !== 'object' || breadcrumb === null || Array.isArray(breadcrumb)) {
if (!isPlainObject(breadcrumb)) {

Copilot uses AI. Check for mistakes.
* - Return nothing (`void` / `undefined` / `null`) — the original event is sent as-is (a warning is logged).
*/
private readonly beforeSend: undefined | ((event: HawkJavaScriptEvent) => HawkJavaScriptEvent | false);
private readonly beforeSend: undefined | ((event: HawkJavaScriptEvent) => HawkJavaScriptEvent | false | void);
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The comment mentions that beforeSend may return null, and the runtime code handles null, but the TypeScript type doesn’t include null (HawkJavaScriptEvent | false | void). Either include null in the return type or remove null from the docs/handling so the public API is consistent.

Suggested change
private readonly beforeSend: undefined | ((event: HawkJavaScriptEvent) => HawkJavaScriptEvent | false | void);
private readonly beforeSend: undefined | ((event: HawkJavaScriptEvent) => HawkJavaScriptEvent | false | void | null);

Copilot uses AI. Check for mistakes.
Comment on lines 451 to 454
* If user returned nothing (void/undefined/null) — warn and keep original payload
*/
if (result === undefined || result === null) {
log(`[Hawk] Invalid beforeSend value: (${String(result)}). It should return event or false. Event is sent without changes.`, 'warn');
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The warning says the event is sent “without changes”, but beforeSend receives the actual payload object—any in-place mutations done before returning undefined/null will still be sent. To make this accurate, either clone/freeze the payload before calling beforeSend (and only use the clone when the hook returns a valid event) or reword the warning to not promise that no changes were applied.

Suggested change
* If user returned nothing (void/undefined/null) warn and keep original payload
*/
if (result === undefined || result === null) {
log(`[Hawk] Invalid beforeSend value: (${String(result)}). It should return event or false. Event is sent without changes.`, 'warn');
* If user returned nothing (void/undefined/null) warn and send current payload as is
*/
if (result === undefined || result === null) {
log(`[Hawk] Invalid beforeSend value: (${String(result)}). It should return event or false. Event will be sent as is.`, 'warn');

Copilot uses AI. Check for mistakes.
Comment on lines +241 to 246
/**
* false means discard
*/
if (result === false) {
return;
}
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

beforeBreadcrumb previously used null to discard breadcrumbs, but this change switches the sentinel to false and now treats null as “no return” (stores the breadcrumb). That’s a breaking behavior change for existing consumers; consider continuing to treat null as discard (possibly with a deprecation warning) or bump the package with an appropriate breaking-change version.

Copilot uses AI. Check for mistakes.
* - If the hook returns an invalid value, a warning is logged and the original breadcrumb is stored.
*/
beforeBreadcrumb?: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => Breadcrumb | null;
beforeBreadcrumb?: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => Breadcrumb | false | void;
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The JSDoc says beforeBreadcrumb can return null, and the runtime path handles null, but the callback type is Breadcrumb | false | void (no null). Align the public type signature with the documented/runtime behavior (either add null or remove null from docs and handling).

Suggested change
beforeBreadcrumb?: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => Breadcrumb | false | void;
beforeBreadcrumb?: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => Breadcrumb | false | null | void;

Copilot uses AI. Check for mistakes.
Comment on lines 249 to 257
* void/undefined/null — warn and keep original breadcrumb
*/
if (result === undefined || result === null) {
log('[Hawk] beforeBreadcrumb returned nothing, storing original breadcrumb.', 'warn');
} else if (isValidBreadcrumb(result)) {
Object.assign(bc, result);
} else {
log(
'[Hawk] beforeBreadcrumb produced invalid breadcrumb (must be an object with numeric timestamp), storing original. '
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The log message says it’s “storing original breadcrumb”, but beforeBreadcrumb is called with the live bc object, so in-place mutations will still be stored even when the hook returns undefined/null or an invalid value. Consider cloning bc before invoking the hook or reword the warning to avoid implying the breadcrumb is unchanged.

Suggested change
* void/undefined/null warn and keep original breadcrumb
*/
if (result === undefined || result === null) {
log('[Hawk] beforeBreadcrumb returned nothing, storing original breadcrumb.', 'warn');
} else if (isValidBreadcrumb(result)) {
Object.assign(bc, result);
} else {
log(
'[Hawk] beforeBreadcrumb produced invalid breadcrumb (must be an object with numeric timestamp), storing original. '
* void/undefined/null warn and keep breadcrumb as last modified by hook
*/
if (result === undefined || result === null) {
log('[Hawk] beforeBreadcrumb returned nothing; keeping breadcrumb as last modified by hook.', 'warn');
} else if (isValidBreadcrumb(result)) {
Object.assign(bc, result);
} else {
log(
'[Hawk] beforeBreadcrumb produced invalid breadcrumb (must be an object with numeric timestamp); keeping breadcrumb as last modified by hook. '

Copilot uses AI. Check for mistakes.
* - Return nothing (`void` / `undefined` / `null`) — the original event is sent as-is (a warning is logged).
*/
beforeSend?(event: HawkJavaScriptEvent): HawkJavaScriptEvent | false;
beforeSend?(event: HawkJavaScriptEvent): HawkJavaScriptEvent | false | void;
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

beforeSend docs mention null as a possible “no return” value, but the declared type is HawkJavaScriptEvent | false | void (no null). If null is intended to be treated like undefined, add null to the signature; otherwise remove it from the docs to keep the API contract consistent.

Suggested change
beforeSend?(event: HawkJavaScriptEvent): HawkJavaScriptEvent | false | void;
beforeSend?(event: HawkJavaScriptEvent): HawkJavaScriptEvent | false | void | null;

Copilot uses AI. Check for mistakes.
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.

3 participants