fix(ui): CDN query re-coding

[renefloor]

Submit a pull request

CLA

• I have signed the Stream CLA (required).
• The code changes follow best practices
• Code changes are tested (add some information if not applicable)

Description of the pull request

Every image attachment fails to load (403). Text, PDFs, avatars, reactions, read receipts all fine — images only, and nothing gets logged because the image cache swallows the error.

Root cause: stream_image_cdn.dart built the resized URL with Uri.replace(queryParameters: {...}), which re-encodes existing query values. The signing params are base64, so = padding became %3D and the signature no longer matched.

Why prod never showed it: the two CDNs sign differently — production CloudFront uses URL-safe base64 (__, ~, -, no = at all), so re-encoding was a no-op. GCP Cloud CDN uses standard base64 with = padding. The bug was always there; the CDN migration just exposed it.

Verified by curl: original URL 200, with resize params and = intact 200, with =→%3D 403. Fixed by rebuilding from the raw query string; all images including GIFs now render on device.

Screenshots / Videos

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[xsahil03x]

Nice catch on this one — the = → %3D story checks out end to end. I rebuilt the old and new resolveUrl side by side and diffed their output over a stack of URLs before believing it.

Two things I'd pull into the description, because they make the change look better than it currently reads.

The padding was only the visible half of the bug. replace(queryParameters:) was also mangling any value containing a sub-delimiter, and quietly collapsing repeated params:

source query	before	after
Signature=abc=	Signature=abc%3D	Signature=abc=
Signature=a+b/c=	Signature=a+b%2Fc%3D	unchanged
a=1;b=2	a=1%3Bb%3D2	unchanged
x=1&x=2	x=2	both kept

Empty query, bare ?, fragments, width: 0 (w=%2A) and overriding an existing w/h/resize all come out the same as before.

And it doesn't bust anyone's image cache — which is the first thing I'd expect to be asked. cacheKey only keeps w/h/resize/crop, and those four survive both code paths identically, so the keys come out byte-for-byte the same before and after. Nothing re-downloads on upgrade. (resolveUrl also has exactly one caller — image_attachment_thumbnail.dart#L77-L78 — which lines up with avatars and reactions being unaffected.)

Couple of comments inline — only the changelog one is really worth acting on.

Caveat on all of the above: I didn't run flutter test on the branch. I ran the new tests' assertions against a standalone copy of the implementation instead.

[xsahil03x]

Heads up — three of these six assertions pass against the unfixed implementation. I checked by running them against the old code:

• queryParameters['URLPrefix'] and queryParameters['Signature'] — Uri.parse decodes %3D straight back to =, so they hold either way. They read like the strongest check in the test and they're the weakest.
• w=200 / h=300 — already pinned by adds resize params when none exist higher up the file.

The three that do fail on the old code are the raw contains on each pair plus the %3D check, so I'd keep just those:

Suggested change

	// The regression: `=` must not come back as `%3D`.
	expect(result, isNot(contains('%3D')));
	expect(result, contains('URLPrefix=$urlPrefix'));
	expect(result, contains('Signature=$signature'));
	expect(
	Uri.parse(result).queryParameters['URLPrefix'],
	equals(urlPrefix),
	);
	expect(
	Uri.parse(result).queryParameters['Signature'],
	equals(signature),
	);
	expect(result, contains('w=200'));
	expect(result, contains('h=300'));
	// The regression: `=` must not come back as `%3D`.
	expect(result, isNot(contains('%3D')));
	expect(result, contains('URLPrefix=$urlPrefix'));
	expect(result, contains('Signature=$signature'));

Same goes for the CloudFront test above — its two queryParameters assertions and the w/h pair can go the same way. Keep the test itself though: it passes with or without the fix, and that's the point of it — it's "here's why production never saw this" in executable form.

One other thing: the (production backend) / (staging backend) labels are going to read backwards the moment this migration lands and GCP is production. Naming by the alphabet instead — "preserves URL-safe base64 signing parameters" and "preserves the = padding of standard base64 signing parameters" — won't rot, and your group comment already says which backend is which.

[xsahil03x]

Mind dropping the second sentence? STYLE_GUIDE.md#L1430-L1435 asks entries to skip implementation notes — "details that only matter to the person writing the PR belong in the PR description" — and the description already explains the %3D mechanics better than a bullet can.

Suggested change

	- Fixed images failing to load (HTTP 403) from CDNs that sign URLs with standard base64. `StreamImageCDN.resolveUrl` rebuilt the query string in a way that re-encoded the `=` padding in signing parameters as `%3D`, invalidating the signature.
	- Fixed images failing to load (HTTP 403) from CDNs that sign URLs with standard base64.

[xsahil03x]

Worth recording why this can't go back to {...uri.queryParameters, 'w': ...} — I tried it, and the obvious repair doesn't hold.

Framing the bug as "re-encodes on write" suggests you could keep the map and just write the query out without escaping =. You can't — uri.queryParameters is lossy on the read:

raw in the URL       URLPrefix=aHR0cHM6+y91cy1lYXN0MS5nY3A/c3RyZWFt==
uri.queryParameters            aHR0cHM6 y91cy1lYXN0MS5nY3A/c3RyZWFt==
written back raw     URLPrefix=aHR0cHM6%20y91cy1lYXN0MS5nY3A/c3RyZWFt==   → still 403

+ means space in a query string, so once you're through the decoded map the original byte is gone and nothing on the write side brings it back. Not hypothetical for this CDN either: a GCP URLPrefix is standard base64, alphabet A–Z a–z 0–9 + /. The == padding is just the character that happened to be in the URL you tested with — a prefix with a + in it fails exactly the same way.

Might be worth widening the comment so the next person doesn't try the same shortcut:

Suggested change

	// Rebuild the query from the raw string rather than via
	// `replace(queryParameters:)`, which re-encodes every existing value.
	// Signed CDN URLs carry base64 parameters (`URLPrefix`, `Signature`) whose
	// `=` padding would become `%3D`, invalidating the signature (HTTP 403).
	// Rebuild the query from the raw string rather than via
	// `replace(queryParameters:)`, which decodes every existing value and
	// re-encodes it. Signed CDN URLs carry standard-base64 parameters
	// (`URLPrefix`, `Signature`) that do not survive that round trip: `=`
	// padding comes back as `%3D` and `+` comes back as `%20`, either of
	// which invalidates the signature (HTTP 403).