fix: join service worker path with a single slash#1471
Conversation
A leading slash in the user-supplied serviceWorkerPath combined with the
default path ("/") produced a "//..." prefix, which the browser treats as a
protocol-relative URL and rejects with a SecurityError during worker
registration. Normalize the join so exactly one slash separates path and
serviceWorkerPath.
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested URL normalization fix.
Extended reasoning...
Overview
This PR tightens the service-worker URL construction in src/shared/helpers/context.ts. Previously getServiceWorkerManager concatenated userConfig.path and userConfig.serviceWorkerPath directly, which could produce //OneSignalSDKWorker.js when path defaulted to / and serviceWorkerPath had a leading slash. Browsers treat //foo as protocol-relative, sending the worker request to a cross-origin URL and failing registration with a SecurityError. The fix strips a trailing slash from path and a leading slash from serviceWorkerPath, then joins with exactly one slash. A new context.test.ts covers the four relevant join cases.
Security risks
None substantive. The change actively reduces a misconfiguration risk (worker URL ending up on an attacker-controlled-looking origin). The regex anchors (/\/+$/ and /^\/+/) are bounded to leading/trailing slashes and safe from ReDoS. No auth, crypto, or permissions surfaces touched.
Level of scrutiny
Low. This is a defensive string-normalization fix with unit-test coverage for the relevant edge cases (root path, leading-slash worker path, nested trailing-slash path, slashes on both sides). The change is localized to a single helper, mechanical in nature, and follows an obvious pattern. The PR description clearly explains the failure mode (subscription-10 symptom) and the fix.
Other factors
The test reaches into the manager via manager['_config'].workerPath._getFullPath(), which is a private-field access; that is a minor stylistic concern but matches how the join result must be inspected without a public accessor, and a future refactor that exposes the path would naturally update the test. No outstanding reviewer comments, and the requester (sherwinski) explicitly asked for review.
|
any changes to size limit? |
2 participants
Description
1 Line Summary
Harden the service-worker path join so a leading slash in
serviceWorkerPathcan't produce a protocol-relative//URL that fails worker registration.Details
Adds a guard to prevent the potential for a simple misconfiguration from silently breaking push.
getServiceWorkerManager(src/shared/helpers/context.ts) builds the worker URL by directly concatenatinguserConfig.pathanduserConfig.serviceWorkerPath:pathdefaults to"/". If an integration configuresserviceWorkerPath: "/OneSignalSDKWorker.js"(with a leading slash), the result is"//OneSignalSDKWorker.js". The browser interprets a leading//as a protocol-relative URL, treatingonesignalsdkworker.jsas a hostname (https://onesignalsdkworker.js/). Worker registration is then rejected cross-origin:With no worker, no push token is ever acquired, which downstream surfaces as
subscription-10("Subscription can't change notification_types to positive integer while disabled") because the subscription is sent with an empty token.The fix strips a trailing slash from
pathand a leading slash fromserviceWorkerPath, then joins them with exactly one slash — so both"/" + "OneSignalSDKWorker.js"and"/" + "/OneSignalSDKWorker.js"resolve to/OneSignalSDKWorker.js.Systems Affected
Validation
Tests
Info
Added
src/shared/helpers/context.test.tscovering the join logic:serviceWorkerPathno longer yields//Checklist
Screenshots
Checklist
Related Tickets
Surfaced while investigating SDK-4341.