Skip to content

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

Merged
rajarsheechatterjee merged 3 commits into
lnreader:masterfrom
RevDev909:fix/wetriedtls-review-round2
Sep 29, 2026
Merged

rajarsheechatterjee merged 3 commits into
lnreader:masterfrom
RevDev909:fix/wetriedtls-review-round2

Conversation

@RevDev909

Copy link
Copy Markdown
Contributor

Supersedes #2602, which was opened from a branch that predates the squash-merge of #2588 — that left both sides adding plugins/english/wetriedtls.ts, an unresolvable add/add conflict. This recreates the same change off current master, so the diff is exactly: master's version → fixed version.

Same content as the latest push to #2602, addressing the second-round review findings:

  • Sanitizer entity-decodes attribute values before the scheme check (catches encoded javascript: URLs, hex/named entities, whitespace smuggling; also blocks vbscript:).
  • parseChapterList throws on any malformed non-blank response — a bad page can no longer silently truncate the chapter list.
  • Block extraction keeps text-carrying unrecognized containers (lists, tables, divs, blockquotes).
  • Bold-only paragraphs are only stripped as title repeats when they match the novel/chapter title; translator/editor credit lines removed as site chrome.

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

@greptile-apps

greptile-apps Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 2/5

[Medium risk] Refines content filtering logic in a translation plugin.

The PR is not safe to merge until the SVG script-URL bypass and incomplete chapter-list validation are addressed.

Findings

  1. P1 Security SVG script links survive sanitization ▶
  2. P1 Malformed lists can appear complete ▶
  3. P2 Container markup gets split ▶
  4. P2 Standalone chapters retain title headers ▶
  5. P2 Chapter title cache grows indefinitely ▶

Summary

The PR tightens chapter-list response validation, changes chapter block and title handling, adds HTML sanitization, and serves covers directly. The sanitizer still permits a script-bearing SVG link, and malformed chapter-list entries can still be silently omitted. Chapter extraction and title caching also have narrower content and lifecycle issues.

Reviews (1) · Last reviewed commit: "fix(english): address review findings in..."

// Neutralize script URLs (href/src/action). The value is entity-decoded
// before the scheme check so encoded variants can't slip through.
out = out.replace(
/\s(href|src|action)\s*=\s*("[^"]*"|'[^']*'|[^\s"'=<>`]+)/gi,

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 SVG script links survive sanitization If chapter content contains an SVG link such as <svg><a xlink:href="javascript:...">click</a></svg>, the new block extraction retains it, but this URL check does not match xlink:href. The reader inserts the resulting HTML into the page, where clicking the link can execute script. How this was verified: The retained SVG attribute bypasses the URL check, and chapter HTML is inserted into the reader DOM without another sanitizer.

Comment on lines +315 to +316
if (!isRecord(root) || !Array.isArray(root.data)) {
throw new Error('Invalid chapter-list response (not the expected JSON)');

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 lists can appear complete This check accepts any data array, but the loop still silently skips entries missing a chapter slug or name. Invalid pagination metadata also leaves lastPage at 1. A damaged response can therefore omit chapters or stop pagination while parseNovel presents the result as a complete list, despite the new fail-loud handling.

Comment on lines +470 to +477
while ((bm = blockRe.exec(body)) !== null) {
const gap = body.slice(last, bm.index);
if (gap.replace(/<[^>]+>/g, '').trim()) blocks.push(gap.trim());
blocks.push(bm[0]);
last = bm.index + bm[0].length;
}
const tail = body.slice(last);
if (tail.replace(/<[^>]+>/g, '').trim()) blocks.push(tail.trim());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Container markup gets split The splitter drops gaps containing only tags but keeps gaps containing text. For <ul><li><p>one</p></li><li>two</li></ul>, it drops the opening list tags and emits the paragraph followed by <li>two</li></ul>. This breaks the structure of the newly retained content and can change how lists or tables display.

async parseChapter(chapterPath: string): Promise<string> {
const result = parseChapterContent(
await fetchText(SITE + '/series/' + chapterPath),
this.chapterTitles[chapterPath],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Standalone chapters retain title headers The chapter UI allows a chapter path to be fetched without first parsing its novel, but only parseNovel populates this title cache. On a direct fetch, title matching receives no titles and leaves the site's repeated bold header in the chapter content, where it was previously stripped.

// parseChapter passes them to parseChapterContent so the site's repeated
// title header can be told apart from a genuine bold-only content line
// (which must be kept).
private chapterTitles: Record<string, string[]> = {};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Chapter title cache grows indefinitely This plugin is a long-lived singleton, and parseNovel adds an entry here for every chapter without removing old entries. Browsing more novels retains all of those title arrays for the rest of the session, causing avoidable memory growth. Bound the cache or release entries that are no longer needed.

@rajarsheechatterjee
rajarsheechatterjee merged commit 9bc29e3 into lnreader:master Sep 29, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants