Improve image signature handling and resizing functionality (#4511)

packages/gitbook/src/lib/images/resizer/gitbook-service.ts

@@ -1,4 +1,4 @@
-import type { CloudflareImageOptions } from './types';
+import type { CloudflareImageOptions, CloudflareResizeImageOptions } from './types';
 import { copyImageResponse } from './utils';
 import { GITBOOK_IMAGE_RESIZE_SALT, GITBOOK_IMAGE_RESIZE_URL } from '@/lib/env';
 import { getLogger } from '@/lib/logger';
@@ -18,11 +18,10 @@ function sdbmHash(str: string): number {
  */
 export async function resizeImageWithGitbookServices(
     input: string,
-    options: CloudflareImageOptions & {
-        signal?: AbortSignal;
-    }
+    options: CloudflareResizeImageOptions
 ): Promise<Response> {
-    const { signal, ...resizeOptions } = options;
+    // Only the resize options are serialized in the URL, everything else is transport-level.
+    const { signal, accept, bypassSkipCheck, ...resizeOptions } = options;
 
     if (!GITBOOK_IMAGE_RESIZE_SALT) {
         throw new Error(
@@ -48,16 +47,33 @@ export async function resizeImageWithGitbookServices(
     return copyImageResponse(
         await fetch(resizeURL, {
             headers: {
-                Accept:
-                    resizeOptions.format === 'json'
-                        ? 'application/json'
-                        : `image/${resizeOptions.format || 'jpeg'}`,
+                Accept: getAcceptHeader(resizeOptions.format, accept),
             },
             signal,
         })
     );
 }
 
+/**
+ * Accept header to send to the image service.
+ * With the default "auto" format, the service negotiates the output itself from the
+ * accept header of the original request.
+ */
+function getAcceptHeader(
+    format: CloudflareImageOptions['format'],
+    accept: string | undefined
+): string {
+    if (format === 'json') {
+        return 'application/json';
+    }
+
+    if (format && format !== 'auto') {
+        return `image/${format}`;
+    }
+
+    return accept || 'image/*';
+}
+
 function stringifyOptions(options: CloudflareImageOptions & { signature: string }): string {
     return Object.entries({ ...options }).reduce((rest, [key, value]) => {
         return `${rest}${rest ? ',' : ''}${key}=${value}`;

packages/gitbook/src/lib/images/resizer/resizeImage.ts

@@ -4,7 +4,11 @@ import assertNever from 'assert-never';
 import { GITBOOK_IMAGE_RESIZE_MODE } from '../../env';
 import { SizableImageAction, checkIsSizableImageURL } from '../checkIsSizableImageURL';
 import { resizeImageWithGitbookServices } from './gitbook-service';
-import type { CloudflareImageJsonFormat, CloudflareImageOptions } from './types';
+import type {
+    CloudflareImageJsonFormat,
+    CloudflareImageOptions,
+    CloudflareResizeImageOptions,
+} from './types';
 import { getLogger } from '@/lib/logger';
 
 /**
@@ -41,15 +45,6 @@ export async function getImageSize(
     }
 }
 
-export type CloudflareResizeImageOptions = CloudflareImageOptions & {
-    signal?: AbortSignal;
-    /**
-     * Bypass the check to see if the image can be resized.
-     * This is useful for some format that are not supported by @next/og and need to be transformed
-     */
-    bypassSkipCheck?: boolean;
-};
-
 /**
  * Execute a Cloudflare Image Resize operation on an image.
  */
@@ -64,12 +59,6 @@ export async function resizeImage(
         );
     }
 
-    if (action === SizableImageAction.Passthrough) {
-        return fetch(input, {
-            signal: options.signal,
-        });
-    }
-
     switch (GITBOOK_IMAGE_RESIZE_MODE) {
         case 'cf-fetch':
         case 'gitbook-service':

packages/gitbook/src/lib/images/resizer/types.ts

@@ -13,7 +13,7 @@ export interface CloudflareImageJsonFormat {
  * https://developers.cloudflare.com/images/image-resizing/resize-with-workers/
  */
 export interface CloudflareImageOptions {
-    format?: 'webp' | 'avif' | 'json' | 'jpeg' | 'png';
+    format?: 'webp' | 'avif' | 'json' | 'jpeg' | 'png' | 'auto';
     fit?: 'scale-down' | 'contain' | 'cover' | 'crop' | 'pad';
     width?: number;
     height?: number;
@@ -21,3 +21,16 @@ export interface CloudflareImageOptions {
     anim?: boolean;
     quality?: number;
 }
+
+export type CloudflareResizeImageOptions = CloudflareImageOptions & {
+    signal?: AbortSignal;
+    /**
+     * Bypass the check to see if the image can be resized.
+     * This is useful for some format that are not supported by @next/og and need to be transformed
+     */
+    bypassSkipCheck?: boolean;
+    /**
+     * Accept header of the incoming request, forwarded to the image service for content negotiation.
+     */
+    accept?: string;
+};

packages/gitbook/src/lib/images/signatures.test.ts

@@ -0,0 +1,99 @@
+import { describe, expect, it, mock } from 'bun:test';
+
+// The signing key is read at module load, so it must be mocked before importing the module.
+const realEnv = await import('@/lib/env');
+mock.module('@/lib/env', () => ({
+    ...realEnv,
+    GITBOOK_IMAGE_RESIZE_SIGNING_KEY: 'test-signing-key',
+}));
+
+const {
+    CURRENT_SIGNATURE_VERSION,
+    generateImageSignature,
+    isSignatureVersion,
+    verifyImageSignature,
+} = await import('./signatures');
+
+const input = {
+    url: 'https://example.com/image.png',
+    imagesContextId: 'example.com',
+};
+
+describe('generateImageSignature', () => {
+    it('should generate a v3 signature', async () => {
+        const { signature, version } = await generateImageSignature(input);
+        expect(version).toBe('3');
+        expect(version).toBe(CURRENT_SIGNATURE_VERSION);
+        expect(signature).toMatch(/^[0-9a-f]{32}$/);
+    });
+
+    it('should be deterministic', async () => {
+        const first = await generateImageSignature(input);
+        const second = await generateImageSignature(input);
+        expect(first.signature).toBe(second.signature);
+    });
+
+    it('should generate a different signature for a different url', async () => {
+        const { signature } = await generateImageSignature(input);
+        const other = await generateImageSignature({
+            ...input,
+            url: 'https://example.com/other.png',
+        });
+        expect(other.signature).not.toBe(signature);
+    });
+
+    it('should generate a different signature for a different images context', async () => {
+        const { signature } = await generateImageSignature(input);
+        const other = await generateImageSignature({ ...input, imagesContextId: 'other.com' });
+        expect(other.signature).not.toBe(signature);
+    });
+});
+
+describe('verifyImageSignature', () => {
+    it('should verify a freshly generated signature', async () => {
+        const { signature, version } = await generateImageSignature(input);
+        expect(await verifyImageSignature(input, { signature, version })).toBe(true);
+    });
+
+    it('should reject a tampered signature', async () => {
+        const { signature, version } = await generateImageSignature(input);
+        const tampered = `${signature.slice(0, -1)}${signature.endsWith('a') ? 'b' : 'a'}`;
+        expect(await verifyImageSignature(input, { signature: tampered, version })).toBe(false);
+    });
+
+    it('should reject a signature generated for another url', async () => {
+        const { signature, version } = await generateImageSignature(input);
+        const verified = await verifyImageSignature(
+            { ...input, url: 'https://example.com/other.png' },
+            { signature, version }
+        );
+        expect(verified).toBe(false);
+    });
+
+    it('should reject a signature generated for another images context', async () => {
+        const { signature, version } = await generateImageSignature(input);
+        const verified = await verifyImageSignature(
+            { ...input, imagesContextId: 'other.com' },
+            { signature, version }
+        );
+        expect(verified).toBe(false);
+    });
+
+    // Older signatures still exist in previously generated and cached content.
+    it('should still verify a v2 signature', async () => {
+        expect(await verifyImageSignature(input, { signature: 'd52f183b', version: '2' })).toBe(
+            true
+        );
+    });
+});
+
+describe('isSignatureVersion', () => {
+    it('should accept all known versions', () => {
+        expect(['0', '1', '2', '3'].every(isSignatureVersion)).toBe(true);
+    });
+
+    it('should reject unknown versions', () => {
+        expect(isSignatureVersion('4')).toBe(false);
+        expect(isSignatureVersion('')).toBe(false);
+    });
+});

packages/gitbook/src/lib/images/signatures.ts

@@ -10,12 +10,12 @@ import { getLogger } from '@/lib/logger';
  * GitBook has supported different version of image signing in the past. To maintain backwards
  * compatibility, we retain the ability to verify older signatures.
  */
-export type SignatureVersion = '0' | '1' | '2';
+export type SignatureVersion = '0' | '1' | '2' | '3';
 
 /**
  * The current version of the signature.
  */
-export const CURRENT_SIGNATURE_VERSION: SignatureVersion = '2';
+export const CURRENT_SIGNATURE_VERSION: SignatureVersion = '3';
 
 type SignFnInput = {
     url: string;
@@ -34,30 +34,57 @@ export async function verifyImageSignature(
     const generator = IMAGE_SIGNATURE_FUNCTIONS[version];
     const generated = await generator(input);
 
+    const matches = safeCompare(generated, signature);
+
     const logger = getLogger().subLogger('imageResizing');
-    if (generated !== signature) {
+    if (!matches) {
         // We only log if the signature does not match, to avoid logging useless information
         logger.log(
             `comparing image signature for "${input.url}" on identifier "${input.imagesContextId}": "${generated}" (expected) === "${signature}" (actual)`
         );
     }
-    return generated === signature;
+    return matches;
 }
 
 /**
  * Generate an image signature. Also returns the version of the image signing algorithm that was used.
- *
- * This function is sync. If you need to implement an async version of image signing, you'll need to change
- * ths signature of this fn and where it's used.
  */
 export async function generateImageSignature(input: SignFnInput): Promise<{
     signature: string;
     version: SignatureVersion;
 }> {
-    const result = await generateSignatureV2(input);
+    const result = await generateSignatureV3(input);
     return { signature: result, version: CURRENT_SIGNATURE_VERSION };
 }
 
+/**
+ * The signature is truncated to 128 bits: enough to make forgery infeasible, while keeping
+ * image URLs short as a page can contain many of them.
+ */
+const SIGNATURE_V3_BYTES = 16;
+
+/**
+ * Same inputs as the v2 algorithm, but hashed with SHA-256 instead of FNV-1a.
+ * FNV-1a is not a cryptographic hash, so a v2 signature is not a sound authentication token.
+ *
+ * We use the Web Crypto API rather than `node:crypto` because signatures are verified from the
+ * middleware, which is bundled for the Cloudflare edge runtime where `node:crypto` is external.
+ */
+const generateSignatureV3: SignFn = async (input) => {
+    assert(GITBOOK_IMAGE_RESIZE_SIGNING_KEY, 'GITBOOK_IMAGE_RESIZE_SIGNING_KEY is not set');
+    const all = [
+        input.url,
+        input.imagesContextId, // The hostname is used to avoid serving images from other sites on the same domain
+        // Kept last so a known prefix can never be length-extended into a valid signature.
+        GITBOOK_IMAGE_RESIZE_SIGNING_KEY,
+    ]
+        .filter(Boolean)
+        .join(':');
+
+    const hash = await crypto.subtle.digest('SHA-256', new TextEncoder().encode(all));
+    return toHex(new Uint8Array(hash, 0, SIGNATURE_V3_BYTES));
+};
+
 // Reused buffer for FNV-1a hashing in the v2 algorithm
 const fnv1aUtf8Buffer = new Uint8Array(512);
 
@@ -102,13 +129,15 @@ const generateSignatureV0: SignFn = async (input) => {
     assert(GITBOOK_IMAGE_RESIZE_SIGNING_KEY, 'GITBOOK_IMAGE_RESIZE_SIGNING_KEY is not set');
     const all = [input.url, GITBOOK_IMAGE_RESIZE_SIGNING_KEY].filter(Boolean).join(':');
     const hash = await crypto.subtle.digest('SHA-256', new TextEncoder().encode(all));
-
-    // Convert ArrayBuffer to hex string
-    const hashArray = Array.from(new Uint8Array(hash));
-    const hashHex = hashArray.map((b) => b.toString(16).padStart(2, '0')).join('');
-    return hashHex;
+    return toHex(new Uint8Array(hash));
 };
 
+function toHex(bytes: Uint8Array): string {
+    return Array.from(bytes)
+        .map((b) => b.toString(16).padStart(2, '0'))
+        .join('');
+}
+
 /**
  * A mapping of signature versions to signature functions.
  */
@@ -116,8 +145,25 @@ const IMAGE_SIGNATURE_FUNCTIONS: Record<SignatureVersion, SignFn> = {
     '0': generateSignatureV0,
     '1': generateSignatureV1,
     '2': generateSignatureV2,
+    '3': generateSignatureV3,
 };
 
 export function isSignatureVersion(input: string): input is SignatureVersion {
     return Object.keys(IMAGE_SIGNATURE_FUNCTIONS).includes(input);
 }
+
+/**
+ * Compare two signatures in constant time, to avoid leaking a valid signature byte by byte.
+ * We can't use `node:crypto`'s `timingSafeEqual` as this also runs on the edge runtime.
+ */
+function safeCompare(expected: string, actual: string): boolean {
+    if (expected.length !== actual.length) {
+        return false;
+    }
+
+    let diff = 0;
+    for (let i = 0; i < expected.length; i++) {
+        diff |= expected.charCodeAt(i) ^ actual.charCodeAt(i);
+    }
+    return diff === 0;
+}

packages/gitbook/src/lib/images/utils.ts

@@ -15,3 +15,27 @@ export async function getResizedImageURL(
 
     return await getURL(options);
 }
+
+/**
+ * Header stamped on every image request the service refuses to serve, so a rejection can be
+ * told apart from a source error without parsing the body.
+ */
+export const IMAGE_REJECT_REASON_HEADER = 'x-gitbook-reject-reason';
+
+/**
+ * Reason an image request was rejected, reported in {@link IMAGE_REJECT_REASON_HEADER}.
+ */
+export enum ImageRejectReason {
+    /** The request path is missing the resizing options or the source URL. */
+    InvalidRequest = 'invalid-request',
+    /** The signature is missing or does not match the source URL. */
+    InvalidSignature = 'invalid-signature',
+    /** The source URL is not a public http(s) host. */
+    UnsafeSourceURL = 'unsafe-source-url',
+    /** The source responded with an error status. */
+    UpstreamError = 'upstream-error',
+    /** The response is not something we are willing to serve as an image. */
+    UnsupportedContentType = 'unsupported-content-type',
+    /** The request failed while being processed. */
+    InternalError = 'internal-error',
+}

packages/gitbook/src/routes/image.ts

@@ -2,7 +2,7 @@ import { NextResponse } from 'next/server';
 
 import {
     CURRENT_SIGNATURE_VERSION,
-    type CloudflareImageOptions,
+    type CloudflareResizeImageOptions,
     type SignatureVersion,
     SizableImageAction,
     checkIsSizableImageURL,
@@ -10,21 +10,9 @@ import {
     parseImageAPIURL,
     resizeImage,
     verifyImageSignature,
+    IMAGE_REJECT_REASON_HEADER,
+    ImageRejectReason,
 } from '@/lib/images';
-import type { CloudflareResizeImageOptions } from '@/lib/images/resizer';
-
-const FORMATS = [
-    {
-        format: 'avif' as const,
-        regexp: /image\/avif/,
-        maxAllowedEdge: 1600,
-    },
-    {
-        format: 'webp' as const,
-        regexp: /image\/webp/,
-        maxAllowedEdge: 1920,
-    },
-];
 
 /**
  * Resize an image using the Cloudflare Image API.
@@ -87,12 +75,12 @@ export async function serveResizedImage(
 
     const defaultFormat = getOriginalFormatFromURL(url);
 
-    // Cloudflare-specific options are in the cf object.
-    const options: CloudflareImageOptions = {
+    const options: CloudflareResizeImageOptions = {
         fit: 'scale-down',
-        // For GIF, we will use webp as default format for resizing.
-        format: defaultFormat === 'gif' ? 'webp' : defaultFormat,
+        // Let the image service negotiate the output format from the accept header.
+        format: 'auto',
         quality: 100,
+        accept: request.headers.get('accept') ?? undefined,
     };
 
     const width = requestURL.searchParams.get('width');
@@ -105,8 +93,6 @@ export async function serveResizedImage(
         options.height = Number(height);
     }
 
-    const longestEdgeValue = Math.max(options.width || 0, options.height || 0);
-
     const dpr = requestURL.searchParams.get('dpr');
     if (dpr) {
         options.dpr = Number(dpr);
@@ -117,24 +103,6 @@ export async function serveResizedImage(
         options.quality = Number(quality);
     }
 
-    // Check the Accept header to handle content negotiation
-    const accept = request.headers.get('accept');
-
-    // We test if we can use AVIF based on the accept header and constraints from Cloudflare
-    // @see https://developers.cloudflare.com/images/transform-images/#limits-per-format
-    if (accept) {
-        for (const entry of FORMATS) {
-            if (entry.regexp.test(accept) && longestEdgeValue <= entry.maxAllowedEdge) {
-                const wantedDpr = options.dpr ?? 1;
-                const dpr = chooseDPR(longestEdgeValue, entry.maxAllowedEdge, wantedDpr);
-                if (dpr === wantedDpr) {
-                    options.format = entry.format;
-                    break;
-                }
-            }
-        }
-    }
-
     return resizeImageWithFallback(
         url,
         options,
@@ -155,8 +123,37 @@ async function resizeImageWithFallback(
     try {
         const response = await resizeImage(url, options);
         if (!response.ok) {
+            const rejectReason = response.headers.get(IMAGE_REJECT_REASON_HEADER);
+            if (rejectReason) {
+                switch (rejectReason) {
+                    case ImageRejectReason.InvalidRequest:
+                        return new Response('Invalid request', { status: 400 });
+                    case ImageRejectReason.InvalidSignature:
+                        return new Response('Invalid signature', { status: 400 });
+                    case ImageRejectReason.UnsafeSourceURL:
+                        return new Response('Unsafe source URL', { status: 400 });
+                    case ImageRejectReason.UpstreamError:
+                        // this one can happen for a lot of reasons, so we fallback to a redirect to the original image
+                        // It sometimes happen when upstream block fetch from  our server
+                        throw new Error('Upstream error, falling back to a redirect');
+                    case ImageRejectReason.UnsupportedContentType:
+                        throw new Error('Unsupported content type, falling back to a redirect');
+                    case ImageRejectReason.InternalError:
+                        throw new Error('Internal error, falling back to a redirect');
+                    default:
+                        throw new Error(
+                            `Unknown reject reason "${rejectReason}", falling back to a redirect`
+                        );
+                }
+            }
             throw new Error(`Failed to resize image, received status code ${response.status}`);
         }
+
+        // The output format is negotiated from the accept header we forwarded.
+        if (!response.headers.get('vary')?.toLowerCase().includes('accept')) {
+            response.headers.append('vary', 'Accept');
+        }
+
         return response;
     } catch (error) {
         if (formatFallback && options.format !== formatFallback) {
@@ -167,12 +164,35 @@ async function resizeImageWithFallback(
             );
         }
 
+        // Never bounce the visitor to a GitBook-hosted URL: it would expose our internal
+        // asset URLs and hide a failure that is ours to fix.
+        if (isGitBookHostedURL(url)) {
+            console.warn('Error while resizing GitBook-hosted image', error);
+            return new Response('Failed to resize image', { status: 502 });
+        }
+
         // Redirect to the original image if resizing fails
         console.warn('Error while resizing image, redirecting to original', error);
         return NextResponse.redirect(url, 302);
     }
 }
 
+const GITBOOK_HOSTED_DOMAINS = ['gitbook.com', 'gitbook.io'];
+
+/**
+ * Check if a URL is hosted on a GitBook domain.
+ */
+function isGitBookHostedURL(url: string): boolean {
+    if (!URL.canParse(url)) {
+        return false;
+    }
+
+    const { hostname } = new URL(url);
+    return GITBOOK_HOSTED_DOMAINS.some(
+        (domain) => hostname === domain || hostname.endsWith(`.${domain}`)
+    );
+}
+
 /**
  * Get the original format from URL.
  */
@@ -188,16 +208,6 @@ function getOriginalFormatFromURL(url: string) {
     return 'jpeg';
 }
 
-/**
- * Choose the DPR allowed to resize an image on Cloudflare.
- * @see https://developers.cloudflare.com/images/transform-images/#limits-per-format
- */
-function chooseDPR(longestEdgeValue: number, maxAllowedEdge: number, wantedDpr: number): number {
-    const maxDprBySize = Math.floor(maxAllowedEdge / longestEdgeValue);
-    // Ensure that the DPR is within the allowed range
-    return Math.max(1, Math.min(maxDprBySize, wantedDpr));
-}
-
 /**
  * Parse the image signature version from a query param. Returns null if the version is invalid.
  */