fix(net): stop the plaintext guard being bypassed by a redirect - #1002
Merged
Conversation
Every claim on the page was checked against release/2.2.0. The model of the feature holds -- the schema-shaped guarantee, the routing, the timeouts, the probe taxonomy and all 27 code-map rows survived. Eight sentences did not. Two are drift rather than error: the page landed on 2026-08-27, metadata stripping (#914) hours later the same day, `portion` (#864) three days after. - **The schema has four fields, not three.** `portion` was missing from the prose and from the sequence diagram, and the page's own excerpt elided the comment defending it. It is not inert -- it is a lookup key into the matched food's own portion list, so the model's word chooses which row is preset. The grams still come from the database, but a page that tells the reader to audit the schema has to state what is in it. - **The encoder fallback does not send the file unmodified.** It strips every metadata block first -- EXIF GPS above all -- and refuses a file it cannot parse. The page understated its own protection while asserting something false, and "unmodified" is the word a privacy reviewer would quote back. Third bound added; diagram node corrected. - **1024 px bounds the shortest edge, not the longest.** flutter_image_compress takes the smaller ratio, so a 4:3 frame leaves at about 1365x1024. The source comment in meal_photo_encoder.dart carries the same misreading and still needs its own fix. - **There is no gallery source.** Only ImageSource.camera is offered; "Camera or picker" read as "camera or gallery". - **The photo path has a second gate.** For a server you run it appears only once the probe's photo leg has passed. - **Two requests carry a retention instruction on the wire** -- `store: false` to OpenAI, `data_collection: "deny"` to OpenRouter -- so retention is not purely a policy question deferred to the README. - **The probe is not once.** It re-runs on every confirm, deliberately: the address and model can be identical and the machine behind them different. - **`rejected` is the provider refusing, not a guardrail.** A plaintext-guard refusal lands on `unknown`, because nothing was sent and nothing was learned. - **The schema row cited a test that does not pin it.** The contract test named asserts a quantity ceiling and would still pass if `calories` were added. Repointed at the test that actually enumerates the exposed fields.
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.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The redirect bypass is addressed at the correct layer and is covered by targeted regression tests for both affected code paths.
Pull request overview
This PR closes a security gap in the plaintext destination guard by ensuring redirects cannot silently bypass PlaintextDestinationGuard.approve() (since redirect-following happens below http.BaseClient.send in dart:io). It updates the guarded HTTP client to always return 30x responses to callers without following them, and adds unit tests to prevent regressions on both the rebound (http:// pinned-to-IP) and https:// pass-through paths.
Changes:
- Disable redirect-following for all requests sent via
GuardedPlaintextClient. - Add unit tests asserting
followRedirects == falseon both rebound andhttps://pass-through branches. - Add a contract test documenting that a 30x surfaces to callers as a response (not a second hop).
File summaries
| File | Description |
|---|---|
| lib/core/utils/plaintext_destination_guard.dart | Forces followRedirects = false in GuardedPlaintextClient.send so redirects can’t become unchecked second destinations. |
| test/unit_test/plaintext_destination_guard_test.dart | Extends the guard test suite with redirect-related assertions and a 30x caller-visible contract check. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+22
to
+26
| Future<http.StreamedResponse> send(http.BaseRequest request) async { | ||
| sent = request; | ||
| return http.StreamedResponse(const Stream.empty(), 200, request: request); | ||
| sentCount++; | ||
| return response ?? | ||
| http.StreamedResponse(const Stream.empty(), 200, request: request); |
Comment on lines
+7
to
+10
| /// Records the request that reached the socket, or the fact that none did. | ||
| /// | ||
| /// [sentCount] matters for the redirect tests: following a 30x would show up | ||
| /// here as a second send, which is exactly what must not happen. |
Second pass over the same review: the GAP and OVERSTATED findings, after the factually-wrong set. Every addition was verified against release/2.2.0 first, and three of the reviewer's suggestions turned out to be wrong. What the page now answers that it did not: - **What travels with a request.** Model id, system prompt, the line you typed, the tool and its schema, the answer cap -- and the app language appended to the prompt as one sentence. Nothing from the diary, profile or history, no earlier request, no identifier. The negative is structural: `requestItems` takes a system string and a `MealContent`, so there is no seam to attach one. - **The other half of the litre story.** Putting `l` in the enum opened the mirror-image failure -- a model answering with a unit nobody typed, 470 kcal logged for a glass of milk. `textStatesAUnit` closed it by corroborating any unit against the text the model was given. - **What a garbled or truncated reply does.** One bad entry is dropped and the batch survives; a reply where every entry drops is refused rather than passed on as an empty list, which would be indistinguishable from "no food here". Truncation is not a case the app recognises, and the page now says so. - **Two requests reach a server you run**, and the model-list one goes out before the consent screen -- it hangs off the address field, not the save path. - **What the app asks each destination not to keep** -- `store: false`, `data_collection: deny` -- with a guardrail row for the latter. - **The probe sends a fixed line and a bundled photograph**, never anything of yours, matching the consent screen's wording from #985. - The `failed --> passed` edge the state diagram was missing, the photo-only count rule, the onboarding entry point, and the consent invariant's real home in credential storage rather than in two widgets. Narrowed where the page claimed more than the code delivers: retraction covers the photo row only; "the disclosure shown before anything is stored" becomes "before any credential is stored", since a provider tag and a model id are written first; and the blanket no-logging sentence now matches what the contract test actually pins about the three clients. The page's own guard is hardened alongside it, since it is the only thing still watching once the page is lifted to the wiki: the link scan read one link form out of several and silently missed reference-style definitions, the slug function diverged from GitHub's on underscores and on runs of spaces, a link resolving outside the repository was mis-resolved rather than failed, and the canary floor sat at 25 against a real 49. Verified by adding a broken reference-style link and watching it fail, which it did not before.
The page said the plaintext check "is the whole of the enforcement". That was overstated while redirects escaped it, and is true again now: the sentence lands here rather than in the documentation PR because it only becomes accurate when this change does. Says why it is whole rather than asserting that it is -- a redirect is not followed, so no hop is made on an approval granted to a different destination -- and covers the https case, which the guard otherwise waves through. This branch carries the doc-accuracy work merged in, so the paragraph sits in its corrected context; #996 remains the PR that reviews it.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The redirect-bypass fix is clear and well-covered by targeted unit tests; only a minor test-fake fidelity nit was noted.
Review details
Suppressed comments (1)
test/unit_test/plaintext_destination_guard_test.dart:26
- When a custom response is provided, this fake client returns it without attaching the current
request. That makesstreamedResponse.request(and thereforehttp.Response.request) null, which diverges from realhttpbehavior and can hide issues in code that reads the request URL from the response (e.g. for logging or error messages). Wrap the provided response sorequestis set consistently.
Future<http.StreamedResponse> send(http.BaseRequest request) async {
sent = request;
sentCount++;
return response ??
http.StreamedResponse(const Stream.empty(), 200, request: request);
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ead indented fences Copilot caught both. The retention sentence had no subject: it now says the Responses API retains request and response content by default, which is the point of sending `store: false`. `withoutFences` anchored its fence pattern at column 0, so a fence indented inside a list item was invisible to it and the sample links and headings in that fence were scanned as though they were the page's own. CommonMark allows up to three spaces before a fence, and the closing run needs the same allowance.
…s overclaiming Copilot, both correct. The injected response dropped `request:`, so an assertion on `response.request` would have read differently depending on whether a test supplied its own response. And the `sentCount` doc comment implied the counter could detect a redirect being followed; it cannot, because that happens inside `dart:io` below this fake. It pins that the guard sends once, which is a smaller claim and the true one. Carries the updated doc branch in as well.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The PR’s scope includes many new/updated research docs beyond the described redirect fix, so the title/description should be updated (or docs split) to keep review and risk clear.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+1
to
+12
| # F-Droid submission: feasibility review of issue #575 | ||
|
|
||
| Research notes for [#575](https://github.com/simonoppowa/OpenNutriTracker/issues/575) ("Submit OpenNutriTracker to the F-Droid repository") and its parent [#126](https://github.com/simonoppowa/OpenNutriTracker/issues/126). Written against `feat/onboarding-rework` at version `2.0.2+61`, and against a shallow clone of [`fdroid/fdroiddata`](https://gitlab.com/fdroid/fdroiddata) at commit `b3626a167c61d17211c3be8e41a61a97c807ed5c` (2026-08-05). Every fdroiddata path below is relative to that repo. | ||
|
|
||
| ## Verdict | ||
|
|
||
| **Blocked — but not on the thing the issue thread is worried about.** | ||
|
|
||
| The `fdcApiKey` that surfaced in the #575 discussion is a nuisance, not a blocker. F-Droid's Inclusion Policy has an explicit line for it, there is a working precedent in the repo for the exact same USDA API, and — as it turns out — the key is wired to dead code in this app anyway. | ||
|
|
||
| The actual blocker is **`mobile_scanner`**, which links `com.google.mlkit:barcode-scanning`. ML Kit is on F-Droid's non-free signature list with `"license": "NonFree"`, and fdroiddata's CI runs a binary scanner over the built APK that fails the pipeline when those classes are found. Barcode scanning is a headline feature of this app — it appears in `full_description.txt`, in the recipe builder, and in three separate import screens — so removing it is a product decision, not a packaging detail. That decision belongs to the maintainer, and #575 cannot be completed until it is made. | ||
|
|
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.
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The redirect fix is solid, but GuardedPlaintextClient can still silently fail to “pin” approved hostnames for non-http.Request request types, and the PR also includes substantial unrelated documentation additions that should be split or explicitly scoped.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
docs/fdroid-submission-feasibility.md:5
- This PR’s title/description are scoped to a network-redirect bypass in
GuardedPlaintextClient, but it also adds several large, unrelated research docs (e.g. F-Droid feasibility, competitive gaps, multiple OpenAI policy notes). That makes the change set harder to review/revert and obscures the security-relevant diff; consider splitting these documentation additions into separate PR(s) or explicitly calling them out in the PR description as part of the intended scope.
# F-Droid submission: feasibility review of issue #575
Research notes for [#575](https://github.com/simonoppowa/OpenNutriTracker/issues/575) ("Submit OpenNutriTracker to the F-Droid repository") and its parent [#126](https://github.com/simonoppowa/OpenNutriTracker/issues/126). Written against `feat/onboarding-rework` at version `2.0.2+61`, and against a shallow clone of [`fdroid/fdroiddata`](https://gitlab.com/fdroid/fdroiddata) at commit `b3626a167c61d17211c3be8e41a61a97c807ed5c` (2026-08-05). Every fdroiddata path below is relative to that repo.
## Verdict
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
153
to
+158
| Future<http.StreamedResponse> send(http.BaseRequest request) async { | ||
| final approved = await _guard.approve(request.url); | ||
| if (approved == request.url) return _inner.send(request); | ||
| return _inner.send(_reboundTo(approved, request)); | ||
| final outgoing = approved == request.url | ||
| ? request | ||
| : _reboundTo(approved, request); | ||
|
|
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 by the
ai-architecture.mdreview, which flagged the page's claim that the guard "is the whole of the enforcement" as overstated. It was — but the honest fix is in the code, not the sentence.The gap
GuardedPlaintextClient.sendapproves the URL the app builds. Redirects are followed insidedart:io, belowBaseClient.send, so a hop never re-enters the guard and never meetsapprove.A private server answering
302with a publichttp://Location had that connection made — out of a check that had just reported the destination private.AiModelListApi.listis a GET through exactly this client, pointed at an address the user typed.The https pass-through had the same hole from the other side.
approvereturns non-http URLs untouched, so an https request keptfollowRedirects = trueand anhttps:// → http://publicredirect was followed unchecked. This is why the fix is insendrather than in_reboundToas first suggested —_reboundToonly runs on the rewritten http path, so that version would have closed one branch and left the other open.The fix
followRedirects = falsefor every request through the client, and a 30x is returned to the caller as the response it is. The guard cannot vouch for a hop it never sees, so it does not let one happen.Behaviour change, and worth being explicit about it: an endpoint that redirects its own API path now fails instead of being followed.
AiModelListApialready treats a non-200 as a failed request, so this surfaces as "that address did not work" rather than a crash. An OpenAI-compatible server redirecting/v1/modelselsewhere is not a case worth chasing to a destination nothing checked.Tests
There were none for redirects, which is how this survived.
redirects are not followed, on the rebound path— fails without the fixredirects are not followed on the https pass-through either— fails without the fixa 30x comes back to the caller as a response, not a second hop— passes either way, and says so in a comment. The following happens insidedart:io, below the fake client, so this pins the caller-visible contract rather than catching the regression.I verified the first two go red with the change reverted rather than assuming they discriminate.
flutter test— 2110 passflutter analyze— cleanRelated
#996 corrects the documentation claim on the same subject. This PR is what makes that claim true.