From my testing, the rules server appears to serve the rules from any endpoint, not just /api/v1/py_rules.

yea, that's technically a bug but the documentation should be what people expect to work.

The Python module looks generally sane and I don't see anything obviously wrong.

From a defence in depth perspective, I do have a general unease around using eval on code received from a server. I understand this is coming from a trusted server, but it does make it easier for an attacker to escalate and move laterally in case he somehow manages to take control of the rules server.

Is there a particular reason why the rules need to be Python code instead of a small (e.g. JSON-based) DSL that is only able to encode the primitives that will reasonably be used for constructing ban expressions?

I agree with @dkasak that this knowingly leaves open a potential insecure vector. Note that the potentially generated rules (e.g. resp["checks"][...]["search"]) seems to be one of:

• event.get(..., '')
• user_profile[...]
• A direct variable (e.g. room_id or user_id).
• UserID.from_string(user_id).domain

Some potential alternatives:

• Have the spam checker call the server for each query and handle a simple yes/no response. 🤷 This would only work for Synapse v1.25.0 where spam checker methods can be async and might have serious performance implications.
• Since the generated expressions are generally accessing variables or dictionary keys, have the server return a dotted dictionary accessor (e.g. user_id or user_profile.user_id and do some magic with splitting on ., something like:

accessor = "user_profile.user_id"  # this would be returned by the server
value = {"user_profile": user_profile}
for key in accessor.split("."):
    value = getattr(value, key, "")
if re.search(check["pattern"], value):
    return True

Note that something special would need to be done to get the user's domain, but that could be handled fairly easily by doing something like the following for the initial value:

value = {
    "user_id": user_id,
    "user_domain": UserID.from_string(user_id).domain,
}

The accessor would then be user_id or user_domain.

In general this approach seems overly flexible for the rules that get generated in RuleServer.ts, but maybe there's a future plan I don't know about. Even my example above seems like it could be simplified quite a bit by knowing the data returned for each rule and what it gets applied against, which would also make the code a bit less abstract.

Note that if the dotted accessor approach is taken, the accessor keys should still be checked against a whitelist instead of allowing everything (which brings us back to a DSL). If a fully general accessor is allowed, it might be possible for an attacker to navigate the object to a secret string and then leak it by successively adapting a rule and checking whether it triggers.

Why even have these in the default config if they don't apply?

aside from the interface being easier to copy/paste and understand, mjolnir doesn't protect people from making mistakes almost by design - if someone really wanted to turn this switch on due to a massive attack of some sort, they could.

Exposing the clock as part of the module API would probably be reasonable, although that would quite widen the API surface we would have to consider stable.

Compiled code...that does what though? Are these lists of callables?

The ModuleApi object has an HTTP client available. It is not the proxied HTTP client, which should be fine though since rules_url is likely internal and doesn't need a proxy?

ah ha, this isn't documented :p

It would help if the inputs and outputs of these methods were documented a bit. It seems that rules is a dictionary with a two keys:

• search: Python code as a string. It seems to be expected to be an expression that has access to some locally defined variables, which differ depending on the check being done.
• pattern: A string regular expression.

A couple of potential improvements:

• Use an attrs class instead of a dictionary for the returned lists (this should use less memory and allow for more efficient access to properties).
• Pre-compile the regular expressions using re.compile and then call check["pattern"].search(search).

Passing in a class here seems a bit scary to me, it might allow an attacker to modify the behavior of that class throughout Synapse via monkeypatching. (I suppose this is kind of true if you allow access to anything that isn't a primitive though.)

I agree with @dkasak that this knowingly leaves open a potential insecure vector. Note that the potentially generated rules (e.g. resp["checks"][...]["search"]) seems to be one of:

• event.get(..., '')
• user_profile[...]
• A direct variable (e.g. room_id or user_id).
• UserID.from_string(user_id).domain

Some potential alternatives:

• Have the spam checker call the server for each query and handle a simple yes/no response. 🤷 This would only work for Synapse v1.25.0 where spam checker methods can be async and might have serious performance implications.
• Since the generated expressions are generally accessing variables or dictionary keys, have the server return a dotted dictionary accessor (e.g. user_id or user_profile.user_id and do some magic with splitting on ., something like:

accessor = "user_profile.user_id"  # this would be returned by the server
value = {"user_profile": user_profile}
for key in accessor.split("."):
    value = getattr(value, key, "")
if re.search(check["pattern"], value):
    return True

Note that something special would need to be done to get the user's domain, but that could be handled fairly easily by doing something like the following for the initial value:

value = {
    "user_id": user_id,
    "user_domain": UserID.from_string(user_id).domain,
}

The accessor would then be user_id or user_domain.

In general this approach seems overly flexible for the rules that get generated in RuleServer.ts, but maybe there's a future plan I don't know about. Even my example above seems like it could be simplified quite a bit by knowing the data returned for each rule and what it gets applied against, which would also make the code a bit less abstract.

Should we ensure that search is a string?

This can throw if the received code doesn't make sense with the input parameters. Right now this will raise an exception, is that the proper behavior?