Page MenuHomePhabricator

ScheduleDeploymentBot should escape wikitext in commit message ({{deploy}} |title= parameter)
Open, Needs TriagePublic

Description

Example: this change added

{{deploy|type=config|gerrit=1270388|title=[abstractwiki] Enable wgParserEnableUserLanguage, so we don't need {{int:lang}}s|status=}}

which displayed as

  • [config] 1270388 (Deploy change) [abstractwiki] Enable wgParserEnableUserLanguage, so we don't need ⧼lang⧽s

but the {{int:lang}} shouldn’t be interpreted as an actual parser function (workaround). The tool should probably wikitext-escape the commit message.

Event Timeline

https://gitlab.wikimedia.org/toolforge-repos/schedule-deployment/-/blob/801a3f492fb9159f845d3b7161104be14cb93a67/src/deployments/utils.py#L178:

def build_backport_item(form, change):
    gid = change["_number"]
    subject = template_arg_safe(form.description.data or change["subject"])
    ctype = get_change_type(change)
    bugs = get_bugs(change)
    return f"{{{{deploy|type={ctype}|gerrit={gid}|title={subject}|status=}}}} {bugs}"

The template_arg_safe helper function already tries to escape the title string, but I apparently thought that {{...}} should pass through when fixing T372750: schedule-deployment should escape `|` characters in values included in template parameters:

https://gitlab.wikimedia.org/toolforge-repos/schedule-deployment/-/blob/801a3f492fb9159f845d3b7161104be14cb93a67/src/deployments/utils.py#L161

def template_arg_safe(s):
    """Make the provided string safe for use as an arg to a wikitext
    template.
    """
    # Wrap lone `{` and `}` in `<nowiki>...</nowiki>`
    s = RE_CURLY_OPEN.sub(r"<nowiki>\1</nowiki>", s)
    s = RE_CURLY_CLOSE.sub(r"<nowiki>\1</nowiki>", s)

    # Replace all embedded `|` with the `{{!}}` magic word
    s = s.replace("|", "{{!}}")

    # Unescape `|` if it looks deliberate
    s = RE_RESTORE_TEMPLATES.sub(r"\g<keep>|", s)

    return s

@Lucas_Werkmeister_WMDE I think I made a narrow solution for T372750: schedule-deployment should escape `|` characters in values included in template parameters in part because I was reflecting on Stashbot and how it is intentionally used to pass wikitext syntax through !log messages into Wikitech. Thinking more about deliberate template use for Deployment planning, I'm not sure yet that I can come up with a reason that I would want the title of a commit to carry a wikitext payload for the purpose of modifying the rendered output.

Can you think of a reason that wrapping the whole input in <nowiki>...</nowiki> would be bad? I suppose someone could make a commit title like </nowiki>{{invoke:bd808 is a n00b}} or something that would defeat that protection, but I can probably come up with a workaround for that.

Thinking more about deliberate template use for Deployment planning, I'm not sure yet that I can come up with a reason that I would want the title of a commit to carry a wikitext payload for the purpose of modifying the rendered output.

Yeah, my feeling is also that we shouldn’t encourage any kind of wikitext in commit messages, since (unlike !log messages) they don’t just appear in the deployment calendar context.

Can you think of a reason that wrapping the whole input in <nowiki>...</nowiki> would be bad? I suppose someone could make a commit title like </nowiki>{{invoke:bd808 is a n00b}} or something that would defeat that protection, but I can probably come up with a workaround for that.

</nowiki> in the commit message is the only thing I was worried about, yes. But I suppose you could just check for that, and if it’s there, schedule the deployment with the title “commit message with wikitext-invalid subject” and tell the schedule-deployment user “you should fix the wikitech page manually” or something like that. (wfEscapeWikiText(), which doesn’t use <nowiki> “as required by the callers”, looks annoying to reimplement.)