From 70f8495e631b282e3251f69eb603e490d122d06a Mon Sep 17 00:00:00 2001 From: dignajar Date: Tue, 22 Sep 2026 16:11:17 +0200 Subject: [PATCH] fix: make the analyzer comment readable A finding said what was wrong, where, and what to do in one run-on sentence, and a mismatch put both values inside it. Comparing two long descriptions meant reading a single line to the end twice. A finding now carries an optional detail, a list of label and value pairs, and the renderer puts each on its own line. The two texts sit under each other and the difference is visible. The source rules repeated the file and the line inside the message while the header already showed both, so the actual problem arrived at the end of a line nobody finishes. They say what is wrong and nothing else now. The advice itself was written for somebody who already knew the answer. SRC_ECHO_INPUT said to pass the value through Sanitize::html, it now says why a crafted link would otherwise run script on the site. META_MISMATCH said the submission is what the directory lists, it now names both files and says which one a visitor reads when. Repeated advice is printed once per section. Two fields disagreeing used to mean the same paragraph twice, which buried the fields. --- .github/scripts/analyze.py | 130 +++++++++++++++++++++++-------------- .github/scripts/render.py | 45 +++++++++---- 2 files changed, 113 insertions(+), 62 deletions(-) diff --git a/.github/scripts/analyze.py b/.github/scripts/analyze.py index 0da8a5c..58e7990 100755 --- a/.github/scripts/analyze.py +++ b/.github/scripts/analyze.py @@ -92,7 +92,13 @@ def __init__(self, plugin_id): self.findings = [] self.passed = [] - def add(self, severity, code, message, hint="", file="", line=0): + def add(self, severity, code, message, hint="", file="", line=0, detail=None): + """message is what is wrong, hint is what to do about it. + + detail is an optional list of (label, value) pairs, for a finding that + is about two values disagreeing. Putting them on their own lines is the + difference between a comparison somebody can read and one they cannot. + """ self.findings.append({ "severity": severity, "code": code, @@ -100,6 +106,7 @@ def add(self, severity, code, message, hint="", file="", line=0): "line": line, "message": message, "hint": hint, + "detail": [list(pair) for pair in (detail or [])], }) def ok(self, label): @@ -197,8 +204,9 @@ def check_submission(path, report): reserved = load_reserved() if plugin_id in reserved["bundledPluginIds"]: report.error("ID_BUNDLED", - "`%s` is a plugin bundled with Bludit." % plugin_id, - "Bundled plugins are not listed in the directory. Choose another id.", file=path) + "`%s` is the id of a plugin that ships with Bludit" % plugin_id, + "Every site already has it, so the directory does not list it, and installing " + "over it would replace part of Bludit. Rename the file to a free id.", file=path) # Two submissions cannot share an id any more, the id is the filename and # the filesystem keeps those unique, so there is nothing left to check here @@ -455,7 +463,7 @@ def check_structure(root, submission, report): if name in junk: relative = os.path.relpath(os.path.join(current, name), root) report.warning("ZIP_JUNK", - "`%s` should not be in the released zip." % relative, + "Development file, should not be in the released zip", "Exclude development files from the release asset.", file=relative) metadata_path = os.path.join(root, "metadata.json") @@ -472,8 +480,12 @@ def check_structure(root, submission, report): for field in METADATA_REQUIRED: if not metadata.get(field): - report.error("META_INCOMPLETE", "`metadata.json` has no `%s`." % field, - "Bludit refuses to install a plugin without it.", file="metadata.json") + report.error("META_INCOMPLETE", + "The plugin has no `%s` in its `metadata.json`" % field, + "Bludit reads that file to decide whether it can install the plugin, and " + "refuses when either `version` or `compatible` is missing. Add it and " + "publish a new release.", + file="%s/metadata.json" % plugin_id) # The submission is what gets listed either way. A difference is reported so # a maintainer can see it, it never changes what goes into index.json. @@ -482,18 +494,23 @@ def check_structure(root, submission, report): continue if metadata[field] != submission.get(field): report.warning("META_MISMATCH", - "`%s` differs: the submission says `%s`, `metadata.json` says `%s`." - % (field, submission.get(field), metadata[field]), - "The submission is what the directory lists. Say in the pull request " - "which one is right.", file="metadata.json") + "`%s` does not match the plugin" % field, + "The directory lists the first one. The second is what your plugin " + "actually ships, and it is what Bludit shows once the plugin is " + "installed. Make them the same, or say in this pull request which " + "one is right.", + detail=[("plugins/%s.json" % plugin_id, submission.get(field)), + ("%s/metadata.json" % plugin_id, metadata[field])]) # An absent type means a regular plugin, which the submission writes as "" if metadata.get("type", "") != submission.get("type", ""): report.warning("META_MISMATCH", - "`type` differs: the submission says `%s`, `metadata.json` says `%s`." - % (submission.get("type", ""), metadata.get("type", "")), - "Leave both empty for a regular plugin, or set the same value in both.", - file="metadata.json") + "`type` does not match the plugin", + "`editor` puts the plugin in the editor list, `theme` in the themes, and " + "empty is an ordinary plugin. Set the same value in both, or leave it out " + "of both.", + detail=[("plugins/%s.json" % plugin_id, submission.get("type", "") or "(empty)"), + ("%s/metadata.json" % plugin_id, metadata.get("type", "") or "(not set)")]) language_path = os.path.join(root, "languages", "en.json") if not os.path.isfile(language_path): @@ -521,12 +538,13 @@ def check_structure(root, submission, report): ("description", english)): if data[field] != listed: report.warning("LANG_MISMATCH", - "`%s` differs: the submission says `%s`, " - "`languages/en.json` says `%s`." - % (field, listed, data[field]), - "The directory lists the submission, Bludit shows this file " - "once the plugin is installed. A visitor would read two " - "different texts.", file="languages/en.json") + "The English %s does not match the plugin" % field, + "Somebody browsing the directory reads the first one, and " + "then sees the second once they install the plugin. Make " + "them the same, or say in this pull request which one is " + "right.", + detail=[("plugins/%s.json" % plugin_id, listed), + ("%s/languages/en.json" % plugin_id, data[field])]) # The directory inside the zip should carry the plugin id if root != os.path.dirname(root) and os.path.basename(root) not in ("", plugin_id): @@ -541,8 +559,12 @@ def check_structure(root, submission, report): if os.path.getsize(path) > MAX_ASSET_BYTES and name.endswith((".js", ".css")): relative = os.path.relpath(path, root) report.info("ASSET_LARGE", - "`%s` is %d KB." % (relative, os.path.getsize(path) // 1024), - "Large vendored assets make every install slower.", file=relative) + "%d KB, everything over %d KB gets flagged here" + % (os.path.getsize(path) // 1024, MAX_ASSET_BYTES // 1024), + "Not a problem, only something to know: every install downloads " + "it. Shipping a minified build, or dropping the parts of the " + "library the plugin does not use, is usually where the weight is.", + file=relative) if not any(f["code"].startswith(("META_", "LANG_")) and f["severity"] == "error" for f in report.findings): @@ -580,7 +602,7 @@ def check_source(root, report): if lint.returncode != 0: message = (lint.stdout + lint.stderr).strip().splitlines() detail = message[0] if message else "syntax error" - report.error("SRC_PARSE", "`%s` does not parse: %s" % (relative, re.sub(r' in /.*', '', detail)), + report.error("SRC_PARSE", "Does not parse: %s" % re.sub(r' in /.*', '', detail), "A plugin that does not parse takes down the whole site, not only the plugin.", file=relative) lint_failed = True @@ -593,9 +615,10 @@ def check_source(root, report): # directly. Worth suggesting, never worth blocking a merge. if not re.search(r"defined\s*\(\s*['\"]BLUDIT['\"]\s*\)", source[:400]): report.info("SRC_NO_GUARD", - "`%s` does not start with the Bludit guard." % relative, - "Optional. Adding ` 1: report.error("SRC_OBFUSCATION", - "`%s` combines %s." % (relative, " and ".join(sorted("`%s()`" % d for d in decoders_seen))), - "Chained decoding is the shape of hidden code. Ship readable source.", + "Combines %s in the same file" + % " and ".join(sorted("`%s()`" % d for d in decoders_seen)), + "One decoder can be ordinary, several chained together is how code is hidden " + "from a reader. Ship the source in a form a person can read.", file=relative) elif decoders_seen: report.warning("SRC_DECODER", - "`%s` uses %s." % (relative, ", ".join(sorted("`%s()`" % d for d in decoders_seen))), - "Fine for real data, suspicious when it hides code.", + "Uses %s" % ", ".join(sorted("`%s()`" % d for d in decoders_seen)), + "Ordinary when it decodes data, a problem when it hides code. Say in this " + "pull request what is being decoded.", file=relative) _scan_echoed_input(items, relative, report) @@ -862,9 +891,10 @@ def _scan_echoed_input(items, relative, report): window = " ".join(t["text"] for t in items[max(0, index - 2):offset]) if "Sanitize" not in window and "htmlspecialchars" not in window: report.warning("SRC_ECHO_INPUT", - "`%s` prints `%s` directly on line %d." - % (relative, items[offset]["text"], items[offset]["line"]), - "Pass it through `Sanitize::html()` first, otherwise it is an XSS.", + "Prints `%s` straight to the page" % items[offset]["text"], + "Anything a visitor puts in the URL ends up in the HTML as it " + "is, so a crafted link can run script on your site. Wrap it in " + "`Sanitize::html()`.", file=relative, line=items[offset]["line"]) break diff --git a/.github/scripts/render.py b/.github/scripts/render.py index a9b92f5..3eac364 100755 --- a/.github/scripts/render.py +++ b/.github/scripts/render.py @@ -59,19 +59,11 @@ def render(report): continue lines.append("### %s" % TITLES[severity]) lines.append("") + # The same rule firing twice would otherwise repeat its whole paragraph + # of advice, which buries the findings themselves + seen = set() for finding in group: - head = "**%s" % escape(finding["message"]) - if not head.endswith("**"): - head += "**" - where = location(finding) - if where: - head = "**%s** — %s" % (where.strip("`"), escape(finding["message"])) - head = head.replace("**%s**" % where.strip("`"), "**`%s`**" % where.strip("`"), 1) - lines.append("%s  `%s`" % (head, escape(finding["code"]))) - hint = escape(finding.get("hint")) - if hint: - lines.append(hint) - lines.append("") + lines.extend(block(finding, seen)) passed = report.get("passed") or [] if passed: @@ -88,6 +80,35 @@ def render(report): return "\n".join(lines) + "\n" +def block(finding, seen=None): + """One finding: what is wrong, the values it is about, what to do. + + Keeping those three apart is the whole point. A finding that reads as one + long sentence with two quoted paragraphs inside it cannot be scanned, and + the reader has to work out for themselves which half they are supposed to + change. + """ + where = location(finding) + message = escape(finding["message"]) + head = "**%s — %s**" % (where, message) if where else "**%s**" % message + + lines = ["%s  `%s`" % (head, escape(finding["code"])), ""] + + for label, value in finding.get("detail") or []: + lines.append("- `%s` — %s" % (escape(label), escape(value))) + if finding.get("detail"): + lines.append("") + + hint = escape(finding.get("hint")) + if hint and (seen is None or hint not in seen): + if seen is not None: + seen.add(hint) + lines.append(hint) + lines.append("") + + return lines + + def summary(errors, warnings, infos): if not errors and not warnings: return "Everything checks out. A maintainer will take it from here."