Reviewed: lmv_bbcode_v3_3_4.zip (17 files) — read-only review, no changes made Scope: LMVBBC.php, install/uninstall scripts (both branches), display patches, reference XML mirrors, package-info.xml, readme, language file, icons
.lmv-viewer-scroll in LMVBBC.php (wrapCard(), CSS block):
.lmv-viewer-scroll {
max-height: 480px !important;
overflow-y: auto !important;
}
@media (max-width: 768px) {
.lmv-viewer-scroll {
max-height: 80vh !important;
}
}.lmv-viewer-noscroll (the default when no scroll flag is given) is max-height: none — unbounded.Fix for a taller desktop default is a one-line change to 480px, or a new size-variant tag (scroll-lg, etc.) alongside the existing scroll/noscroll forms.
is_md never matches a real filename (autodisplay is non-functional)Every place this package sets the is_md context key uses:
'is_md' => !empty($attachment['filename']) && strtolower(substr($attachment['filename'], -4)) === '.md',substr($filename, -4) takes the last 4 characters; .md is 3 characters. A 4-character string can never === a 3-character string, so this is false for every realistic filename. Verified empirically:
| filename | substr(-4) |
matches .md? |
|---|---|---|
notes.md |
s.md |
no |
readme.md |
e.md |
no |
a.md |
a.md |
no |
x.md |
x.md |
no |
.md (no basename at all) |
.md |
yes |
It only matches a file literally named .md. Compare with LMVBBC.php's own (correct) check in renderAttachmentById():
if (strtolower(substr($row['filename'], -3)) !== '.md') // -3, correctImpact: Display.template.php's inserted block is gated on !empty($attachment['is_md']). Since that key is essentially always false, LMVBBCode::shouldAutoDisplay() is never reached, so [lmv=autodisplay] / [lmv=autodisplay-scroll] toggles record intent but autodisplay never actually renders, regardless of the toggle state.
Where it appears (all need the same fix, -4 → -3):
lmv_display_install_20.php (line 35)lmv_display_install_21.php (line 33)lmv_display_uninstall_20.php (line 17 — the removal needle; fixing install without fixing this needle would make future uninstalls stop matching the live file)lmv_display_uninstall_21.php (line 17 — same needle issue)lmv_display_patch.xml line 40, lmv_display_patch_21.xml line 32) — not executed by package-info.xml, but worth fixing too since your own header comments treat them as the canonical record to copy from if you ever revert to <modification>-style installs.Single find-and-replace across 6 files (-4 → -3 in that one exact string).
isSafeFetchUrl() only checks the URL scheme:
private static function isSafeFetchUrl($url)
{
if (!preg_match('/^([a-zA-Z][a-zA-Z0-9+.\-]*):/', $url, $m))
return false;
return in_array(strtolower($m[1]), array('http', 'https'), true);
}Any forum member who can post [lmv] (i.e. anyone with normal posting rights, unless the tag is disabled) can write [lmv]http://127.0.0.1:PORT/path[/lmv] or an internal LAN address, and fetchCapped() will have the server make that request and render up to 2 MB of the response. There's no blocklist for loopback/private/link-local ranges, and file_get_contents() follows redirects by default — so even adding a host blocklist wouldn't fully close it unless redirects are also disabled or re-validated per hop.
Practical risk: internal service/port probing and banner-grabbing against whatever else is reachable from the Live box — not remote file read (scheme is locked to http/https, so file:// is already blocked), but real network-reconnaissance surface. Very likely a pattern inherited from pdf-bbcode-mod's equivalent URL-fetch code (same architecture, deliberately mirrored) — not independently confirmed since that file wasn't in this package, but if [pdf] has the same one-line scheme check, it has the same exposure.
If closing it later: resolve the hostname, reject anything in the private/loopback/link-local ranges (and reject if it resolves to one after following a redirect, not just the original host), and set 'max_redirects' => 0 in the stream context (or resolve each hop yourself).
In wrapCard()'s inline <script>:
localStorage.setItem("ise_pdf_theme", themeName);
...
var savedTheme = localStorage.getItem("ise_pdf_theme");The LMV card's theme switcher reads/writes ise_pdf_theme — the PDF viewer's key, not an lmv-specific one. Almost certainly a copy-paste leftover from mirroring PDFBBC.php's architecture. Effect: choosing a theme in an [lmv] card also silently changes the saved theme for [pdf] cards and vice versa, on the same visitor's browser. Not a security issue, just cross-module coupling that's probably unintended given how deliberately separate these two mods have been kept everywhere else. Fix: rename to something like ise_lmv_theme.
resolveAttachmentPath()'s legacy (no-hash) candidate list includes:
$dir . '/' . $filename, // very old/imported: literal original filename$filename comes straight from the attachments.filename DB column with no basename() or traversal check before concatenation. Modern SMF uploads sanitize this on the way in, so it's low-likelihood in practice — but this path exists specifically for older/imported rows that may predate that sanitization, which is exactly the case where clean input can't be assumed. Worth a basename($filename) (or a real-path-stays-inside-$dir check) before use, as defense-in-depth rather than an active exploit today.
wrapCard() emits the full <style> block (~200 lines) and the theme-switcher <script> (including its self-invoking init) every time it's called — once per [lmv] tag rendered on a page. A topic page with several [lmv] cards (or several autodisplayed .md attachments once the is_md bug above is fixed) will repeat the same CSS and re-run the same theme-init IIFE once per card. Harmless functionally (last-write-wins on identical rules, redundant DOM queries), but real page bloat on any thread with more than one card.
"Optional scroll/noscroll flags default is scroll (unbounded height)"
Unbounded height is the noscroll behavior (confirmed in code: no flag → $scrollClass = '' → noscroll-equivalent, unbounded). The sentence names the wrong keyword for the behavior it correctly describes.
Per the standing convention (@version/@date doc-block on every module, plus a visible version banner in rendered output): LMVBBC.php fully complies (v3.3.4, 2026-09-02, visible banner added this version). lmv_check_collabcore.php also complies. But lmv_register.php, lmv_unregister.php, lmv_display_install_20/21.php, lmv_display_install_template.php, and the three uninstall scripts have only plain // comments — no @version/@date tags. This convention exists specifically because of confusion over which version of installer/no-visible-output code was actually deployed — these installer scripts are exactly the category it was written for.
$smcFunc['db_query'] with {int:...}/{string:...} placeholders — no string-concatenated SQL anywhere.$docTitle (which can come from an attacker-controlled filename or URL basename) is passed through htmlspecialchars(..., ENT_QUOTES, 'UTF-8') before output in the card header. Error messages go through the same escaping in renderError().MAX_BYTES (2 MB), preventing a runaway render on an oversized file.file://, data:, ftp:// etc. are all correctly rejected by isSafeFetchUrl() — only the host isn't restricted (see SSRF finding above).is_md key insertion and the Display.template.php block insertion correctly check for existing markers before writing, including self-healing against older/stopgap variants of the block — genuinely solves the double-insertion bug hit before on the PDF mod's <modification>-based patch.lmv_register.php, manual hook + pre-include registration) vs 2.1.x (native <hook> XML tags) split is internally consistent with each fork's actual API, and Subs.php is never touched on either branch, matching the file's own claim.<br>/entity transforms by hand instead of re-entering parse_bbc() — is implemented exactly as described and doesn't reintroduce the recursion it was fixing.This review produced findings only. No patched/rewritten package has been generated. If you want the fixes applied (starting with the is_md substr bug, since that's the one silently breaking a shipped feature), say so explicitly and specify scope — targeted fix to just the flagged lines, vs. anything broader.