Skip to content

fix(dashboard): display dashboard images configured via external URL link #249

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
deaflynx merged 5 commits into from
Sep 8, 2026
+36 −19
Merged

fix(dashboard): display dashboard images configured via external URL link #249

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
48790cb
fix: display dashboard images configured via external URL link
deaflynx Aug 20, 2026
c7a0292
fix: resolve dashboard image links relative to the active endpoint
deaflynx Aug 21, 2026
1eb4637
docs: clarify HTTP(S)-only wording on image link resolver
deaflynx Sep 4, 2026
23eb34e
docs: drop inaccurate browser-resolution clause from image link resolver
deaflynx Sep 4, 2026
6e4f884
docs: say root-relative in image link resolver comment
deaflynx Sep 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 37 additions & 19 deletions lib/utils/utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ abstract class Utils {
onError: onError,
);
} else {
final newImageUrl = _removeTbImagePrefix(imageUrl);
if (_isImageResourceUrl(newImageUrl)) {
final resolvedImageUrl = _removeTbImagePrefix(imageUrl);
if (_isImageResourceUrl(resolvedImageUrl)) {
final jwtToken = tbClient.getJwtToken();
if (jwtToken == null) {
return _onErrorImage(
Expand All @@ -98,7 +98,7 @@ abstract class Utils {
onError: onError,
);
}
final parts = newImageUrl.split('/');
final parts = resolvedImageUrl.split('/');
final key = parts[parts.length - 1];
parts[parts.length - 1] = Uri.encodeComponent(key);
final encodedUrl = parts.join('/');
Expand All @@ -115,29 +115,31 @@ abstract class Utils {
semanticLabel: semanticLabel,
onError: onError,
);
} else if (_isBase64DataImageUrl(imageUrl)) {
} else if (_isBase64DataImageUrl(resolvedImageUrl)) {
return _imageFromBase64(
context,
imageUrl,
color: color,
width: width,
height: height,
semanticLabel: semanticLabel,
onError: onError,
);
} else if (_isValidUrl(imageUrl)) {
return _networkImage(
context,
imageUrl,
resolvedImageUrl,
color: color,
width: width,
height: height,
semanticLabel: semanticLabel,
onError: onError,
);
} else {
return _onErrorImage(
final imageLink = _resolveNetworkImageLink(resolvedImageUrl);
if (imageLink == null) {
return _onErrorImage(
context,
color: color,
width: width,
height: height,
semanticLabel: semanticLabel,
onError: onError,
);
}
return _networkImage(
context,
imageLink,
color: color,
width: width,
height: height,
Expand Down Expand Up @@ -287,15 +289,31 @@ abstract class Utils {
}

static String _removeTbImagePrefix(String url) {
return url.replaceFirst(_tbImagePrefix, '');
return url.startsWith(_tbImagePrefix)
? url.substring(_tbImagePrefix.length)
: url;
}

static bool _isImageResourceUrl(String url) {
return _imagesUrlRegexp.hasMatch(url);
}

static bool _isValidUrl(String url) {
return Uri.tryParse(url) != null;
/// Absolute links are fetched as they are. Links relative to the platform,
/// such as an image public link, are resolved against the active endpoint the
/// same way a browser resolves them against its origin. Anything else has no

@vvlladd28 vvlladd28 Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verified this clause against the implementation, and it is the one part that does not hold: for a protocol-relative value the resolution is not browser-equivalent. Uri.tryParse('//evil.com/x.png') gives hasScheme == false with host == 'evil.com', and since it starts with / it takes the endpoint branch and yields https://mytb.example.com//evil.com/x.png, where a browser resolving against its origin would have gone to https://evil.com/x.png. Checked on Dart 3.29.0, the version FVM pins for this repo.

I am not asking for a code change, and I want to be explicit about why rather than leave it looking like an open question. The branch attaches no auth header, so nothing leaks; the placeholder rendered for this input before this PR too, so there is no regression; and the platform never emits //host/path — only hand-typed input reaches it, since the Image link field has no validators. Uri.resolve would be the wrong repair: it would make the sentence true by actually fetching from the foreign host, which is worse than the broken render you have today. Your string concatenation is the safer behaviour here.

So this is purely about the sentence. Dropping the browser half and leaving "are resolved against the active endpoint" would be accurate without overclaiming. Perfectly reasonable to leave it as is, too — you have already reworded this comment once at my request, and one imprecise clause in a doc comment is not worth a third round.

@deaflynx deaflynx Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 23eb34e — dropped the browser clause; the comment now says only that platform-relative links are resolved against the active endpoint. Behaviour unchanged, per your note.

/// meaningful target and is rendered as a missing image.
static String? _resolveNetworkImageLink(String url) {
final uri = Uri.tryParse(url);
if (uri == null) {
return null;
}
if (uri.scheme == 'http' || uri.scheme == 'https') {

@vvlladd28 vvlladd28 Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only nit I have on the new helper: the comment above says "Absolute links are fetched as they are", but this gate passes only http/https — ftp:, file: and any custom scheme fall through to the final return null alongside genuinely unresolvable input. That's the behaviour you want, the sentence just makes it read like an oversight. "HTTP(S) links are fetched as they are" would make the restriction look as deliberate as it is.

For the record I did go looking for a hole in this gate and didn't find one worth acting on: uri.scheme is normalised, so HTTPS://… is handled, and the one input that slips through is https:example.com/pic.png (the missing-// typo), which reaches Image.network with an empty host. It renders the placeholder either way, before and after this PR, so guarding it would add a line for no observable change — noting it only so it doesn't get re-raised later.

@deaflynx deaflynx Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1eb4637 — reworded to "HTTP(S) links are fetched as they are" so the scheme gate reads as intentional.

return url;
}
if (!uri.hasScheme && url.startsWith('/')) {
return getIt<IEndpointService>().getCachedEndpoint() + url;
}
return null;
}

static double degreesToRadians(double degrees) {
Expand Down