-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(dingtalk): cap media download size to 50MB to match feishu #7361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9bbd91a
6964450
681549e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| import { | ||
| describe, | ||
| it, | ||
| expect, | ||
| vi, | ||
| beforeEach, | ||
| afterEach, | ||
| type MockInstance, | ||
| } from 'vitest'; | ||
| import { downloadMedia } from './media.js'; | ||
|
|
||
| describe('downloadMedia (DingTalk)', () => { | ||
| let fetchSpy: MockInstance<typeof fetch>; | ||
|
|
||
| beforeEach(() => { | ||
| fetchSpy = vi.spyOn(globalThis, 'fetch'); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| fetchSpy.mockRestore(); | ||
| }); | ||
|
|
||
| // First fetch is the DingTalk download-code API that returns a downloadUrl; | ||
| // the second fetch is the actual file download that the size cap guards. | ||
| function mockApiResponse( | ||
| downloadUrl = 'https://dl.dingtalk.example/file', | ||
| ): Response { | ||
| return { | ||
| ok: true, | ||
| json: vi.fn().mockResolvedValue({ downloadUrl }), | ||
| } as unknown as Response; | ||
| } | ||
|
|
||
| it('downloads a file within the size limit', async () => { | ||
| const mockData = new Uint8Array([1, 2, 3, 4]); | ||
| const fileResp = { | ||
| ok: true, | ||
| headers: { | ||
| get: (key: string) => { | ||
| if (key === 'content-length') return '4'; | ||
| if (key === 'content-type') return 'image/png'; | ||
| return null; | ||
| }, | ||
| }, | ||
| body: { | ||
| getReader: () => ({ | ||
| read: vi | ||
| .fn() | ||
| .mockResolvedValueOnce({ done: false, value: mockData }) | ||
| .mockResolvedValueOnce({ done: true, value: undefined }), | ||
| cancel: vi.fn(), | ||
| }), | ||
| }, | ||
| }; | ||
|
|
||
| fetchSpy | ||
| .mockResolvedValueOnce(mockApiResponse()) | ||
| .mockResolvedValueOnce(fileResp as unknown as Response); | ||
|
|
||
| const result = await downloadMedia('code', 'robot', 'token'); | ||
|
|
||
| expect(result).not.toBeNull(); | ||
| expect(result?.buffer).toEqual(Buffer.from(mockData)); | ||
| expect(result?.mimeType).toBe('image/png'); | ||
| }); | ||
|
|
||
| it('rejects a Content-Length exceeding 50MB and releases the connection', async () => { | ||
| const largeSize = 60 * 1024 * 1024; // 60 MB | ||
| const bodyCancel = vi.fn(); | ||
| const fileResp = { | ||
| ok: true, | ||
| headers: { | ||
| get: (key: string) => | ||
| key === 'content-length' ? largeSize.toString() : null, | ||
| }, | ||
| body: { | ||
| cancel: bodyCancel, | ||
| getReader: () => ({ | ||
| read: vi.fn().mockResolvedValue({ done: true, value: undefined }), | ||
| cancel: vi.fn(), | ||
| }), | ||
| }, | ||
| }; | ||
|
|
||
| fetchSpy | ||
| .mockResolvedValueOnce(mockApiResponse()) | ||
| .mockResolvedValueOnce(fileResp as unknown as Response); | ||
|
|
||
| const result = await downloadMedia('code', 'robot', 'token'); | ||
|
|
||
| expect(result).toBeNull(); | ||
| expect(bodyCancel).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('rejects a stream exceeding 50MB with no Content-Length header', async () => { | ||
| const chunkSize = 10 * 1024 * 1024; // 10 MB per chunk | ||
| const mockData = new Uint8Array(chunkSize); | ||
| const cancelMock = vi.fn(); | ||
| const fileResp = { | ||
| ok: true, | ||
| headers: { | ||
| get: () => null, // no content-length → must be caught while streaming | ||
| }, | ||
| body: { | ||
| getReader: () => ({ | ||
| read: vi.fn().mockResolvedValue({ done: false, value: mockData }), // never ends | ||
| cancel: cancelMock, | ||
| }), | ||
| }, | ||
| }; | ||
|
|
||
| fetchSpy | ||
| .mockResolvedValueOnce(mockApiResponse()) | ||
| .mockResolvedValueOnce(fileResp as unknown as Response); | ||
|
|
||
| const result = await downloadMedia('code', 'robot', 'token'); | ||
|
|
||
| expect(result).toBeNull(); | ||
| expect(cancelMock).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('returns null when the file response body is null', async () => { | ||
| const fileResp = { | ||
| ok: true, | ||
| headers: { get: () => null }, | ||
| body: null, | ||
| }; | ||
|
|
||
| fetchSpy | ||
| .mockResolvedValueOnce(mockApiResponse()) | ||
| .mockResolvedValueOnce(fileResp as unknown as Response); | ||
|
|
||
| const result = await downloadMedia('code', 'robot', 'token'); | ||
|
|
||
| expect(result).toBeNull(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -62,18 +62,58 @@ export async function downloadMedia( | |||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Step 2: Download the actual file | ||||||||||||||||||||||||||||
| const fileResp = await fetch(downloadUrl); | ||||||||||||||||||||||||||||
| // Step 2: Download the actual file. Bound the request so a stalled CDN | ||||||||||||||||||||||||||||
| // connection can't hang the streaming loop below indefinitely (matches | ||||||||||||||||||||||||||||
| // the Feishu adapter). | ||||||||||||||||||||||||||||
| const fileResp = await fetch(downloadUrl, { | ||||||||||||||||||||||||||||
| signal: AbortSignal.timeout(30_000), | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
| if (!fileResp.ok) { | ||||||||||||||||||||||||||||
| process.stderr.write( | ||||||||||||||||||||||||||||
| `[DingTalk] downloadMedia file fetch failed: HTTP ${fileResp.status}\n`, | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const MAX_DOWNLOAD_BYTES = 50 * 1024 * 1024; // 50 MB | ||||||||||||||||||||||||||||
| const contentLength = fileResp.headers.get('content-length'); | ||||||||||||||||||||||||||||
| if (contentLength && parseInt(contentLength, 10) > MAX_DOWNLOAD_BYTES) { | ||||||||||||||||||||||||||||
| // Release the connection instead of letting the unread body pin it | ||||||||||||||||||||||||||||
| // until GC. | ||||||||||||||||||||||||||||
| await fileResp.body?.cancel(); | ||||||||||||||||||||||||||||
| process.stderr.write( | ||||||||||||||||||||||||||||
| `[DingTalk] downloadMedia rejected: size ${contentLength} exceeds ${MAX_DOWNLOAD_BYTES} byte limit\n`, | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+80
to
+88
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] When rejecting based on Content-Length, the response body is not consumed or cancelled before returning The streaming rejection path (line 96) correctly calls Concrete cost: repeated Content-Length rejections leak HTTP connections until GC finalizes the Response objects.
Suggested change
— qwen3.7-max via Qwen Code /review |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const mimeType = | ||||||||||||||||||||||||||||
| fileResp.headers.get('content-type') || 'application/octet-stream'; | ||||||||||||||||||||||||||||
| const buffer = Buffer.from(await fileResp.arrayBuffer()); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Stream-read with size enforcement (handles chunked transfer without Content-Length) | ||||||||||||||||||||||||||||
| const reader = fileResp.body?.getReader(); | ||||||||||||||||||||||||||||
| if (!reader) { | ||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+94
to
+97
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The download Failure scenario: DingTalk CDN accepts the connection but delivers bytes extremely slowly or stalls mid-stream → Consider adding — qwen3.7-max via Qwen Code /review |
||||||||||||||||||||||||||||
| const chunks: Buffer[] = []; | ||||||||||||||||||||||||||||
| let totalSize = 0; | ||||||||||||||||||||||||||||
| while (true) { | ||||||||||||||||||||||||||||
| const { done, value } = await reader.read(); | ||||||||||||||||||||||||||||
| if (done) break; | ||||||||||||||||||||||||||||
| totalSize += value.byteLength; | ||||||||||||||||||||||||||||
| if (totalSize > MAX_DOWNLOAD_BYTES) { | ||||||||||||||||||||||||||||
| // Await the cancel so a stream error during teardown surfaces here | ||||||||||||||||||||||||||||
| // instead of becoming an unhandled rejection (matches the body.cancel | ||||||||||||||||||||||||||||
| // above). | ||||||||||||||||||||||||||||
| await reader.cancel(); | ||||||||||||||||||||||||||||
| process.stderr.write( | ||||||||||||||||||||||||||||
| `[DingTalk] downloadMedia rejected: actual size exceeds ${MAX_DOWNLOAD_BYTES} byte limit\n`, | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| chunks.push(Buffer.from(value)); | ||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion]
Suggested change
And update the type: const chunks: Uint8Array[] = [];— qwen3.7-max via Qwen Code /review |
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| const buffer = Buffer.concat(chunks); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return { buffer, mimeType }; | ||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The size-capped streaming download logic (~30 lines) is duplicated line-for-line between this file and
packages/channels/feishu/src/media.ts:61-95. Both also independently declare identicalMediaFileinterfaces.Concrete cost: a bug fix to the size enforcement (e.g., the missing body cancel above) must be independently applied to both adapters. A fix applied to one and missed in the other creates divergent security behavior for the same class of attack.
Consider extracting a shared helper — e.g.,
downloadWithSizeCap(url, headers?, label)— intopackages/channels/base/src/and having both adapters delegate to it.— qwen3.7-max via Qwen Code /review