asf-tooling commented on issue #991:
URL:
https://github.com/apache/tooling-trusted-releases/issues/991#issuecomment-4409925387
<!-- gofannon-issue-triage-bot v2 -->
**Automated triage** — analyzed at `main@2da7807a`
**Type:** `bug_fix` • **Classification:** `actionable` • **Confidence:**
`high`
**Application domain(s):** `announcement_publishing`, `voting`,
`shared_infrastructure`
### Summary
The issue correctly identifies a template variable injection vulnerability
in `atr/construct.py`. Sequential `str.replace()` calls mean that
user-controlled values substituted early (like `revision_tag` for `{{TAG}}`)
can contain patterns like `{{YOUR_FULL_NAME}}` that are expanded by later
replacements. This enables identity confusion attacks where a committer can
make an announcement email appear to be from whoever triggers it. The fix is to
switch to single-pass substitution using `re.sub` with a lookup dictionary.
### Where new code would go
- `atr/construct.py` — after TEMPLATE_VARIABLES definition, before
AnnounceReleaseOptions
A private helper function `_substitute_template` should be defined here as
it's used by all three affected functions in this module.
- `tests/unit/test_construct_announce.py` — after
test_announce_release_subject_and_body_uses_top_level_downloads_url
New test verifying that injected {{VAR}} patterns in revision tags are not
expanded.
### Proposed approach
Implement single-pass template substitution (Option 2 from the issue) using
`re.sub` with a callback. Define a private `_substitute_template(template: str,
variables: dict[str, str]) -> str` function that compiles a regex matching all
known variable names simultaneously and uses a lambda to replace each match
with its corresponding value from a dictionary. This eliminates the sequential
replacement vulnerability because all substitutions happen in one pass — values
inserted for early variables are never re-scanned for later variable patterns.
Replace all sequential `.replace()` blocks in
`announce_release_subject_and_body`, `start_vote_subject_and_body`, and
`checklist_body` with calls to this function, building the variable dictionary
from the same values currently used. Add unit tests confirming that revision
tags containing `{{YOUR_FULL_NAME}}` are rendered literally rather than
expanded.
### Suggested patches
#### `atr/construct.py`
Add single-pass _substitute_template helper and refactor all three affected
functions to use it.
````diff
--- a/atr/construct.py
+++ b/atr/construct.py
@@ -17,6 +17,7 @@
import dataclasses
import datetime
import hashlib
+import re
from typing import Literal
import aiofiles.os
@@ -31,6 +32,18 @@
type Context = Literal["announce", "announce_subject", "checklist", "vote",
"vote_subject"]
+
+def _substitute_template(template: str, variables: dict[str, str]) -> str:
+ """Perform single-pass template substitution to prevent variable
injection.
+
+ All {{VAR}} patterns are resolved simultaneously, so values from one
+ substitution cannot be interpreted as template variables.
+ """
+ if not variables:
+ return template
+ pattern = re.compile(r"\{\{(" + "|".join(re.escape(k) for k in
variables) + r")\}\}")
+ return pattern.sub(lambda m: variables[m.group(1)], template)
+
+
TEMPLATE_VARIABLES: list[tuple[str, str, set[Context]]] = [
("CHECKLIST_URL", "URL to the release checklist", {"vote"}),
("COMMITTEE", "Committee display name", {"announce", "checklist",
"vote", "vote_subject"}),
@@ -101,20 +114,22 @@
if options.download_path_suffix is not None:
download_url += f"/{options.download_path_suffix!s}"
download_url += "/"
- # Perform substitutions in the subject
- subject = subject.replace("{{PROJECT}}", project_display_name)
- subject = subject.replace("{{VERSION}}", str(options.version_key))
-
- # Perform substitutions in the body
- body = body.replace("{{COMMITTEE}}", committee.display_name)
- body = body.replace("{{DISCLAIMER}}",
_podling_disclaimer(release.project, committee))
- body = body.replace("{{DOWNLOAD_URL}}", download_url)
- body = body.replace("{{PROJECT}}", project_display_name)
- body = body.replace("{{REVISION}}", revision_number)
- body = body.replace("{{TAG}}", revision_tag)
- body = body.replace("{{VERSION}}", str(options.version_key))
- body = body.replace("{{YOUR_ASF_ID}}", options.asfuid)
- body = body.replace("{{YOUR_FULL_NAME}}", options.fullname)
+ # Single-pass substitution prevents variable injection
+ subject = _substitute_template(subject, {
+ "PROJECT": project_display_name,
+ "VERSION": str(options.version_key),
+ })
+
+ body = _substitute_template(body, {
+ "COMMITTEE": committee.display_name,
+ "DISCLAIMER": _podling_disclaimer(release.project, committee),
+ "DOWNLOAD_URL": download_url,
+ "PROJECT": project_display_name,
+ "REVISION": revision_number,
+ "TAG": revision_tag,
+ "VERSION": str(options.version_key),
+ "YOUR_ASF_ID": options.asfuid,
+ "YOUR_FULL_NAME": options.fullname,
+ })
return subject, body
````
#### `atr/construct.py`
Refactor checklist_body to use single-pass substitution.
````diff
--- a/atr/construct.py
+++ b/atr/construct.py
@@ -155,12 +155,13 @@
review_path = util.as_url(vote.selected, project_key=project.key,
version_key=version_key)
review_url = f"https://{host}{review_path}";
- markdown = markdown.replace("{{COMMITTEE}}", committee.display_name)
- markdown = markdown.replace("{{PROJECT}}", project.short_display_name)
- markdown = markdown.replace("{{REVIEW_URL}}", review_url)
- markdown = markdown.replace("{{REVISION}}", revision_number)
- markdown = markdown.replace("{{TAG}}", revision_tag)
- markdown = markdown.replace("{{VERSION}}", str(version_key))
+ markdown = _substitute_template(markdown, {
+ "COMMITTEE": committee.display_name,
+ "PROJECT": project.short_display_name,
+ "REVIEW_URL": review_url,
+ "REVISION": revision_number,
+ "TAG": revision_tag,
+ "VERSION": str(version_key),
+ })
return markdown
````
#### `atr/construct.py`
Refactor start_vote_subject_and_body to use single-pass substitution.
````diff
--- a/atr/construct.py
+++ b/atr/construct.py
@@ -220,25 +220,27 @@
revision=revision,
)
- # Perform substitutions in the subject
- subject = subject.replace("{{COMMITTEE}}", committee.display_name)
- subject = subject.replace("{{PROJECT}}", str(project_display_name))
- subject = subject.replace("{{REVISION}}", revision_number)
- subject = subject.replace("{{TAG}}", revision_tag)
- subject = subject.replace("{{VERSION}}", str(options.version_key))
- subject = subject.replace("{{VOTE_ENDS_UTC}}", vote_end_str)
-
- # Perform substitutions in the body
- # TODO: Handle the DURATION == 0 case
- body = body.replace("{{CHECKLIST_URL}}", checklist_url)
- body = body.replace("{{COMMITTEE}}", committee.display_name)
- body = body.replace("{{DURATION}}", str(options.vote_duration))
- body = body.replace("{{KEYS_FILE}}", keys_file or "(Sorry, the KEYS
file is missing!)")
- body = body.replace("{{PROJECT}}", str(project_display_name))
- body = body.replace("{{RELEASE_CHECKLIST}}", checklist_content)
- body = body.replace("{{REVIEW_URL}}", review_url)
- body = body.replace("{{REVISION}}", revision_number)
- body = body.replace("{{TAG}}", revision_tag)
- body = body.replace("{{VERSION}}", str(options.version_key))
- body = body.replace("{{YOUR_ASF_ID}}", options.asfuid)
- body = body.replace("{{YOUR_FULL_NAME}}", options.fullname)
+ # Single-pass substitution prevents variable injection
+ subject = _substitute_template(subject, {
+ "COMMITTEE": committee.display_name,
+ "PROJECT": str(project_display_name),
+ "REVISION": revision_number,
+ "TAG": revision_tag,
+ "VERSION": str(options.version_key),
+ "VOTE_ENDS_UTC": vote_end_str,
+ })
+
+ # TODO: Handle the DURATION == 0 case
+ body = _substitute_template(body, {
+ "CHECKLIST_URL": checklist_url,
+ "COMMITTEE": committee.display_name,
+ "DURATION": str(options.vote_duration),
+ "KEYS_FILE": keys_file or "(Sorry, the KEYS file is missing!)",
+ "PROJECT": str(project_display_name),
+ "RELEASE_CHECKLIST": checklist_content,
+ "REVIEW_URL": review_url,
+ "REVISION": revision_number,
+ "TAG": revision_tag,
+ "VERSION": str(options.version_key),
+ "YOUR_ASF_ID": options.asfuid,
+ "YOUR_FULL_NAME": options.fullname,
+ })
return subject, body
````
#### `tests/unit/test_construct_announce.py`
Add test proving that injected template variables in revision tag are not
expanded.
````diff
--- a/tests/unit/test_construct_announce.py
+++ b/tests/unit/test_construct_announce.py
@@ -103,6 +103,38 @@
assert body == "https://downloads.apache.org/downloads/myproject/";
[email protected]
+async def
test_announce_release_subject_and_body_does_not_expand_injected_variables(monkeypatch)
-> None:
+ """Test that {{VAR}} patterns in user-controlled values like revision
tag are not expanded."""
+ committee = SimpleNamespace(key="myproject", is_podling=False,
display_name="Apache MyProject")
+ project = SimpleNamespace(short_display_name="Apache MyProject",
name="MyProject", key="myproject")
+ release = SimpleNamespace(key="myproject-1.0.0", committee=committee,
project=project)
+ # Attacker sets the revision tag to a template variable
+ revision = SimpleNamespace(number="1", tag="{{YOUR_FULL_NAME}}")
+
+ monkeypatch.setattr(construct.config, "get", lambda:
SimpleNamespace(APP_HOST="downloads.apache.org"))
+ monkeypatch.setattr(construct.db, "session",
_mock_session_factory(MockDBSession(release, revision)))
+
+ subject, body = await construct.announce_release_subject_and_body(
+ "{{PROJECT}} {{VERSION}} {{TAG}}",
+ "Tag is: {{TAG}}\nName is: {{YOUR_FULL_NAME}}",
+ construct.AnnounceReleaseOptions(
+ asfuid="victim",
+ fullname="Victim User",
+ project_key=safe.ProjectKey("myproject"),
+ version_key=safe.VersionKey("1.0.0"),
+ revision_number=safe.RevisionNumber("1"),
+ ),
+ )
+
+ # The tag should appear literally, NOT be expanded to the announcer's
name
+ assert "{{YOUR_FULL_NAME}}" in body.split("\n")[0] # tag line keeps
literal pattern
+ assert "Victim User" in body.split("\n")[1] # actual YOUR_FULL_NAME
expansion is only here
+ # Subject should also keep the literal tag
+ assert "{{YOUR_FULL_NAME}}" in subject
+ # The attacker's injected variable must NOT resolve to "Victim User" in
the tag position
+ assert body.split("\n")[0] == "Tag is: {{YOUR_FULL_NAME}}"
+
+
def _mock_session_factory(data: MockDBSession):
@contextlib.asynccontextmanager
async def _session():
````
### Open questions
- Should the _substitute_template function also handle the DISCLAIMER
variable which can contain newlines and nested content from
_podling_disclaimer? (Currently it's safe since it's not user-controlled, but
worth verifying.)
- Should RELEASE_CHECKLIST be pre-sanitized before being passed as a
variable value to the outer template, since checklist_body output could
theoretically contain literal {{VAR}} patterns from user-authored checklist
markdown?
- Are there other entry points where revision_tag or other user-controlled
strings flow into template substitution beyond the three functions identified?
### Files examined
- `atr/construct.py`
- `migrations/versions/0033_2025.12.31_f2d97d96.py`
- `tests/unit/test_construct_announce.py`
- `atr/post/voting.py`
- `tests/unit/test_mail.py`
- `atr/models/args.py`
- `atr/post/announce.py`
- `atr/models/mail.py`
---
*Draft from a triage agent. A human reviewer should validate before merging
any change. The agent did not run tests or verify diffs apply.*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
