I think this can just be 200 like other {rule <args>} rules. If that isn't right, other rules need to be fixed anyway so it at least makes it easy to fix.

The priority stuff mostly only matters when rules could produce a different result when matched in a different order, but the {rule <args>} rules are pretty unambiguous and just need to run early enough that this:

{rule name=**STARS**}

...gets interpreted as a "call" instead of "some garbage, then some bold text, then some more garbage".

You should include the isFlatText() check from NavigationRemarkupRule, etc., here.

This check is a safety feature which protects us from re-evaluating text which has already been matched by another rule. Such text "should" never reach us, but if it does we can end up with security issues -- the text may contain replacement tokens, which we then put inside attributes and are later replaced by other rules with tags, so we end up with something like this:

<img src="<strong>some bold text</strong>" />

There's a short path from here to XSS.

The isFlatText() check makes sure that we haven't matched text containing tokens, avoiding this risk.

For consistency, use null as the non-value of a variable instead of empty string. This doesn't really cause any behavior differences in reasonable situations, it's just a convention thing.

A minor argument for it is that properties on objects are default-initialized to null, so default-initializing locals to null makes it less likely that refactoring (e.g., to move a local variable to an object property) will cause weird surprises.

If you typehint a method with a class like this:

public function doStuff(Triangle $t) { ... }

...you can also allow the method to accept null by adjusting the typehint:

public function doStuff(Triangle $t = null) { ... }

However, this method can never accept "" (empty string).

(These are pretty weak reasons to use null as the empty/unset value over "", but I don't think there are any real arguments in favor of "" over null.)

I'm guessing there's a non-hardcoded way to get this

This should be validated as PhabricatorEnv::isValidURIForLink(). Although we have lower-level protections against javascript: URIs, this defuses weird future attacks with, e.g., unusual protocols that aren't yet known to be vulnerable (tel://, fax://, execute-as-root://) and are not in the uri.allowed-protocols whitelist.

If the value is not valid, ignore it.

I don't think this is a real HTML/CSS property. In {Fxxx}, it gets turned into some CSS classes I think. Maybe just drop it for now, width and height should be fine as-is.

Ideally, we should share layout/width/height code with the {Fxxx} file code, but we can consolidate that later.

Should a utility function like this go in libphutil? Or should I just leave it here for now?

You could probably trim() this too to handle this:

{img                http://...}

(lots of extra spaces, or a newline)

Unfairly excludes many fine TLDs including .museum, .horse, and .technology.

I don't think we really need most of this. This regexp probably gets a lot of weird edge cases wrong too: beyond the TLD issue, this is a perfectly cromulent URI which it will fail to recognize:

https://secure.phabricator.com/w/☃/

I think the password is optional, the username/password may contain a wider range of characters than the character sets above include, ports 1-9 are technically valid (probably? I guess I've never seen one in use), this matches silly port 00000 which is technically fine, the path can be full of all kinds of stuff like the snowman above (it looks like multiple / are not permitted?) and so on.

I think we're no worse off doing a very simple check for https?://. If users type stuff like this:

{img http://domain+com/blah/blah.jpg}

..and get a broken image because there's a "+" in the URL instead of a ".", when they could have gotten their text verbatim instead with a more aggressive regexp, I think that's perfectly fine.

(Also, fine to put this function here.)

If you do want to match more aggressively, you can just use new PhutilURI($string) and see if it parsed anything as getDomain() as a rough sanity check.

You could probably just inline this regex now for a mild improvement in clarity.

Prefer \z over $ for "end of string" -- $ matches end of string and also "end-of-string, except for a newline". We had some obscure security issues with this in the past (e.g., permitting registration of email addresses with a terminal newline). That is, this regex:

/^[a-z]+$/

...matches the string "dog" but also the string "dog\n". With \z, only "dog" matches.

Writing this as '~^'.$regex.'\z~i' might make it more clear which parts are or aren't variables, although just merging the variable into the string is probably reasonable now.