diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 7adc50a..5135c69 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -35,6 +35,9 @@ jobs: with: python-version: '3.12' + - name: Install jsonschema + run: python -m pip install --quiet jsonschema + - name: Rebuild the index run: python3 scripts/build_index.py --output index.json diff --git a/.github/workflows/selftest.yml b/.github/workflows/selftest.yml new file mode 100644 index 0000000..6a23c1a --- /dev/null +++ b/.github/workflows/selftest.yml @@ -0,0 +1,45 @@ +# Check the analyzer against its own corpus. +# +# The source rules in scripts/analyze.py are the gate every submission passes +# through. A rule that quietly stops matching would not fail anything, every +# pull request would simply start passing, so the corpus in tests/ pins what +# each rule has to report and this runs it on every change to the scripts. + +name: Self test + +on: + push: + branches: [main] + paths: + - 'scripts/**' + - 'tests/**' + - 'rules/**' + - '.github/workflows/selftest.yml' + pull_request: + paths: + - 'scripts/**' + - 'tests/**' + - 'rules/**' + - '.github/workflows/selftest.yml' + +permissions: + contents: read + +jobs: + selftest: + name: Analyzer corpus + runs-on: ubuntu-latest + timeout-minutes: 5 + + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + + - name: Show the PHP version used to tokenize the corpus + run: php --version + + - name: Run the corpus + run: python3 scripts/selftest.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f0973d5..444b3bc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -5,29 +5,41 @@ fill it in, open a pull request. That is the whole thing. ```json { - "id": "hello-world", "name": "Hello World", - "description": "One line describing what the plugin does.", + "description": { + "en": "One line describing what the plugin does.", + "es": "Una linea describiendo lo que hace el plugin." + }, "author": "Your Name", "website": "https://github.com/your-user/hello-world", "license": "MIT", "compatible": "4.0", "version": "1.0.0", - "releaseDate": "2026-01-31", "download": "https://github.com/your-user/hello-world/releases/download/v1.0.0/hello-world.zip", "type": "", "tags": ["example"] } ``` +`type` and `tags` are optional, everything above them is required. `id`, +`sha256` and `size` are added by the workflow, do not write them yourself. + Three things trip people up: -- **The filename must match the `id`**, and the `id` is the directory Bludit - creates inside `bl-plugins`. +- **The filename is the id.** `plugins/hello-world.json` becomes + `bl-plugins/hello-world`, so use lowercase letters, digits and hyphens. - **`download` has to be a zip attached to a GitHub release.** Not `/archive/main.zip` — GitHub regenerates those, so the bytes change and the checksum recorded for your plugin would stop matching. -- **`version` has to be the same** as the one in your `metadata.json`. +- **`compatible` decides who is offered the plugin.** Bludit only lists a + plugin that names the `major.minor` the site is running, so `4.0` today. + +`description` takes one line per language, keyed by a Bludit language code. +English is required and is what a site falls back to. + +Nothing is read out of your zip. This file is the listing, so it is worth +getting right. The bot does compare the two and points out any difference for a +maintainer to look at, but it never rewrites what you wrote. A bot checks the pull request and comments with anything that needs fixing, pointing at the file and the line. Push a fix and the comment updates itself. @@ -51,23 +63,33 @@ and the zip is built and attached to the release. ## Releasing a new version -Open a pull request changing `version`, `releaseDate` and `download`. +Open a pull request changing `version` and `download`. The checksum recorded for a plugin is what guarantees the bytes people install are the bytes that were reviewed, so a new version is reviewed too. +## Selling a plugin + +Set `price_in_usd` and leave `download` out. Bludit cannot install an asset it +has to pay for, so a priced plugin is a listing only: it is hidden from the +plugin directory in the admin panel, it carries no checksum, and **its source +is never analyzed**. Sell and deliver it from your own website. + ## What the bot rejects **Blocks the merge:** a zip that cannot be downloaded or is not a valid plugin, entries with `..` or symbolic links, a missing `plugin.php`, `metadata.json` or -`languages/en.json`, a version that disagrees with `metadata.json`, PHP that does -not parse, no class extending `Plugin`, an id or class name already used by -Bludit, and `eval`, shell commands or hidden encoded code. +`languages/en.json`, a `metadata.json` without `version` or `compatible` +(Bludit refuses to install it), PHP that does not parse, no class extending `Plugin`, an id or class name already used by +Bludit, and `eval`, shell commands or hidden encoded code. Also blocked: +`include` or `unserialize` reaching `$_GET`, `$_POST`, `$_REQUEST` or `$_COOKIE`, +and calling a function whose name was assembled at runtime. **Flagged for a person to read, not blocked:** outbound HTTP requests, writing -files, dynamic calls, printing `$_GET` without `Sanitize::html()`. These are -legitimate for plenty of plugins — mention in the pull request why you need -them and it will go faster. +files, calling a closure held in a variable, including a path built from +constants, `unserialize` on your own data, printing `$_GET` without +`Sanitize::html()`. These are legitimate for plenty of plugins — mention in the +pull request why you need them and it will go faster. The checks read the parsed PHP, so the word `system` in a comment or a string is not a problem. diff --git a/rules/plugin.schema.json b/rules/plugin.schema.json index 109b969..39f5e5e 100644 --- a/rules/plugin.schema.json +++ b/rules/plugin.schema.json @@ -2,27 +2,31 @@ "$schema": "http://json-schema.org/draft-07/schema#", "$id": "https://raw.githubusercontent.com/bludit/plugins/main/rules/plugin.schema.json", "title": "Bludit plugin submission", - "description": "One file per plugin inside plugins/. The filename must be .json. Fields derived by CI (sha256, size) must not be present.", + "description": "One file per plugin inside plugins/. The filename is the id, it is the directory Bludit creates inside bl-plugins. Nothing here is read from the zip, this file is the record. Fields derived by CI (id, sha256, size) must not be present.", "type": "object", "additionalProperties": false, - "required": ["id", "name", "description", "author", "website", "license", "compatible", "version", "releaseDate", "download"], + "required": ["name", "description", "author", "website", "license", "compatible", "version"], "properties": { - "id": { - "type": "string", - "pattern": "^[a-z0-9][a-z0-9-]{1,48}$", - "description": "Directory name created inside bl-plugins. Must match the filename and the directory inside the zip." - }, "name": { "type": "string", "minLength": 2, - "maxLength": 60 + "maxLength": 60, + "description": "Shown in the plugin directory of the admin panel." }, "description": { - "type": "string", - "minLength": 10, - "maxLength": 200, - "pattern": "^[^\\n\\r]+$", - "description": "One line, no line breaks." + "type": "object", + "description": "One line per language, the key is a Bludit language code such as en, es or pt_BR. English is required and is the fallback when the admin panel runs in a language the plugin does not provide.", + "required": ["en"], + "additionalProperties": false, + "maxProperties": 40, + "patternProperties": { + "^[a-z]{2,3}(_[A-Z]{2})?$": { + "type": "string", + "minLength": 10, + "maxLength": 300, + "pattern": "^[^\\n\\r]+$" + } + } }, "author": { "type": "string", @@ -33,7 +37,8 @@ "type": "string", "format": "uri", "pattern": "^https://", - "maxLength": 300 + "maxLength": 300, + "description": "Where a user reads about the plugin. Linked from the author name in the admin panel." }, "license": { "type": "string", @@ -44,23 +49,25 @@ "compatible": { "type": "string", "pattern": "^[0-9]+\\.[0-9]+(,[0-9]+\\.[0-9]+)*$", - "description": "Comma separated list of major.minor Bludit versions, for example 4.0 or 4.0,4.1" + "description": "Comma separated list of major.minor Bludit versions, for example 4.0 or 4.0,4.1. Bludit only offers a plugin that names the version the site is running." }, "version": { "type": "string", "minLength": 1, "maxLength": 20, - "description": "Must be identical to the version inside the metadata.json of the plugin." - }, - "releaseDate": { - "type": "string", - "pattern": "^[0-9]{4}-[0-9]{2}-[0-9]{2}$" + "description": "The version this submission lists. Bump it in a new pull request to publish a new release." }, "download": { "type": "string", "format": "uri", "pattern": "^https://github\\.com/[^/]+/[^/]+/releases/download/[^/]+/[^/]+\\.zip$", - "description": "GitHub release asset. Archives generated by GitHub (/archive/*.zip) are not accepted, their bytes are not stable and the checksum would not hold." + "description": "GitHub release asset. Archives generated by GitHub (/archive/*.zip) are not accepted, their bytes are not stable and the checksum recorded for the plugin would stop matching. Required for a free plugin, and must be absent when price_in_usd is set." + }, + "price_in_usd": { + "type": "number", + "exclusiveMinimum": 0, + "maximum": 100000, + "description": "Leave it out for a free plugin. A priced plugin is a listing only: Bludit cannot install an asset it has to pay for, so it is hidden from the plugin directory in the admin panel and its source is never analyzed. Sell it from your website." }, "type": { "type": "string", diff --git a/scripts/analyze.py b/scripts/analyze.py index 44aa4d4..dc009c0 100755 --- a/scripts/analyze.py +++ b/scripts/analyze.py @@ -27,6 +27,16 @@ MAX_UNCOMPRESSED_BYTES = 40 * 1024 * 1024 # keep in sync with PLUGINS_MAX_UNCOMPRESSED_SIZE MAX_ASSET_BYTES = 200 * 1024 # single vendored asset, advisory only +# Nothing in index.json is ever read from the zip. The submission is the record +# a maintainer reviewed and merged, so a listing can never change because an +# author replaced an asset. The zip is still compared against it and a +# difference is reported for a person to judge, never used to overwrite a field. +COMPARED_FIELDS = ("author", "website", "license", "compatible", "version") + +# metadata.json without these two is refused by PluginInstaller, so a plugin +# missing them cannot be installed at all +METADATA_REQUIRED = ("version", "compatible") + ALLOWED_HOSTS = { "github.com", "objects.githubusercontent.com", @@ -70,6 +80,10 @@ SUPERGLOBALS = {"$_GET", "$_POST", "$_REQUEST", "$_COOKIE"} +# include and require are language constructs, not functions, so they never +# reach the T_STRING branch and need their own token names +INCLUDE_TOKENS = ("T_INCLUDE", "T_INCLUDE_ONCE", "T_REQUIRE", "T_REQUIRE_ONCE") + class Report: def __init__(self, plugin_id): @@ -127,8 +141,12 @@ def load_reserved(): def check_submission(path, report): - """Validate the JSON file itself. Returns the parsed submission or None.""" - filename_id = os.path.basename(path)[:-5] if path.endswith(".json") else None + """Validate the JSON file itself. Returns the parsed submission or None. + + The filename is the id. Carrying it inside the file as well would only + create a way for the two to disagree. + """ + plugin_id = os.path.basename(path)[:-5] if path.endswith(".json") else None try: with open(path) as fh: @@ -143,7 +161,7 @@ def check_submission(path, report): return None # Derived fields are computed by CI, a submission must not carry them - for field in ("sha256", "size", "checkedAt"): + for field in ("id", "sha256", "size", "checkedAt"): if field in data: report.error("FIELD_DERIVED", "The field `%s` is calculated by the workflow and must not be in the submission." % field, @@ -153,12 +171,27 @@ def check_submission(path, report): for msg, hint in schema_errors: report.error("SCHEMA", msg, hint, file=path) - plugin_id = data.get("id") - - if plugin_id and filename_id and plugin_id != filename_id: + if plugin_id and not re.match(r"^[a-z0-9][a-z0-9-]{1,48}$", plugin_id): report.error("ID_FILENAME", - "The id `%s` does not match the filename `%s.json`." % (plugin_id, filename_id), - "Rename the file to `plugins/%s.json`, or change the id." % plugin_id, file=path) + "`%s.json` is not a usable id." % plugin_id, + "The filename becomes the directory inside bl-plugins, so it has to be " + "lowercase letters, digits and hyphens.", file=path) + + # A priced plugin is a listing. Bludit cannot install an asset it has to pay + # for, so there is nothing to pin a checksum to and nothing to analyze. + priced = data.get("price_in_usd") is not None + if priced and data.get("download"): + report.error("PRICE_DOWNLOAD", + "A paid plugin cannot have a download link.", + "`price_in_usd` and `download` are exclusive. Bludit cannot pay for an " + "asset, so a paid plugin is listed and sold from your own website, never " + "installed from the admin panel. Remove `download` to sell it, or remove " + "`price_in_usd` to publish it for free.", file=path) + elif not priced and not data.get("download"): + report.error("DOWNLOAD_MISSING", + "The submission has no `download`.", + "A free plugin needs the zip attached to a GitHub release. Set " + "`price_in_usd` instead if the plugin is sold from your website.", file=path) reserved = load_reserved() if plugin_id in reserved["bundledPluginIds"]: @@ -166,13 +199,8 @@ def check_submission(path, report): "`%s` is a plugin bundled with Bludit." % plugin_id, "Bundled plugins are not listed in the directory. Choose another id.", file=path) - # Another submission already using this id - for other in sorted(os.listdir(os.path.join(ROOT, "plugins"))): - if not other.endswith(".json") or other == os.path.basename(path): - continue - if other[:-5] == plugin_id: - report.error("ID_DUPLICATE", - "The id `%s` is already used by `plugins/%s`." % (plugin_id, other), 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 if ok and not report.errors: report.ok("submission") @@ -214,6 +242,21 @@ def _validate_manual(data, schema): if spec is None: errors.append(("`%s` is not a known field." % field, "")) continue + if spec.get("type") == "number": + if isinstance(value, bool) or not isinstance(value, (int, float)): + errors.append(("`%s` must be a number." % field, _hint_for(field, schema))) + elif "exclusiveMinimum" in spec and value <= spec["exclusiveMinimum"]: + errors.append(("`%s` must be greater than %s." + % (field, spec["exclusiveMinimum"]), "")) + elif "maximum" in spec and value > spec["maximum"]: + errors.append(("`%s` must not be greater than %s." % (field, spec["maximum"]), "")) + continue + if spec.get("type") == "object": + errors.extend(_validate_object(field, value, spec)) + continue + if spec.get("type") == "array": + errors.extend(_validate_array(field, value, spec)) + continue if spec.get("type") == "string": if not isinstance(value, str): errors.append(("`%s` must be a string." % field, "")) @@ -230,6 +273,57 @@ def _validate_manual(data, schema): return not errors, errors +def _validate_array(field, value, spec): + """The arrays in the schema, today only tags.""" + if not isinstance(value, list): + return [("`%s` must be a list." % field, spec.get("description", ""))] + + errors = [] + if "maxItems" in spec and len(value) > spec["maxItems"]: + errors.append(("`%s` has more than %d entries." % (field, spec["maxItems"]), "")) + + rule = spec.get("items", {}) + for index, item in enumerate(value): + if rule.get("type") == "string" and not isinstance(item, str): + errors.append(("`%s[%d]` must be a string." % (field, index), "")) + continue + if rule.get("pattern") and not re.search(rule["pattern"], item): + errors.append(("`%s[%d]` does not have the expected format." % (field, index), + spec.get("description", ""))) + return errors + + +def _validate_object(field, value, spec): + """The keyed objects in the schema, today only description.""" + if not isinstance(value, dict): + return [("`%s` must be an object." % field, spec.get("description", ""))] + + errors = [] + for key in spec.get("required", []): + if key not in value: + errors.append(("`%s` has no `%s`." % (field, key), spec.get("description", ""))) + + patterns = spec.get("patternProperties", {}) + for key, text in value.items(): + rule = next((r for p, r in patterns.items() if re.match(p, key)), None) + if rule is None: + errors.append(("`%s.%s` is not a known key." % (field, key), + spec.get("description", ""))) + continue + if not isinstance(text, str): + errors.append(("`%s.%s` must be a string." % (field, key), "")) + continue + if "maxLength" in rule and len(text) > rule["maxLength"]: + errors.append(("`%s.%s` is longer than %d characters." + % (field, key, rule["maxLength"]), "")) + if "minLength" in rule and len(text) < rule["minLength"]: + errors.append(("`%s.%s` is shorter than %d characters." + % (field, key, rule["minLength"]), "")) + if rule.get("pattern") and not re.search(rule["pattern"], text): + errors.append(("`%s.%s` does not have the expected format." % (field, key), "")) + return errors + + def _hint_for(field, schema): spec = schema["properties"].get(field.split(".")[0], {}) return spec.get("description", "") @@ -352,7 +446,7 @@ def find_root(extract_to, report): # --------------------------------------------------------------------------- def check_structure(root, submission, report): - plugin_id = submission.get("id", "") + plugin_id = report.plugin_id junk = {".git", "node_modules", ".DS_Store", "__MACOSX", ".idea", ".vscode"} for current, directories, files in os.walk(root): @@ -375,22 +469,30 @@ def check_structure(root, submission, report): report.error("META_INVALID", "`metadata.json` is not valid JSON: %s" % exc, file="metadata.json") return - for field in ("version", "compatible"): + for field in METADATA_REQUIRED: if not metadata.get(field): - report.error("META_INCOMPLETE", "`metadata.json` has no `%s`." % field, file="metadata.json") - - if metadata.get("version") and metadata["version"] != submission.get("version"): - report.error("META_VERSION_MISMATCH", - "The submission says `%s`, `metadata.json` says `%s`." - % (submission.get("version"), metadata["version"]), - "The two must be identical, otherwise the directory advertises a version it does not ship.", - file="metadata.json") + report.error("META_INCOMPLETE", "`metadata.json` has no `%s`." % field, + "Bludit refuses to install a plugin without it.", file="metadata.json") - if metadata.get("compatible") and metadata["compatible"] != submission.get("compatible"): - report.error("META_COMPATIBLE_MISMATCH", - "The submission says `%s`, `metadata.json` says `%s`." - % (submission.get("compatible"), metadata["compatible"]), - file="metadata.json") + # 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. + for field in COMPARED_FIELDS: + if not metadata.get(field): + 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") + + # 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") language_path = os.path.join(root, "languages", "en.json") if not os.path.isfile(language_path): @@ -411,11 +513,22 @@ def check_structure(root, submission, report): "`languages/en.json` needs `plugin-data.name` and `plugin-data.description`.", 'For example: {"plugin-data":{"name":"Hello","description":"Says hello."}}', file="languages/en.json") + else: + english = submission.get("description") + english = english.get("en") if isinstance(english, dict) else None + for field, listed in (("name", submission.get("name")), + ("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 directory inside the zip should carry the plugin id if root != os.path.dirname(root) and os.path.basename(root) not in ("", plugin_id): - if os.path.basename(root) != os.path.basename(os.path.normpath(root)): - pass report.info("ZIP_DIRNAME", "The directory inside the zip is `%s`, the id is `%s`." % (os.path.basename(root), plugin_id), @@ -430,7 +543,8 @@ def check_structure(root, submission, report): "`%s` is %d KB." % (relative, os.path.getsize(path) // 1024), "Large vendored assets make every install slower.", file=relative) - if not any(f["code"].startswith(("META_", "LANG_")) for f in report.findings): + if not any(f["code"].startswith(("META_", "LANG_")) and f["severity"] == "error" + for f in report.findings): report.ok("structure") @@ -530,6 +644,8 @@ def scan_tokens(tokens, relative, reserved, report): items = significant(tokens) found_plugin_class = False decoders_seen = set() + assembled = _assembled_variables(items) + inside_backticks = False for index, token in enumerate(items): name, text, line = token["name"], token["text"], token["line"] @@ -544,11 +660,58 @@ def scan_tokens(tokens, relative, reserved, report): file=relative, line=line) continue + # A backtick expression is delimited by two identical tokens, so the + # closing one has to be swallowed or every shell call is reported twice if name == "T_SHELL_EXEC" or (name == "CHAR" and text == "`"): - report.error("SRC_SHELL", - "`%s` runs a shell command with backticks on line %d." % (relative, line), - "There is no legitimate use for this in a plugin listed in the directory.", - file=relative, line=line) + inside_backticks = not inside_backticks + if inside_backticks: + report.error("SRC_SHELL", + "`%s` runs a shell command with backticks on line %d." % (relative, line), + "There is no legitimate use for this in a plugin listed in the directory.", + file=relative, line=line) + continue + + # include and require reach a file path that Bludit will execute. A + # request controlled path is a remote code execution, anything else + # computed is worth a human reading it. + if name in INCLUDE_TOKENS: + window = _statement_window(items, index) + variables = [t for t in window if t["name"] == "T_VARIABLE"] + tainted = [t for t in variables if t["text"] in SUPERGLOBALS] + if tainted: + report.error("SRC_DYNAMIC_INCLUDE", + "`%s` includes a path taken from `%s` on line %d." + % (relative, tainted[0]["text"], line), + "Request data must never reach include or require, that is a remote " + "code execution. Include a fixed path instead.", + file=relative, line=line) + elif variables: + report.warning("SRC_DYNAMIC_INCLUDE", + "`%s` includes a computed path on line %d." % (relative, line), + "Fine when the path is built from constants such as `PATH_PLUGINS`. " + "Please say in the pull request what it loads.", + file=relative, line=line) + continue + + # Calling a variable bypasses every check that matches on a function + # name, so the name it was built from decides the severity + if name == "T_VARIABLE" and index + 1 < len(items) and items[index + 1]["text"] == "(": + previous = items[index - 1]["name"] if index else "" + if previous in ("T_OBJECT_OPERATOR", "T_DOUBLE_COLON", "T_FUNCTION", + "T_NULLSAFE_OBJECT_OPERATOR"): + continue + if text in assembled: + report.error("SRC_DYNAMIC_CALL", + "`%s` calls `%s()`, a function name assembled at runtime, on line %d." + % (relative, text, line), + "Building a function name from pieces hides which function is called " + "and defeats every other check here. Call it by its name.", + file=relative, line=line) + else: + report.warning("SRC_DYNAMIC_CALL", + "`%s` calls the variable `%s()` on line %d." % (relative, text, line), + "Fine for a closure. Please say in the pull request what it calls.", + file=relative, line=line) continue # --- class declarations --- @@ -582,6 +745,25 @@ def scan_tokens(tokens, relative, reserved, report): lowered = text.lower() + # unserialize on request data is object injection, on its own data it + # is ordinary + if lowered == "unserialize": + window = _statement_window(items, index) + tainted = [t for t in window + if t["name"] == "T_VARIABLE" and t["text"] in SUPERGLOBALS] + if tainted: + report.error("SRC_UNSERIALIZE", + "`%s` unserializes `%s` on line %d." % (relative, tainted[0]["text"], line), + "Unserializing request data lets a visitor build any object in Bludit. " + "Use `json_decode()` instead.", + file=relative, line=line) + else: + report.warning("SRC_UNSERIALIZE", + "`%s` calls `unserialize()` on line %d." % (relative, line), + "Safe only when the data is yours. Prefer `json_decode()`.", + file=relative, line=line) + continue + if lowered in BANNED_CALLS: report.error(BANNED_CALLS[lowered], "`%s` calls `%s()` on line %d." % (relative, text, line), @@ -615,6 +797,51 @@ def scan_tokens(tokens, relative, reserved, report): return found_plugin_class +def _statement_window(items, index, limit=24): + """The tokens of the expression starting after index, up to the statement end.""" + window = [] + depth = 0 + for offset in range(index + 1, min(index + limit, len(items))): + text = items[offset]["text"] + if text == "(": + depth += 1 + elif text == ")": + depth -= 1 + if depth < 0: + break + elif text == ";" and depth <= 0: + break + window.append(items[offset]) + return window + + +def _assembled_variables(items): + """Variables assigned from concatenated literals or from a decoder. + + This is the shape that defeats every name based check in this file: + + $f = 'ass' . 'ert'; + $f($_POST['x']); + + Nothing here ever calls a banned function by its name, so matching on the + call site alone would let it through. Knowing which variables were built + rather than written is what turns that back into an error. + """ + assembled = set() + for index, token in enumerate(items): + if token["name"] != "T_VARIABLE": + continue + if index + 1 >= len(items) or items[index + 1]["text"] != "=": + continue + window = _statement_window(items, index + 1) + strings = [t for t in window if t["name"] == "T_CONSTANT_ENCAPSED_STRING"] + concatenated = any(t["text"] == "." for t in window) and len(strings) > 1 + decoded = any(t["name"] == "T_STRING" and t["text"].lower() in DECODERS for t in window) + if concatenated or decoded: + assembled.add(token["text"]) + return assembled + + def _reads_remote(items, index): for offset in range(index + 1, min(index + 4, len(items))): if items[offset]["name"] == "T_CONSTANT_ENCAPSED_STRING": @@ -661,7 +888,16 @@ def main(): submission = check_submission(args.submission, report) - if submission and not args.skip_download and not report.errors: + priced = bool(submission) and submission.get("price_in_usd") is not None + if priced and not report.errors: + report.info("PRICE_LISTING", + "This is a paid listing, there is no asset to check.", + "Bludit hides it from the plugin directory in the admin panel because it " + "cannot install it. Nothing below was reviewed: no archive, no PHP, no " + "checksum. Read the plugin yourself before merging.", + file=os.path.basename(args.submission)) + + if submission and not priced and not args.skip_download and not report.errors: import tempfile with tempfile.TemporaryDirectory() as workdir: if args.local_zip: diff --git a/scripts/build_index.py b/scripts/build_index.py index 26412f5..7456a46 100755 --- a/scripts/build_index.py +++ b/scripts/build_index.py @@ -21,14 +21,18 @@ HERE = os.path.dirname(os.path.abspath(__file__)) ROOT = os.path.dirname(HERE) +sys.path.insert(0, HERE) + +from analyze import Report, check_submission # noqa: E402 SCHEMA_VERSION = 1 MAX_ZIP_BYTES = 10 * 1024 * 1024 -# The order of the keys in every entry of index.json +# The order of the keys in every entry of index.json. id, sha256 and size are +# added here, everything else is copied from the submission unchanged. FIELD_ORDER = [ "id", "name", "description", "author", "website", "license", - "compatible", "version", "releaseDate", "download", "type", "tags", + "compatible", "version", "download", "price_in_usd", "type", "tags", "sha256", "size", ] @@ -56,11 +60,26 @@ def build(fail_fast=False): for path in submissions(): name = os.path.basename(path) - try: - with open(path) as fh: - data = json.load(fh) - except json.JSONDecodeError as exc: - failures.append("%s: invalid JSON, %s" % (name, exc)) + # Validate again before publishing. The pull request is gated, but this + # is what index.json is actually generated from, so a file that reached + # main any other way must not be able to publish itself. + report = Report(name[:-5]) + data = check_submission(path, report) + if data is None or report.errors: + for finding in report.errors: + failures.append("%s: [%s] %s" % (name, finding["code"], finding["message"])) + continue + + # The filename is the id, the submission does not carry it + data["id"] = name[:-5] + + # A priced plugin is a listing. There is no public asset to fetch, so it + # carries no checksum and Bludit hides it from the admin panel. + if data.get("price_in_usd") is not None: + entry = {key: data[key] for key in FIELD_ORDER if key in data} + entries.append(entry) + print(" %-30s %s paid listing, no asset" + % (entry["id"], entry.get("version", "?")), file=sys.stderr) continue try: diff --git a/scripts/healthcheck.py b/scripts/healthcheck.py index ba138fa..4a37536 100755 --- a/scripts/healthcheck.py +++ b/scripts/healthcheck.py @@ -63,6 +63,10 @@ def main(): gone, mismatch = [], [] for entry in index.get("plugins", []): + # A paid listing has no asset of ours to verify + if entry.get("price_in_usd") is not None: + print(" %-30s paid listing, skipped" % entry["id"], file=sys.stderr) + continue status, detail = check(entry) print(" %-30s %s %s" % (entry["id"], status, detail), file=sys.stderr) if status == "gone": diff --git a/scripts/selftest.py b/scripts/selftest.py new file mode 100755 index 0000000..59a6dde --- /dev/null +++ b/scripts/selftest.py @@ -0,0 +1,213 @@ +#!/usr/bin/env python3 +"""Check the rules in analyze.py against fixed fixtures. + +Two suites, both driven by a file that records exactly what has to be reported: + + tests/corpus one PHP file per case, an attack that has to be caught or + a legitimate pattern that must not be flagged + tests/payload a submission next to the zip it claims to describe, so + the cross-check keeps refusing a submission that does not + match what the author actually uploaded + tests/submission one case per field of the submission itself, run with + jsonschema and with the fallback so the two agree + +A rule that stops firing, or starts firing on real code, fails here instead of +on somebody's pull request. + + python3 scripts/selftest.py + +Exit status is 0 when everything matches, 1 otherwise. +""" + +import copy +import json +import os +import shutil +import sys +from unittest import mock + +HERE = os.path.dirname(os.path.abspath(__file__)) +ROOT = os.path.dirname(HERE) +sys.path.insert(0, HERE) + +import analyze # noqa: E402 + +CORPUS = os.path.join(ROOT, "tests", "corpus") +EXPECTED = os.path.join(ROOT, "tests", "expected.json") +PAYLOAD = os.path.join(ROOT, "tests", "payload") +PAYLOAD_EXPECTED = os.path.join(ROOT, "tests", "payload-expected.json") +SUBMISSION_EXPECTED = os.path.join(ROOT, "tests", "submission-expected.json") + +# The guard is advisory and the corpus files do not carry it, it would only add +# the same noise to all of them +IGNORED = {"SRC_NO_GUARD"} + + +def findings(php, path, reserved): + report = analyze.Report("selftest") + tokens = analyze.tokenize(php, path) + if tokens is None: + raise SystemExit("unable to tokenize %s, is PHP installed?" % path) + analyze.scan_tokens(tokens, os.path.basename(path), reserved, report) + return sorted("%s/%s" % (f["severity"], f["code"]) + for f in report.findings if f["code"] not in IGNORED) + + +def payload_findings(case): + """Run the structure checks over one submission and its extracted zip.""" + directory = os.path.join(PAYLOAD, case) + with open(os.path.join(directory, "submission.json")) as fh: + submission = json.load(fh) + + roots = [n for n in sorted(os.listdir(directory)) + if os.path.isdir(os.path.join(directory, n))] + if len(roots) != 1: + raise SystemExit("%s must contain exactly one plugin directory" % case) + + # The id is the name of the submission file, which in a fixture is the name + # of the plugin directory sitting next to it + report = analyze.Report(roots[0]) + analyze.check_structure(os.path.join(directory, roots[0]), submission, report) + return sorted("%s/%s" % (f["severity"], f["code"]) for f in report.findings) + + +def compare(label, cases, load): + """Run every case and print one line each. Returns the failures.""" + failures = [] + for name in cases: + got, want = load(name) + if got == want: + print(" ok %-22s %s" % (name, ", ".join(got) or "no findings")) + else: + print(" FAIL %-22s" % name) + print(" expected: %s" % (", ".join(want) or "no findings")) + print(" got: %s" % (", ".join(got) or "no findings")) + failures.append(name) + return failures + + +def run_payload(): + expected = {k: v for k, v in json.load(open(PAYLOAD_EXPECTED)).items() + if not k.startswith("_")} + on_disk = {n for n in os.listdir(PAYLOAD) if os.path.isdir(os.path.join(PAYLOAD, n))} + + failures = [] + for name in sorted(on_disk | set(expected)): + if name not in expected: + failures.append("%s: in payload/ but not in payload-expected.json" % name) + print(" FAIL %-22s not in payload-expected.json" % name) + elif name not in on_disk: + failures.append("%s: in payload-expected.json but not in payload/" % name) + print(" FAIL %-22s not in payload/" % name) + + cases = sorted(on_disk & set(expected)) + failures += compare("payload", cases, + lambda n: (payload_findings(n), sorted(expected[n]))) + return failures, len(expected) + + +def submission_case(spec, base): + """Build one submission from the base and return the errors it produces.""" + data = copy.deepcopy(base) + for field in spec.get("remove", []): + data.pop(field, None) + data.update(spec.get("set", {})) + + directory = os.path.join(ROOT, "plugins") + name = spec.get("filename", "hello-world.json") + path = os.path.join(directory, name) + try: + with open(path, "w") as fh: + json.dump(data, fh) + report = analyze.Report(name[:-5]) + analyze.check_submission(path, report) + return sorted({f["code"] for f in report.errors}) + finally: + if os.path.exists(path): + os.remove(path) + + +def run_submission(): + spec = json.load(open(SUBMISSION_EXPECTED)) + base = spec["_base"] + cases = spec["cases"] + + failures = [] + for name in sorted(cases): + want = sorted(set(cases[name]["expect"])) + + got = submission_case(cases[name], base) + + # The same case again with jsonschema unavailable. An entry of None in + # sys.modules makes the import raise, which is the fallback path. + with mock.patch.dict(sys.modules, {"jsonschema": None}): + got_fallback = submission_case(cases[name], base) + + if got != want: + print(" FAIL %-26s expected: %s" % (name, ", ".join(want) or "accepted")) + print(" %-26s got: %s" % ("", ", ".join(got) or "accepted")) + failures.append(name) + elif got_fallback != want: + print(" FAIL %-26s jsonschema and the fallback disagree" % name) + print(" %-26s jsonschema: %s" % ("", ", ".join(got) or "accepted")) + print(" %-26s fallback: %s" % ("", ", ".join(got_fallback) or "accepted")) + failures.append(name) + else: + print(" ok %-26s %s" % (name, ", ".join(got) or "accepted")) + + return failures, len(cases) + + +def main(): + php = shutil.which("php") + if php is None: + print("PHP is not installed, the source checks cannot run.", file=sys.stderr) + return 1 + + reserved = json.load(open(os.path.join(ROOT, "rules", "reserved.json"))) + expected = {k: v for k, v in json.load(open(EXPECTED)).items() + if not k.startswith("_")} + + print("Source rules against the corpus") + on_disk = {n for n in os.listdir(CORPUS) if n.endswith(".php")} + failures = [] + + for name in sorted(on_disk | set(expected)): + if name not in expected: + failures.append("%s: in the corpus but not in expected.json" % name) + continue + if name not in on_disk: + failures.append("%s: in expected.json but not in the corpus" % name) + continue + + got = findings(php, os.path.join(CORPUS, name), reserved) + want = sorted(expected[name]) + if got == want: + print(" ok %-22s %s" % (name, ", ".join(got) or "no findings")) + else: + print(" FAIL %-22s" % name) + print(" expected: %s" % (", ".join(want) or "no findings")) + print(" got: %s" % (", ".join(got) or "no findings")) + failures.append(name) + + print("") + print("The submission file") + submission_failures, submission_total = run_submission() + + print("") + print("Submission against the uploaded zip") + payload_failures, payload_total = run_payload() + + print("") + if failures or payload_failures or submission_failures: + print("%d source, %d submission and %d payload case(s) failed." + % (len(failures), len(submission_failures), len(payload_failures)), + file=sys.stderr) + return 1 + print("All %d corpus files, %d submission cases and %d payload cases match." + % (len(expected), submission_total, payload_total)) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/templates/plugin.json b/templates/plugin.json index 081c6f1..9ba62e5 100644 --- a/templates/plugin.json +++ b/templates/plugin.json @@ -1,13 +1,14 @@ { - "id": "hello-world", "name": "Hello World", - "description": "One line describing what the plugin does, shown in the admin panel.", + "description": { + "en": "One line describing what the plugin does, shown in the admin panel.", + "es": "Una linea describiendo lo que hace el plugin." + }, "author": "Your Name", "website": "https://github.com/your-user/hello-world", "license": "MIT", "compatible": "4.0", "version": "1.0.0", - "releaseDate": "2026-01-31", "download": "https://github.com/your-user/hello-world/releases/download/v1.0.0/hello-world.zip", "type": "", "tags": ["example"] diff --git a/tests/corpus/01-dropper.php b/tests/corpus/01-dropper.php new file mode 100644 index 0000000..bf1bf36 --- /dev/null +++ b/tests/corpus/01-dropper.php @@ -0,0 +1,4 @@ +"; } +} diff --git a/tests/corpus/05-arbwrite.php b/tests/corpus/05-arbwrite.php new file mode 100644 index 0000000..e3ea294 --- /dev/null +++ b/tests/corpus/05-arbwrite.php @@ -0,0 +1,4 @@ +getValue('tag')).'">'; } +} diff --git a/tests/corpus/10-backtick.php b/tests/corpus/10-backtick.php new file mode 100644 index 0000000..8879089 --- /dev/null +++ b/tests/corpus/10-backtick.php @@ -0,0 +1,4 @@ +getValue('title')); + $data = unserialize($this->getValue('cache')); + } +} diff --git a/tests/corpus/12-hidden.php b/tests/corpus/12-hidden.php new file mode 100644 index 0000000..610910f --- /dev/null +++ b/tests/corpus/12-hidden.php @@ -0,0 +1,7 @@ +