Note we had similar issues in the past (T289067 and subtasks). Can we do something to prevent them from reoccurring?

More generically: Anything coming from CommunityConfiguration is unsafe for direct inclusion in HTML. Can we add directives to CommunityConfiguration that would teach Phan that? The (simplified) pattern is this:

function loadConfiguration(): stdClass {
    $config = new stdClass;
    // ...
    return $config;
}

// returns HTML
function buildUI(): string {
    return Html::rawElement( 'div', [], loadConfiguration()->GEHomepageCommunityUpdatesContentBody );
}

The issue is present for the title and body. The link label is handled by LinkRenderer. Since only exploitable by beta admins I pushed a patch escaping the text before inclusion to gerrit.

In T373889#10113416, @Urbanecm_WMF wrote:

More generically: Anything coming from CommunityConfiguration is unsafe for direct inclusion in HTML. Can we add directives to CommunityConfiguration that would teach Phan that? The (simplified) pattern is this:

function loadConfiguration(): stdClass {
    $config = new stdClass;
    // ...
    return $config;
}

// returns HTML
function buildUI(): string {
    return Html::rawElement( 'div', [], loadConfiguration()->GEHomepageCommunityUpdatesContentBody );
}

We should be able to use what is described in https://github.com/wikimedia/mediawiki-tools-phan-SecurityCheckPlugin?tab=readme-ov-file#customizing to teach phan about tainted fields. Have not tried it though, and do find it a bit hard to understand at first glance, but maybe that's ok.

Change #1070278 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] SECURITY: Fix XSS vulnerabilities in community updates module

https://gerrit.wikimedia.org/r/1070278

In T373889#10113702, @Michael wrote:

We should be able to use what is described in https://github.com/wikimedia/mediawiki-tools-phan-SecurityCheckPlugin?tab=readme-ov-file#customizing to teach phan about tainted fields. Have not tried it though, and do find it a bit hard to understand at first glance, but maybe that's ok.

I tried that, but I can't figure out how. I tried https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CommunityConfiguration/+/1070287 (+equivalent change in GE), which adds @return-taint tainted to the loading methods, but that didn't seem to do the trick. Maybe this is because we proxy things through a StatusValue (which is not reflected in the simplification from above)? Maybe if we tainted StatusValue::getValue directly, things would work.

I think this is happening, because in reality, we proxy the data through a StatusValue, which I did not reflect in the simplification in my original comment. Maybe if we tainted StatusValue::getValue, things would work as expected, but I'm not sure if that's something we should do. On one hand, getValue can be literally anything. On the other hand, we can't guarantee anything is safe, so the declaration might very well be appropriate. On yet another hand, that anything can be eg. HtmlArmor, which is safe. I'm also unsure how much the stdClass object affects things.

Any thoughts on this?

Tagging @Daimona and @Bawolff for guidance re: phan-taint-check plugin questions above. Though I know they are not official maintainers at this point.

In T373889#10116126, @Urbanecm_WMF wrote:

I think this is happening, because in reality, we proxy the data through a StatusValue, which I did not reflect in the simplification in my original comment.

I haven't tested any of this, but it sounds very much plausible.

Maybe if we tainted StatusValue::getValue, things would work as expected, but I'm not sure if that's something we should do.

You're out of luck here, because StatusValue is a nightmare in terms of tracking taintedness. As you mentioned, the value property can be pretty much anything, and taint-check (and phan) cannot keep track of the taintedness (or, respectively, union type) of this property independently for each StatusValue instance. As far as phan is concerned, all StatusValue objects share the same value property. The immediate consequence of this is that we cannot mark it as tainted (or safe), because that would affect way too much code.

Your best bet might be to create a StatusValue subclass with a getConfiguration() (or similar method) that you can annotate with @return-taint tainted, and that would essentially just do return $this->value. This would also allow you add better type hints.

On a somewhat related note, I filed T368541: Provide better type inference for the value of StatusValue objects a while ago, but that's just for union types (not taintedness), and even then, it's not easy.

@sbassett Not from our end!