fix(net): read the zone id on a link-local address - #1005
Closed
simonoppowa wants to merge 1 commit into
Closed
Conversation
A link-local address needs a zone to be routable on a host with more than one interface, and a URI spells it escaped: `Uri` stores `http://[fe80::1%wlan0]` with a host of `fe80::1%25wlan0`. Typing the bare `%` produces the same thing, so this is not a corner you have to know RFC 6874 to reach. `InternetAddress.tryParse` answers null to that escaped form, so the address fell through to the DNS branch, where the lookup cannot succeed. The caller was told the server is unreachable — about a link-local server this check exists to permit, and which an unguarded client reaches fine. That is the wrong half of the unreachable / insecureDestination split, and it sends someone to fix the wrong thing. `dart:io` performs this same substitution in `escapeLinkLocalAddress` before it resolves or connects, so doing it here is what makes the check agree with the socket layer it guards. A host that still fails to parse falls through unchanged; the `25` comes off only when it directly follows the first `%`. Fail-closed, so the guarantee was never at risk — a refusal, not a leak. The tests are fussier than they look, and two earlier versions of them passed with the fix removed: - A numeric zone tests nothing. `%1` escapes to `%251`, which parses as scope 251, so the guard never stumbled. Only a named zone reproduces it. - A named zone cannot be hardcoded: `tryParse` resolves it against the running machine's interfaces, so `%eth0` is null on a host with no eth0. The name is asked of the machine. - There is no zoned-public-address case, because `tryParse` rejects a zone on anything not link-local, so such a URL never reached this branch.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to literal parsing, preserves DNS behavior for hostnames, and is backed by focused tests that exercise the previously failing zoned link-local case.
Pull request overview
Fixes a correctness gap in PlaintextDestinationGuard where an IPv6 link-local literal with a zone id (stored by Uri as %25...) failed InternetAddress.tryParse, incorrectly falling into the DNS-lookup branch and surfacing an “unreachable” error for destinations the guard is intended to allow.
Changes:
- Unescapes the RFC 6874
%25zone-id marker (only for literal parsing) before callingInternetAddress.tryParse. - Adds unit tests covering zoned link-local literals (including equivalence of typed
%vs%25) and asserting hostnames are never rewritten by the zone handling.
File summaries
| File | Description |
|---|---|
| lib/core/utils/plaintext_destination_guard.dart | Adds _withZoneUnescaped() and uses it for literal parsing so zoned link-local IPv6 literals are handled consistently with dart:io. |
| test/unit_test/plaintext_destination_guard_test.dart | Adds targeted tests reproducing the zoned-literal failure mode and guarding against accidental hostname rewriting. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Owner
Author
|
Closing in favour of #1004, which now carries this commit onto Nothing here is abandoned: the commit is Background on the bug is preserved in #1004's description, including why two earlier versions of its tests passed with the fix removed. |
simonoppowa
added a commit
that referenced
this pull request
Sep 1, 2026
#1004) Carries the plaintext-guard fixes onto the release branch. The first two were raised in review of #988 and were never fixed there, because the work landed on a branch targeting `develop` (#1002, since merged) while `release/2.2.0` is cut from `main`. - **A redirect bypassed the guard entirely.** `send` validated the initial URI and then handed the request to a client that follows redirects by default, below `BaseClient.send` where the guard never sees the hop. An approved private endpoint answering 30x with a public `http://` target would have had that connection made. Automatic redirects are now off, so a 30x returns to the caller as the response it is — for `https://` too. - **The Host header dropped the port.** Rebinding `http://ollama.lan:11434` to its resolved address wrote `Host: ollama.lan`, a different authority from the one typed, and a local model server is essentially never on 80. - **A zoned link-local address was refused.** `Uri` escapes the zone id, so `fe80::1%wlan0` reaches the guard as `fe80::1%25wlan0`, which `InternetAddress.tryParse` rejects; the address fell through to a DNS lookup that cannot succeed and the caller was told the server is unreachable. `dart:io` performs the same unescaping itself before it connects, which is why an unguarded client worked. Fail-closed, and it predates 2.2.0. Found by audit, not by review. The review also objected that the guard's tests used a recording fake, which cannot follow a redirect, so they pinned that `followRedirects` was set false rather than that it stops the second hop. There is now a group running against a real `HttpServer` on loopback and a real `dart:io` client, over both branches through the guard, with a control test proving an unguarded client really does follow the 302. All three fixes are mutation-tested: reverting the redirect guard fails four tests, the port two, the zone two. Also adds `release/**` to the workflow's `pull_request` trigger. This PR opened with exactly one check — the Copilot reviewer — while reporting `CLEAN`, which is the silent failure the trigger list's own comment warns about, now hit on a release branch. Cherry-picked from #1002 and from #1005, which is closed in favour of this. `plaintext_destination_guard.dart` and its test file are byte-identical across both branches.
simonoppowa
added a commit
that referenced
this pull request
Sep 1, 2026
* fix(net): three plaintext-guard fixes for 2.2.0, and CI on release PRs (#1004) Carries the plaintext-guard fixes onto the release branch. The first two were raised in review of #988 and were never fixed there, because the work landed on a branch targeting `develop` (#1002, since merged) while `release/2.2.0` is cut from `main`. - **A redirect bypassed the guard entirely.** `send` validated the initial URI and then handed the request to a client that follows redirects by default, below `BaseClient.send` where the guard never sees the hop. An approved private endpoint answering 30x with a public `http://` target would have had that connection made. Automatic redirects are now off, so a 30x returns to the caller as the response it is — for `https://` too. - **The Host header dropped the port.** Rebinding `http://ollama.lan:11434` to its resolved address wrote `Host: ollama.lan`, a different authority from the one typed, and a local model server is essentially never on 80. - **A zoned link-local address was refused.** `Uri` escapes the zone id, so `fe80::1%wlan0` reaches the guard as `fe80::1%25wlan0`, which `InternetAddress.tryParse` rejects; the address fell through to a DNS lookup that cannot succeed and the caller was told the server is unreachable. `dart:io` performs the same unescaping itself before it connects, which is why an unguarded client worked. Fail-closed, and it predates 2.2.0. Found by audit, not by review. The review also objected that the guard's tests used a recording fake, which cannot follow a redirect, so they pinned that `followRedirects` was set false rather than that it stops the second hop. There is now a group running against a real `HttpServer` on loopback and a real `dart:io` client, over both branches through the guard, with a control test proving an unguarded client really does follow the 302. All three fixes are mutation-tested: reverting the redirect guard fails four tests, the port two, the zone two. Also adds `release/**` to the workflow's `pull_request` trigger. This PR opened with exactly one check — the Copilot reviewer — while reporting `CLEAN`, which is the silent failure the trigger list's own comment warns about, now hit on a release branch. Cherry-picked from #1002 and from #1005, which is closed in favour of this. `plaintext_destination_guard.dart` and its test file are byte-identical across both branches. (cherry picked from commit ae8ad7c) * fix(bulk-add): round a converted quantity to the precision the field takes The amount field accepts at most two decimals: `_quantityPattern` in bulk_add_screen.dart is both the submit check and the field's input formatter. A converted imperial quantity carries far more — `1 lb` is 453.59237 g — and the prefill passed it through untouched. So an ordinary imperial line was unloggable. `1 lb mince` prefilled "453.59237", the submit check refused it with a message naming only the field, and because that check validates the whole batch before writing anything, one such row blocked every correct row beside it. Recovery was worse: the pattern is anchored, so the formatter matched nothing on the non-conforming string and the field blanked entirely on the first keystroke. Rounding at the prefill is what keeps the two in step. The floor is the same concern from the other end — a positive quantity below 0.005 would round to "0.00", which the check rejects for being non-positive, and a backend serving weight can be that small. `lb` is a deliberate unit, not a stray one: it is in the model tool schema enum because the model was otherwise mapping "1 pound of mince" onto `oz`, so the model path reached this too. (cherry picked from commit fcb2323)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while auditing the guard for bypasses the review of #988 might have
missed. It is not one: it is the guard refusing a destination it exists to
permit.
What happens
A link-local address needs a zone id to be routable on a host with more than
one interface, and a URI spells that zone escaped.
Uristoreshttp://[fe80::1%wlan0]with a host offe80::1%25wlan0— and typing thebare
%produces the identical string, so this is not a corner you have toknow RFC 6874 to reach.
InternetAddress.tryParseanswersnullto that escaped form. The addresstherefore fell through to the DNS branch, where the lookup cannot succeed,
and the caller was told the server is unreachable — about a link-local
server this check exists to permit, and which an unguarded client reaches
perfectly well. That is the wrong half of the
unreachable/insecureDestinationsplit those two answers exist to keep apart, and itsends someone to fix the wrong thing.
Verified against a real
HttpServerbound to a link-local address: unguardedclient
200, guarded clientSocketException: Failed host lookup.Fail-closed. The guarantee was never at risk — this is a refusal, not a
leak. Severity is low and the affected population is narrow (a zoned
link-local address means a direct-link setup), but for anyone who lands on it
the feature is broken with a misleading error.
The fix
dart:ioperforms this same substitution itself, inescapeLinkLocalAddress, before it resolves or connects — which is exactlywhy an unguarded client works. Doing it here is what makes the check agree
with the socket layer it guards.
A host that still fails to parse falls through to the lookup unchanged, so a
name is never touched: the
25comes off only when it directly follows thefirst
%.The tests are fussier than they look
Two earlier versions of them passed with the fix removed. Recorded here
because each one is a trap that produces a green, worthless test:
%1escapes to%251, which parseshappily as scope 251, so the guard never stumbled. Only a named zone
reproduces the failure.
tryParseresolves it against therunning machine's interfaces, so
%eth0isnullon a host with no eth0.The tests ask
NetworkInterface.list()for a name, and skip if there isnone.
tryParserejects azone on anything that is not link-local (
2001:db8::1%loisnull), sosuch a URL never reached this branch. An earlier test asserting otherwise
was asserting a fiction; it is gone, with a comment in its place.
The tests are hermetic on the passing path — the injected lookup calls
fail(), so reaching the resolver is the failure. With the fix revertedthey report
a zoned literal reached the resolver: fe80::1%25lo.Locally: 2118 tests pass,
flutter analyzeclean.Not proposed for 2.2.0
This predates the release, so it is not a regression in it. #1004 carries the
#988 review fixes onto
release/2.2.0and is already in review; adding thiswould widen it. Happy to cherry-pick it across if you would rather it shipped.