fix(net): three plaintext-guard fixes for 2.2.0, and CI on release PRs by simonoppowa · Pull Request #1004 · simonoppowa/OpenNutriTracker

simonoppowa · 2026-09-01T17:05:41Z

Carries the plaintext-guard fixes onto the release branch. Both findings were
raised in review of #988; neither was fixed there, because the work landed on
a branch targeting develop (#1002) and release/2.2.0 is cut from main.

P1 — a redirect bypassed the guard entirely

send validated the initial URI and then handed the request to an inner
client that follows redirects by default. Redirects are acted on inside
dart:io, below BaseClient.send, so a hop never re-entered the guard: an
approved private endpoint answering 302 with a public http:// target
would have had that connection made, out of a check that reported the
destination private. A same-host HTTPS-to-HTTP redirect could also carry the
optional authorization header onward.

Fixed by disabling automatic redirects, so a 30x returns to the caller as
the response it is. This holds for https:// too, which the guard otherwise
waves through — an encrypted first hop says nothing about where a Location
points.

P2 — 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 the user typed. A
reverse proxy or a strict server routes by what that header says, and a local
model server is essentially never on 80, so this was the common case rather
than a corner one. The port is now written whenever the URL carried one, and
omitted when it did not.

The tests the review asked for

Review noted that the guard's tests use a recording fake, which cannot follow
a redirect — so they pinned that followRedirects was set false, not that
setting it false stops the second hop.

Added a group that runs against a real HttpServer on loopback and a real
dart:io client, over both branches through the guard (literal pass-through
and the rebound path). A control test proves the setup has teeth by showing an
unguarded client really does follow the 302; without it the others would
pass just as well against a server that never redirected. A fourth reads the
Host header off the wire at the server, the only place that proves it survives
dart:io setting a Host of its own from the connection URI.

Both fixes were mutation-tested: removing the redirect guard fails 4 tests,
dropping the port fails 2.

A third fix: a zoned link-local address was refused

Found auditing the guard for bypasses the #988 review might have missed. It
is not one — it is the guard refusing a destination it exists to permit.

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: Uri stores
http://[fe80::1%wlan0] with a host of fe80::1%25wlan0, and typing the
bare % produces the identical string. InternetAddress.tryParse answers
null to that, so the address fell through to the DNS branch, the lookup
failed, and the caller was told the server is unreachable — about a
link-local server an unguarded client reaches perfectly well. Verified
against a real HttpServer bound to a link-local address: unguarded 200,
guarded SocketException.

dart:io performs the same substitution in escapeLinkLocalAddress before
it resolves or connects, which is why an unguarded client works; doing it
here makes the check agree with the socket layer it guards.

Fail-closed, so the guarantee was never at risk — a refusal, not a leak. It
also predates 2.2.0, so it is not a regression in this release; it is here
because it was asked for, and it is separately open against develop as
#1005. The two branches carry identical content for these files.

Two earlier versions of its tests passed with the fix removed, and the
reasons are recorded in the test file: a numeric zone escapes to a valid
different scope and proves nothing, a named zone cannot be hardcoded because
tryParse resolves it against the running machine's interfaces, and there is
no zoned-public-address case because tryParse rejects a zone on anything
not link-local.

Notes for review

Five commits cherry-picked from fix/plaintext-guard-redirects (fix(net): stop the plaintext guard being bypassed by a redirect #1002,
merged) and fix/plaintext-guard-link-local-zone (fix(net): read the zone id on a link-local address #1005), plus one ci:
commit of this PR's own.
-x trailers record the origins.
The third is a partial pick: upstream it also carried 18 documents this
branch does not have, and taking the whole of it would have dropped ~11k
lines of unrelated docs onto a release branch. Test file only.
After these, plaintext_destination_guard.dart and its test file are
byte-identical on both branches, so the eventual merge sees no conflict in
them.
Locally: 25/25 in the guard's tests on this baseline, flutter analyze
clean.

A fifth commit: this PR had no CI

On opening, it registered exactly one check — the Copilot reviewer — and
reported CLEAN. default_workflow.yml fired pull_request on main,
develop and feature/** only; release/** was not listed, which is the
silent failure its own comment already warns about, now hit on a release
branch.

ci: run the workflow on PRs into release branches adds it. For
pull_request GitHub builds the workflow from the merge ref, so it took
effect here: all six platform jobs now run on this PR. That commit is the
reason to review this as a PR rather than a direct push.

GuardedPlaintextClient approves the URL the app builds, but redirects are followed inside `dart:io`, below `BaseClient.send`. A hop therefore never re-entered the guard and never met `approve`. A private server answering 30x with a public `http://` Location had that connection made -- out of a check that had just reported the destination private. The https pass-through had the same hole from the other direction: `approve` waves https straight through, and an encrypted first hop says nothing about where a `Location` points, so an https -> http://public redirect was followed unchecked too. Setting the flag only in `_reboundTo`, as first suggested, would have missed that half; it is set in `send` for every request instead. The guard cannot vouch for a hop it never sees, so it does not let one happen: `followRedirects` is off and a 30x is returned to the caller as the response it is. `AiModelListApi` already treats a non-200 as a failed request, and an OpenAI-compatible endpoint that redirects its own API path is not a case worth following blindly to a destination nothing checked. Two tests pin it -- one per branch, both failing without the change. A third records what a caller sees; it passes either way, because the following happens below the fake, and it says so rather than looking like a regression test. (cherry picked from commit e848fda)

…s overclaiming Cherry-picked from affd838, test file only. That commit also carried 18 documents this branch does not have; taking the whole of it would have dropped 11k lines of unrelated docs into a release branch. Taken so the guard's test file is byte-identical on both branches, which keeps the eventual merge conflict-free.

Rebinding `http://ollama.lan:11434` to its resolved address kept the name in the Host header, but wrote it as `ollama.lan` — dropping the `:11434`. That is a different authority from the one the user typed. A reverse proxy or a strict server routes by what that header says, so it can reject or misroute the request, and this is the common case rather than a corner one: a local model server is essentially never on 80. The port is written only when the URL carried an explicit one, so a plain `http://ollama.lan` still sends the bare name rather than a redundant `:80`. Reported against the release branch, where the guard is otherwise unchanged. (cherry picked from commit 0954f5c)

The existing redirect tests assert against a recording fake, which cannot follow a redirect — `dart:io` does that below `BaseClient.send`, where no fake reaches. So they pin that `followRedirects` was set to false, but not that setting it false actually stops the second hop. Review of the release branch made exactly that objection. These run against a real `HttpServer` on loopback and a real `dart:io` client, on both branches through the guard: the literal pass-through and the rebound path. A control test proves the setup has teeth by showing an unguarded client really does follow the 302 to `/second` — without it the other two would pass just as well against a server that never redirected. A fourth reads the Host header off the wire at the server, which is the only place that proves it survives `dart:io` setting a Host of its own from the connection URI. `flutter_test` installs an HttpOverrides that answers every request with a canned 400 instead of opening a socket; the group clears it and puts it back. (cherry picked from commit 28ce71a)

`pull_request` fired on `main`, `develop` and `feature/**` only, so this PR into `release/2.2.0` registered exactly one check — the Copilot reviewer — and still reported `CLEAN`. That is the silent failure the comment above already warns about, now hit on a release branch. A release branch is the last place that should merge unverified: everything on it is by definition about to become `main`. For `pull_request` GitHub builds the workflow from the merge ref, so this takes effect on the PR that carries it.

Copilot

🟡 Changes recommended

The real-socket test suite snapshots HttpOverrides.current but restores it into HttpOverrides.global, which can unintentionally promote a zone override into a global override and should be made symmetric by saving/restoring HttpOverrides.global.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Ports the plaintext-destination guard fixes onto the release/2.2.0 branch to prevent redirect-based bypasses of the guard and preserve the original authority (including port) in the Host header when rebinding to a resolved private IP.

Changes:

Disable automatic redirect following for all requests sent through GuardedPlaintextClient, ensuring 30x responses surface to callers instead of becoming unchecked second hops.
Preserve the original Host header authority including port when present during rebound-to-approved-IP request rebuilding.
Add integration-style tests using a real loopback HttpServer to prove redirect behavior and on-the-wire Host header preservation with a real dart:io client stack.

File summaries

File	Description
lib/core/utils/plaintext_destination_guard.dart	Turns off redirect following at the guard boundary and preserves `Host` authority with port during rebound.
test/unit_test/plaintext_destination_guard_test.dart	Extends unit tests and adds real-socket coverage for redirects and `Host` header behavior.

Review details

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+      savedOverrides = HttpOverrides.current;
+      HttpOverrides.global = null;
+


Copilot

🔵 Needs a closer look

The new Host header reconstruction is invalid for IPv6 literals with a port (needs bracketed form like [::1]:11434) and should be corrected before release.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

lib/core/utils/plaintext_destination_guard.dart:196

Host header construction will be invalid for IPv6 literals when a port is present (e.g. http://[::1]:11434 becomes Host: ::1:11434), which can break routing and HTTP parsing. Wrap IPv6 hosts in [] when appending the port so the Host header is syntactically correct.

    final origin = request.url;
    return http.Request(request.method, url)
      ..headers.addAll(request.headers)
      ..headers['host'] = origin.hasPort
          ? '${origin.host}:${origin.port}'
          : origin.host

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Lite

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. (cherry picked from commit 347dbd6)

Copilot

🟢 Approval recommended

The changes directly address the stated security/behavior gaps and add both unit and real-socket tests that validate the fixed behavior.

Review details

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Lite

* 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)

simonoppowa added 4 commits September 1, 2026 19:00

Copilot AI lite review requested due to automatic review settings September 1, 2026 17:05

Copilot started reviewing on behalf of simonoppowa September 1, 2026 17:06 View session

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Comment thread test/unit_test/plaintext_destination_guard_test.dart

Comment on lines +363 to +365

savedOverrides = HttpOverrides.current;

HttpOverrides.global = null;

Copilot AI review requested due to automatic review settings September 1, 2026 17:08

Copilot started reviewing on behalf of simonoppowa September 1, 2026 17:08 View session

Copilot AI reviewed Sep 1, 2026

View reviewed changes

simonoppowa mentioned this pull request Sep 1, 2026

fix(net): read the zone id on a link-local address #1005

Closed

Copilot AI review requested due to automatic review settings September 1, 2026 17:24

Copilot started reviewing on behalf of simonoppowa September 1, 2026 17:25 View session

Copilot AI reviewed Sep 1, 2026

View reviewed changes

simonoppowa changed the title ~~fix(net): close the plaintext guard's redirect bypass and Host-header port loss on 2.2.0~~ fix(net): three plaintext-guard fixes for 2.2.0, and CI on release PRs Sep 1, 2026

simonoppowa merged commit ae8ad7c into release/2.2.0 Sep 1, 2026
8 checks passed

simonoppowa mentioned this pull request Sep 1, 2026

chore: backport the 2.2.0 release-branch fixes to develop #1006

Merged

Navigation Menu

Uh oh!

fix(net): three plaintext-guard fixes for 2.2.0, and CI on release PRs - #1004

fix(net): three plaintext-guard fixes for 2.2.0, and CI on release PRs#1004
simonoppowa merged 6 commits into
release/2.2.0from
fix/plaintext-guard-2.2.0

simonoppowa commented Sep 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		savedOverrides = HttpOverrides.current;
		HttpOverrides.global = null;

Uh oh!

Conversation

simonoppowa commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

P1 — a redirect bypassed the guard entirely

P2 — the Host header dropped the port

The tests the review asked for

A third fix: a zoned link-local address was refused

Notes for review

A fifth commit: this PR had no CI

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

simonoppowa commented Sep 1, 2026 •

edited

Loading