Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in Mobile Safari 18.x: start poll request when document is visible and awaken #1552

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions lib/authn/util/poll.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import AuthSdkError from '../../errors/AuthSdkError';
import AuthPollStopError from '../../errors/AuthPollStopError';
import { AuthnTransactionState } from '../types';
import { getStateToken } from './stateToken';
import { isIOS } from '../../features';
import { isMobileSafari18 } from '../../features';

interface PollOptions {
delay?: number;
Expand Down Expand Up @@ -79,13 +79,14 @@ export function getPollFn(sdk, res: AuthnTransactionState, ref) {
var href = pollLink.href + toQueryString(opts);
return post(sdk, href, getStateToken(res), {
saveAuthnState: false,
withCredentials: true
withCredentials: true,
pollingIntent: true,
});
}

const delayNextPoll = (ms) => {
// no need for extra logic in non-iOS environments, just continue polling
if (!isIOS()) {
if (!isMobileSafari18()) {
return delayFn(ms);
}

Expand Down Expand Up @@ -135,7 +136,7 @@ export function getPollFn(sdk, res: AuthnTransactionState, ref) {
}

// don't trigger polling request if page is hidden wait until window is visible again
if (isIOS() && document.hidden) {
if (isMobileSafari18() && document.hidden) {
let handler;
return new Promise<void>((resolve) => {
handler = () => {
Expand Down
1 change: 1 addition & 0 deletions lib/base/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export interface FeaturesAPI {
isIE11OrLess(): boolean;
isDPoPSupported(): boolean;
isIOS(): boolean;
isMobileSafari18(): boolean;
}


Expand Down
1 change: 1 addition & 0 deletions lib/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

export const STATE_TOKEN_KEY_NAME = 'oktaStateToken';
export const DEFAULT_POLLING_DELAY = 500;
export const IOS_PAGE_AWAKEN_TIMEOUT = 500;
lesterchoi-okta marked this conversation as resolved.
Show resolved Hide resolved
export const DEFAULT_MAX_CLOCK_SKEW = 300;
export const DEFAULT_CACHE_DURATION = 86400;
export const TOKEN_STORAGE_NAME = 'okta-token-storage';
Expand Down
11 changes: 11 additions & 0 deletions lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
/* eslint-disable node/no-unsupported-features/node-builtins */
/* global document, window, TextEncoder, navigator */

import { UAParser } from 'ua-parser-js';
import { webcrypto } from './crypto';

const isWindowsPhone = /windows phone|iemobile|wpdesktop/i;
Expand Down Expand Up @@ -95,3 +96,13 @@ export function isIOS () {
// @ts-expect-error - MSStream is not in `window` type, unsurprisingly
(/iPad|iPhone|iPod/.test(navigator.userAgent) && !window.MSStream);
}

export function isMobileSafari18 () {
if (isBrowser() && typeof navigator !== 'undefined' && typeof navigator.userAgent !== 'undefined') {
// eslint-disable-next-line new-cap
const { browser, os } = UAParser(navigator.userAgent);
Comment on lines +101 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

Does UAParser not have these kinds of safety checks? If you wrote it this way because to navigator.userAgent "might be undefined", let UAParser find the user-agent string? The new will also avoid the need for the eslint annotation

    const { browser, os } = new UAParser().getResult();

I think your implementation/invocation is useful when you're running it in a node context or analyzing data.

return os.name?.toLowerCase() === 'ios' && !!browser.name?.toLowerCase()?.includes('safari')
&& browser.major === '18';
}
return false;
}
90 changes: 86 additions & 4 deletions lib/http/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

/* eslint-disable complexity */
import { isString, clone, isAbsoluteUrl, removeNils } from '../util';
import { STATE_TOKEN_KEY_NAME, DEFAULT_CACHE_DURATION } from '../constants';
import { STATE_TOKEN_KEY_NAME, DEFAULT_CACHE_DURATION, IOS_PAGE_AWAKEN_TIMEOUT } from '../constants';
import {
OktaAuthHttpInterface,
RequestOptions,
Expand All @@ -23,8 +23,22 @@ import {
HttpResponse
} from './types';
import { AuthApiError, OAuthError, APIError, WWWAuthError } from '../errors';
import { isMobileSafari18 } from '../features';


// For iOS track last date when document became visible
let dateDocumentBecameVisible = 0;
let trackDateDocumentBecameVisible: () => void;
if (isMobileSafari18()) {
dateDocumentBecameVisible = Date.now();
trackDateDocumentBecameVisible = () => {
if (!document.hidden) {
dateDocumentBecameVisible = Date.now();
}
};
document.addEventListener('visibilitychange', trackDateDocumentBecameVisible);
}

const formatError = (sdk: OktaAuthHttpInterface, error: HttpResponse | Error): AuthApiError | OAuthError => {
if (error instanceof Error) {
// fetch() can throw exceptions
Expand Down Expand Up @@ -96,6 +110,7 @@ const formatError = (sdk: OktaAuthHttpInterface, error: HttpResponse | Error): A
return err;
};

// eslint-disable-next-line max-statements
export function httpRequest(sdk: OktaAuthHttpInterface, options: RequestOptions): Promise<any> {
options = options || {};

Expand All @@ -113,7 +128,8 @@ export function httpRequest(sdk: OktaAuthHttpInterface, options: RequestOptions)
withCredentials = options.withCredentials === true, // default value is false
storageUtil = sdk.options.storageUtil,
storage = storageUtil!.storage,
httpCache = sdk.storageManager.getHttpCache(sdk.options.cookies);
httpCache = sdk.storageManager.getHttpCache(sdk.options.cookies),
pollingIntent = options.pollingIntent;

if (options.cacheResponse) {
var cacheContents = httpCache.getStorage();
Expand Down Expand Up @@ -142,8 +158,74 @@ export function httpRequest(sdk: OktaAuthHttpInterface, options: RequestOptions)
withCredentials
};

var err, res;
return sdk.options.httpRequestClient!(method!, url!, ajaxOptions)
var err, res, promise;

if (pollingIntent && isMobileSafari18()) {
let waitForVisibleAndAwakenDocument: () => Promise<void>;
let waitForAwakenDocument: () => Promise<void>;
let recursiveFetch: () => Promise<HttpResponse>;

// Safari on iOS has a bug:
// Performing `fetch` right after document became visible can fail with `Load failed` error.
// Running fetch after short timeout fixes this issue.
waitForAwakenDocument = () => {
const timeSinceDocumentIsVisible = Date.now() - dateDocumentBecameVisible;
if (timeSinceDocumentIsVisible < IOS_PAGE_AWAKEN_TIMEOUT) {
return new Promise<void>((resolve) => setTimeout(() => {
if (!document.hidden) {
resolve();
} else {
resolve(waitForVisibleAndAwakenDocument());
}
}, IOS_PAGE_AWAKEN_TIMEOUT - timeSinceDocumentIsVisible));
} else {
return Promise.resolve();
}
};

// Returns a promise that resolves when document is visible for 500 ms
waitForVisibleAndAwakenDocument = () => {
if (document.hidden) {
let pageVisibilityHandler: () => void;
return new Promise<void>((resolve) => {
pageVisibilityHandler = () => {
if (!document.hidden) {
document.removeEventListener('visibilitychange', pageVisibilityHandler);
resolve(waitForAwakenDocument());
}
};
document.addEventListener('visibilitychange', pageVisibilityHandler);
});
} else {
return waitForAwakenDocument();
}
};

// Restarts fetch on 'Load failed' error
// This error can occur when `fetch` does not respond
// (due to CORS error, non-existing host, or network error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to differentiate between/among these types of errors?

Seems to me isNetworkError will be true in all those cases and retry the fetch, although we don't want to retry on CORS, non-existent host, etc.

LMK if I'm misreading/misunderstanding the behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably good to add some tests to ensure we don't catch and retry CORS errors, etc

Copy link
Contributor Author

@denysoblohin-okta denysoblohin-okta Dec 20, 2024

Choose a reason for hiding this comment

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

You're correct, isNetworkError will be true in all those 3 cases.
There is no way to distinguish programmatically in JS.
(The difference can be seen only in browser console)

That's why I added canRetry option to enable retry logic only for polling requests.
Polling requests are repeatable by design so I think it's fine to retry it due to real network loss.

Since all API requests should be performed to the same host and poll request is never a first request in a list of ones performed by SIW, possible CORS error should be caught before polling is started.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Is there any logic to say we should stop trying polling after n failed attempts?

const retryableFetch = (): Promise<HttpResponse> => {
return sdk.options.httpRequestClient!(method!, url!, ajaxOptions).catch((err) => {
const isNetworkError = err?.message === 'Load failed';
if (isNetworkError) {
return recursiveFetch();
}
throw err;
});
};

// Final promise to fetch that wraps logic with waiting for visible document
// and retrying fetch request on network error
recursiveFetch = (): Promise<HttpResponse> => {
return waitForVisibleAndAwakenDocument().then(retryableFetch);
};

promise = recursiveFetch();
} else {
promise = sdk.options.httpRequestClient!(method!, url!, ajaxOptions);
}

return promise
.then(function(resp) {
res = resp.responseText;
if (res && isString(res)) {
Expand Down
1 change: 1 addition & 0 deletions lib/http/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export interface RequestOptions {
storageUtil?: StorageUtil;
cacheResponse?: boolean;
headers?: RequestHeaders;
pollingIntent?: boolean;
}

export interface FetchOptions {
Expand Down
11 changes: 8 additions & 3 deletions lib/idx/idxState/v1/generateIdxAction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*/

/* eslint-disable max-len, complexity */
import { httpRequest } from '../../../http';
import { httpRequest, RequestOptions } from '../../../http';
import { OktaAuthIdxInterface } from '../../types'; // auth-js/types
import { IdxActionFunction, IdxActionParams, IdxResponse, IdxToPersist } from '../../types/idx-js';
import { divideActionParamsByMutability } from './actionParser';
Expand All @@ -36,13 +36,18 @@ const generateDirectFetch = function generateDirectFetch(authClient: OktaAuthIdx
});

try {
const response = await httpRequest(authClient, {
const options: RequestOptions = {
url: target,
method: actionDefinition.method,
headers,
args: body,
withCredentials: toPersist?.withCredentials ?? true
});
};
const isPolling = actionDefinition.name === 'poll' || actionDefinition.name?.endsWith('-poll');
if (isPolling) {
options.pollingIntent = true;
}
const response = await httpRequest(authClient, options);

return authClient.idx.makeIdxResponse({ ...response }, toPersist, true);
}
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@
"node-cache": "^5.1.2",
"p-cancelable": "^2.0.0",
"tiny-emitter": "1.1.0",
"ua-parser-js": "^2.0.0",
"webcrypto-shim": "^0.1.5",
"xhr2": "0.1.3"
},
Expand Down Expand Up @@ -211,7 +212,7 @@
"rollup-plugin-cleanup": "^3.2.1",
"rollup-plugin-license": "^2.8.1",
"rollup-plugin-multi-input": "^1.3.1",
"rollup-plugin-typescript2": "^0.30.0",
"rollup-plugin-typescript2": "^0.36.0",
"rollup-plugin-visualizer": "~5.5.4",
"shelljs": "0.8.5",
"ts-jest": "^28.0.2",
Expand Down
3 changes: 2 additions & 1 deletion test/spec/TokenManager/browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ const mocked = {
isBrowser: () => typeof window !== 'undefined',
isIE11OrLess: () => false,
isLocalhost: () => false,
isTokenVerifySupported: () => true
isTokenVerifySupported: () => true,
isMobileSafari18: () => false
}
};
jest.mock('../../../lib/features', () => {
Expand Down
3 changes: 2 additions & 1 deletion test/spec/TokenManager/expireEvents.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
jest.mock('../../../lib/features', () => {
return {
isLocalhost: () => true, // to allow configuring expireEarlySeconds
isIE11OrLess: () => false
isIE11OrLess: () => false,
isMobileSafari18: () => false
};
});

Expand Down
20 changes: 10 additions & 10 deletions test/spec/authn/mfa-challenge.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ jest.mock('lib/features', () => {
const actual = jest.requireActual('../../../lib/features');
return {
...actual,
isIOS: () => false
isMobileSafari18: () => false
};
});
import OktaAuth from '@okta/okta-auth-js';
Expand Down Expand Up @@ -1581,7 +1581,7 @@ describe('MFA_CHALLENGE', function () {
});

// mocks iOS environment
jest.spyOn(mocked.features, 'isIOS').mockReturnValue(true);
jest.spyOn(mocked.features, 'isMobileSafari18').mockReturnValue(true);

const { response: mfaPush } = await util.generateXHRPair({
uri: 'https://auth-js-test.okta.com'
Expand All @@ -1592,17 +1592,17 @@ describe('MFA_CHALLENGE', function () {
}, 'success', 'https://auth-js-test.okta.com');

// mocks flow of wait, wait, wait, success
context.httpSpy = jest.spyOn(mocked.http, 'post')
.mockResolvedValueOnce(mfaPush.response)
.mockResolvedValueOnce(mfaPush.response)
.mockResolvedValueOnce(mfaPush.response)
.mockResolvedValueOnce(success.response);

context.httpSpy = jest.fn()
.mockResolvedValueOnce({responseText: JSON.stringify(mfaPush.response)})
.mockResolvedValueOnce({responseText: JSON.stringify(mfaPush.response)})
.mockResolvedValueOnce({responseText: JSON.stringify(mfaPush.response)})
.mockResolvedValueOnce({responseText: JSON.stringify(success.response)});

const oktaAuth = new OktaAuth({
issuer: 'https://auth-js-test.okta.com'
issuer: 'https://auth-js-test.okta.com',
httpRequestClient: context.httpSpy
});

context.transaction = oktaAuth.tx.createTransaction(mfaPush.response);
});

Expand Down
41 changes: 30 additions & 11 deletions test/spec/features/browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,29 +64,48 @@ describe('features (browser)', function() {
});
});

describe('isIOS', () => {
it('can succeed', () => {
const iOSAgents = [
'Mozilla/5.0 (iPhone; CPU iPhone OS 14_7 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) CriOS/92.0.4515.90 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 14_2 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0.1 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 14_7 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) CriOS/92.0.4515.90 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 14_7_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.1.2 Mobile/15E148 Safari/604.1'
];
describe('isIOS, isMobileSafari18', () => {
const iOSAgents = [
'Mozilla/5.0 (iPhone; CPU iPhone OS 14_7 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) CriOS/92.0.4515.90 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 14_2 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0.1 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 14_7 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) CriOS/92.0.4515.90 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 14_7_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.1.2 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 18_2 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) EdgiOS/130.0.2849.80 Version/18.0 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 18_2_0 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) EdgiOS/132.0.2957.32 Version/18.0 Mobile/15E148 Safari/604.1',
];
const mobileSafari18Agents = [
'Mozilla/5.0 (iPhone; CPU iPhone OS 18_0 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/18.0 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 18_2 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Teak/5.9 Version/18 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPhone; CPU iPhone OS 18_2 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/18.2 Mobile/15E148 Safari/604.1',
'Mozilla/5.0 (iPad; CPU OS 18_2 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/18.2 Mobile/15E148 Safari/604.1',
];

for (let userAgent of iOSAgents) {
for (let userAgent of iOSAgents) {
it('can succeed for ' + userAgent, () => {
jest.spyOn(global.navigator, 'userAgent', 'get').mockReturnValue(userAgent);
expect(OktaAuth.features.isIOS()).toBe(true);
}
});
expect(OktaAuth.features.isMobileSafari18()).toBe(false);
});
}
for (let userAgent of mobileSafari18Agents) {
// eslint-disable-next-line jasmine/no-spec-dupes
it('can succeed for ' + userAgent, () => {
jest.spyOn(global.navigator, 'userAgent', 'get').mockReturnValue(userAgent);
expect(OktaAuth.features.isIOS()).toBe(true);
expect(OktaAuth.features.isMobileSafari18()).toBe(true);
});
}

it('returns false if navigator is unavailable', () => {
jest.spyOn(global, 'navigator', 'get').mockReturnValue(undefined as never);
expect(OktaAuth.features.isIOS()).toBe(false);
expect(OktaAuth.features.isMobileSafari18()).toBe(false);
});

it('returns false if userAgent is unavailable', () => {
jest.spyOn(global.navigator, 'userAgent', 'get').mockReturnValue(undefined as never);
expect(OktaAuth.features.isIOS()).toBe(false);
expect(OktaAuth.features.isMobileSafari18()).toBe(false);
});
});
});
Loading