Harden extraction WebView, cache Readability.js, sharpen YouTube thumbs

- Cache the ~90KB Readability.js asset read (identical every time)
  instead of a disk read + UTF-8 decode on the main thread for every
  single extraction.
- Lock down allowFileAccess/allowContentAccess/allowUniversalAccess:
  this WebView runs full JS against any article link, feed URL, or
  (via ShareReceiverActivity) any URL another app hands it -- no
  legitimate reason to ever load file://.
- hq720 thumbnail instead of hqdefault (a letterboxed 4:3 crop).
- Validate frameSrc is actually http(s) before using it verbatim as a
  fallback link -- it's attacker-controlled article content that only
  had to match a substring filter, not the strict video-ID regex, so a
  crafted file://, content://, or intent:// src couldn't ride along
  into an ACTION_VIEW intent or a JS-enabled WebView.loadUrl.

1 files changed, 35 insertions, 4 deletions

diff --git a/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt b/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt
index 1edef0e..03a7d7c 100644
--- a/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt
+++ b/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt
@@ -40,6 +40,18 @@ object ReaderExtractor {
     private const val TIMEOUT_MS = 25_000L
     private val json = Json { ignoreUnknownKeys = true }
 
+    // The vendored asset (~90KB) is identical every time regardless of which Context reads
+    // it -- caching it once avoids a disk read + UTF-8 decode on the MAIN thread (this
+    // whole class runs its WebView work on Dispatchers.Main) on every single extraction,
+    // which adds up over a reading session that opens/re-extracts many articles. No
+    // synchronization needed: extractOnMainThread only ever runs on Dispatchers.Main, so
+    // this is never accessed from more than one thread.
+    private var cachedReadabilityJs: String? = null
+
+    private fun loadReadabilityJs(context: Context): String =
+        cachedReadabilityJs ?: context.assets.open("Readability.js").bufferedReader().use { it.readText() }
+            .also { cachedReadabilityJs = it }
+
     suspend fun extract(context: Context, url: String): ExtractedArticle? =
         withTimeoutOrNull(TIMEOUT_MS) {
             withContext(Dispatchers.Main) {
@@ -52,6 +64,15 @@ object ReaderExtractor {
         suspendCancellableCoroutine { continuation ->
             val webView = WebView(context)
             webView.settings.javaScriptEnabled = true
+            // This WebView runs full JS against whatever URL it's given -- every article
+            // link, every feed URL, and (via ShareReceiverActivity) any URL another
+            // installed app hands this one. It has no legitimate reason to ever load a
+            // file:// URL, so don't leave that door open for a malicious page to redirect
+            // through (WebView defaults allowFileAccess to true pre-API 30).
+            webView.settings.allowFileAccess = false
+            webView.settings.allowContentAccess = false
+            webView.settings.allowFileAccessFromFileURLs = false
+            webView.settings.allowUniversalAccessFromFileURLs = false
 
             // A slow-loading page's evaluateJavascript callback can arrive right as the
             // outer withTimeoutOrNull cancels this coroutine, racing finish() (JS-callback
@@ -70,7 +91,7 @@ object ReaderExtractor {
 
             webView.webViewClient = object : WebViewClient() {
                 override fun onPageFinished(view: WebView, finishedUrl: String) {
-                    val readabilityJs = context.assets.open("Readability.js").bufferedReader().use { it.readText() }
+                    val readabilityJs = loadReadabilityJs(context)
                     val script = """
                         (function() {
                             try {
@@ -231,7 +252,11 @@ object ReaderExtractor {
                                         var videoId = match[1];
                                         link.href = 'https://www.youtube.com/watch?v=' + videoId;
                                         var thumb = document.createElement('img');
-                                        thumb.src = 'https://i.ytimg.com/vi/' + videoId + '/hqdefault.jpg';
+                                        // hq720 (true 16:9 crop) beats hqdefault (a 480x360 4:3
+                                        // crop that letterboxes any widescreen video with black
+                                        // bars) and, unlike maxresdefault, is reliably present for
+                                        // essentially every video -- no existence check needed.
+                                        thumb.src = 'https://i.ytimg.com/vi/' + videoId + '/hq720.jpg';
                                         thumb.alt = 'YouTube video';
                                         link.appendChild(thumb);
                                         var caption = document.createElement('figcaption');
@@ -240,8 +265,14 @@ object ReaderExtractor {
                                     } else {
                                         // No confident single video ID (a playlist, a live stream, or
                                         // lazy-load resolution above still came up empty) -- a plain
-                                        // link beats a broken thumbnail or a dead iframe.
-                                        link.href = frameSrc || 'https://www.youtube.com/';
+                                        // link beats a broken thumbnail or a dead iframe. frameSrc is
+                                        // attacker-controlled article content though (it only needed to
+                                        // match the querySelector's substring src filter, not the strict
+                                        // video-ID regex) -- only use it verbatim if it's actually an
+                                        // http(s) URL, so a crafted file://, content://, or intent://
+                                        // iframe src can't ride along into this href and downstream into
+                                        // an ACTION_VIEW intent or a JS-enabled WebView.loadUrl.
+                                        link.href = /^https?:\/\//i.test(frameSrc) ? frameSrc : 'https://www.youtube.com/';
                                         link.textContent = 'Watch on YouTube';
                                     }
                                     frame.parentNode.replaceChild(figure, frame);