Skip to content

fix: [SDK-5304] reject null and empty identity strings instead of crashing - #1997

Open
abdulraqeeb33 wants to merge 3 commits into
mainfrom
ar/sdk-5304
Open

abdulraqeeb33 wants to merge 3 commits into
mainfrom
ar/sdk-5304

Conversation

@abdulraqeeb33

@abdulraqeeb33 abdulraqeeb33 commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Description

One Line Summary

Return from React Native identity string APIs when the value is null or empty instead of crashing Kotlin non-null String parameters.

Details

Motivation

Fixes SDK-5304. JS null reaches Kotlin OneSignal.login(externalId: String) (and the same class of APIs: addEmail, initialize(appId)), which throws IllegalArgumentException: Parameter specified as non-null is null. The TurboModule method is void and uncaught, so the host process dies.

Empty string does not NPE, but login("") is not a valid user id. Rejected in the same helper.

Other SDKs: SDK-5309.

Scope

isNonEmptyString in JS (null, undefined, empty). Used by login, initialize, email/sms/alias/language, tags, and triggers. Native login also returns on null/nil or empty. addTrigger now returns after the invalid-input log instead of still calling native. Public TS types stay string.

Testing

Unit testing

src/index.test.ts and src/helpers.test.ts: 289 tests passed. Covers login/initialize/addEmail null and empty, helper table, and addTrigger no longer calling native.

Manual testing

Not run on a device. The crash is a null argument at the bridge. Unit coverage is the JS helper. Native login guards match null/empty.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

Co-authored-by: Cursor <cursoragent@cursor.com>
@abdulraqeeb33
abdulraqeeb33 requested a review from a team as a code owner September 21, 2026 15:58

@nan-li nan-li left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We have login(externalId: string) defined as string instead of optional string in the API:

export function login(externalId: string) {

Maybe we can do the check there instead of at both bridges, like we do for addTag where we check:

      if (!key || value === undefined || value === null) {
        console.error('OneSignal: addTag: must include a key and a value');
        return;
      }

Are there other methods where devs are submitting invalid inputs that can crash?

Comment thread src/index.ts Outdated
Kotlin non-null String APIs crash when JS passes null. Empty login is
invalid too. Guard both in isNonEmptyString and skip the native call.

Co-authored-by: Cursor <cursoragent@cursor.com>
@abdulraqeeb33 abdulraqeeb33 changed the title fix: [SDK-5304] ignore null login externalId instead of crashing fix: [SDK-5304] reject null and empty identity strings instead of crashing Sep 21, 2026
@abdulraqeeb33

Copy link
Copy Markdown
Contributor Author

We have login(externalId: string) defined as string instead of optional string in the API:

export function login(externalId: string) {

Maybe we can do the check there instead of at both bridges, like we do for addTag where we check:

      if (!key || value === undefined || value === null) {
        console.error('OneSignal: addTag: must include a key and a value');
        return;
      }

Are there other methods where devs are submitting invalid inputs that can crash?

Good cal @nan-li - updated it in other methods as well. please check it out.

NSLog bypasses setLogLevel. Use the same OneSignalLog error path as
the rest of the iOS bridge.

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread src/index.ts
/** Initializes the OneSignal SDK. This should be called during startup of the application. */
export function initialize(appId: string) {
if (!isNativeModuleLoaded(RNOneSignal)) return;
if (!isNonEmptyString(appId)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can just be !appId , similar in other places

@fadi-george fadi-george left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

id make the empty string changes a separate ticket and get agreement on changing for all sdks

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