Repository navigation
fix(release-tools): treat proposal titles as untrusted in add-release-to-changelog.sh - #12083
pietrodimarco-dfinity wants to merge 1 commit into
Conversation
…-to-changelog.sh
add-release-to-changelog.sh derives a canister name and commit ID from an
NNS proposal title and uses them to pick which CHANGELOG.md to rewrite.
That title is free text, and it was handled as if it were trusted:
- get_{nns,sns}_canister_code_location looked the canister name up via
bash indirect expansion (${!name}). Bash evaluates array subscripts in
such names, including command substitutions. The lookup is now a case
statement over the known canister names, and unknown names fail.
- The "is the proposal's commit checked out" guard compared against an
unquoted glob, so an empty or wildcard commit passed. The commit ID must
now be 7-40 lowercase hex characters and is compared literally.
- get_proposal_info was fetched with a query, i.e. the answer of a single
replica. It is now fetched with --update, so the reply is certified.
- Title-derived values were printed via print_* (echo -e), which turns
escape sequences into terminal control characters. The canister name and
commit are validated before being printed, and malformed values (and an
unparseable title) are printed with printf %q.
Output for every known canister name is unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the release changelog tooling hardening changes.
No open issues 🎉
What changed in this PR
This PR hardens the release changelog tooling against untrusted proposal titles before using extracted values to select and rewrite changelog files.
Changes:
- Replaces indirect canister-name lookups with fixed allowlists.
- Validates canister names and commit IDs, compares commit prefixes literally, and safely prints malformed input.
- Fetches proposal information through an update call for a certified response.
| File | Description |
|---|---|
rs/nervous_system/tools/release/lib/functions.sh |
Uses explicit canister mappings and rejects unknown names. |
rs/nervous_system/tools/release/add-release-to-changelog.sh |
Adds input validation, safe diagnostics, literal commit matching, and certified proposal fetching. |
Review effort: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
✅ No security or compliance issues detected. Reviewed everything up to 6e0e604. Security OverviewDetected Code Changes
|
daniel-wong-dfinity-org-twin
left a comment
daniel-wong-dfinity-org-twin
left a comment
There was a problem hiding this comment.
Suggestion for title:
fix(release-tools): Make add-release-to-changelog.sh handle proposal title more robustly.
"untrusted" is confusing, because in general, we are reading in our own proposals, not random proposals.
| # | ||
| # --update is used (even though get_proposal_info is a query method) so that | ||
| # the reply is certified by the NNS subnet, rather than being the word of | ||
| # whichever single replica happens to answer a query. The title of the reply | ||
| # determines which files this script rewrites. |
There was a problem hiding this comment.
The last sentence seems unrelated to the rest of the paragraph.
Also, it is hardly telling us anything. It goes without saying that the response will somehow affect the outcome.
if you want to mention that we use the title, ok, but let's throw the reader more of a bone:
We glean the canister and commit from the proposal's title.
| # --update is used (even though get_proposal_info is a query method) so that | ||
| # the reply is certified by the NNS subnet, rather than being the word of | ||
| # whichever single replica happens to answer a query. The title of the reply |
There was a problem hiding this comment.
--update is good, but the first sentence of this paragraph seems superfluous. I think readers will almost certainly find it obvious that --update is more secure.
| # Map of locations. CANISTER_NAME may come from untrusted input (e.g. a | ||
| # proposal title), so it is only ever compared against this fixed list. | ||
| # Do NOT build a variable name from it and use indirect expansion | ||
| # (${!name}): bash evaluates array subscripts in such names, which runs | ||
| # any embedded $(...). |
There was a problem hiding this comment.
| # Map of locations. CANISTER_NAME may come from untrusted input (e.g. a | |
| # proposal title), so it is only ever compared against this fixed list. | |
| # Do NOT build a variable name from it and use indirect expansion | |
| # (${!name}): bash evaluates array subscripts in such names, which runs | |
| # any embedded $(...). | |
| # Where canister code lives. |
Why delete warning: It sort of references old code/bug. Comments should not do that, because it assumes the reader knows about the old buggy code, and they wouldn't know about it.
I mean, yes, "avoid !" is good general advice, but I don't think anyone would look at this case and think, "ah. Let me convert this case to code_location__${canister} (or similar)!". If people tend to make such changes, ok, let's keep this warning, but otherwise, the warning seems spammy.
| # Map of locations. See get_nns_canister_code_location for why this is a | ||
| # case statement rather than an indirect variable lookup. |
There was a problem hiding this comment.
Ditto.
| # Fail if what was extracted from the (untrusted) title is malformed. This | ||
| # must happen before either value is printed with print_* (see above for why); | ||
| # offending values are printed with printf %q instead. | ||
| # | ||
| # Proposal titles contain an abbreviated (7 character) commit ID, but a full | ||
| # one is also accepted. |
There was a problem hiding this comment.
| # Fail if what was extracted from the (untrusted) title is malformed. This | |
| # must happen before either value is printed with print_* (see above for why); | |
| # offending values are printed with printf %q instead. | |
| # | |
| # Proposal titles contain an abbreviated (7 character) commit ID, but a full | |
| # one is also accepted. | |
| # Check bits we gleaned from the title: | |
| # 1. canister | |
| # 2. commit |
| # untrusted, and print_red uses `echo -e`, which would turn escape | ||
| # sequences in it into terminal control characters. | ||
| print_red "💀 Unable to parse proposal title:" >&2 | ||
| printf '%q\n' "${TITLE}" >&2 |
There was a problem hiding this comment.
Seems to me, the right way is like this:
# Replaces "strange" characters with question marks.
#
# All printable characters are ok. Ditto for newline and tab.
# (Everything else is "strange".)
#
# This should be used when printing external content to the terminal.
# Otherwise, the output might be misleading.
censor_strange_characters() {
printf '%s' "${*//[^[:print:]$'\n'$'\t']/?}"
}
print_red() {
printf '%b%s%b\n' "${RED_TEXT}" "$(censor_strange_characters "$@")" "${NO_COLOR}" 1>&2
}
Ditto for other colors, ofc.
| # one is also accepted. | ||
| if [[ ! "${CANISTER_NAME}" =~ ^[a-z0-9_-]+$ ]]; then | ||
| print_red "💀 Malformed canister name in proposal title:" >&2 | ||
| printf '%q\n' "${CANISTER_NAME}" >&2 |
There was a problem hiding this comment.
My suggestion involving censor_strange_characters could be applied here.
| print_red "💀 Malformed commit ID in proposal title:" >&2 | ||
| printf '%q\n' "${DESTINATION_COMMIT_ID}" >&2 |
There was a problem hiding this comment.
Ditto.
| # Fail if the proposal's commit is not checked out. The commit ID is quoted so | ||
| # that it is compared literally (as a prefix), not as a glob pattern. |
There was a problem hiding this comment.
Fail if the proposal's commit is not checked out.
Ok, but why? Ofc, this issue existed before, so I will not block on this issue. I'm just bringing this up since you are touching this anyway.
| # Fail if the proposal's commit is not checked out. The commit ID is quoted so | ||
| # that it is compared literally (as a prefix), not as a glob pattern. |
There was a problem hiding this comment.
The commit ID is quoted so that it is compared literally (as a prefix), not as a glob pattern.
Again, this seems to be referencing old code/bugs. Do not mention such things. To the vast majority of readers, such comments are very noisy.
rs/nervous_system/tools/release/add-release-to-changelog.sh(run by release engineers perchangelog_process.md, and byrelease-runscriptfor each proposal ID) derives a canister name and a commit ID from an NNS proposal's title. It uses them to pick whichCHANGELOG.md/unreleased_changelog.mdto rewrite. The title is free text, but the script handled it as if it were trusted. This PR fixes that.Changes
lib/functions.sh).get_nns_canister_code_locationandget_sns_canister_code_locationused to build a variable name from the canister name and read it with${!name}. Bash evaluates array subscripts in such names, including command substitutions. Both functions now use acaseover the known canister names and return an error for any other name. Output for every known name (hyphenated or underscored) is byte-identical to before. Unknown names already failed before, with "unbound variable" underset -u.add-release-to-changelog.sh). The commit ID from the title must be 7–40 lowercase hex characters. It is compared withHEADas a quoted literal prefix. Before, the pattern was unquoted, so an empty or wildcard commit passed the guard.get_proposal_infois now called withdfx canister call --update. The reply is then certified by the NNS subnet instead of being one replica's query answer. NNS governance has noinspect_message, andget_proposal_infois a plain#[query], so it works as an update call. Checked against mainnet: same JSON as the query.print_*helpers useecho -e, which turns escape sequences into control characters. The canister name (^[a-z0-9_-]+$) and the commit are now validated before either is printed. Malformed values and an unparseable title are printed withprintf %q.Testing
These scripts have no Bazel targets. I tested in a throwaway git repo with a stub
dfxthat returns a given proposal title, plus the realjq/idl2json:$(...)in the canister name (NNS and SNS)\e) in the name, the commit, or an unparseable title. No stray control characters reach the output.Cycles-minting) and SNS titles. They produce the same changelog output as before.bash -npasses on bash 5.2 and 3.2 (the macOS default).//pre-commit:shfmt-checkpasses. shellcheck reports no new warnings.🤖 Generated with Claude Code