Skip to content

fix(english): address review findings in We Tried TLS plugin - #2602

Closed
RevDev909 wants to merge 8 commits into
lnreader:masterfrom
RevDev909:feat/wetriedtls-plugin
Closed

RevDev909 wants to merge 8 commits into
lnreader:masterfrom
RevDev909:feat/wetriedtls-plugin

Conversation

@RevDev909

Copy link
Copy Markdown
Contributor

Follow-up to #2588 (merged with the pre-fix version). Addresses the review-bot findings on plugins/english/wetriedtls.ts:

  • Chapter HTML is now sanitized before it reaches the reader: script/iframe/object-style elements are removed, event-handler attributes (e.g. onerror) are stripped, and javascript: URLs are neutralized. All other markup is preserved.
  • Image-only chapters (illustrations with no text) are now treated as real content instead of 'empty'.
  • A failed chapter-list page can no longer silently truncate the list: a blank response throws, so parseNovel fails loudly instead of returning an incomplete list as though it were complete.
  • Covers now use the site's direct CDN URL. A single URL field can't carry a fallback, so the images.weserv.nl proxy dependency is gone (in-chapter illustration shrinking is unchanged).

No test files added, per your note on #2575.

@greptile-apps

greptile-apps Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 0/5

[Medium risk] Adds a new novel-scraping plugin for a third-party site.

The PR does not appear safe to merge until the executable-link bypass and silent chapter-data loss paths are fixed.

Findings

  1. P1 Security Encoded script links survive ▶
  2. P1 Malformed pages truncate chapters ▶
  3. P1 Unmatched chapter content disappears ▶
  4. P1 Bold chapter lines disappear ▶

Summary

This PR adds the We Tried TLS plugin and its icon, with chapter extraction, catalog and status filtering, pagination, direct cover URLs, and chapter-HTML sanitization. The review found an encoded-URL sanitizer bypass and three ways chapter content or chapter lists can be silently lost.

Reviews (1) · Last reviewed commit: "Address review findings: sanitize chapte..."

Comment thread plugins/english/wetriedtls.ts Outdated
Comment on lines +183 to +184
const unquoted = val.replace(/^['"]|['"]$/g, '');
return /^\s*javascript:/i.test(unquoted) ? '' : _m;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 security Encoded script links survive If chapter HTML contains a link such as <a href="java&#115;cript:alert(1)">, this check leaves it intact because it tests the encoded text. The reader renders the returned HTML directly, so clicking the link can execute script. How this was verified: The URL check preserves the encoded attribute, and the reader inserts the returned chapter HTML with dangerouslySetInnerHTML.

Comment thread plugins/english/wetriedtls.ts Outdated
Comment on lines +252 to +255
const root = safeJson(jsonText);
const items: ChapterInfo[] = [];
let lastPage = 1;
if (isRecord(root)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Malformed pages truncate chapters If a later free-chapter request returns a nonblank error page or malformed JSON, it passes the blank-response check. This parser then returns no chapters and defaults lastPage to 1, so parseNovel stops fetching pages and presents an incomplete chapter list as though it were complete.

Comment thread plugins/english/wetriedtls.ts Outdated
Comment on lines +393 to +395
const blocks = body.match(
/<p[\s\S]*?<\/p>|<h[1-6][\s\S]*?<\/h[1-6]>|<figure[\s\S]*?<\/figure>|<img[^>]*>/gi,
) || [body];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Unmatched chapter content disappears If a chapter mixes paragraphs with a list, table, or text in another unsupported container, body.match() keeps only the recognized blocks. For example, <p>Introduction</p><ul><li>Important note</li></ul> loses the note, so the reader displays an incomplete chapter.

Comment thread plugins/english/wetriedtls.ts Outdated
.replace(/^<p[^>]*>/i, '')
.replace(/<\/p>$/i, '')
.trim();
return /^<strong>[\s\S]*<\/strong>$/.test(inner);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Bold chapter lines disappear If a chapter starts or ends with a genuine paragraph made entirely of bold text, this check treats it as a repeated title and removes it. That drops real content from the reader; if it was the only block, the chapter is reported empty.

@RevDev909

Copy link
Copy Markdown
Contributor Author

Addressed the new review findings:

  • The sanitizer now entity-decodes attribute values before the scheme check, so &#106;avascript:, &#x6A;…, javascript&colon;, and whitespace-smuggled variants are all neutralized (plus vbscript:).
  • parseChapterList now throws on any malformed non-blank response (error pages, wrong JSON shape), not just blank ones — a bad page can no longer silently truncate the chapter list.
  • Block extraction now keeps text-carrying containers it doesn't explicitly recognize (lists, tables, divs, blockquotes) instead of dropping them.
  • A bold-only paragraph is only treated as the site's repeated title header when it matches the novel/chapter title (recorded from the chapter list and passed into the chapter parser); genuine bold content lines are kept, and translator/editor credit lines are stripped as site chrome.
    No test files added, per your note on feat(english): add Nightjar Reads source plugin #2575.

@RevDev909

Copy link
Copy Markdown
Contributor Author

Superseded by #2604 — recreated off current master to resolve the add/add conflict (this branch predates the squash-merge of #2588). Same fixes, clean diff.

@RevDev909 RevDev909 closed this Sep 28, 2026
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Oct 1, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant