fix(favorites): normalize scheme-less URLs so favicons resolve
A favorite entered as a bare host ("ctl.de") passed @IsUrl() but had no
scheme, so `new URL()` threw inside icon discovery and it silently fell
back to a relative "/favicon.ico" — which 502'd through the icon proxy
and left the widget showing the first-letter placeholder ("C").
- add normalizeUrl() (prepend https:// when no scheme present)
- apply it in discoverFavoriteIconUrl and when storing the favorite url,
so both the link and discovery use the normalized value
- on update, re-run discovery when the icon field is cleared, so editing
a previously-broken favorite repairs its icon
- tests: normalizeUrl cases + end-to-end discovery (apple-touch extraction,
scheme-less fallback stays absolute)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -8,7 +8,7 @@ import {
|
|||||||
import { PrismaService } from '../prisma/prisma.service';
|
import { PrismaService } from '../prisma/prisma.service';
|
||||||
import { CreateFavoriteDto } from './dto/create-favorite.dto';
|
import { CreateFavoriteDto } from './dto/create-favorite.dto';
|
||||||
import { UpdateFavoriteDto } from './dto/update-favorite.dto';
|
import { UpdateFavoriteDto } from './dto/update-favorite.dto';
|
||||||
import { IconDiscoveryService } from './icon-discovery.service';
|
import { IconDiscoveryService, normalizeUrl } from './icon-discovery.service';
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Service for managing per-user, per-widget favorite links.
|
* Service for managing per-user, per-widget favorite links.
|
||||||
@@ -42,11 +42,14 @@ export class FavoritesService {
|
|||||||
* If iconUrl is not provided, triggers server-side icon discovery with SSRF protection.
|
* If iconUrl is not provided, triggers server-side icon discovery with SSRF protection.
|
||||||
*/
|
*/
|
||||||
async create(userId: string, tenantId: string, dto: CreateFavoriteDto) {
|
async create(userId: string, tenantId: string, dto: CreateFavoriteDto) {
|
||||||
|
// Normalize so a scheme-less entry like "ctl.de" is stored (and discovered)
|
||||||
|
// as "https://ctl.de" — otherwise the link and icon discovery both break.
|
||||||
|
const url = normalizeUrl(dto.url);
|
||||||
let iconUrl = dto.iconUrl ?? null;
|
let iconUrl = dto.iconUrl ?? null;
|
||||||
|
|
||||||
// Server-side icon discovery (D-05) — only when caller did not supply an icon
|
// Server-side icon discovery (D-05) — only when caller did not supply an icon
|
||||||
if (!iconUrl) {
|
if (!iconUrl) {
|
||||||
iconUrl = await this.iconDiscovery.discoverFavoriteIconUrl(dto.url);
|
iconUrl = await this.iconDiscovery.discoverFavoriteIconUrl(url);
|
||||||
}
|
}
|
||||||
|
|
||||||
return this.prisma.favoriteLink.create({
|
return this.prisma.favoriteLink.create({
|
||||||
@@ -55,7 +58,7 @@ export class FavoritesService {
|
|||||||
tenantId,
|
tenantId,
|
||||||
widgetId: dto.widgetId,
|
widgetId: dto.widgetId,
|
||||||
title: dto.title,
|
title: dto.title,
|
||||||
url: dto.url,
|
url,
|
||||||
iconUrl,
|
iconUrl,
|
||||||
position: dto.position ?? 0,
|
position: dto.position ?? 0,
|
||||||
},
|
},
|
||||||
@@ -77,10 +80,26 @@ export class FavoritesService {
|
|||||||
const data: Record<string, unknown> = {};
|
const data: Record<string, unknown> = {};
|
||||||
|
|
||||||
if (dto.title !== undefined) data.title = dto.title;
|
if (dto.title !== undefined) data.title = dto.title;
|
||||||
if (dto.url !== undefined) data.url = dto.url;
|
|
||||||
if ('iconUrl' in dto) data.iconUrl = dto.iconUrl; // Allows explicit null
|
const normalizedUrl =
|
||||||
|
dto.url !== undefined ? normalizeUrl(dto.url) : undefined;
|
||||||
|
if (normalizedUrl !== undefined) data.url = normalizedUrl;
|
||||||
|
|
||||||
if (dto.position !== undefined) data.position = dto.position;
|
if (dto.position !== undefined) data.position = dto.position;
|
||||||
|
|
||||||
|
if ('iconUrl' in dto) {
|
||||||
|
if (dto.iconUrl) {
|
||||||
|
// Explicit icon URL supplied — respect it as-is.
|
||||||
|
data.iconUrl = dto.iconUrl;
|
||||||
|
} else {
|
||||||
|
// Icon cleared (empty/null) — re-run discovery against the effective
|
||||||
|
// (new or existing) url so editing a broken favorite repairs its icon.
|
||||||
|
const effectiveUrl = normalizedUrl ?? link.url;
|
||||||
|
data.iconUrl =
|
||||||
|
await this.iconDiscovery.discoverFavoriteIconUrl(effectiveUrl);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
return this.prisma.favoriteLink.update({
|
return this.prisma.favoriteLink.update({
|
||||||
where: { id },
|
where: { id },
|
||||||
data,
|
data,
|
||||||
|
|||||||
@@ -1,5 +1,9 @@
|
|||||||
import { afterEach, describe, expect, it, vi } from 'vitest';
|
import { afterEach, describe, expect, it, vi } from 'vitest';
|
||||||
import { IconDiscoveryService, isPublicHttpUrl } from './icon-discovery.service';
|
import {
|
||||||
|
IconDiscoveryService,
|
||||||
|
isPublicHttpUrl,
|
||||||
|
normalizeUrl,
|
||||||
|
} from './icon-discovery.service';
|
||||||
|
|
||||||
function mockResponse(options: {
|
function mockResponse(options: {
|
||||||
contentType?: string;
|
contentType?: string;
|
||||||
@@ -46,6 +50,71 @@ describe('isPublicHttpUrl', () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('normalizeUrl', () => {
|
||||||
|
it('prepends https:// to a scheme-less host', () => {
|
||||||
|
expect(normalizeUrl('ctl.de')).toBe('https://ctl.de');
|
||||||
|
expect(normalizeUrl('www.ctl.de/path')).toBe('https://www.ctl.de/path');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('leaves an existing scheme untouched', () => {
|
||||||
|
expect(normalizeUrl('http://ctl.de')).toBe('http://ctl.de');
|
||||||
|
expect(normalizeUrl('https://ctl.de')).toBe('https://ctl.de');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('trims surrounding whitespace', () => {
|
||||||
|
expect(normalizeUrl(' ctl.de ')).toBe('https://ctl.de');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns empty string unchanged', () => {
|
||||||
|
expect(normalizeUrl(' ')).toBe('');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('IconDiscoveryService.discoverFavoriteIconUrl', () => {
|
||||||
|
afterEach(() => {
|
||||||
|
vi.restoreAllMocks();
|
||||||
|
vi.unstubAllGlobals();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('extracts the apple-touch-icon from page HTML', async () => {
|
||||||
|
const html = `<html><head>
|
||||||
|
<link rel="icon" href="https://ctl.de/fav-32.jpg" sizes="32x32" />
|
||||||
|
<link rel="apple-touch-icon" href="https://ctl.de/apple-180.jpg" />
|
||||||
|
</head></html>`;
|
||||||
|
vi.stubGlobal(
|
||||||
|
'fetch',
|
||||||
|
vi.fn().mockResolvedValue({
|
||||||
|
ok: true,
|
||||||
|
status: 200,
|
||||||
|
headers: {
|
||||||
|
get: (n: string) =>
|
||||||
|
n.toLowerCase() === 'content-type'
|
||||||
|
? 'text/html; charset=utf-8'
|
||||||
|
: null,
|
||||||
|
},
|
||||||
|
text: async () => html,
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
|
||||||
|
const service = new IconDiscoveryService();
|
||||||
|
// Public IP avoids a real DNS lookup in the SSRF guard.
|
||||||
|
const icon = await service.discoverFavoriteIconUrl('http://8.8.8.8');
|
||||||
|
|
||||||
|
expect(icon).toBe('https://ctl.de/apple-180.jpg');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('normalizes a scheme-less URL so the fallback is absolute, not "/favicon.ico"', async () => {
|
||||||
|
// fetch fails → discovery falls back. The fallback must be an absolute
|
||||||
|
// https origin URL, not the broken relative path that produced the bug.
|
||||||
|
vi.stubGlobal('fetch', vi.fn().mockRejectedValue(new Error('network')));
|
||||||
|
|
||||||
|
const service = new IconDiscoveryService();
|
||||||
|
const icon = await service.discoverFavoriteIconUrl('ctl.de');
|
||||||
|
|
||||||
|
expect(icon).toBe('https://ctl.de/favicon.ico');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('IconDiscoveryService.fetchIconBytes', () => {
|
describe('IconDiscoveryService.fetchIconBytes', () => {
|
||||||
afterEach(() => {
|
afterEach(() => {
|
||||||
vi.restoreAllMocks();
|
vi.restoreAllMocks();
|
||||||
|
|||||||
@@ -124,6 +124,21 @@ export async function isPublicHttpUrl(url: URL): Promise<boolean> {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Normalize a user-entered site URL by prepending https:// when no scheme is
|
||||||
|
* present, so "ctl.de" becomes "https://ctl.de". Without this, `new URL()`
|
||||||
|
* throws on a bare host and icon discovery silently falls back to a broken
|
||||||
|
* relative "/favicon.ico" (which then 502s through the icon proxy and the
|
||||||
|
* widget shows the first-letter placeholder instead of the real favicon).
|
||||||
|
*/
|
||||||
|
export function normalizeUrl(raw: string): string {
|
||||||
|
const trimmed = raw.trim();
|
||||||
|
if (!trimmed) return trimmed;
|
||||||
|
// Already has a scheme (http://, https://, ftp://, ...) — leave untouched.
|
||||||
|
if (/^[a-z][a-z0-9+.-]*:\/\//i.test(trimmed)) return trimmed;
|
||||||
|
return `https://${trimmed}`;
|
||||||
|
}
|
||||||
|
|
||||||
function getOriginFaviconUrl(pageUrl: string): string {
|
function getOriginFaviconUrl(pageUrl: string): string {
|
||||||
try {
|
try {
|
||||||
const url = new URL(pageUrl);
|
const url = new URL(pageUrl);
|
||||||
@@ -301,10 +316,11 @@ export class IconDiscoveryService {
|
|||||||
* private IP ranges, blocked hostnames, and forced-proxy vectors (T-08-05).
|
* private IP ranges, blocked hostnames, and forced-proxy vectors (T-08-05).
|
||||||
*/
|
*/
|
||||||
async discoverFavoriteIconUrl(pageUrl: string): Promise<string> {
|
async discoverFavoriteIconUrl(pageUrl: string): Promise<string> {
|
||||||
const fallback = getOriginFaviconUrl(pageUrl);
|
const normalized = normalizeUrl(pageUrl);
|
||||||
|
const fallback = getOriginFaviconUrl(normalized);
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const url = new URL(pageUrl);
|
const url = new URL(normalized);
|
||||||
const htmlResult = await fetchHtml(url);
|
const htmlResult = await fetchHtml(url);
|
||||||
|
|
||||||
if (!htmlResult) return fallback;
|
if (!htmlResult) return fallback;
|
||||||
|
|||||||
Reference in New Issue
Block a user