diff options
Diffstat (limited to 'app/src/main/java')
7 files changed, 284 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)) |