diff --git a/src/reactUtils.js b/src/reactUtils.js index b92e7fa202a..b7ba00b4aaf 100644 --- a/src/reactUtils.js +++ b/src/reactUtils.js @@ -1,6 +1,20 @@ /* @flow strict-local */ import invariant from 'invariant'; -import { useRef, useEffect, useState } from 'react'; +import * as React from 'react'; + +/** + * Like React.ElementConfig, but includes the pseudoprops `ref` and `key`. + * + * That is, this contains exactly the set of JSX attributes one can pass + * when creating an element of this component-type. + * + * Assumes the underlying props type is an exact object type. + */ +export type ElementConfigFull<+C> = {| + ...$Exact>, + +ref?: React.Ref, + +key?: React.Key, +|}; /** * A Hook for the value of a prop, state, etc., from the previous render. @@ -25,8 +39,8 @@ import { useRef, useEffect, useState } from 'react'; // (because effectively the `?` would handle it instead), and so `U` would // be the empty type and `T | U` would be just `T`. export function usePrevious(value: T, initValue: U): T | U { - const ref = useRef(initValue); - useEffect(() => { + const ref = React.useRef(initValue); + React.useEffect(() => { ref.current = value; }); return ref.current; @@ -48,7 +62,7 @@ export function useDebugAssertConstant(value: T) { // Conditional, but on a per-process constant. // eslint-disable-next-line react-hooks/rules-of-hooks - const origValue = useRef(value); + const origValue = React.useRef(value); invariant(value === origValue.current, ''); } @@ -71,9 +85,9 @@ export function useDebugAssertConstant(value: T) { export function useHasNotChangedForMs(value: mixed, duration: number): boolean { useDebugAssertConstant(duration); - const [result, setResult] = useState(false); + const [result, setResult] = React.useState(false); - useEffect(() => { + React.useEffect(() => { setResult(false); const id = setTimeout(() => setResult(true), duration); return () => clearTimeout(id); @@ -134,4 +148,4 @@ export const useHasStayedTrueForMs = (value: boolean, duration: number): boolean // docs could be clearer about that: // https://reactjs.org/docs/hooks-effect.html#tip-optimizing-performance-by-skipping-effects export const useConditionalEffect = (cb: () => void | (() => void), value: boolean): void => - useEffect(() => (value ? cb() : undefined), [value, cb]); + React.useEffect(() => (value ? cb() : undefined), [value, cb]); diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 8e6e7353560..1726f2d9cc7 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -1,5 +1,5 @@ /* @flow strict-local */ -import React, { Component, type ComponentType } from 'react'; +import * as React from 'react'; import { Platform, NativeModules } from 'react-native'; import { WebView } from 'react-native-webview'; @@ -33,11 +33,10 @@ import messageListElementHtml from './html/messageListElementHtml'; import generateInboundEvents from './generateInboundEvents'; import { handleWebViewOutboundEvent } from './handleOutboundEvents'; import { base64Utf8Encode } from '../utils/encoding'; -import * as logging from '../utils/logging'; -import { tryParseUrl } from '../utils/url'; import { caseNarrow, isConversationNarrow } from '../utils/narrow'; import { type BackgroundData, getBackgroundData } from './backgroundData'; import { ensureUnreachable } from '../generics'; +import { renderSinglePageWebView } from './SinglePageWebView'; type OuterProps = $ReadOnly<{| narrow: Narrow, @@ -76,20 +75,27 @@ export type Props = $ReadOnly<{| /** * The URL of the platform-specific assets folder. * - * - On iOS: We can't easily hardcode this because it includes UUIDs. - * So we bring it over the React Native bridge in ZLPConstants.m. - * - * - On Android: Different apps' WebViews see different (virtual) root - * directories as `file:///`, and in particular the WebView provides - * the APK's `assets/` directory as `file:///android_asset/`. [1] - * We can easily hardcode that, so we do. - * - * [1] Oddly, this essential feature doesn't seem to be documented! It's - * widely described in how-tos across the web and StackOverflow answers. - * It's assumed in some related docs which mention it in passing, and - * treated matter-of-factly in some Chromium bug threads. Details at: - * https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/android.20filesystem/near/796440 + * This will be a `file:` URL. */ +// It could be perfectly reasonable for this to be an `http:` or `https:` +// URL instead, at least in development. We'd then just need to adjust +// the `originWhitelist` we pass to `WebView`. +// +// - On iOS: We can't easily hardcode this because it includes UUIDs. +// So we bring it over the React Native bridge in ZLPConstants.m. +// It's a file URL because the app bundle's `resourceURL` is: +// https://developer.apple.com/documentation/foundation/bundle/1414821-resourceurl +// +// - On Android: Different apps' WebViews see different (virtual) root +// directories as `file:///`, and in particular the WebView provides +// the APK's `assets/` directory as `file:///android_asset/`. [1] +// We can easily hardcode that, so we do. +// +// [1] Oddly, this essential feature doesn't seem to be documented! It's +// widely described in how-tos across the web and StackOverflow answers. +// It's assumed in some related docs which mention it in passing, and +// treated matter-of-factly in some Chromium bug threads. Details at: +// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/android.20filesystem/near/796440 const assetsUrl = Platform.OS === 'ios' ? new URL(NativeModules.ZLPConstants.resourceURL) @@ -102,7 +108,21 @@ const assetsUrl = */ const webviewAssetsUrl = new URL('webview/', assetsUrl); -class MessageListInner extends Component { +/** + * Effective URL of the MessageList webview. + * + * It points to `index.html` in the webview-assets folder, which + * doesn't exist. + * + * It doesn't need to exist because we provide all HTML at + * creation (or refresh) time. This serves only as a placeholder, + * so that relative URLs (e.g., to `base.css`, which does exist) + * and cross-domain security restrictions have somewhere to + * believe that this document originates from. + */ +const baseUrl = new URL('index.html', webviewAssetsUrl); + +class MessageListInner extends React.Component { webviewRef = React.createRef>(); sendInboundEventsIsReady: boolean; unsentInboundEvents: WebViewInboundEvent[] = []; @@ -154,13 +174,7 @@ class MessageListInner extends Component { _, } = this.props; const contentHtml = messageListElementsForShownMessages - .map(element => - messageListElementHtml({ - backgroundData, - element, - _, - }), - ) + .map(element => messageListElementHtml({ backgroundData, element, _ })) .join(''); const { auth, theme } = backgroundData; const html: string = getHtml( @@ -175,80 +189,13 @@ class MessageListInner extends Component { backgroundData.serverEmojiData, ); - /** - * Effective URL of the MessageList webview. - * - * It points to `index.html` in the webview-assets folder, which - * doesn't exist. - * - * It doesn't need to exist because we provide all HTML at - * creation (or refresh) time. This serves only as a placeholder, - * so that relative URLs (e.g., to `base.css`, which does exist) - * and cross-domain security restrictions have somewhere to - * believe that this document originates from. - */ - const baseUrl = new URL('index.html', webviewAssetsUrl); - - // Paranoia^WSecurity: only load `baseUrl`, and only load it once. Any other - // requests should be handed off to the OS, not loaded inside the WebView. - const onShouldStartLoadWithRequest = (() => { - // Inner closure to actually test the URL. - const urlTester: (url: string) => boolean = (() => { - // On Android this function is documented to be skipped on first load: - // therefore, simply never return true. - if (Platform.OS === 'android') { - return (url: string) => false; - } - - // Otherwise (for iOS), return a closure that evaluates to `true` _exactly - // once_, and even then only if the URL looks like what we're expecting. - let loaded_once = false; - return (url: string) => { - const parsedUrl = tryParseUrl(url); - if (!loaded_once && parsedUrl && parsedUrl.toString() === baseUrl.toString()) { - loaded_once = true; - return true; - } - return false; - }; - })(); - - // Outer closure to perform logging. - return event => { - const ok = urlTester(event.url); - if (!ok) { - logging.warn('webview: rejected navigation event', { - navigation_event: { ...event }, - expected_url: baseUrl.toString(), - }); - } - return ok; - }; - })(); - - // The `originWhitelist` and `onShouldStartLoadWithRequest` props are - // meant to mitigate possible XSS bugs, by interrupting an attempted - // exploit if it tries to navigate to a new URL by e.g. setting - // `window.location`. - // - // Note that neither of them is a hard security barrier; they're checked - // only against the URL of the document itself. They cannot be used to - // validate the URL of other resources the WebView loads. - // - // Worse, the `originWhitelist` parameter is completely broken. See: - // https://github.com/react-native-community/react-native-webview/pull/697 - return ( - - ); + return renderSinglePageWebView(html, baseUrl, { + decelerationRate: 'normal', + style: { backgroundColor: 'transparent' }, + ref: this.webviewRef, + onMessage: this.handleMessage, + onError: this.handleError, + }); } } @@ -283,7 +230,7 @@ const marksMessagesAsRead = (narrow: Narrow): boolean => mentioned: () => false, }); -const MessageList: ComponentType = connect( +const MessageList: React.ComponentType = connect( (state, props: OuterProps) => { // If this were a function component with Hooks, these would be // useGlobalSelector calls and would coexist perfectly smoothly with diff --git a/src/webview/SinglePageWebView.js b/src/webview/SinglePageWebView.js new file mode 100644 index 00000000000..fd522715e8f --- /dev/null +++ b/src/webview/SinglePageWebView.js @@ -0,0 +1,99 @@ +// @flow strict-local +import * as React from 'react'; +import { Platform } from 'react-native'; +import { WebView } from 'react-native-webview'; + +import * as logging from '../utils/logging'; +import { tryParseUrl } from '../utils/url'; +import type { ElementConfigFull } from '../reactUtils'; + +/** + * Return a suitable onShouldStartLoadWithRequest for a single-page WebView. + * + * When passed as the onShouldStartLoadWithRequest prop to a WebView, the + * returned callback will ensure that the webview never navigates away from + * `baseUrl`. + * + * This is a hardening measure for our message-list WebView. We already + * intercept clicks/touches and open links in a separate browser, but this + * ensures that if something slips through that it still doesn't break our + * security assumptions. + */ +// See upstream docs for this WebView prop: +// https://github.com/react-native-webview/react-native-webview/blob/v11.22.2/docs/Reference.md#onshouldstartloadwithrequest +function makeOnShouldStartLoadWithRequest( + baseUrl: URL, +): React.ElementConfig['onShouldStartLoadWithRequest'] { + let loaded_once = false; + + return event => { + // eslint-disable-next-line no-use-before-define + const ok = urlIsOk(event.url); + if (!ok) { + logging.warn('webview: rejected navigation event', { + navigation_event: { ...event }, + expected_url: baseUrl.toString(), + }); + } + return ok; + }; + + function urlIsOk(url: string): boolean { + // On Android the onShouldStartLoadWithRequest prop is documented to be + // skipped on first load; therefore, simply never return true. + if (Platform.OS === 'android') { + return false; + } + + // Otherwise (for iOS), return `true` only if the URL looks like what + // we're expecting, and only the first such time. + const parsedUrl = tryParseUrl(url); + if (!loaded_once && parsedUrl && parsedUrl.toString() === baseUrl.toString()) { + loaded_once = true; + return true; + } + return false; + } +} + +/** + * Render a WebView that shows the given HTML at the given base URL, only. + * + * The WebView will show the page described by the HTML string `html`. Any + * attempts to navigate to a new page will be rejected. + * + * Relative URL references to other resources (scripts, images, etc.) will + * be resolved relative to `baseUrl`. + * + * Assumes `baseUrl` has the scheme `file:`. No actual file need exist at + * `baseUrl` itself, because the page is taken from the string `html`. + */ +// TODO: This should ideally be a proper React component of its own. The +// thing that may require care when doing that is our use of +// `shouldComponentUpdate` in its caller, `MessageList`. +export const renderSinglePageWebView = ( + html: string, + baseUrl: URL, + moreProps: $Rest< + ElementConfigFull, + {| source: mixed, originWhitelist: mixed, onShouldStartLoadWithRequest: mixed |}, + >, +): React.Node => ( + // The `originWhitelist` and `onShouldStartLoadWithRequest` props are + // meant to mitigate possible XSS bugs, by interrupting an attempted + // exploit if it tries to navigate to a new URL by e.g. setting + // `window.location`. + // + // Note that neither of them is a hard security barrier; they're checked + // only against the URL of the document itself. They cannot be used to + // validate the URL of other resources the WebView loads. + // + // Worse, the `originWhitelist` parameter is completely broken. See: + // https://github.com/react-native-community/react-native-webview/pull/697 + +);