diff options
Diffstat (limited to 'app')
10 files changed, 580 insertions, 25 deletions
diff --git a/app/src/main/java/org/terst/feedreader/data/CachedExtraction.kt b/app/src/main/java/org/terst/feedreader/data/CachedExtraction.kt new file mode 100644 index 0000000..313d467 --- /dev/null +++ b/app/src/main/java/org/terst/feedreader/data/CachedExtraction.kt @@ -0,0 +1,41 @@ +package org.terst.feedreader.data + +import org.terst.feedreader.readability.ConsentNotice + +/** What a cached `articles.extractedContent` value turned out to be worth. */ +data class CachedExtraction( + /** The content to show, or null if there is nothing usable. */ + val content: String?, + /** + * True when a cached consent notice was thrown away. The caller has nothing to + * show and should extract again -- the cache is the reason the article looked + * broken, so dropping it silently would just leave a blank reader. + */ + val discardedConsentNotice: Boolean +) + +/** + * Reads a cached extraction, dropping it if it is the site's cookie/consent notice + * rather than the article. + * + * Needed because the extractor's own guard can only keep a bad extraction from + * being written in future. The reported bug ("reader mode just extracts cookie + * disclaimer", with `extractedContent cached: true`) was already on disk: every + * subsequent open read it straight back out, so no amount of fixing the extractor + * would have changed what the user saw. A poisoned row is cleared here rather than + * in a schema migration, so it is repaired whenever the article is next opened -- + * including for anything extracted before this check existed. + * + * [clearCache] is passed in rather than taking the DAO so this stays a plain + * decision that can be tested without a database. + */ +suspend fun loadCachedExtraction( + cached: String?, + clearCache: suspend () -> Unit +): CachedExtraction { + if (!ConsentNotice.looksLikeConsentNotice(cached)) { + return CachedExtraction(content = cached, discardedConsentNotice = false) + } + clearCache() + return CachedExtraction(content = null, discardedConsentNotice = true) +} diff --git a/app/src/main/java/org/terst/feedreader/readability/ConsentNotice.kt b/app/src/main/java/org/terst/feedreader/readability/ConsentNotice.kt new file mode 100644 index 0000000..5699595 --- /dev/null +++ b/app/src/main/java/org/terst/feedreader/readability/ConsentNotice.kt @@ -0,0 +1,132 @@ +package org.terst.feedreader.readability + +/** + * Recognises an extraction result that is the page's cookie/consent notice rather + * than its article. + * + * Why this is needed at all: Readability's first pass discards "unlikely + * candidates" by class/id, which a consent banner normally is. But when that pass + * comes back under Readability's own 500-character threshold -- which is what + * happens whenever the real article body is not in the DOM at the moment the + * script runs -- it retries with that stripping switched OFF. A consent banner is + * then a perfectly ordinary candidate, and the ones sites actually ship are long + * enough to clear the threshold (the reported New Republic/Didomi notice is 510 + * characters). Readability hands it back as the article, with the right title + * attached, and nothing downstream could tell the difference. + * + * The bar for rejecting something has to be high: throwing away a real article is + * worse than showing a cookie notice. So two independent, deliberately narrow + * signals, and a hard length cap on both -- a real article body is far longer than + * any banner, so nothing long is ever rejected no matter what it says. + */ +object ConsentNotice { + + /** + * Plain-text length past which a result is never treated as a banner, however + * much consent boilerplate it contains. Readability's own `charThreshold` is + * 500; this leaves generous room above it for the longest real banners + * (multi-paragraph "we and our 847 partners" notices) while staying far below + * any actual article. + */ + private const val MAX_NOTICE_TEXT_LENGTH = 2_000 + + /** How many distinct consent phrases a result needs before wording alone convicts it. */ + private const val MIN_PHRASE_MATCHES = 2 + + /** + * Container ids belonging to consent-management platforms. Readability strips + * class attributes but preserves ids, so the CMP's own container id survives + * into the extracted HTML -- the most precise signal available, since no real + * article body is ever inside one of these. + */ + private val CMP_CONTAINER_IDS = listOf( + "didomi-host", + "didomi-notice", + "didomi-popup", + "onetrust-banner-sdk", + "onetrust-consent-sdk", + "onetrust-policy-text", + "optanon-alert-box-wrapper", + "cybotcookiebotdialog", + "qc-cmp2-container", + "sp_message_container", + "truste-consent-track", + "usercentrics-root", + "usercentrics-cmp-ui", + "osano-cm-dialog", + "iubenda-cs-banner", + "cookie-law-info-bar", + "cookie-notice", + "cookie-banner", + "cookieconsent", + "cmpbox", + ) + + /** + * Phrases that only really occur in consent boilerplate. Individually weak -- + * an article can mention any one of them -- so [MIN_PHRASE_MATCHES] are + * required, and only within the length cap. + */ + private val CONSENT_PHRASES = listOf( + "uses cookies", + "use cookies", + "using cookies", + "tracking technologies", + "by continuing to use this", + "by continuing to browse", + "accept all cookies", + "reject all", + "manage your preferences", + "manage preferences", + "cookie preferences", + "cookie policy", + "cookie settings", + "we and our partners", + "our advertising and analytics", + "your privacy choices", + "consent to our use of cookies", + "personalise content and ads", + "personalize content and ads", + ) + + /** + * True when [html] is a consent/cookie notice rather than article content. + * + * Takes the stored HTML rather than Readability's `textContent` so the same + * check works on a value already cached in `articles.extractedContent`, where + * the text form was never kept. + */ + fun looksLikeConsentNotice(html: String?): Boolean { + if (html.isNullOrBlank()) return false + + // Length is judged on the text, not the markup: a banner wrapped in enough + // nested divs and inline styles clears 2KB of characters while still being + // three sentences of text. Checked first, so nothing long is ever rejected + // however much consent boilerplate it happens to quote. + val text = plainText(html) + val withinNoticeLength = text.isNotBlank() && text.length <= MAX_NOTICE_TEXT_LENGTH + + return withinNoticeLength && (hasCmpContainer(html) || readsAsConsentBoilerplate(text)) + } + + private fun hasCmpContainer(html: String): Boolean { + val markup = html.lowercase() + return CMP_CONTAINER_IDS.any { markup.contains("id=\"$it") || markup.contains("id='$it") } + } + + private fun readsAsConsentBoilerplate(text: String): Boolean { + val lowerText = text.lowercase() + return CONSENT_PHRASES.count { lowerText.contains(it) } >= MIN_PHRASE_MATCHES + } + + /** + * Crude tag strip -- enough to measure length and match phrases. Deliberately + * not a parser: this runs on every article open, and getting the character + * count roughly right is all the decision needs. + */ + private fun plainText(html: String): String = + html.replace(TAG_REGEX, " ").replace(WHITESPACE_REGEX, " ").trim() + + private val TAG_REGEX = Regex("<[^>]*>") + private val WHITESPACE_REGEX = Regex("\\s+") +} diff --git a/app/src/main/java/org/terst/feedreader/readability/EvalDecision.kt b/app/src/main/java/org/terst/feedreader/readability/EvalDecision.kt new file mode 100644 index 0000000..ed0033d --- /dev/null +++ b/app/src/main/java/org/terst/feedreader/readability/EvalDecision.kt @@ -0,0 +1,61 @@ +package org.terst.feedreader.readability + +/** What to do with one `evaluateJavascript` result. */ +sealed interface EvalDecision { + /** Nothing more to try: this is the extraction's outcome. */ + data class Done(val result: ExtractionResult) : EvalDecision + + /** The page probably was not ready; inject the script again. */ + data object Retry : EvalDecision +} + +/** + * Decides, for one script result, whether extraction succeeded, should be + * retried, or has failed and why. + * + * Lifted out of the WebView callback it used to live inside so it can be + * exercised without a device. The bug that prompted that: a result consisting + * only of the page's cookie-consent banner was non-blank and error-free, so it + * counted as a success, was cached to `articles.extractedContent`, and was never + * reconsidered. Reported live as "reader mode just extracts cookie disclaimer". + */ +internal fun decideEvalOutcome( + article: ExtractedArticle?, + rawResult: String, + attempt: Int, + maxAttempts: Int +): EvalDecision { + val readabilityError = article?.error?.takeIf { it.isNotBlank() } + // A banner is exactly what a half-loaded page produces -- the consent script + // runs early, the article body may not be in the DOM yet -- so this is not + // usable, which routes it into the same retry the extractor already does for + // an empty result rather than being accepted. + val isConsentNotice = article != null && + readabilityError == null && + ConsentNotice.looksLikeConsentNotice(article.content) + val usable = article != null && + readabilityError == null && + !article.content.isNullOrBlank() && + !isConsentNotice + + return when { + usable -> EvalDecision.Done(ExtractionResult.Success(article!!)) + + // Readability ran and objected: that verdict will not change. + readabilityError != null -> + EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.ReadabilityError(readabilityError))) + + attempt < maxAttempts -> EvalDecision.Retry + + // Every attempt came back with nothing but the consent notice, so this is + // what the page actually serves -- report it as its own failure rather + // than passing a cookie disclaimer off as the article. + isConsentNotice -> + EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.ConsentInterstitial)) + + article == null -> + EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.BadResult(rawResult))) + + else -> EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.NoContent)) + } +} diff --git a/app/src/main/java/org/terst/feedreader/readability/ExtractionFailure.kt b/app/src/main/java/org/terst/feedreader/readability/ExtractionFailure.kt index 532b109..63fbdb2 100644 --- a/app/src/main/java/org/terst/feedreader/readability/ExtractionFailure.kt +++ b/app/src/main/java/org/terst/feedreader/readability/ExtractionFailure.kt @@ -28,6 +28,12 @@ sealed interface ExtractionFailure { /** The JS result could not be decoded. */ data class BadResult(val raw: String) : ExtractionFailure + + /** + * Readability returned content, but it was the page's cookie/consent notice, + * not the article -- see [ConsentNotice]. + */ + data object ConsentInterstitial : ExtractionFailure } private const val MILLIS_PER_SECOND = 1_000 @@ -47,6 +53,7 @@ fun ExtractionFailure.userMessage(): String = when (this) { is ExtractionFailure.ReadabilityError -> "Couldn't parse the article ($message)" ExtractionFailure.NoContent -> "No article text found on the page" is ExtractionFailure.BadResult -> "Unexpected result from the page" + ExtractionFailure.ConsentInterstitial -> "Only the site's cookie notice came through, not the article" } /** @@ -61,4 +68,6 @@ fun ExtractionFailure.diagnostic(): String = when (this) { is ExtractionFailure.ReadabilityError -> "ReadabilityError $message" ExtractionFailure.NoContent -> "NoContent (Readability returned no usable body)" is ExtractionFailure.BadResult -> "BadResult raw=${raw.take(RAW_DIAGNOSTIC_CHARS)}" + ExtractionFailure.ConsentInterstitial -> + "ConsentInterstitial (extracted content was the page's cookie/consent notice)" } 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 5061444..21633b4 100644 --- a/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt +++ b/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt @@ -436,10 +436,9 @@ object ReaderExtractor { * Runs the extraction script, retrying while the result looks like * the page simply was not ready yet. * - * A Readability error is final -- retrying will not change what the - * parser thinks of the markup. A null result or empty content is - * not: both are what an injection against a half-loaded document - * produces, and the next attempt usually succeeds. + * The accept/retry/give-up decision itself lives in + * [decideEvalOutcome], which is testable without a WebView; this + * only decodes the payload and carries the decision out. */ private fun evaluate(view: WebView, script: String, attempt: Int) { if (finished) return @@ -451,24 +450,18 @@ object ReaderExtractor { val article = unwrapped?.let { runCatching { json.decodeFromString<ExtractedArticle>(it) }.getOrNull() } - val usable = article != null && - article.error.isNullOrBlank() && - !article.content.isNullOrBlank() - when { - usable -> finish(ExtractionResult.Success(article!!)) - - // Readability ran and objected: that verdict will not change. - article != null && !article.error.isNullOrBlank() -> - fail(ExtractionFailure.ReadabilityError(article.error)) - - attempt < MAX_EVAL_ATTEMPTS -> + when ( + val decision = decideEvalOutcome( + article = article, + rawResult = rawResult ?: "null", + attempt = attempt, + maxAttempts = MAX_EVAL_ATTEMPTS + ) + ) { + is EvalDecision.Done -> finish(decision.result) + EvalDecision.Retry -> view.postDelayed({ evaluate(view, script, attempt + 1) }, EVAL_RETRY_DELAY_MS) - - article == null -> - fail(ExtractionFailure.BadResult(rawResult ?: "null")) - - else -> fail(ExtractionFailure.NoContent) } } } diff --git a/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt b/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt index 4ed0722..72f6c1f 100644 --- a/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt +++ b/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt @@ -51,6 +51,7 @@ import org.terst.feedreader.data.DEFAULT_READER_FONT_SIZE_PX import org.terst.feedreader.data.Settings import org.terst.feedreader.data.ensureStandaloneArticle import org.terst.feedreader.data.saveArticleToStash +import org.terst.feedreader.data.loadCachedExtraction import org.terst.feedreader.data.urlToArticleId import org.terst.feedreader.readability.ReaderExtractor import java.net.URLEncoder @@ -95,9 +96,15 @@ fun LinkReaderScreen(url: String, onBack: () -> Unit, onOpenLink: (String) -> Un LaunchedEffect(url) { val existing = db.articleDao().getById(articleId) article = existing - if (existing?.extractedContent != null) { - extractedTitle = existing.title - extractedContent = existing.extractedContent + // A cached extraction can be the site's cookie-consent notice rather than the + // article (see CachedExtraction) -- drop it and extract again instead of showing + // a cookie disclaimer as the page. + val cached = loadCachedExtraction(existing?.extractedContent) { + db.articleDao().clearExtractedContent(articleId) + } + if (cached.content != null) { + extractedTitle = existing?.title + extractedContent = cached.content } else { val extracted = ReaderExtractor.extract(context, url) val content = extracted?.content diff --git a/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt b/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt index 6240b77..f08088b 100644 --- a/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt +++ b/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt @@ -68,6 +68,7 @@ import org.terst.feedreader.data.Settings import org.terst.feedreader.data.ensureStandaloneArticle import org.terst.feedreader.data.saveArticleToStash import org.terst.feedreader.data.setReaderModeEnabled +import org.terst.feedreader.data.loadCachedExtraction import org.terst.feedreader.readability.ReaderExtractor import java.net.URLEncoder @@ -219,15 +220,26 @@ private fun ReaderContent( feed = loadedFeed feedContent = loaded.feedContentHtml - readerContent = loaded.extractedContent + // A previously-cached extraction can be the site's cookie-consent notice rather + // than the article (reported live on a New Republic feature). That is dropped + // from the cache on read, which leaves nothing to show -- so re-extract, or the + // repair would just turn a wrong article into a blank one. + val cachedExtraction = loadCachedExtraction(loaded.extractedContent) { + db.articleDao().clearExtractedContent(articleId) + } + readerContent = cachedExtraction.content // Reader mode is NEVER heuristic/automatic (no more "content looks thin, switch // for you") -- it strictly follows the feed's persisted, explicitly-set opt-in. // Once a feed has it on, articles from it default to the extracted view and // auto-extract on open (that's the point of persisting it per-feed: no re-tapping - // per article), but nothing here ever turns it on by itself. + // per article), but nothing here ever turns it on by itself. Replacing a discarded + // consent notice does not turn it on either -- for a feed-less saved article there + // is no feed to opt in, and the reader falls back to the extracted content anyway. if (loadedFeed?.readerModeEnabled == true) { isReaderMode = true + } + if (loadedFeed?.readerModeEnabled == true || cachedExtraction.discardedConsentNotice) { if (readerContent == null) { isExtracting = true val result = ReaderExtractor.extractResult(context, loaded.link) @@ -509,6 +521,10 @@ private fun ReaderContent( appendLine("Session isReaderMode: $isReaderMode") appendLine("isSaved: ${a?.isSaved}, stashArtifactId: ${a?.stashArtifactId}") appendLine("extractedContent cached: ${readerContent != null}") + // Without this, a report about bad/absent reader content could + // not say whether extraction had failed, let alone how -- which + // is exactly what the cookie-disclaimer report ran into. + appendLine("Last extraction error: ${extractionError ?: "none"}") } val clipboard = context.getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager clipboard.setPrimaryClip(ClipData.newPlainText("Bug report", report)) diff --git a/app/src/test/java/org/terst/feedreader/data/CachedExtractionTest.kt b/app/src/test/java/org/terst/feedreader/data/CachedExtractionTest.kt new file mode 100644 index 0000000..72dc73f --- /dev/null +++ b/app/src/test/java/org/terst/feedreader/data/CachedExtractionTest.kt @@ -0,0 +1,67 @@ +package org.terst.feedreader.data + +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The reported bug came with "extractedContent cached: true", which is the half + * that a fixed extractor cannot reach: the cookie disclaimer was already written + * to `articles.extractedContent`, and every open after that read it straight back + * out without ever looking at it again. + * + * So a poisoned cache has to be recognised and dropped on read, not just kept out + * on write. + */ +class CachedExtractionTest { + + private val cookieNotice = + "<div id=\"readability-page-1\" class=\"page\"><div id=\"didomi-host\"><p>This website uses " + + "cookies and other tracking technologies (“Cookies”) to enhance user experience, to " + + "display targeted ads, and to analyze performance and user activity on our website. For " + + "more information about our use of Cookies and our privacy practices, see our Privacy " + + "Policy. By continuing to use this website, you agree to our Terms and Conditions.</p>" + + "</div></div>" + + @Test + fun `real cached content is used as-is and the cache is left alone`() = runTest { + var cleared = false + val content = "<p>" + "The actual article body. ".repeat(60) + "</p>" + val result = loadCachedExtraction(content) { cleared = true } + assertEquals(content, result.content) + assertFalse(result.discardedConsentNotice) + assertFalse("a good cache must not be cleared", cleared) + } + + @Test + fun `a cached consent notice is discarded and deleted`() = runTest { + var cleared = false + val result = loadCachedExtraction(cookieNotice) { cleared = true } + assertNull("the notice must not be shown as the article", result.content) + assertTrue(result.discardedConsentNotice) + assertTrue("the poisoned row must be cleared so a re-extract can replace it", cleared) + } + + @Test + fun `nothing cached touches nothing`() = runTest { + var cleared = false + val result = loadCachedExtraction(null) { cleared = true } + assertNull(result.content) + assertFalse(result.discardedConsentNotice) + assertFalse(cleared) + } + + // A blank value is as good as nothing cached -- it is not a consent notice, so + // reporting it as a discarded one would make the caller re-extract on a false + // premise. + @Test + fun `a blank cached value is not reported as a discarded notice`() = runTest { + var cleared = false + val result = loadCachedExtraction(" ") { cleared = true } + assertFalse(result.discardedConsentNotice) + assertFalse(cleared) + } +} diff --git a/app/src/test/java/org/terst/feedreader/readability/ConsentNoticeTest.kt b/app/src/test/java/org/terst/feedreader/readability/ConsentNoticeTest.kt new file mode 100644 index 0000000..7f4e9cc --- /dev/null +++ b/app/src/test/java/org/terst/feedreader/readability/ConsentNoticeTest.kt @@ -0,0 +1,140 @@ +package org.terst.feedreader.readability + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Reported live: reader mode on a New Republic feature article showed nothing but + * the site's cookie disclaimer, and cached it. + * + * Reproduced outside the app by running the extractor's own injected script + * (ReaderExtractor's `script`, Readability.js and all) against the real page: when + * the article body is present, extraction is perfect. When the body is NOT in the + * DOM at the moment Readability runs, Readability's own fallback pass kicks in -- + * its first pass strips "unlikely candidates" (which the consent banner is, its + * class says `didomi-notice-banner`), comes back under its 500-character + * threshold, and so it retries with that stripping disabled. The Didomi notice's + * single paragraph is 510 characters, i.e. just over that threshold, so it is + * accepted as the article. + * + * [EXTRACTED_COOKIE_NOTICE] below is not a hand-written approximation of that -- + * it is the verbatim `content` string that reproduction produced. + * + * Nothing on the Kotlin side looked at that: any non-blank `content` counted as a + * successful extraction, got written to `articles.extractedContent`, and was never + * re-checked. Hence a permanently cached cookie disclaimer. + */ +class ConsentNoticeTest { + + @Test + fun `the reported New Republic extraction is recognised as a consent notice`() { + assertTrue(ConsentNotice.looksLikeConsentNotice(EXTRACTED_COOKIE_NOTICE)) + } + + // Readability strips class attributes but keeps ids, so the CMP container's id + // survives into the extracted HTML and is the most precise signal available. + @Test + fun `other consent-management platforms are recognised by their container id`() { + val cmpIds = listOf( + "didomi-host", + "onetrust-banner-sdk", + "onetrust-consent-sdk", + "CybotCookiebotDialog", + "qc-cmp2-container", + "sp_message_container_1234", + "truste-consent-track", + "usercentrics-root", + "osano-cm-dialog", + "iubenda-cs-banner", + "optanon-alert-box-wrapper", + "cookie-law-info-bar", + ) + for (id in cmpIds) { + val html = """<div id="readability-page-1"><div id="$id"><p>Please review your choices.</p></div></div>""" + assertTrue("$id should be recognised as a consent container", ConsentNotice.looksLikeConsentNotice(html)) + } + } + + // Not every cookie banner is a known CMP. A short result whose text is + // unmistakably consent boilerplate counts too, with no id to go on. + @Test + fun `a short unbranded cookie banner is recognised from its wording`() { + val html = """ + <div id="readability-page-1"><div><p>We use cookies and similar tracking technologies + to personalise content and ads. By continuing to use this site you consent to our use + of cookies. You can manage your preferences at any time.</p></div></div> + """.trimIndent() + assertTrue(ConsentNotice.looksLikeConsentNotice(html)) + } + + // One stray phrase is not enough -- plenty of ordinary writing mentions cookies. + @Test + fun `a single incidental phrase is not enough`() { + val html = """<div id="readability-page-1"><div><p>The recipe calls for cookies. + We use cookies in the frosting too.</p></div></div>""" + assertFalse(ConsentNotice.looksLikeConsentNotice(html)) + } + + // The whole point is to not throw away real articles. A genuine article ABOUT + // cookie-consent law would hit every phrase there is; length is what separates + // it from a banner. Nothing long is ever rejected. + @Test + fun `a long article about cookie consent is never rejected`() { + val paragraph = "<p>We use cookies and other tracking technologies, and by continuing " + + "to use this website you consent to our use of cookies -- that is the boilerplate " + + "the GDPR produced, and this article is about where it came from. You can manage " + + "your preferences, in theory.</p>" + val html = "<div id=\"readability-page-1\"><div>${paragraph.repeat(12)}</div></div>" + assertTrue("fixture must be past the length cap", html.length > 2_000) + assertFalse(ConsentNotice.looksLikeConsentNotice(html)) + } + + // Short posts are normal (kottke-style linkblogging is called out in + // ReaderExtractor's own comments) and must survive. + @Test + fun `an ordinary short post is not a consent notice`() { + val html = """ + <div id="readability-page-1"><div><p>A lovely short note about a bridge in Lisbon, + with one link and nothing else. This is the entire post.</p></div></div> + """.trimIndent() + assertFalse(ConsentNotice.looksLikeConsentNotice(html)) + } + + @Test + fun `null and blank content are not consent notices`() { + assertFalse(ConsentNotice.looksLikeConsentNotice(null)) + assertFalse(ConsentNotice.looksLikeConsentNotice("")) + assertFalse(ConsentNotice.looksLikeConsentNotice(" \n ")) + } + + // Markup must not count towards the length budget: a banner wrapped in enough + // nested divs and inline styles to exceed the cap in raw characters is still a + // banner. + @Test + fun `length is measured on text not markup`() { + val padding = "<div style=\"${"border-color: rgb(0, 0, 0); ".repeat(80)}\">" + val html = padding + EXTRACTED_COOKIE_NOTICE + "</div>" + assertTrue("fixture must be past the length cap in raw characters", html.length > 2_000) + assertTrue(ConsentNotice.looksLikeConsentNotice(html)) + } + + companion object { + /** + * Verbatim output of running ReaderExtractor's injected script against the + * reported page in a state where the article body was absent. Do not tidy + * this up -- its value is that it is exactly what the app produced. + */ + const val EXTRACTED_COOKIE_NOTICE: String = + "<div id=\"readability-page-1\" class=\"page\"><div id=\"didomi-host\" data-testid=\"notice\" " + + "data-nosnippet=\"true\" lang=\"en\" data-lang-dir=\"ltr\"><p>This website uses cookies and " + + "other tracking technologies (“Cookies”) to enhance user experience, to display " + + "targeted ads, and to analyze performance and user activity on our website. We also may " + + "share information about your activity on our website with our advertising and analytics " + + "providers. For more information about our use of Cookies and our privacy practices, see " + + "our <a href=\"https://newrepublic.com/pages/privacy\" target=\"_blank\">Privacy Policy</a>. " + + "By continuing to use this website, you agree to our " + + "<a href=\"https://newrepublic.com/pages/terms-and-conditions\" target=\"_blank\">Terms and " + + "Conditions</a>, including our dispute resolution provisions.</p></div></div>" + } +} diff --git a/app/src/test/java/org/terst/feedreader/readability/EvalOutcomeTest.kt b/app/src/test/java/org/terst/feedreader/readability/EvalOutcomeTest.kt new file mode 100644 index 0000000..8995e22 --- /dev/null +++ b/app/src/test/java/org/terst/feedreader/readability/EvalOutcomeTest.kt @@ -0,0 +1,89 @@ +package org.terst.feedreader.readability + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The decision ReaderExtractor makes about each `evaluateJavascript` result: + * accept it, inject again because the page probably was not ready, or give up + * with a reason. It used to live inline in a WebView callback, which meant the + * only way to exercise it was on a device against a live page. + * + * The case that mattered and was never covered: a result that is perfectly + * well-formed and non-blank, and is the page's cookie-consent banner rather than + * the article. That counted as a success, got cached, and stayed cached. + */ +class EvalOutcomeTest { + + private val maxAttempts = 6 + + private fun decide(article: ExtractedArticle?, attempt: Int, raw: String = "\"{}\"") = + decideEvalOutcome(article, raw, attempt, maxAttempts) + + private fun article(content: String?, error: String? = null) = + ExtractedArticle(title = "t", content = content, textContent = content, error = error) + + @Test + fun `real content is accepted on the first attempt`() { + val a = article("<p>" + "The actual article body. ".repeat(60) + "</p>") + val decision = decide(a, attempt = 1) + assertEquals(EvalDecision.Done(ExtractionResult.Success(a)), decision) + } + + // Retrying cannot change Readability's opinion of the markup, so a thrown + // error is final however early it arrives. + @Test + fun `a readability error is final immediately`() { + val decision = decide(article(null, error = "TypeError: x is null"), attempt = 1) + assertEquals( + EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.ReadabilityError("TypeError: x is null"))), + decision + ) + } + + @Test + fun `an undecodable result retries and then reports a bad result`() { + assertEquals(EvalDecision.Retry, decide(null, attempt = 1, raw = "null")) + val last = decide(null, attempt = maxAttempts, raw = "null") + assertEquals(EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.BadResult("null"))), last) + } + + @Test + fun `blank content retries and then reports no content`() { + assertEquals(EvalDecision.Retry, decide(article(""), attempt = 1)) + assertEquals( + EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.NoContent)), + decide(article(""), attempt = maxAttempts) + ) + } + + // A cookie banner is exactly what a half-loaded page yields -- the banner is + // injected early, the article body may not be in the DOM yet -- so the right + // first move is the retry the extractor already does for an empty result. + @Test + fun `a consent notice retries rather than being accepted`() { + val decision = decide(article(ConsentNoticeTest.EXTRACTED_COOKIE_NOTICE), attempt = 1) + assertEquals(EvalDecision.Retry, decision) + } + + @Test + fun `a consent notice that survives every attempt is reported as a failure`() { + val decision = decide(article(ConsentNoticeTest.EXTRACTED_COOKIE_NOTICE), attempt = maxAttempts) + assertEquals( + EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.ConsentInterstitial)), + decision + ) + } + + // The failure has to be distinguishable from "no content": one means the page + // has no article, the other means the page would not show it without consent. + @Test + fun `the consent failure says what happened`() { + val f: ExtractionFailure = ExtractionFailure.ConsentInterstitial + assertTrue(f.userMessage().isNotBlank()) + assertTrue(f.diagnostic().isNotBlank()) + assertTrue(f.userMessage().lowercase().contains("cookie") || f.userMessage().lowercase().contains("consent")) + assertTrue(f.userMessage() != ExtractionFailure.NoContent.userMessage()) + } +} |