Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for storing custom icons directly in the database for template repositories. Icons can be provided as either SVG content or HTTPS URLs, with validation applied to prevent malicious content and ensure proper formatting.
Changes:
- Added database column and migration for storing icon content
- Implemented validation logic for SVG content sanitization and HTTPS URL verification
- Updated all API endpoints to include icon field in database queries and responses
- Modified frontend components to render custom icons with fallback to static assets
- Updated TypeScript types across all layers to include the optional icon field
Reviewed changes
Copilot reviewed 38 out of 41 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/providers/devbox/prisma/schema.prisma | Added optional icon field to TemplateRepository model |
| frontend/providers/devbox/prisma/migrations/20260205000000_add_template_repository_icon/migration.sql | Database migration to add icon column |
| frontend/providers/devbox/utils/validate.ts | Added validation functions for icon content with security checks |
| frontend/providers/devbox/components/RuntimeIcon.tsx | Enhanced to render SVG content or HTTPS URLs with fallback handling |
| frontend/providers/devbox/types/*.d.ts | Updated TypeScript type definitions to include icon field |
| frontend/providers/devbox/app/api/**/route.ts | Updated API endpoints to query and return icon field |
| frontend/providers/devbox/components/**/*.tsx | Updated UI components to pass and display icon data |
| description: query.description, | ||
| name: query.templateRepositoryName, | ||
| isPublic: query.isPublic | ||
| isPublic: query.isPublic, |
There was a problem hiding this comment.
The conditional update logic using hasIconInput may lead to unexpected behavior. When hasIconInput is true but the icon value is null or empty string, normalizedIcon will be null (from normalizeTemplateIcon). This means setting icon to explicitly null will update the database field to null, but if icon is undefined in the request, the field won't be updated at all.
This is correct behavior for an optional field update, but it's worth noting that there's an asymmetry: you can explicitly set icon to null to clear it, but you cannot distinguish between "field not provided" and "field provided as null" at the schema level since z.string().nullable().optional() accepts both. The hasIconInput check addresses this, which is good. However, consider documenting this behavior or adding a comment explaining the intended semantics.
| isPublic: query.isPublic, | |
| isPublic: query.isPublic, | |
| // NOTE: `hasIconInput` is used to distinguish "no icon field in request" | |
| // from "icon explicitly provided (possibly as null)". | |
| // - If hasIconInput is false, we omit `icon` so the DB value is untouched. | |
| // - If hasIconInput is true, we always include `icon: normalizedIcon`; | |
| // `normalizedIcon` may be null to explicitly clear the stored icon. | |
| // This preserves partial-update semantics despite using | |
| // `z.string().nullable().optional()` in the schema. |
| if (icon) { | ||
| const trimmed = icon.trim(); | ||
| if (isSvgContent(trimmed)) { | ||
| return `data:image/svg+xml;utf8,${encodeURIComponent(trimmed)}`; |
There was a problem hiding this comment.
The data URI encoding for SVG content uses 'utf8' encoding which doesn't properly escape all characters that could break the URI. While encodeURIComponent is used which handles most cases, there's a potential issue with SVG content containing certain Unicode characters or very large SVGs.
Additionally, the data URI uses 'utf8' as the charset parameter, but the standard encoding parameter for data URIs should be either omitted (defaults to US-ASCII with URL encoding) or use 'charset=utf-8' (with a hyphen). The current format data:image/svg+xml;utf8, may not be recognized by all browsers.
Consider using the more standard format: data:image/svg+xml;charset=utf-8,${encodeURIComponent(trimmed)} or simply data:image/svg+xml,${encodeURIComponent(trimmed)}
| return `data:image/svg+xml;utf8,${encodeURIComponent(trimmed)}`; | |
| return `data:image/svg+xml;charset=utf-8,${encodeURIComponent(trimmed)}`; |
| if (icon) { | ||
| const trimmed = icon.trim(); | ||
| if (/^<svg[\s>]/i.test(trimmed)) { | ||
| return `data:image/svg+xml;utf8,${encodeURIComponent(trimmed)}`; |
There was a problem hiding this comment.
The same data URI encoding issue exists here as in RuntimeIcon. The format data:image/svg+xml;utf8, should be data:image/svg+xml;charset=utf-8, or data:image/svg+xml, to be standards-compliant.
Since this logic is duplicated from RuntimeIcon (see related maintainability comment), fixing it in one place would require fixing it in both places, which reinforces the need to extract this into a shared utility function.
| try { | ||
| const url = new URL(normalized); | ||
| if (url.protocol !== 'https:') { | ||
| return { ok: false, error: 'icon url must use https' }; | ||
| } | ||
| } catch { | ||
| return { ok: false, error: 'icon must be svg content or https url' }; | ||
| } | ||
| return { ok: true, value: normalized, kind: 'url' as const }; |
There was a problem hiding this comment.
The URL validation only checks that the protocol is 'https:' but doesn't validate the URL structure or check for potential bypasses. For example:
- URLs like 'https:////attacker.com' might bypass certain validations
- URLs with embedded credentials like 'https://user:pass@example.com' are allowed but may pose security risks
- URLs with unusual ports or localhost/internal IPs are not blocked
- The URL constructor will throw for invalid URLs, but the catch block returns a generic error message that doesn't distinguish between "not a URL" and "not HTTPS"
Consider adding additional validation:
- Check that url.hostname is not empty and doesn't point to internal/private networks
- Validate URL length limits
- Consider blocking URLs with authentication credentials
- Provide more specific error messages to help users understand what went wrong
| const handleError = () => { | ||
| setImgSrc((prev) => (prev !== fallbackSrc ? fallbackSrc : '/images/runtime/custom.svg')); | ||
| }; |
There was a problem hiding this comment.
The fallback error handling in the RuntimeIcon component has a potential issue. When the first fallback (based on iconId) fails, it falls back to '/images/runtime/custom.svg'. However, if that also fails, the onError handler will be triggered again, creating an infinite loop condition.
The current logic at line 67 checks if prev !== fallbackSrc, but if fallbackSrc itself fails to load, and then custom.svg also fails, there's no proper handling. Consider adding a flag to track whether we've already fallen back to the final fallback, or use a more robust error boundary pattern.
|
|
||
| const TEMPLATE_ICON_MAX_BYTES = 64 * 1024; | ||
| const SVG_PREFIX_RE = /^<svg[\s>]/i; | ||
| const SVG_UNSAFE_PATTERNS = [/<script[\s>]/i, /on\w+\s*=/i, /foreignobject/i, /javascript:/i]; |
There was a problem hiding this comment.
The pattern /foreignobject/i only checks for 'foreignobject' (all lowercase) due to case-insensitive flag, but SVG elements are case-sensitive in XML. The correct SVG element name is 'foreignObject' (with capital O). While the case-insensitive regex will match it, it would also match invalid variants. More importantly, this pattern won't catch namespace-prefixed versions like 'svg:foreignObject' or attributes like 'xlink:href' that could reference external content.
Consider updating the pattern to /foreignObject/i or /<foreignObject[\s>]/i to be more precise, and add additional patterns for other potentially dangerous SVG features.
|
|
||
| const TEMPLATE_ICON_MAX_BYTES = 64 * 1024; | ||
| const SVG_PREFIX_RE = /^<svg[\s>]/i; | ||
| const SVG_UNSAFE_PATTERNS = [/<script[\s>]/i, /on\w+\s*=/i, /foreignobject/i, /javascript:/i]; |
There was a problem hiding this comment.
The SVG sanitization patterns are incomplete and may not protect against all XSS vectors. The current patterns check for script tags, event handlers, foreignObject, and javascript: URIs, but they miss several other dangerous SVG features:
- The pattern /on\w+\s*=/i won't catch event handlers with whitespace before the equals sign (e.g., "onclick = 'alert(1)'")
- Missing checks for:
- tags with href to external resources
tags with href that could load malicious content
- , , <iframe> tags
- data: URIs in xlink:href or href attributes
- XML entities and CDATA sections that could bypass filtering
- , tags that could be used for clickjacking
- style attributes or <style> tags that could contain expression() or other CSS-based XSS
Consider using a well-tested SVG sanitization library like DOMPurify or svg-sanitizer instead of maintaining custom patterns. If custom patterns are required, the regex should be more comprehensive and thoroughly tested against known XSS payloads.
| const src = (() => { | ||
| if (icon) { | ||
| const trimmed = icon.trim(); | ||
| if (/^<svg[\s>]/i.test(trimmed)) { | ||
| return `data:image/svg+xml;utf8,${encodeURIComponent(trimmed)}`; | ||
| } | ||
| if (/^https:\/\//i.test(trimmed)) { | ||
| return trimmed; | ||
| } | ||
| } | ||
| return fallbackSrc; | ||
| })(); |
There was a problem hiding this comment.
The icon resolution and SVG detection logic is duplicated between this component and RuntimeIcon. The TemplateCard component has inline logic in lines 117-128 that replicates the functionality of RuntimeIcon's resolveRuntimeIconSrc function. This duplication increases maintenance burden and risk of inconsistencies.
Consider extracting the icon resolution logic into a shared utility function that both components can use, or refactor to use RuntimeIcon's resolveRuntimeIconSrc function directly.
|
|
||
| const TEMPLATE_ICON_MAX_BYTES = 64 * 1024; | ||
| const SVG_PREFIX_RE = /^<svg[\s>]/i; | ||
| const SVG_UNSAFE_PATTERNS = [/<script[\s>]/i, /on\w+\s*=/i, /foreignobject/i, /javascript:/i]; |
There was a problem hiding this comment.
The event handler pattern /on\w+\s*=/i has a potential bypass. It requires whitespace after the event handler name but before the equals sign, which means "onclick='alert(1)'" (without space) would be caught, but the pattern uses \s* which allows zero or more whitespace characters. However, it won't catch event handlers with newlines, tabs, or other Unicode whitespace variations between the handler name and equals sign in all cases.
More critically, the pattern doesn't account for event handlers that might be encoded or obfuscated. Consider strengthening this pattern to /on\w+\s*=/i (which is already present) but also add checks for encoded variations and XML namespace prefixes.
No description provided.