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

[deaflynx]

Summary

Dashboard cards on the Home grid showed an empty white box instead of the thumbnail when the dashboard image was configured via the "Image link" field. Images uploaded to the Image Gallery displayed fine.

Fixes PROD-8502.

Two distinct inputs produced the same empty box, and both are fixed here:

1. An external web URL — https://example.com/pic.png
2. An image public link copied from the gallery — /api/images/public/{key}, found while reviewing (1) and reproduced on a device

Root cause

The platform stores every dashboard image value with a tb-image; prefix, external URLs included (prependTbImagePrefix in gallery-image-input.component.ts). Utils.imageFromTbImage computed the prefix-stripped URL but only used it in the gallery-resource branch, while the base64 and external-URL branches checked and loaded the raw prefixed string. Uri.tryParse('tb-image;https://…') returns null, so external links fell through to the error placeholder — a transparent 1×1 GIF stretched by the card's FittedBox, which renders as the empty white box.

The public-link case has a separate cause. TbResourceInfo.getPublicLink() returns a relative path, the "Embed image" dialog hands users exactly that string, and the "Image link" field has no validators. IMAGES_URL_REGEXP matches only tenant|system, so a public link is classified as external, and _isValidUrl — which was just Uri.tryParse(url) != null and therefore true for almost any string — let the relative path reach Image.network, where it cannot be fetched at all (ArgumentError: No host specified in URI).

Changes

• Use the prefix-stripped URL in every branch, and rename it to resolvedImageUrl to match ImageService.resolveImageUrl on the web side.
• Anchor the tb-image; strip with startsWith/substring instead of replaceFirst, so an occurrence inside a link is no longer removed.
• Replace _isValidUrl with _resolveNetworkImageLink, which classifies and resolves in one step: absolute http/https links are fetched as they are, platform-relative links resolve against the active endpoint the way a browser resolves them against its origin, and anything without a meaningful target renders the missing image. Resolved relative links carry no auth header, matching the platform — /api/images/public/{key} is a noauth endpoint and a browser <img src> does not send the JWT either.

Since imageFromTbImage also backs device and device-profile images, this repairs image links there too, not only the dashboard grid.

Behaviour

Value in "Image link"	Result
https://example.com/pic.png	fetched as is
tb-image;https://example.com/pic.png	prefix stripped, fetched
/api/images/tenant/pic.png	endpoint + path, with auth header
/api/images/public/pic.png	endpoint + path, no auth header
data:image/png;base64,…	decoded in memory
ftp://…, or text that is not a link	missing image placeholder

Test plan

• Dashboard image set via an external URL → thumbnail displays on the Home grid
• Dashboard image set to an image public link copied from the gallery Embed dialog → thumbnail displays
• Dashboard image uploaded to the Image Gallery → still displays
• Dashboard without an image → placeholder icon still displays
• Device and device profile images still display

[vvlladd28]

Review summary

Reviewed 1 changed file in fix(dashboard): display dashboard images configured via external URL link. Left 5 comment(s) inline.

The fix is correct and the root cause in the description checks out — Uri.tryParse('tb-image;https://example.com/a.png') really does return null. Using the stripped value in every branch matches ImageService.resolveImageUrl (ui-ngx/src/app/core/http/image.service.ts:166), which does exactly this: strip the prefix first, then dispatch on shape. Since imageFromTbImage also backs device and device-profile images (devices_base.dart:235,441, device_profiles_base.dart:326), this repairs external image links there too, not just the dashboard grid.

Worth knowing while reviewing the comments below: the platform's "Image link" field is new FormControl(null) with no validators at all (gallery-image-input.component.ts:82), so dashboard.image can hold any string a user types — including relative paths and absolute URLs pointing back at the same instance. Two of the branches this method dispatches into don't handle those, which is the same user-visible symptom this PR is fixing.

Checked and dismissed

One thing I flagged initially and then disproved, noting it so it doesn't get re-raised: the base64 branch calls UriData.parse / contentAsBytes(), which throw synchronously and would escape Image.memory's errorBuilder. That is unreachable in practice — DashboardServiceImpl.saveDashboard:177 and DeviceProfileServiceImpl:184 unconditionally run the value through BaseImageService.convertToImageUrl, which decodes it with Base64.getDecoder().decode outside its try/catch (line 467) and rewrites it to tb-image;/api/images/... (line 516). A malformed payload therefore fails the save instead of being persisted. The only two shapes the backend leaves untouched are the imageData == null cases, data:image/png;base64, and data:image/png,hello, and both parse cleanly in Dart (0 and 5 bytes) into a harmless Image.memory error. No change needed.

Additional findings

These are about existing code outside the PR's diff — spotted while reading surrounding context, and each verified against the platform source.

• lib/utils/utils.dart:293 — _isImageResourceUrl uses an unanchored hasMatch, so an absolute URL whose path contains /api/images/tenant/… is claimed by the gallery branch instead of the external branch this PR fixes. The likely trigger isn't a foreign server — it's a user pasting their own instance's full image URL into "Image link", which the unvalidated field accepts. https://mytb.example.com/api/images/tenant/logo.png matches the regexp, so the code builds getCachedEndpoint() + <that absolute URL> → https://mytb.example.comhttps://mytb.example.com/api/images/..., and attaches X-Authorization: Bearer <jwt> to it. Web survives this because getImageDataUrl does http.get(imageUrl) rather than concatenating. Anchoring the pattern with ^ keeps absolute URLs out of the branch and closes the header leak too. Note the platform's IMAGES_URL_REGEXP (resource.models.ts:159) is unanchored in the same way, so this needs fixing here rather than copying the web behaviour.
• lib/utils/utils.dart — the gallery branch never requests the server-side thumbnail. ImageController exposes IMAGE_URL + "/preview" (line 216) and the web asks for it via getImageDataUrl(imageUrl, preview, …) (image.service.ts:107). The mobile app always downloads the full-size original, including for the small Home grid card. Appending /preview for the grid would cut both transfer and decode cost noticeably on image-heavy tenants.
• No test coverage anywhere in the repo — verified: no test/ or integration_test/ directory and no *_test.dart file, even though flutter_test, integration_test, mocktail and bloc_test are already in dev_dependencies, so the setup cost of the first test is close to zero. That's a real gap for this bug class: all four dispatch paths are selected purely from string shape, and the bug was "the prefix is stripped for one branch and not the others". The reason it isn't testable today is that classification is fused into widget construction — extracting the pure part (strip prefix, classify, and for the gallery case build the encoded endpoint URL) into a returned value, mirroring the resolveImageUrl shape you're aligning with, would make all four branches unit-testable in a few lines, with a WidgetTester test only for the widget wiring.

---

This review was auto-generated. Findings may contain errors — please verify before applying changes.

[vvlladd28]

Now that every branch consumes the stripped value, imageUrl is only used for the null/empty guard and newImageUrl is the real subject of the method — which makes the name unhelpful: "new" says nothing about what it holds, and a reader has to scroll back to line 88 to learn it's the prefix-free value. resolvedImageUrl would line up with the resolveImageUrl naming on the web side you're mirroring here.

Style-only, but while you're here the method would also read better flattened: an early return _onErrorImage(...) for the null/empty case drops the whole else block and one level of indentation off this branch chain.

[deaflynx]

Renamed to resolvedImageUrl — the point about newImageUrl being uninformative holds, and it lines up with ImageService.resolveImageUrl on the web side.

Skipping the flatten. Dropping the else and early-returning touches every line of the method, which turns a 4-line bugfix into a full-method rewrite on an LTS branch and makes both the CE→PE merge and any backport noisier for zero behavior change. Worth doing as its own cleanup commit.

[vvlladd28]

Nit, not for this PR: the color / width / height / semanticLabel / onError quintet is now forwarded verbatim at six call sites inside this one method (three _onErrorImage, two _networkImage, one _imageFromBase64), and again down the _networkImage → _svgImageFromUrl → _onErrorImage chain. Every future parameter addition means touching a dozen places, and this diff — where two of those blocks were passing the wrong variable while the others were fine — is a small illustration of how easily they drift apart. Threading a small record or _ImageStyle holding those five fields through the private helpers would collapse the noise and make the branch structure, which is the actual logic here, visible at a glance.

[deaflynx]

Agreed the repetition is real, and not doing it here.

One correction on the motivation: the drift in this diff was the URL argument, not the style quintet — color/width/height/semanticLabel/onError were forwarded correctly in every branch, before and after. So this diff isn't evidence for the extraction. It's still a reasonable refactor for a dedicated PR, where it can be applied to CE and PE together.

[vvlladd28]

_isValidUrl is just Uri.tryParse(url) != null, which succeeds for nearly anything — the old tb-image;https://… value was one of the rare inputs that actually failed, which is why this bug looked like a missing branch rather than a wrong argument. With the prefix now correctly stripped, this predicate accepts relative paths too, and that turns out to be a reachable case rather than a hypothetical one:

• TbResourceInfo.getPublicLink() returns a relative /api/images/public/{key}, and the gallery surfaces exactly that string to the user (image-dialog.component.ts:135, embedded as <img src> in embed-image-dialog.component.ts:78).
• IMAGES_URL_REGEXP only matches tenant|system, not public, so a public link is classified as an external link, and the "Image link" field has no validators to stop it.
• Uri.tryParse('/api/images/public/abc123') → non-null with isAbsolute == false, so we hand a schemeless URL to Image.network. The fetch fails, the SVG retry fails, and the user gets the placeholder.

So "copy an image's public link from the gallery, paste it into Image link" renders fine on web and still shows the empty box on mobile after this fix — the same symptom you're fixing, one input away. Tightening the check to what the branch actually needs would cover it and stop the trailing else from being effectively dead code:

static bool _isValidUrl(String url) {
  final uri = Uri.tryParse(url);
  return uri != null && (uri.scheme == 'http' || uri.scheme == 'https');
}

Handling relative platform links properly (resolving them against the cached endpoint, as the gallery branch already does) would be the fuller fix, but is arguably out of scope here — happy either way, as long as it's a deliberate call.

[deaflynx]

Verified the whole chain and it holds:

• TbResourceInfo.getPublicLink() returns a relative /api/images/public/{key} (TbResourceInfo.java:132, asserted in ImageControllerTest.java:277)
• the Embed dialog hands users exactly that string as <img src="/api/images/public/…"> (embed-image-dialog.component.ts:78)
• IMAGES_URL_REGEXP matches only tenant|system, so it classifies as external
• the field really has no validators — externalLinkControl = new FormControl(null)
• Uri.tryParse('/api/images/public/abc123') → non-null, isAbsolute == false

So "copy a public link, paste it into Image link" showed the placeholder even with the original fix. Reproduced on a device, and fixed here rather than filed separately — it is the same reported symptom, so splitting it would have left PROD-8502 half closed.

Rather than only tightening the predicate, _isValidUrl is replaced by a helper that classifies and resolves in one step:

static String? _resolveNetworkImageLink(String url) {
  final uri = Uri.tryParse(url);
  if (uri == null) return null;
  if (uri.scheme == 'http' || uri.scheme == 'https') return url;
  if (!uri.hasScheme && url.startsWith('/')) {
    return getIt<IEndpointService>().getCachedEndpoint() + url;
  }
  return null;
}

Absolute http/https links are fetched as they are; platform-relative links resolve against the active endpoint the way a browser resolves them against its origin; anything else renders the missing-image placeholder, so that branch is now reachable for a reason rather than by accident. No auth header goes on the resolved relative links — /api/images/public/{key} is a noauth endpoint, and a browser <img src> does not carry the JWT either, since the Angular interceptor only adds it to XHR. Tenant and system links keep the authenticated path in the earlier branch.

Tightening alone would only have changed how it failed, so it was worth doing the resolution properly.

[vvlladd28]

Since this branch becomes reachable for the first time, it's worth looking at what it now pulls in: _networkImage calls Image.network with no cacheWidth/cacheHeight and no loadingBuilder. An "Image link" points at an arbitrary user-supplied image that the platform never resizes (unlike gallery resources, which have a server-side /preview variant), and the Home grid renders it into a small card via FittedBox(fit: BoxFit.cover) (dashboard_grid_card.dart:31) — so a multi-megapixel photo is decoded at full resolution and held in the image cache, once per dashboard. Deriving cacheWidth/cacheHeight from the requested width/height, or wrapping in ResizeImage, would bound that cheaply. I haven't measured it, so treat the size as an open question rather than a claim.

Separately on UX: these cards will now sit blank until the remote fetch completes, with no placeholder or fade-in. A loadingBuilder reusing the existing placeholder would keep that transient state from looking like the bug you just fixed.

[deaflynx]

The decode-cost concern is fair, but the proposed remedy doesn't apply to the case it cites: dashboard_grid_card.dart:31 calls imageFromTbImage with no width or height, and so do devices_base.dart:235 and :441. Only device_profiles_base.dart:326 passes width: 64. Deriving cacheWidth/cacheHeight from those parameters is therefore a no-op for the Home grid — the one place a multi-megapixel external image is squeezed into a small card.

Bounding it properly needs dimensions from the layout (or a hard cap) at the call site, which is a separate change from this bugfix. Leaving it out here rather than adding something that reads like a fix but isn't measurable — you flagged the sizing as unmeasured too.

The loadingBuilder point is reasonable and cheap; also leaving it for a follow-up so this PR stays a one-cause fix.

[vvlladd28]

Theoretical, and I'd understand skipping it — flagging it only because this line is what newly exposes it. _removeTbImagePrefix is url.replaceFirst(_tbImagePrefix, ''), which isn't anchored to position 0, so it strips the first occurrence wherever it sits:

'https://cdn.example.com/tb-image;logo.png'.replaceFirst('tb-image;', '')
// -> 'https://cdn.example.com/logo.png'   (404)

For anything written by the platform UI this can't bite, since prependTbImagePrefix only prepends when the prefix is absent, so the first occurrence is always the real prefix. It needs a value stored without the prefix that happens to contain tb-image; — reachable only via direct REST or import, because the backend doesn't force the prefix. Before this change the branch used the raw imageUrl and was immune; now it isn't.

Anchoring it is free and makes the helper say what it means:

static String _removeTbImagePrefix(String url) =>
    url.startsWith(_tbImagePrefix) ? url.substring(_tbImagePrefix.length) : url;

For the record, the platform's own removeTbImagePrefix (resource.models.ts:168) has the same unanchored shape, so leaving it as-is keeps mobile and web bug-compatible — a defensible choice, just worth making deliberately now that the blast radius grew from one branch to three.

[deaflynx]

Anchored:

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

Confirmed the old behavior on 'https://cdn.example.com/tb-image;logo.png' → 'https://cdn.example.com/logo.png'. Accepting the divergence from the web's removeTbImagePrefix deliberately: it's strictly safer and unobservable for anything the platform UI writes, and the blast radius did grow from one branch to three here.

[deaflynx]

_isImageResourceUrl unanchored (utils.dart:293). The misclassification is real and anchoring is the right fix, so filing it — with one correction and one ordering constraint.

The correction: there is no header leak. I ran the concatenation, and https://mytb.example.comhttps://mytb.example.com/api/images/tenant/logo.png parses with host == "mytb.example.comhttps" — an unresolvable name that is not attacker-controlled, and stays unresolvable for a foreign-host input too (https://evil.com/... yields the same garbled host). The JWT never reaches a third party; the request just fails. It's a broken-render bug, not a credential disclosure.

The ordering constraint, which matters more: PE's copy of this method tests the prefixed value here (_isImageResourceUrl(imageUrl)), which the unanchored regexp currently forgives. Anchoring with ^ before PE is brought in line would classify every gallery image as external and break all dashboard thumbnails in PE. The consistency fix has to land first — it's included in the PE-side change for this PR.

Server-side /preview (ImageController:216). Confirmed it exists and that the web opts in via getImageDataUrl(imageUrl, preview, …). Real saving for the Home grid; filing separately, since picking it up needs a check that the preview variant is large enough for the card at high DPR.

Test coverage. The gap is real, and this PR does not close it — the repo still has no test suite, which stays a separate discussion about whether this project takes one on.

Every change here was developed against a throwaway suite of twelve cases covering all four dispatch branches — prefixed and unprefixed external links, base64 asserted on decoded bytes, gallery resource with auth header, key URL-encoding, the whitelabel login-logo path, public and generic relative links, the anchored-strip case, and the placeholder fallbacks — each verified to fail against the pre-fix code rather than pass vacuously. It is not part of the commit.

Your suggestion to extract the pure classify-and-resolve step is partly taken up: _resolveNetworkImageLink is exactly that shape for the network branch, returning a resolved link or null instead of fusing the decision into widget construction. Pulling the remaining branches out the same way is a larger refactor of a method four call sites depend on across CE and PE, and belongs in its own change.

[vvlladd28]

Re-review summary

Re-reviewed fix(dashboard): display dashboard images configured via external URL link — verified 8 finding(s) from the previous review (5 inline + 3 in the body).

Status	Count
✅ Resolved	3
💬 Acknowledged	5
❌ Unresolved	0

One new inline comment on the fix commit, and it's a doc-wording nit. I initially had three; the other two didn't survive checking them against the platform source, and I've written up why below rather than leaving them in — the fix itself is correct and I'd merge it.

Verified against the server while re-reviewing, all of it supporting the change as written:

• /api/images/public/** is in NON_TOKEN_BASED_AUTH_ENTRY_POINTS (ThingsboardSecurityConfiguration.java:162), so resolving public links without the JWT is right, not a lucky omission.
• getPublicLink() returns "/api/images/public/" + publicResourceKey (TbResourceInfo.java:151), and the key is RandomStringUtils.secure().nextAlphanumeric(32) (BaseImageService.java:207) — always rooted, always alphanumeric. So this branch genuinely doesn't need the last-segment Uri.encodeComponent the gallery branch does, and it can never receive a protocol-relative value from the platform.
• prependTbImagePrefix prepends unconditionally when the prefix is absent (resource.models.ts:199), which is the root cause in the description — confirmed.

Finding details

Inline findings

• ✅ lib/utils/utils.dart:118 — newImageUrl is an uninformative name now that every branch consumes it — Renamed to resolvedImageUrl. The flatten half was declined for a good reason (rewriting every line of the method on an LTS branch for zero behaviour change makes the CE→PE merge and any backport noisier); agreed that belongs in its own cleanup commit.
• 💬 lib/utils/utils.dart:122 — the color/width/height/semanticLabel/onError quintet is forwarded verbatim at six call sites — Deferred to a dedicated PR so it can be applied to CE and PE together. Their correction is right and I checked it: the quintet was forwarded correctly in every branch before and after; the drift in that diff was the URL argument. So that diff wasn't evidence for the extraction — the refactor stands on its own merits.
• ✅ lib/utils/utils.dart:128 — _isValidUrl accepts nearly anything, and relative public links reach Image.network — Fixed, and fixed more thoroughly than suggested: _isValidUrl is gone, replaced by _resolveNetworkImageLink, which resolves platform-relative links against the cached endpoint instead of only rejecting them. The whole chain checks out, and the server side above confirms the no-auth-header decision. Folding this into PROD-8502 rather than filing it separately was the right call.
• 💬 lib/utils/utils.dart:129 — _networkImage has no cacheWidth/cacheHeight or loadingBuilder — Their rebuttal is correct and I verified it in the worktree: dashboard_grid_card.dart:31, devices_base.dart:235 and :441 pass no width/height at all, and only device_profiles_base.dart:326 passes width: 64. Deriving cache dimensions from those parameters really would be a no-op for the Home grid — the one place a multi-megapixel external image is squeezed into a small card. loadingBuilder deferred to a follow-up.
• ✅ lib/utils/utils.dart:131 — _removeTbImagePrefix used an unanchored replaceFirst — Anchored with startsWith/substring. Deliberately diverging from the web's removeTbImagePrefix, which is the right way round: strictly safer, and unobservable for anything the platform UI writes.

Body findings

• 💬 lib/utils/utils.dart:297 — _isImageResourceUrl is unanchored, so an absolute URL containing /api/images/tenant/… is claimed by the gallery branch — Filed separately, with a PE-ordering constraint I hadn't accounted for: PE's copy tests the prefixed value here, which the unanchored regexp forgives, so anchoring before PE is aligned would classify every gallery image as external and break all PE thumbnails. And their correction to my "header leak" claim is right — I reran it: https://mytb.example.comhttps://mytb.example.com/api/images/tenant/logo.png parses with host == "mytb.example.comhttps", and a foreign host garbles identically, so the JWT never leaves the device. It's a broken render, not credential disclosure. My original wording overstated it.
• 💬 ImageController /preview — the mobile app always downloads the full-size original — Confirmed present and filed separately, gated on checking the preview variant is large enough for the card at high DPR. Fair.
• 💬 No test coverage anywhere in the repo — Still open, and correctly scoped out of this PR — the repo has no suite to add to, which is a separate decision. Worth recording that the changes here were developed against a throwaway twelve-case suite covering all four dispatch branches, and that the suggestion to extract the pure classify-and-resolve step was partly taken up — _resolveNetworkImageLink is exactly that shape for the network branch.

Additional findings

This one is about existing code outside the PR's diff, and is the reason two of my inline comments came back out.

• lib/utils/utils.dart:106 and :314 — the cached endpoint can carry a trailing slash, and both concatenation sites assume it can't. formatBaseUrl (DefaultSystemSecurityService.java:404) only guarantees a scheme, it does not strip a trailing slash; the admin Base URL field is baseUrl: ['', [Validators.required]] with no pattern (general-settings.component.ts:86); and that value is handed to the app verbatim as deepLink + "&host=" + baseUrl (QrCodeSettingsController.java:199). So an admin who types https://mytb.example.com/ gets https://mytb.example.com//api/images/... out of both sites. TB defends against exactly this elsewhere — TrendzClient.java:355 strips a trailing slash from its base URL before use — so it's a known-real shape rather than a hypothetical. I have not verified what the server does with the doubled slash, so treat the impact as open.

  Deliberately not asking for a change in this PR: line 106 has behaved this way since long before it, and fixing only the new branch would leave the two paths resolving differently. The same applies to the resolveAgainstEndpoint(String path) helper on IEndpointService worth extracting here — home_dashboard_page.dart:40 and dashboard_widget.dart:191 interpolate the endpoint the same way, so that's four call sites for one ticket. Passing the endpoint into _resolveNetworkImageLink as a parameter rather than reaching for getIt becomes the natural shape at that point too; on its own today it would only make the helper inconsistent with the getIt lookup twenty lines above it in the same method.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[vvlladd28]

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]

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

[vvlladd28]

Re-review summary

Re-reviewed fix(dashboard): display dashboard images configured via external URL link — verified the 1 finding from the previous review.

Status	Count
✅ Resolved	1
💬 Acknowledged	0
❌ Unresolved	0

All previous findings are addressed. One new inline comment on the fix commit, and it needs no code change — it is about one clause of the reworded sentence, not about behaviour. The fix itself is correct and I would merge it.

This round I went back over every finding raised across all three reviews and checked it against the server, the web UI, the PE mobile app and the Spring Security source, rather than re-stating earlier conclusions. Two of those earlier conclusions were mis-scored and are corrected below.

Finding details

• ✅ lib/utils/utils.dart:301 — the doc comment claimed "Absolute links are fetched as they are" while the gate admits only http/https, making the restriction read like an oversight — Fixed in 1eb4637: reworded to "HTTP(S) links are fetched as they are". Checked the new sentence against the helper rather than taking the reply at its word — the first clause now matches the scheme gate exactly, and it is strictly more accurate than the original, which implied ftp: and file: would be fetched. The remaining inaccuracy is in the browser clause, commented inline.

Review of the fix commit

c7a0292..1eb4637 is one commit touching one comment line, with no code change — confirmed through the compare endpoint, not just the commit message. A dedicated quality pass over it came back empty. I re-read the surrounding method to be sure the reword had not drifted from the code: the gallery branch still attaches Authorization (utils.dart:111), the new external branch still passes no headers (utils.dart:140-148).

Verification of the fix, against the platform

Every link in the chain the description claims, confirmed in source:

• getPublicLink() returns a relative /api/images/public/{key} — TbResourceInfo.java:151, asserted in ImageControllerTest.java:319
• the Embed dialog hands users that string verbatim — embed-image-dialog.component.ts:93
• IMAGES_URL_REGEXP = /\/api\/images\/(tenant|system)\/(.*)/ matches only tenant/system — resource.models.ts:185
• the field really has no validators — externalLinkControl = new FormControl(null), gallery-image-input.component.ts:119
• /api/images/public/** is in NON_TOKEN_BASED_AUTH_ENTRY_POINTS and permitAll() — ThingsboardSecurityConfiguration.java:162 and :397. Resolving public links without the JWT is correct by design, not a lucky omission.
• the public key is RandomStringUtils.secure().nextAlphanumeric(32) — BaseImageService.java:207. Always rooted, always alphanumeric, so this branch genuinely does not need the last-segment Uri.encodeComponent the gallery branch does.
• prependTbImagePrefix prepends unconditionally when absent — resource.models.ts:199. Root cause as described.

Deciding to resolve platform-relative links rather than merely reject them was the right call: rejecting alone would have left the reported symptom — paste a public link, get an empty box — unfixed.

Additional findings

All three are about code outside this PR's diff. None of them should change this PR; two of them correct a score I gave earlier.

• Correction — the trailing-slash issue is a hard 400, not a cosmetic double slash. I previously left the impact open. It resolves: Spring Security's default StrictHttpFirewall blocklists the literal "//" in both the encoded and decoded URL blocklists (StrictHttpFirewall.java:103-104, added in the constructor at :157, thrown at :545-551), and the platform registers no custom HttpFirewall. So https://host//api/images/tenant/x.png is rejected with RequestRejectedException, not silently normalised — every image 400s. The path there is unguarded end to end: formatBaseUrl only prepends a scheme (DefaultSystemSecurityService.java:404-409), the admin field is baseUrl: ['', [Validators.required]] with no pattern (general-settings.component.ts:86), the value is handed over verbatim as deepLink + "&host=" + baseUrl (QrCodeSettingsController.java:199), and EndpointService.setEndpoint stores it unnormalised. Reachability is narrower than it first looks — it needs both a trailing slash in Base URL and a custom app domain, since &host= is only appended when !appDomain.equals(platformDomain). Still correctly out of scope here, since utils.dart:106 has behaved this way since long before this PR and fixing only the new branch would leave the two paths resolving differently, but the ticket deserves a higher priority than "informational". On the fix: one trailing-slash strip in setEndpoint covers all four building sites at once (utils.dart:106, :314, dashboard_widget.dart:191, home_dashboard_page.dart:40). I withdraw the resolveAgainstEndpoint(String path) helper I floated last round — normalising at the concatenation sites would leave the dirty value in the cache and is the worse shape.

• Correction — the ImageController /preview follow-up should be closed, not implemented. The endpoint exists (ImageController.java:266), but the preview is capped at 250 px: ImageUtils.processImage(data, descriptor.getMediaType(), 250), hardcoded at BaseImageService.java:188. The Home card is 156×150 logical (pagination_grid_widget.dart:41, childAspectRatio: 156 / 150) less the 44 px title strip, under FittedBox(fit: BoxFit.cover) — roughly 468 physical px wide at DPR 3. A 250 px preview would be upscaled and visibly softer, so switching to it trades bandwidth for worse thumbnails. /preview also only exists for gallery images, so it never applies to the external and public links this PR is about. The bandwidth observation stands; the proposed remedy does not. The useful version of it is the cacheWidth cap below.

• The _isImageResourceUrl anchoring deferral holds, and the PE constraint is real. I expected this claim not to survive, because the web strips the prefix before testing (image.service.ts:191-197). It survives on the mobile side: PE's flutter_thingsboard_pe_app (master, lib/utils/utils.dart:105) calls _isImageResourceUrl(imageUrl) on the raw prefixed value, where CE's base uses the stripped one, and RegExp('/api/images/(tenant|system)/(.*)').hasMatch('tb-image;/api/images/tenant/x.png') is true — verified on Dart 3.29.0. So the unanchored regexp is what keeps PE working, and anchoring before PE:105 is aligned would classify every PE gallery image as external. Sequence the ticket that way round. Severity stays low: I re-confirmed that https://mytb.example.com + https://evil.com/api/images/tenant/x.png parses with host == "mytb.example.comhttps", so the JWT never leaves the device — a broken render, not credential disclosure.

On the two deferrals I am not re-raising: the color/width/height/semanticLabel/onError extraction is real duplication but weak value as a standalone PR — a new type for six call sites of one private method — and is worth folding into the same cleanup commit as the if/else flatten rather than shipping separately. The cacheWidth/cacheHeight concern is real and your rebuttal was right: dashboard_grid_card.dart:31 passes no width/height at all, so deriving cache dimensions from those parameters is a no-op. The workable form is a hard cap around 500-600 px (or LayoutBuilder), which is a line or two, and belongs in the follow-up.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[vvlladd28]

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]

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.

[vvlladd28]

Re-review summary

Re-reviewed fix(dashboard): display dashboard images configured via external URL link — verified the 1 finding from the previous review.

Status	Count
✅ Resolved	1
💬 Acknowledged	0
❌ Unresolved	0

Also one new inline comment on the fix commit. It is a single wording observation on the doc comment and needs no change — recorded so it is not re-raised later rather than because it should be acted on. The code is unchanged since the last round and I would merge this.

Finding details

• ✅ lib/utils/utils.dart:303 — the comment claimed platform-relative links resolve "the same way a browser resolves them against its origin", which does not hold for a protocol-relative //host/path value — Fixed in 23eb34e: the browser clause is dropped, leaving "are resolved against the active endpoint". That is exactly the repair I suggested, and it is the right one — changing the code to match the old sentence would have made the app fetch from the foreign host, which is worse than the broken render. Behaviour is unchanged, as the reply states; I confirmed the fix commit touches nothing but the comment.

Review of the fix commit

1eb4637..23eb34e is one commit, three comment lines rewrapped, no code change — confirmed through the compare endpoint rather than from the commit message. The helper body at utils.dart:305-317 is byte-identical to the revision I verified against the platform last round, so none of that verification needs redoing.

The comment now maps cleanly onto the three exits: http/https returns url unchanged, a scheme-less path starting with / is concatenated onto getCachedEndpoint(), and everything else returns null for the caller to render as _onErrorImage.

Additional findings

Nothing new in this PR. Three items carried over, none of which should change it:

• The PE ordering constraint still applies, and the PE-side change has not landed. flutter_thingsboard_pe_app origin/master still reads _isImageResourceUrl(imageUrl) on the raw prefixed value (lib/utils/utils.dart:105), where CE uses the stripped one. So anchoring _isImageResourceUrl with ^ still has to wait for that consistency fix, exactly as the earlier comment said. Flagging only that the prerequisite is still outstanding, so the anchoring ticket does not get picked up first by whoever takes it.

• The trailing-slash issue stands at the priority I corrected it to last round — a hard 400 rather than a cosmetic double slash, since Spring Security's default StrictHttpFirewall blocklists the literal "//" and the platform registers no custom firewall. Still correctly out of scope here, since utils.dart:106 predates this PR. The one-line strip in EndpointService.setEndpoint remains the right shape.

• The /preview follow-up is still the one to close rather than implement — and the case is stronger than I put it last round, where I derived the card size wrongly by reading the 156 in childAspectRatio: 156 / 150 as a logical width rather than a ratio. The real slot comes from (screenWidth - 48) / 2 with crossAxisCount: 2 (pagination_grid_widget.dart:31-46, and dashboards_grid.dart:31 takes that default): roughly 478×341 physical px on a Pixel 7, 513×358 on an iPhone 14, 637×455 on a Pixel 7 Pro at DPR 3.5. Against a preview capped at 250 px on the longest side (BaseImageService.java:188), adopting it would mean upscaling by 2.0-2.5× on current phones, so it trades bandwidth for visibly softer thumbnails. By the same correction, the cacheWidth cap I suggested at 500-600 px should be nearer 640 to cover high-DPR devices — or better, taken from LayoutBuilder and devicePixelRatio rather than hardcoded.

One process note while reading through the discussion: all seven review threads on this PR are still open on GitHub, five of them marked outdated. Every one of them has been answered in a reply, so this is only about the resolved flag — worth closing them out so the next reader can tell what is actually still live.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[vvlladd28]

One imprecision left in the reworded sentence, and I am recording it rather than asking for anything — I said last round that a single imprecise clause was not worth a third pass at this comment, and that still applies. Take it only if you touch the file again.

"Links relative to the platform" is a shade broader than the gate below it. !uri.hasScheme && url.startsWith('/') accepts only root-relative links, so api/images/public/{key} without the leading slash falls through to null, as do images/logo.png, ./logo.png and ../logo.png — checked on Dart 3.29.0. That is the correct behaviour, since getPublicLink() and getLink() both return rooted paths and the platform never emits the other form, but "root-relative" would tell the next reader exactly where the boundary sits.

I also had a second wording point — that the closing sentence describes what the caller does with null rather than what this function returns — and dropped it after checking: the helper has exactly one call site (utils.dart:129), so describing the end-to-end behaviour is more useful here than documenting the null contract, and changing it would not be an improvement.

[deaflynx]

Fixed in 6e4f884 — reworded to "Root-relative links" so the sentence matches the leading-slash gate.

[vvlladd28]

Re-review summary

Re-reviewed fix(dashboard): display dashboard images configured via external URL link — verified the 1 finding from the previous review.

Status	Count
✅ Resolved	1
💬 Acknowledged	0

All previous findings have been addressed, and the fix commit is comment-only, so there are no inline comments this round. Ready to merge from my side.

This round I also re-validated every carried-over item against the platform sources rather than restating it, and two of the three turned out to be worth closing rather than deferring. Details in Additional findings.

Finding details

• ✅ lib/utils/utils.dart:301 — the doc comment said "Links relative to the platform", one notch broader than the !uri.hasScheme && url.startsWith('/') gate below it, which accepts only rooted paths — Fixed in 6e4f884: it now reads "Root-relative links", exactly the wording suggested. Confirmed through the compare endpoint that the commit touches nothing but those comment lines.

Review of the fix commit

23eb34e..6e4f884 is one commit: the four-line doc comment rewrapped to three, no code change — confirmed through the compare endpoint rather than from the commit message. The helper body at utils.dart:304-316 is byte-identical to the revision verified in the earlier rounds, so none of that verification needs redoing.

This time I ran the classifier rather than reading it, on Dart 3.7.0 (the SDK in the Flutter version this repo pins):

Input	_resolveNetworkImageLink
https://example.com/pic.png	returned unchanged
HTTPS://example.com/pic.png	returned unchanged — Dart normalizes the scheme, so uppercase works
/api/images/public/abc	<endpoint>/api/images/public/abc
//evil.example.com/pic.png	<endpoint>//evil.example.com/pic.png
ftp://h/x.png, api/images/public/abc, ./logo.png, ../logo.png, http://[, free text	null → _onErrorImage

So the comment now maps cleanly onto the three exits, and the scheme-less-but-not-rooted forms fall through to the placeholder as the wording implies.

The one case the comment does not cover is the protocol-relative //host/path form, which the gate also accepts. That is not a defect to fix — leave it as it is. The concatenation sends it to the platform host, whose request URI then contains a literal //; Spring Security's default StrictHttpFirewall blocklists exactly that string (FORBIDDEN_DOUBLE_FORWARDSLASH in StrictHttpFirewall.java:103-104, rejected in rejectedBlocklistedUrls, :545-551), and the platform registers no custom HttpFirewall anywhere, so the result is a 400 and a placeholder. Making the resolver browser-accurate here would instead fetch the image from an attacker-supplied host, which is strictly worse than a blank thumbnail. Recording it as closed on the merits, not as a deferred nit.

Additional findings

Nothing new in the code. On the three carried-over items, re-verified against the platform rather than restated:

• Anchoring _imagesUrlRegexp with ^ — I am closing this rather than deferring it, which reverses my earlier recommendation. The web UI's own regexp is unanchored in the same way (IMAGES_URL_REGEXP = /\/api\/images\/(tenant|system)\/(.*)/, resource.models.ts:185), and ImageService.resolveImageUrl strips the prefix and then tests the stripped value (image.service.ts:192-198) — which is precisely what this PR makes the mobile app do. So CE mobile is now consistent with the platform, and anchoring would make it diverge unilaterally. The PE ordering constraint is real (PE origin/master still tests the raw prefixed value at utils.dart:105, so ^ would break gallery images there), but it no longer matters if the change should not happen at all. The only misclassification ^ prevents is an external URL that itself contains /api/images/tenant/…, which does not occur in practice and which the web mishandles identically. Keeping a coordinated three-repository change alive for that is not worth it; if it is ever worth fixing, it starts in the platform, not in the mobile client.

• Trailing-slash endpoint — real, one line, and worth its own small PR. EndpointService.setEndpoint (endpoint_service.dart:24-37) stores whatever it is handed with no normalization and getCachedEndpoint returns it verbatim; a trailing slash is reachable through the build-time String.fromEnvironment define (app_constants.dart:4) for self-hosted flavors and through the host of a noauth link (noauth_provider.dart:87), while the region presets are clean. With such an endpoint the concatenation yields …com//api/images/public/abc — confirmed by running it — which the firewall above rejects with a 400. What makes this worth a ticket is the asymmetry I had not pinned down before: dio quietly normalizes the same mistake away (RequestOptions.uri does s[1].replaceAll('//', '/') then .normalizePath(), dio/options.dart:628-643), so login succeeds and all data loads, and only the hand-concatenated image URLs break. That is why it would present as "images are blank on this one deployment" rather than as a broken endpoint. Correctly out of scope here — the same concatenation at utils.dart:106 predates this PR — and low priority, since it needs a misconfigured endpoint to trigger. The one-line strip in setEndpoint remains the right shape and the right place.

• /preview — closing this one too, and the evidence against it is now conclusive. There is no preview variant for public links at all: /preview exists only on IMAGE_URL (ImageController.java:266), while downloadPublicImage (:207) has neither a preview route nor a preview flag. So it could not serve the public-link case this PR fixes, nor external URLs — only the tenant/system branch. On top of that the preview is capped at 250 px on the longest side (BaseImageService.java:188 → ImageUtils.processImage(data, mediaType, 250); small SVGs bump to 512, ImageUtils.java:198-201), against a card slot of roughly 478 physical px wide on a Pixel 7 — the geometry being padding 16, spacing 16, crossAxisCount: 2, childAspectRatio: 156 / 150 (pagination_grid_widget.dart:34-46). That is a ~1.9× upscale. One of three branches, softer thumbnails, plus a URL-rewriting rule: not worth it.

  The cacheWidth half of that idea stands on its own and is unrelated to /preview. The decode cost of a multi-megapixel external image into a small card is real, and the earlier reply was right that deriving the cap from the existing width/height parameters would be a no-op here, since the card passes neither (dashboard_grid_card.dart:31). Doing it properly means taking the bound from LayoutBuilder and devicePixelRatio at the call site — a separate performance change, and worth doing only if memory pressure is actually observed.

Three things I checked this round and am explicitly not raising, so they do not resurface:

• The new relative branch does not Uri.encodeComponent the last path segment while the gallery branch does. Not a problem: public keys are RandomStringUtils.secure().nextAlphanumeric(32) (BaseImageService.java:207-209), and any /api/images/tenant|system/… value is routed to the gallery branch, which does encode.
• Omitting the auth header on resolved relative links is correct, not an oversight: /api/images/public/** is listed in NON_TOKEN_BASED_AUTH_ENTRY_POINTS (ThingsboardSecurityConfiguration.java:162).
• Uppercase schemes are handled, per the table above.

One process note, unchanged from last round: all eight review threads on this PR are open, seven of them marked outdated, and every one already has a reply. This is only about the resolved flag — worth closing them out so the next reader can tell what is still live.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.