Repository navigation
fix(gui): neutralise the release tag in both prompts. Principle VIII. - #6407
Conversation
Two prompts showed the GitHub release tag_name as markup. The newer-version test does not filter it: VersionNumber::parse reads only the leading digits of each part, so `99.0.0<a href=...>x</a>` parses as 99.0.0 and counts as newer. - "Outdated Version" (TitleBar, before filing an issue): the body is rich text by design (<p>, <b>), so the tag is HTML-escaped. - "AetherSDR Update Available" (MainWindow, Help > Check for Updates): neither line carries markup, so the box is Qt::PlainText and shows the tag verbatim. Escaping is not enough there: under Qt::AutoText an escaped tag without "<" is not detected as rich text and would show its entities literally. Both texts are built in src/gui/ReleaseTagText.h, used by the two call sites and by release_tag_text_test. No change for a well-formed tag. VersionNumber.h gains the <QStringList> include it already needed; the new test is the first file to include it without a transitive one. VersionNumber::parse is unchanged. Fixes aethersdr#6405 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed at 9044579. I built and ran the new test locally, reproduced both of the mutations the body claims, and probed four attacks the test does not cover. The fix holds up under all of them. Details below so the clean verdict is checkable rather than asserted.
1. Issue fit
Yes, and wider than #6405 asked — correctly so. #6405 scoped only the "Outdated Version" prompt; the triage comment on it (aethersdr-agent, 2026-10-12 06:45) named the second sink (UpdateChecker → MainWindow.cpp "Update Available", reached by the ordinary Help → Check for Updates) and proposed exactly the two mechanisms this PR implements: latestTag.toHtmlEscaped() for the rich-text body, and box.setTextFormat(Qt::PlainText) for the box that carries no intentional markup. The triage also explicitly rejected the issue's alternative (latestVer.toString()) because normalisation would show a version that does not match the Releases page — and this PR's table gives that same reason. Every requirement in the issue and its thread maps to a hunk, and the two dispositions the triage ruled out of scope (WhatsNewDialog markdown body, FirmwareStager) are left alone and disclosed in the body.
2. Scope
Everything in the diff is explained by #6405 plus its triage comment.
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/gui/ReleaseTagText.h |
New header; the two prompt-text builders | Yes | In scope. Referenced by both call sites and the test — not a dead addition. |
src/gui/TitleBar.cpp:1449 |
Sink 1 now calls outdatedVersionPromptHtml |
Yes (#6405 body) | In scope. Text identical for a well-formed tag. |
src/gui/MainWindow.cpp:1430 |
Sink 2 now calls setUpdateAvailableText |
Yes (triage comment) | In scope. Named in the thread, and the PR title says "both prompts". |
src/core/VersionNumber.h:4 |
#include <QStringList> |
Yes | Required, not churn. I compiled the header standalone with that line deleted: error: implicit instantiation of undefined template 'QList<QString>' at s.split('.'). It only ever compiled transitively; the new test is the first TU to include it alone. |
tests/release_tag_text_test.cpp |
New socket-free test | Yes | In scope. |
tests/tests.cmake:7155-7164 |
Registers the target | Yes | In scope, and correctly not added to ci.yml — tools/check_ci_test_gate.py freezes that list and I confirmed it still reports ok: per-PR gate matches the frozen list (7 frozen / 762 registered). |
No new public surface, no protocol verb, no settings key, no changed default, no CHANGELOG.md entry, no removed guard. No commit on the branch is unrelated to the issue. Reading the - lines: the only deletions are the two inlined format strings being replaced by calls to the helpers that produce the same text.
Socket-test disclosure: none. tests/release_tag_text_test.cpp contains no QTcpServer/QTcpSocket/QUdpSocket/QLocalServer/QWebSocketServer, no bind/listen/connectToHost, no peer process and no Fake* device class. The tag is passed in as a literal; the network paths (UpdateChecker, TitleBar::showFeatureRequestDialog) are never entered. Nothing is removed either.
3. Blockers
None.
4. Nits (all non-blocking)
src/gui/ReleaseTagText.h:20—currentis not escaped whilelatestTagis:Harmless today: every caller passes.arg(current, latestTag.toHtmlEscaped());
QCoreApplication::applicationVersion(), set from the compile-timeAETHERSDR_VERSION(src/main.cpp:119,:422). I confirmed the asymmetry is live — passing<b>pwn</b>ascurrentrenders as markup — so it is one.toHtmlEscaped()away from being defence-in-depth rather than a latent trap for the next caller:.arg(current.toHtmlEscaped(), latestTag.toHtmlEscaped());
tests/release_tag_text_test.cpp:71— the test builds a stockQMessageBox; production passes aFramelessMessageBox(src/gui/MainWindow.cpp:1427). I readsrc/gui/FramelessMessageBox.{h,cpp}end to end: it only overlays a title bar and re-applies contents margins inshowEvent, and never touches the label ortextFormat, so the widget-class difference is benign here. Worth a line in the body's Limits list alongside the author's own "the test covers the text builders, not the two call sites".- The same Limits bullet is otherwise accurate and appreciated: nothing fails if a call site stops using the helpers. A source-text assertion is the in-tree idiom for pinning that (#6402 does it for
MainWindow, which links into no test) — optional, and arguably not worth it for a two-line diff.
5. CodeGuard dispositions
All 31 findings refuted; none intersects a diff hunk. The hunks are MainWindow.cpp 115-118 / 1426-1433, TitleBar.cpp 8-11 / 1446-1452, VersionNumber.h 1-4 — every flagged line is outside them, consistent with the scanner having walked the whole file at main's tip.
CS-PII-IP—VersionNumber.h:10is the comment// Strips non-numeric suffixes (e.g., "0.4.7a" → 0.4.7.0).A version string read as a dotted quad. Untouched text, shifted one line by the new include.MainWindow.cpp:6917is the WSJT-X multicast default224.0.0.1.CS-VLN-CMDI-BACKTICK(16 hits) — lexical matches on Markdown backticks inside//comments, e.g.MainWindow.cpp:538// Null when \id` is empty or not in `devices`.` No shell anywhere near them.CS-VLN-CMDI-AMP— the C++ reference/logical-AND token, e.g.TitleBar.cpp:728void TitleBar::setActiveRadio(const QString& id).CS-INJ-role_override—MainWindow.cpp:11753is the comment// CNN override: if carrierScore > 0.70, …; the word "override".CS-INJ-homoglyphat line 0 is em-dashes and arrows in comments.CS-VLN-PATH-PROC—QProcess::startDetachedwith the argument-list overload (no shell) relaunchingapplicationFilePath()witharguments().mid(1). Untouched region.
security-audit (CodeGuard) reached the same dispositions independently and also confirmed the positive security property: the escaped tag lands in element content rather than an attribute, so <>&" coverage suffices, and no anchor can form. That last point matters because QMessageBox's main label has openExternalLinks enabled — I measured openExternalLinks=1 — which is what made a crafted tag able to put a clickable arbitrary URL in a dialog titled "Outdated Version" before this change.
6. What I tried to break
- Built and ran the test (
clang++ -std=c++20, Homebrew Qt 6.11.2,QT_QPA_PLATFORM=offscreen): 16/16[ OK ],ALL PASS, exit 0. The author's run was Qt 6.12.0; the behaviour is the same at 6.11.2. - Attacked the test itself — reproduced both mutations on a scratch copy of the header. Dropping
.toHtmlEscaped()→ exit 1 with[FAIL] outdated: the tag's markup does not reach the bodyand[FAIL] outdated: the tag is shown escaped, the body printing the live<a href="http://example.invalid">. Droppingbox.setTextFormat(Qt::PlainText)→ exit 1 with[FAIL] update: the box is plain text (textFormat=2)and the same for the label. Four failures, matching the body's table line for line. This test would not pass against the unfixed code. %-placeholder re-expansion — a tag of9.9.9%1could in principle be re-substituted into the format string. It is not: the two-argumentQString::arg(a, b)substitutes in a single pass, and the output carries a literal%1. The multi-arg form is load-bearing here;.arg(current).arg(tag)would not be.- Attribute-context escaping —
toHtmlEscaped()leaves'alone, so I tried a tag of9.9.9' onx='y. The tag sits in element content inside<b>, so the quote cannot close an attribute. No injection. - Production call ordering — the real site calls
setIcon, thensetUpdateAvailableText, then twoaddButtons, andFramelessMessageBox::showEventwarns thatsetText/addButtonrebuild the grid layout. I replicated that exact sequence: the label staysQt::PlainTextafter bothaddButtoncalls and aftershow(). The format survives the rebuild. - Informative label — I measured it as
Qt::PlainTexttoo (fmt=0) aftersetUpdateAvailableText, so the format propagates. Moot either way:setInformativeTextcarries only the localapplicationVersion(). - Siblings left behind — all four other callers of
VersionNumber::parsechecked.UpdateChecker.cpp:59emitscurrent(not the tag) onupToDate, soMainWindow.cpp:1445's AutoText box never sees network text;WhatsNewDialog.cpp:482escapes its status label and:408its error text;RadioSetupDialog's firmware version comes through a digits-and-dots regex. "View Latest Release" opens the constantUpdateChecker::kReleasesPageUrl, not a tag-derived URL. No unfixed sink remains for the release tag. - Repo static checks —
check_test_registration.py --strict,gen_touchpoint_manifest.py --checkandcheck_ci_test_gate.py --strictall pass on the head checkout. CI is green on all five checks at9044579. - Could not check: the dialogs on a screen (headless), and the real network paths with a crafted release. Everything above is reasoned from the code plus a locally built test binary — not from observing the shipping app.
One thing outside this PR, surfaced by the audit so it is not lost: src/gui/WhatsNewDialog.cpp:277 hands QDesktopServices::openUrl a URL from the release-body markdown, and githubUrlForMaybeRelativeLink (:46-51) rewrites only relative URLs, so an absolute file:/smb: scheme would reach the OS handler on a user click. Pre-existing, maintainer-authored source, click-gated, and not part of this change — worth its own issue rather than a note here.
7. Recommendation
Approve with nits. The fix matches the triage's proposal on both sinks, the mechanism choice per prompt is justified in the code and pinned by a test that genuinely dies when either half is removed, the one extra line (<QStringList>) is provably required, and there is no behaviour change for a well-formed tag. The two nits are a one-word escape on current and a Limits-list line about the test's widget class; neither is worth a round trip on its own.
One thing for the maintainer rather than the author: #6405 carries maintainer-review, and this PR implements the agent triage's recommendation verbatim — including its rejection of the issue author's own latestVer.toString() alternative. Merging is implicitly ratifying that triage call. I agree with it (showing the tag as published is truthful; the normalised form could disagree with the Releases page the operator is about to open), but it is your call, not mine.
Thanks for picking up the second sink and for the mutation table — it made this reviewable in minutes rather than hours.
🤖 aethersdr-agent · cost: $13.6098 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (36 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $13.7772 · model: claude-opus-5 |
… VIII. Review of aethersdr#6407: outdatedVersionPromptHtml() escaped the tag and not `current`. Every caller passes the compile-time application version, so nothing rendered wrongly; escaping both leaves no trap for a later caller. One check pins it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review, and for building and breaking the test yourself. Fixed in
Not added: a source-text pin on the two call sites. I agree it is the in-tree idiom, and that it is arguably not worth it for a two-line diff; say if you want it. For the maintainer, as you put it: this takes the triage's choice of escaping the tag over showing The Full ctest at 73, ON8ST |
rfoust
left a comment
rfoust
left a comment
There was a problem hiding this comment.
#6407 fixes #6405 at the reported Outdated Version prompt and at the second release-tag sink in Update Available. The rich-text prompt escapes the tag; the plain-text prompt forces Qt::PlainText. Both preserve the published tag for display. I found no blockers.
| Changed files | Scope | Verdict |
|---|---|---|
ReleaseTagText.h, TitleBar.cpp, MainWindow.cpp |
Two release prompts | In scope |
VersionNumber.h |
Direct include needed by the new test | In scope |
release_tag_text_test.cpp, tests.cmake |
Socket-free regression coverage | In scope |
Verification: reviewed head 45d3637d79009cf82b37f3d9517cb4943942885b merged cleanly with current main 20122a8bc7e801532c89e12633654c14e25eb88b. Built the merged result on macOS ARM64; release_tag_text_test passed. Confirmed the build system and executable are ARM64 and RNNoise has no x86 sources. All five PR checks are green. In scratch, independently removing the tag escape, the plain-text setting, and the current-version escape made the regression test fail; restoring them made it pass. I also checked the sibling release-tag uses and the normal release display. An isolated TX-disabled demo run reached the ordinary update prompt; a crafted network release was not injected into the full app. The test exercises the text builders with a stock QMessageBox, while production uses FramelessMessageBox; source review found no text-format override there.
The scope and the test boundary are stated accurately in the PR. Approve.
2 participants
Fixes #6405
Limits
test but does not run it; it runs on the push to
main.in a test;
UpdateCheckerand the title bar's network path were not run.a call site stops using them.
QMessageBox; the app passes aFramelessMessageBox. That class adds a title bar and margins and does nottouch the label or its text format (read, not tested).
VersionNumber::parseis unchanged. It still accepts99.0.0<b>xas99.0.0; the test pins that, so the prompts do not lean on it. Tightening it
is a behaviour change (it also reads tags such as
0.4.7a) and is left fora separate decision.
src/core/VersionNumber.hgains#include <QStringList>. It callsQString::splitand compiled only through a transitive include; the newtest is the first file to include it without one.
What was wrong
At
bbded449, a GitHub releasetag_namereached two prompts as markup.src/gui/TitleBar.cpp:1442-1452, "Outdated Version" (shown before filingan issue): the tag, leading
vremoved, went through.arg(..., latest)into
<p>…<b>v%2</b>…</p>.src/core/UpdateChecker.cpp:47-58emits the tag onupdateAvailable;src/gui/MainWindow.cpp:1424-1429showed it withbox.setText(QString("AetherSDR v%1 is available.").arg(ver)). The boxnever set a text format, so
Qt::AutoTextrendered a tag that looks likemarkup. This is Help → Check for Updates.
The newer-version test in front of both is not a filter:
VersionNumber::parse(src/core/VersionNumber.h:14-27) reads only theleading digits of each part.
Change
Both texts are built in a new header,
src/gui/ReleaseTagText.h, used by thetwo call sites and by the test.
latestTag.toHtmlEscaped()<p>,<b>), so it cannot be plain. Escaping shows the tag as published;latestVer.toString()would show a normalised version that may not match the Releases page.box.setTextFormat(Qt::PlainText)Qt::AutoTextan escaped tag without<(for example99.0.0&x) is not detected as rich text and would show&literally. The test pins that case.No change for a well-formed tag such as
26.10.3.Other places the release feed reaches a widget, and what was done:
MainWindow.cpp,upToDateboxWhatsNewDialog.cpp, status label (name,tag_name)setStatusTextalready escapes.WhatsNewDialog.cpp, release body and the "no notes for " fallbackFirmwareStager.cpp→RadioSetupDialog.cppUpdateChecker::updateAvailablehas one consumer (MainWindow.cpp). There isno title-bar update indicator, tooltip or status-bar label that shows the tag.
Tests
release_tag_text_test(new, registered intests/tests.cmake; offscreen, nonetwork, no sockets), 17 checks, with the tag
99.0.0<a href="http://example.invalid">x</a>:99.0.0<b>xand the tag above;<ain the body, the tag is present escaped(
<a href="…), the body's own<p>and<b>are intact, and26.10.3is shown unchanged;Qt::PlainText, the tag is shown verbatim,26.10.3is shown unchanged.Control:
Qt::mightBeRichText()is true for that text, so the format iswhat protects it.
Mutations, each on a scratch copy of the header, test rebuilt:
.toHtmlEscaped()inoutdatedVersionPromptHtml[FAIL] outdated: the tag's markup does not reach the body,[FAIL] outdated: the tag is shown escapedbox.setTextFormat(Qt::PlainText)insetUpdateAvailableText[FAIL] update: the box is plain text(textFormat=2),[FAIL] update: the label that draws the text is plain text(textFormat=2)From this PR's review (
45d3637d): the running version is escaped in theOutdated Version body too, and one check pins it.
Full
ctestat45d3637d,-j8: 773 tests, 771 passed, 2 failed, 4 skipped(
nr_stereo_independence_{dfnr,bnr}_{24000,48000}_test).vkamp_connection_testfails, at-j8and rerun serially, with only oneof its two known checks, "commands after a disconnect are dropped, not
queued" (Replace retained socket fixtures with injected and opt-in coverage #5254).
peripheral_auth_dialog_interaction_testfails, at-j8and rerunserially, with
dialog 960x680, viewport 658, page minimum 784, horizontal scroll max 126. This is feat(peripherals): authenticate direct TGXL, PGXL, and Antenna Genius connections #6008: it fails the same way on cleanmainonthis Mac, and this change touches no dialog file.
unified_title_bar_testandrelease_tag_text_testpass.Static checks:
tools/check_*.py --strictpass (the two that need a build orpackage directory were not run),
check_a11y.pyon the touched GUI filesreports no findings,
gen_touchpoint_manifest.py --checkandtools/docs/gen_reference.py --checkare up to date.73, ON8ST
🤖 Generated with Claude Code