Skip to content

fix: path was url decoded but it should not for browser - #176029

Closed
ArthurHlt wants to merge 1 commit into
flutter:masterfrom
ArthurHlt:fix/uri-decode-on-web
Closed

ArthurHlt wants to merge 1 commit into
flutter:masterfrom
ArthurHlt:fix/uri-decode-on-web

Conversation

@ArthurHlt

Copy link
Copy Markdown

Web engine on routeInformationUpdated navigation message was handling uri parameter wrongly by decoding url encoded path leading to browser setting incorrect url.

Fix Issue:

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on [Discord].

@flutter-dashboard

Copy link
Copy Markdown

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@github-actions github-actions Bot added engine flutter/engine related. See also e: labels. platform-web Web applications specifically labels Sep 25, 2025

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly addresses an issue where URL paths were being unnecessarily decoded, which could lead to incorrect URLs in the browser. The fix removes the Uri.decodeComponent call. However, the new code introduces a syntax error by omitting a semicolon, which I've pointed out in a comment.

@ArthurHlt
ArthurHlt force-pushed the fix/uri-decode-on-web branch from 9fe17d5 to 1541e32 Compare September 25, 2025 16:00

@mdebbar mdebbar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

Have you tried it in a Flutter app to confirm that this fix works?

We also need some tests before merging the PR.

@flutter-dashboard

Copy link
Copy Markdown

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@mdebbar

mdebbar commented Dec 3, 2025

Copy link
Copy Markdown
Contributor

From triage: @ArthurHlt are you still planning to work on this PR?

@mdebbar

mdebbar commented Jan 7, 2026

Copy link
Copy Markdown
Contributor

From triage: @ArthurHlt is this still on your radar?

@mdebbar

mdebbar commented Jan 28, 2026

Copy link
Copy Markdown
Contributor

Please feel free to re-open.

@mdebbar mdebbar closed this Jan 28, 2026
ahmedsameha1 pushed a commit to ahmedsameha1/flutter that referenced this pull request Oct 3, 2026
…189835)

When the framework reports new route information via the
`routeInformationUpdated`
navigation message, the web engine rebuilds the URL and runs
`Uri.decodeComponent`
over the whole string before handing it to the browser history
(introduced in
flutter-team-archive/engine#40250). This strips one level of percent-encoding from
the address
bar on every report:

| Framework reports | Address bar before | Address bar after this PR |
|---|---|---|
| `/item/foo%2Fbar` | `/item/foo/bar` — refresh/share resolves to a
different route | `/item/foo%2Fbar` |
| `?q=x%26y` | `?q=x&y` — value splits into two query parameters |
`?q=x%26y` |
| `?color=%23ffffff` | `?color=#ffffff` — rest of the URL becomes a
fragment | `?color=%23ffffff` |

Because routers decode path parameters exactly once, this also made
`GoRouterState.pathParameters` differ between in-app navigation and a
subsequent
browser refresh of the same URL: each round trip through the engine
silently
dropped one encoding level.

## Fix

`handleNavigationMessage` keeps the scheme/authority stripping, but now
reassembles
the URL from the raw (still encoded) `path`, `query`, and `fragment`
components
instead of decoding the result. The browser's address bar now reflects
exactly what
the framework reported.

This supersedes flutter#176029, which was closed for inactivity (missing
tests). Unlike
that attempt, this PR also avoids reconstructing the query via
`Uri(queryParameters:)`, which re-encoded values differently than
reported
(e.g. `%20` in a query value came back as `+`), and it adds a regression
test.

## Tests

Adds a regression test to `lib/web_ui/test/engine/routing_test.dart`
asserting
that `%20`, `%2F`, and `%26` in path, query, and fragment survive the
round trip
into browser history unchanged. The existing `routeInformationUpdated
can handle
uri` test (scheme/authority stripping) is unaffected and still passes.

Fixes flutter#180373
Fixes flutter#171757
Fixes flutter#147857
Fixes flutter#155992

cc @mdebbar (you triaged flutter#176029 and invited a re-open — this picks that
up with tests)

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

If this change needs to override an active code freeze, provide a
comment explaining why. The code freeze workflow can be overridden by
code reviewers. See pinned issues for any active code freezes with
guidance.

**Note**: The Flutter team is currently trialing the use of [Gemini Code
Assist for
GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).
Comments from the `gemini-code-assist` bot should not be taken as
authoritative feedback from the Flutter team. If you find its comments
useful you can update your code accordingly, but if you are unsure or
disagree with the feedback, please feel free to wait for a Flutter team
member's review for guidance on which automated comments should be
addressed.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

---------

Co-authored-by: Mouad Debbar <[email protected]>
Co-authored-by: zhongliugo <[email protected]>
suryaKommana2662 pushed a commit to suryaKommana2662/flutter that referenced this pull request Oct 29, 2026
…189835)

When the framework reports new route information via the
`routeInformationUpdated`
navigation message, the web engine rebuilds the URL and runs
`Uri.decodeComponent`
over the whole string before handing it to the browser history
(introduced in
flutter-team-archive/engine#40250). This strips one level of percent-encoding from
the address
bar on every report:

| Framework reports | Address bar before | Address bar after this PR |
|---|---|---|
| `/item/foo%2Fbar` | `/item/foo/bar` — refresh/share resolves to a
different route | `/item/foo%2Fbar` |
| `?q=x%26y` | `?q=x&y` — value splits into two query parameters |
`?q=x%26y` |
| `?color=%23ffffff` | `?color=#ffffff` — rest of the URL becomes a
fragment | `?color=%23ffffff` |

Because routers decode path parameters exactly once, this also made
`GoRouterState.pathParameters` differ between in-app navigation and a
subsequent
browser refresh of the same URL: each round trip through the engine
silently
dropped one encoding level.

## Fix

`handleNavigationMessage` keeps the scheme/authority stripping, but now
reassembles
the URL from the raw (still encoded) `path`, `query`, and `fragment`
components
instead of decoding the result. The browser's address bar now reflects
exactly what
the framework reported.

This supersedes flutter#176029, which was closed for inactivity (missing
tests). Unlike
that attempt, this PR also avoids reconstructing the query via
`Uri(queryParameters:)`, which re-encoded values differently than
reported
(e.g. `%20` in a query value came back as `+`), and it adds a regression
test.

## Tests

Adds a regression test to `lib/web_ui/test/engine/routing_test.dart`
asserting
that `%20`, `%2F`, and `%26` in path, query, and fragment survive the
round trip
into browser history unchanged. The existing `routeInformationUpdated
can handle
uri` test (scheme/authority stripping) is unaffected and still passes.

Fixes flutter#180373
Fixes flutter#171757
Fixes flutter#147857
Fixes flutter#155992

cc @mdebbar (you triaged flutter#176029 and invited a re-open — this picks that
up with tests)

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

If this change needs to override an active code freeze, provide a
comment explaining why. The code freeze workflow can be overridden by
code reviewers. See pinned issues for any active code freezes with
guidance.

**Note**: The Flutter team is currently trialing the use of [Gemini Code
Assist for
GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).
Comments from the `gemini-code-assist` bot should not be taken as
authoritative feedback from the Flutter team. If you find its comments
useful you can update your code accordingly, but if you are unsure or
disagree with the feedback, please feel free to wait for a Flutter team
member's review for guidance on which automated comments should be
addressed.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

---------

Co-authored-by: Mouad Debbar <[email protected]>
Co-authored-by: zhongliugo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine flutter/engine related. See also e: labels. platform-web Web applications specifically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants