From 16604d697b16da0a27dc5292907908ebc2fe512d Mon Sep 17 00:00:00 2001 From: timmy Date: Mon, 10 Aug 2026 15:03:45 +0000 Subject: [PATCH] feat: use binary screenshot transport (Closes #491) --- README.md | 7 ++- frontend/authored-outbox.js | 3 +- frontend/background-issue-sync.js | 38 ++++++++----- frontend/dashboard.js | 7 +-- frontend/issue-attachment.js | 36 +++++++----- frontend/issue-outbox.js | 29 +++++++--- requirements.txt | 1 + src/main.py | 88 ++++++++++++++++++----------- src/request_boundary.py | 15 ++++- tests/test_authored_outbox.py | 14 +++-- tests/test_background_issue_sync.py | 55 +++++++++++++++--- tests/test_issue_attachment_ui.py | 81 +++++++++++++++++--------- tests/test_issue_attachments.py | 26 +++++++++ tests/test_issue_outbox.py | 34 ++++++++--- tests/test_request_boundary.py | 2 +- 15 files changed, 306 insertions(+), 130 deletions(-) diff --git a/README.md b/README.md index adb34ec..b94777a 100644 --- a/README.md +++ b/README.md @@ -26,10 +26,11 @@ admit their text and image bytes to IndexedDB before confirmation, keep only bou localStorage, and use checkpointed upload/comment identities so reconnect retries cannot duplicate either stage. The mobile **New issue** sheet accepts the same image formats and stores the screenshot with its account-bound -outbox capture. Durable admission writes the complete screenshot capture to IndexedDB +outbox capture. Durable admission writes the complete screenshot capture as a binary Blob to IndexedDB before confirmation; localStorage keeps only bounded attachment metadata, avoiding base64 -quota pressure and synchronous multi-megabyte writes. Existing queued screenshot payloads -migrate to the IndexedDB-backed form on the next durable admission or worker reconciliation. +quota pressure and synchronous multi-megabyte writes. Online and background delivery send the +original bytes as multipart form data, avoiding the roughly 33% base64 wire expansion. Existing +queued base64 screenshot payloads remain readable and are converted only at delivery time. Delivery creates the issue exactly once, then uploads and comments with the image; after a partial failure, retry resumes with the confirmed issue instead of creating a duplicate. diff --git a/frontend/authored-outbox.js b/frontend/authored-outbox.js index 65ed834..3fe7cb3 100644 --- a/frontend/authored-outbox.js +++ b/frontend/authored-outbox.js @@ -106,7 +106,8 @@ function createAuthoredOutbox({ storage, fetchJson, coordinator, backgroundSync, attachment: { filename: String(message.attachment.filename || ''), contentType: String(message.attachment.contentType || ''), - data: String(message.attachment.data || ''), + ...(message.attachment.blob ? { blob: message.attachment.blob } : + { data: String(message.attachment.data || '') }), }, } : candidate); try { diff --git a/frontend/background-issue-sync.js b/frontend/background-issue-sync.js index 039ce66..a40d1f3 100644 --- a/frontend/background-issue-sync.js +++ b/frontend/background-issue-sync.js @@ -77,8 +77,10 @@ function createIssueSyncStore({ } continue; } - if (current.operationId === replacement.operationId && current.attachment?.data && - replacement.attachment?.stored && !replacement.attachment.data) { + if (current.operationId === replacement.operationId && + (current.attachment?.data || current.attachment?.blob) && + replacement.attachment?.stored && + !replacement.attachment.data && !replacement.attachment.blob) { replacement = { ...replacement, attachment: current.attachment }; incoming.set(current.id, replacement); } @@ -153,7 +155,8 @@ function createIssueSyncStore({ if (current && ((current.status === 'sending' && Number(current.claimUntil) > Number(now())) || (current.status === 'attention' && item.status === 'attention') || current.status === 'sent')) return current; - const preservedAttachment = current?.attachment?.data && item?.attachment?.stored && !item.attachment.data + const preservedAttachment = (current?.attachment?.data || current?.attachment?.blob) && + item?.attachment?.stored && !item.attachment.data && !item.attachment.blob ? { attachment: current.attachment } : {}; const next = current && current.operationId === item.operationId ? { ...item, ...preservedAttachment, @@ -417,6 +420,19 @@ function createBackgroundIssueSync({ return String(operationId || '').slice(0, 128 - suffix.length) + suffix; } + function attachmentMultipart(attachment) { + let blob = attachment?.blob; + if (!blob && attachment?.data) { + const binary = atob(String(attachment.data)); + const bytes = Uint8Array.from(binary, character => character.charCodeAt(0)); + blob = new Blob([bytes], { type: String(attachment.contentType || '') }); + } + if (!blob) throw new Error('The saved screenshot is unavailable. Retry before sending.'); + const form = new FormData(); + form.append('file', blob, String(attachment.filename || 'screenshot')); + return form; + } + async function deliverIssueCapture(item) { const repository = String(item.repository || '').split('/').map(encodeURIComponent).join('/'); let deliveredIssue = item.deliveredIssue; @@ -433,14 +449,10 @@ function createBackgroundIssueSync({ { method: 'POST', headers: { - Accept: 'application/json', 'Content-Type': 'application/json', + Accept: 'application/json', 'Idempotency-Key': stageOperationId(item.operationId, 'attachment'), }, - body: JSON.stringify({ - filename: item.attachment.filename, - content_type: item.attachment.contentType, - data: item.attachment.data, - }), + body: attachmentMultipart(item.attachment), }, ); attachmentMarkdown = String(uploaded?.markdown || ''); @@ -476,14 +488,10 @@ function createBackgroundIssueSync({ { method: 'POST', headers: { - Accept: 'application/json', 'Content-Type': 'application/json', + Accept: 'application/json', 'Idempotency-Key': stageOperationId(item.operationId, 'attachment'), }, - body: JSON.stringify({ - filename: item.attachment.filename, - content_type: item.attachment.contentType, - data: item.attachment.data, - }), + body: attachmentMultipart(item.attachment), }, ); attachmentMarkdown = String(uploaded?.markdown || ''); diff --git a/frontend/dashboard.js b/frontend/dashboard.js index f9ab728..bf48503 100644 --- a/frontend/dashboard.js +++ b/frontend/dashboard.js @@ -296,14 +296,9 @@ method: 'POST', headers: { Accept: 'application/json', - 'Content-Type': 'application/json', 'Idempotency-Key': payload.operation_id, }, - body: JSON.stringify({ - filename: payload.filename, - content_type: payload.content_type, - data: payload.data, - }), + body: issueAttachment.multipart(payload), }, ); }, diff --git a/frontend/issue-attachment.js b/frontend/issue-attachment.js index 0aa3ea7..388ae1f 100644 --- a/frontend/issue-attachment.js +++ b/frontend/issue-attachment.js @@ -8,8 +8,20 @@ const MAX_BYTES = 2 * 1024 * 1024; const IMAGE_TYPES = new Set(['image/png', 'image/jpeg', 'image/webp']); + function multipart(attachment) { + let blob = attachment?.blob; + if (!blob && attachment?.data) { + const binary = atob(String(attachment.data)); + const bytes = Uint8Array.from(binary, character => character.charCodeAt(0)); + blob = new Blob([bytes], { type: String(attachment.contentType || '') }); + } + if (!blob) throw new Error('The saved screenshot is unavailable. Retry before sending.'); + const form = new FormData(); + form.append('file', blob, String(attachment.filename || 'screenshot')); + return form; + } + function create(options) { - const readDataUrl = options.readDataUrl; const upload = options.upload; const createOperationId = options.createOperationId || (() => { if (typeof globalThis !== 'undefined' && globalThis.crypto?.randomUUID) { @@ -46,15 +58,16 @@ function restore(value) { const contentType = String(value?.contentType || ''); const filename = String(value?.filename || ''); + const blob = value?.blob; const data = String(value?.data || ''); - if (!data) { + if (!blob && !data) { clear(); throw new Error('The saved screenshot is unavailable. Retry before editing this issue.'); } const padding = (data.match(/=*$/) || [''])[0].length; - const size = Math.max(1, Math.floor(data.length * 3 / 4) - padding); - select({ name: filename, type: contentType, size }); - serialized = { filename, contentType, data }; + const size = blob ? Number(blob.size) : Math.max(1, Math.floor(data.length * 3 / 4) - padding); + select({ name: filename, type: contentType, size, ...(blob ? { blob } : {}) }); + serialized = blob ? { filename, contentType, blob } : { filename, contentType, data }; return state(); } @@ -69,14 +82,10 @@ async function serialize() { if (!selected) return null; if (!serialized) { - const dataUrl = await readDataUrl(selected); - const marker = ';base64,'; - const markerAt = String(dataUrl).indexOf(marker); - if (markerAt < 0) throw new Error('The screenshot could not be read. Choose it again.'); serialized = { filename: selected.name, contentType: selected.type, - data: String(dataUrl).slice(markerAt + marker.length), + blob: selected.blob || selected, }; } return { ...serialized }; @@ -92,7 +101,7 @@ number: item.number, filename: attachment.filename, content_type: attachment.contentType, - data: attachment.data, + ...(attachment.blob ? { blob: attachment.blob } : { data: attachment.data }), operation_id: operationId, }); if (!confirmed || typeof confirmed.markdown !== 'string' || !confirmed.markdown) { @@ -144,7 +153,8 @@ function restorePreview(value) { clearPreview(); const restored = controller.restore(value); - previewUrl = 'data:' + value.contentType + ';base64,' + value.data; + previewUrl = value.blob ? options.createObjectURL(value.blob) : + 'data:' + value.contentType + ';base64,' + value.data; options.image.src = previewUrl; options.meta.textContent = restored.name + ' ยท ' + Math.ceil(restored.size / 1024) + ' KB'; options.preview.hidden = false; @@ -155,5 +165,5 @@ return Object.assign(controller, { clear: clearPreview, restore: restorePreview }); } - return { create, mount, MAX_BYTES }; + return { create, mount, multipart, MAX_BYTES }; }); diff --git a/frontend/issue-outbox.js b/frontend/issue-outbox.js index fe8a06f..9f09cdd 100644 --- a/frontend/issue-outbox.js +++ b/frontend/issue-outbox.js @@ -8,8 +8,10 @@ function createIssueOutbox({ storage, fetchJson, coordinator, backgroundSync, ge function captureAttachment(value) { const contentType = String(value?.contentType || ''); const filename = String(value?.filename || '').slice(0, 255); + const blob = value?.blob; const data = String(value?.data || ''); if (!filename || !['image/png', 'image/jpeg', 'image/webp'].includes(contentType)) return undefined; + if (blob) return { filename, contentType, blob }; if (!data && value?.stored === true) return { filename, contentType, stored: true }; if (!data) return undefined; return { filename, contentType, data }; @@ -61,7 +63,7 @@ function createIssueOutbox({ storage, fetchJson, coordinator, backgroundSync, ge } function localIndexItem(item) { - if (!item?.attachment?.data) return item; + if (!item?.attachment?.data && !item?.attachment?.blob) return item; return { ...item, attachment: { @@ -103,13 +105,13 @@ function createIssueOutbox({ storage, fetchJson, coordinator, backgroundSync, ge async function hydrateForEdit(id) { const item = read().find(candidate => candidate.id === id); if (!item) return null; - if (!item.attachment?.stored || item.attachment.data) return { ...item }; + if (!item.attachment?.stored || item.attachment.data || item.attachment.blob) return { ...item }; if (!backgroundSync?.get) { throw new Error('The saved screenshot is unavailable. Retry before editing this issue.'); } const durable = await backgroundSync.get(id); const attachment = captureAttachment(durable?.attachment); - if (!attachment?.data || durable?.operationId !== item.operationId) { + if ((!attachment?.data && !attachment?.blob) || durable?.operationId !== item.operationId) { throw new Error('The saved screenshot is unavailable. Retry before editing this issue.'); } return { ...item, attachment }; @@ -196,6 +198,19 @@ function createIssueOutbox({ storage, fetchJson, coordinator, backgroundSync, ge return String(operationId || '').slice(0, 128 - suffix.length) + suffix; } + function attachmentMultipart(attachment) { + let blob = attachment?.blob; + if (!blob && attachment?.data) { + const binary = atob(String(attachment.data)); + const bytes = Uint8Array.from(binary, character => character.charCodeAt(0)); + blob = new Blob([bytes], { type: String(attachment.contentType || '') }); + } + if (!blob) throw new Error('The saved screenshot is unavailable. Retry before sending.'); + const form = new FormData(); + form.append('file', blob, String(attachment.filename || 'screenshot')); + return form; + } + async function sendDirect(item, repository) { let issue = item.deliveredIssue; if (!issue) { @@ -221,14 +236,10 @@ function createIssueOutbox({ storage, fetchJson, coordinator, backgroundSync, ge { method: 'POST', headers: { - Accept: 'application/json', 'Content-Type': 'application/json', + Accept: 'application/json', 'Idempotency-Key': stageOperationId(item.operationId, 'attachment'), }, - body: JSON.stringify({ - filename: item.attachment.filename, - content_type: item.attachment.contentType, - data: item.attachment.data, - }), + body: attachmentMultipart(item.attachment), }, ); markdown = String(uploaded?.markdown || ''); diff --git a/requirements.txt b/requirements.txt index 0446007..f6c7419 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,7 @@ fastapi==0.133.1 httpx==0.28.1 pydantic==2.13.4 +python-multipart==0.0.22 pytest==9.1.1 rjsmin==1.2.5 uvicorn==0.41.0 diff --git a/src/main.py b/src/main.py index 3ee5679..2baa82d 100644 --- a/src/main.py +++ b/src/main.py @@ -19,7 +19,8 @@ from fastapi import FastAPI, Header, HTTPException, Path as PathParam, Query, Re from fastapi.exceptions import RequestValidationError from fastapi.responses import JSONResponse, RedirectResponse from fastapi.staticfiles import StaticFiles -from pydantic import BaseModel, Field, PositiveInt, field_validator, model_validator +from pydantic import BaseModel, Field, PositiveInt, ValidationError, field_validator, model_validator +from starlette.datastructures import UploadFile from src import dashboard_auth, gitea_proxy, passkeys from src.available_issue_snapshot_store import AvailableIssueSnapshotStore @@ -359,6 +360,34 @@ class IssueComment(BaseModel): return value +def _validate_attachment_metadata(filename: str, content_type: str) -> None: + expected = { + "image/png": {"png"}, + "image/jpeg": {"jpg", "jpeg"}, + "image/webp": {"webp"}, + } + if content_type not in expected: + raise ValueError("attachment content type must be PNG, JPEG, or WebP") + if "/" in filename or "\\" in filename or any(ord(character) < 32 for character in filename): + raise ValueError("attachment filename must be a plain file name") + extension = filename.rsplit(".", 1)[-1].lower() if "." in filename else "" + if extension not in expected[content_type]: + raise ValueError("attachment filename must match the selected image type") + + +def _validate_attachment_content(content_type: str, content: bytes) -> bytes: + if len(content) > 2 * 1024 * 1024: + raise ValueError("screenshot must be 2 MB or smaller") + signatures = { + "image/png": content.startswith(b"\x89PNG\r\n\x1a\n"), + "image/jpeg": content.startswith(b"\xff\xd8\xff"), + "image/webp": content.startswith(b"RIFF") and content[8:12] == b"WEBP", + } + if not signatures.get(content_type, False): + raise ValueError("file contents do not match the selected image type") + return content + + class IssueAttachment(BaseModel): filename: str = Field(min_length=1, max_length=255) content_type: Literal["image/png", "image/jpeg", "image/webp"] @@ -366,20 +395,7 @@ class IssueAttachment(BaseModel): @model_validator(mode="after") def validate_filename(self): - if ( - "/" in self.filename - or "\\" in self.filename - or any(ord(character) < 32 for character in self.filename) - ): - raise ValueError("attachment filename must be a plain file name") - extension = self.filename.rsplit(".", 1)[-1].lower() if "." in self.filename else "" - expected = { - "image/png": {"png"}, - "image/jpeg": {"jpg", "jpeg"}, - "image/webp": {"webp"}, - } - if extension not in expected[self.content_type]: - raise ValueError("attachment filename must match the selected image type") + _validate_attachment_metadata(self.filename, self.content_type) return self def content(self) -> bytes: @@ -387,16 +403,12 @@ class IssueAttachment(BaseModel): content = base64.b64decode(self.data, validate=True) except (ValueError, binascii.Error) as exc: raise ValueError("attachment data must be valid base64") from exc - if len(content) > 2 * 1024 * 1024: - raise ValueError("screenshot must be 2 MB or smaller") - signatures = { - "image/png": content.startswith(b"\x89PNG\r\n\x1a\n"), - "image/jpeg": content.startswith(b"\xff\xd8\xff"), - "image/webp": content.startswith(b"RIFF") and content[8:12] == b"WEBP", - } - if not signatures[self.content_type]: - raise ValueError("file contents do not match the selected image type") - return content + return _validate_attachment_content(self.content_type, content) + + +def _validate_binary_attachment(filename: str, content_type: str, content: bytes) -> bytes: + _validate_attachment_metadata(filename, content_type) + return _validate_attachment_content(content_type, content) class IssueCreation(BaseModel): @@ -3107,7 +3119,7 @@ async def comment_on_assigned_issue( @app.post("/api/v1/repos/{owner}/{repo}/issues/{number}/attachments", status_code=201) async def attach_to_assigned_issue( - attachment: IssueAttachment, + request: Request, owner: str, repo: str, number: int = PathParam(gt=0), @@ -3115,15 +3127,27 @@ async def attach_to_assigned_issue( ): repository = f"{owner}/{repo}" try: - content = attachment.content() - except ValueError as exc: + if request.headers.get("content-type", "").lower().startswith("multipart/form-data"): + form = await request.form() + uploaded = form.get("file") + if not isinstance(uploaded, UploadFile): + raise ValueError("screenshot file is required") + filename = str(uploaded.filename or "") + content_type = str(uploaded.content_type or "") + content = _validate_binary_attachment(filename, content_type, await uploaded.read()) + else: + attachment = IssueAttachment.model_validate(await request.json()) + filename = attachment.filename + content_type = attachment.content_type + content = attachment.content() + except (ValueError, ValidationError) as exc: raise HTTPException(status_code=422, detail=str(exc)) from exc async def upload_attachment(): result = await gitea_proxy.upload_assigned_issue_attachment( repository, number, - attachment.filename, - attachment.content_type, + filename, + content_type, content, ) safe_name = ( @@ -3142,8 +3166,8 @@ async def attach_to_assigned_issue( "issue-attachment", repository, number, - attachment.filename, - attachment.content_type, + filename, + content_type, hashlib.sha256(content).hexdigest(), ), timeout=ISSUE_ACTION_TIMEOUT_SECONDS, diff --git a/src/request_boundary.py b/src/request_boundary.py index 38ffba4..69c8737 100644 --- a/src/request_boundary.py +++ b/src/request_boundary.py @@ -5,7 +5,8 @@ from starlette.types import ASGIApp, Message, Receive, Scope, Send SESSION_BODY_LIMIT = 16 * 1024 API_MUTATION_BODY_LIMIT = 64 * 1024 -ISSUE_ATTACHMENT_BODY_LIMIT = 3 * 1024 * 1024 +ISSUE_ATTACHMENT_BODY_LIMIT = 2 * 1024 * 1024 + 64 * 1024 +LEGACY_JSON_ATTACHMENT_BODY_LIMIT = 3 * 1024 * 1024 MUTATION_METHODS = frozenset({"POST", "PUT", "PATCH"}) @@ -50,6 +51,11 @@ class RequestBodyLimitMiddleware: if limit is None: await self.app(scope, receive, send) return + if ( + limit == ISSUE_ATTACHMENT_BODY_LIMIT + and self._header(scope, b"content-type").startswith(b"application/json") + ): + limit = LEGACY_JSON_ATTACHMENT_BODY_LIMIT declared_length = self._content_length(scope) if declared_length is not None and declared_length > limit: @@ -77,6 +83,13 @@ class RequestBodyLimitMiddleware: await self.app(scope, replay, send) + @staticmethod + def _header(scope: Scope, wanted: bytes) -> bytes: + for name, value in scope.get("headers", []): + if name.lower() == wanted: + return value.lower() + return b"" + @staticmethod def _content_length(scope: Scope) -> int | None: for name, value in scope.get("headers", []): diff --git a/tests/test_authored_outbox.py b/tests/test_authored_outbox.py index 8ac2581..3e2846b 100644 --- a/tests/test_authored_outbox.py +++ b/tests/test_authored_outbox.py @@ -498,7 +498,7 @@ async def test_mobile_dashboard_loads_and_operates_authored_message_outbox(): assert "authoredOutbox.reconcileBackground(records)" in html -def test_screenshot_comment_admits_bytes_to_indexeddb_without_putting_them_in_localstorage(): +def test_screenshot_comment_admits_blob_to_indexeddb_without_putting_bytes_in_localstorage(): script = f""" const createAuthoredOutbox = require({json.dumps(str(OUTBOX))}); const values = new Map(); const mirrors=[]; @@ -508,12 +508,15 @@ const outbox=createAuthoredOutbox({{ backgroundSync:{{reconcile:async(items,lane)=>mirrors.push({{items,lane}}),requestSync:async()=>{{}}}}, }}); (async()=>{{ + const blob=new Blob(['PRIVATE-IMAGE-BYTES'],{{type:'image/png'}}); const admission=await outbox.enqueueDurably({{ kind:'issue-comment',repository:'stackchain/dashboard',number:477, body:'Broken at 320px',operationId:'comment-image-477', - attachment:{{filename:'phone.png',contentType:'image/png',data:'PRIVATE-IMAGE-BYTES'}}, + attachment:{{filename:'phone.png',contentType:'image/png',blob}}, }}); - process.stdout.write(JSON.stringify({{admission,local:values.get('stackchain.authored-outbox.v1'),mirrored:mirrors[0]}})); + const durable=mirrors[0].items[0].attachment; + process.stdout.write(JSON.stringify({{admission,local:values.get('stackchain.authored-outbox.v1'), + mirrored:{{lane:mirrors[0].lane,isBlob:durable.blob instanceof Blob,size:durable.blob?.size,text:await durable.blob?.text()}}}})); }})(); """ output = run_node(script) @@ -523,8 +526,9 @@ const outbox=createAuthoredOutbox({{ assert local_item["attachment"] == { "filename": "phone.png", "contentType": "image/png", "stored": True } - assert output["mirrored"]["lane"] == "authored" - assert output["mirrored"]["items"][0]["attachment"]["data"] == "PRIVATE-IMAGE-BYTES" + assert output["mirrored"] == { + "lane": "authored", "isBlob": True, "size": 19, "text": "PRIVATE-IMAGE-BYTES" + } assert output["admission"]["durability"] == "background" diff --git a/tests/test_background_issue_sync.py b/tests/test_background_issue_sync.py index db7d51f..fd2ac8d 100644 --- a/tests/test_background_issue_sync.py +++ b/tests/test_background_issue_sync.py @@ -82,7 +82,10 @@ const store = {{ }}; let uploadAttempts=0; const fetchJson=async(url,options={{}})=>{{ - state.calls.push({{url,key:options.headers?.['Idempotency-Key'],body:options.body&&JSON.parse(options.body)}}); + let body=null; + if(options.body instanceof FormData){{const file=options.body.get('file');body={{filename:file.name,multipart:true}};}} + else if(options.body)body=JSON.parse(options.body); + state.calls.push({{url,key:options.headers?.['Idempotency-Key'],body}}); if(url==='api/v1/background-identity')return{{login:'timmy'}}; if(url.endsWith('/issues'))return{{repository:'stackchain/dashboard',number:469,title:'Broken mobile layout'}}; if(url.endsWith('/attachments') && uploadAttempts++ === 0){{const error=new Error('Upload unavailable');error.status=503;throw error;}} @@ -113,6 +116,37 @@ const fetchJson=async(url,options={{}})=>{{ assert output["second"]["confirmed"][0]["number"] == 469 +def test_closed_app_sync_uploads_blob_as_multipart_and_preserves_original_bytes(): + script = f""" +const createBackgroundIssueSync=require({json.dumps(str(SYNC))}); +const blob=new Blob(['offline-binary'],{{type:'image/png'}}); +const item={{id:'binary',operationId:'binary',ownerLogin:'timmy',repository:'o/r',title:'Visual',body:'',labelIds:[], + attachment:{{filename:'phone.png',contentType:'image/png',blob}}}}; +let queued=true; +const observed={{}}; +const store={{claimNext:async()=>queued?(queued=false,{{...item}}):null,update:async()=>{{}},complete:async()=>{{}},release:async()=>{{}},fail:async()=>{{}},countBlocked:async()=>0}}; +const fetchJson=async(url,options={{}})=>{{ + if(url==='api/v1/background-identity')return{{login:'timmy'}}; + if(url.endsWith('/issues'))return{{number:7}}; + if(url.endsWith('/attachments')){{const file=options.body.get('file');Object.assign(observed,{{ + isFormData:options.body instanceof FormData,contentTypeHeader:options.headers['Content-Type']||null, + filename:file.name,type:file.type,size:file.size,text:await file.text() + }});return{{markdown:'![phone](url)'}};}} + return{{id:8}}; +}}; +(async()=>{{await createBackgroundIssueSync({{store,fetchJson}}).flush();process.stdout.write(JSON.stringify(observed));}})(); +""" + + assert run_node(script) == { + "isFormData": True, + "contentTypeHeader": None, + "filename": "phone.png", + "type": "image/png", + "size": 14, + "text": "offline-binary", + } + + def test_capture_attachment_stage_keys_remain_within_idempotency_limit(): script = f""" const createBackgroundIssueSync=require({json.dumps(str(SYNC))}); @@ -175,7 +209,10 @@ const store={{claimNext:async()=>item?{{...item}}:null,update:async(_id,transfor fail:async()=>{{}},countBlocked:async()=>0}}; const fetchJson=async(url,options={{}})=>{{ if(url==='api/v1/background-identity')return{{login:'timmy'}}; - state.calls.push({{url,key:options.headers?.['Idempotency-Key'],body:options.body&&JSON.parse(options.body)}}); + let body=null; + if(options.body instanceof FormData){{const file=options.body.get('file');body={{filename:file.name,multipart:true}};}} + else if(options.body)body=JSON.parse(options.body); + state.calls.push({{url,key:options.headers?.['Idempotency-Key'],body}}); if(url.endsWith('/attachments'))return{{markdown:'![phone.png](https://forge.example/phone.png)'}}; if(commentAttempts++===0){{const error=new Error('Comment unavailable');error.status=503;throw error;}} return{{id:91}}; @@ -435,7 +472,7 @@ const transaction=work=>{{const run=tail.then(()=>work({{ ] -def test_reconciling_a_lightweight_attachment_reference_preserves_indexeddb_bytes(): +def test_reconciling_a_lightweight_attachment_reference_preserves_indexeddb_blob(): script = f""" const createBackgroundIssueSync = require({json.dumps(str(SYNC))}); const records=new Map();let tail=Promise.resolve(); @@ -447,21 +484,23 @@ const transaction=work=>{{const run=tail.then(()=>work({{ const store=createBackgroundIssueSync.createIssueSyncStore({{transaction}}); await store.reconcile([{{ id:'first',operationId:'first',ownerLogin:'timmy',status:'queued', - attachment:{{filename:'one.png',contentType:'image/png',data:'first-image-bytes'}}, + attachment:{{filename:'one.png',contentType:'image/png',blob:new Blob(['first-image-bytes'],{{type:'image/png'}})}}, }}]); await store.reconcile([ {{id:'first',operationId:'first',ownerLogin:'timmy',status:'queued', attachment:{{filename:'one.png',contentType:'image/png',stored:true}}}}, {{id:'second',operationId:'second',ownerLogin:'timmy',status:'queued', - attachment:{{filename:'two.png',contentType:'image/png',data:'second-image-bytes'}}}}, + attachment:{{filename:'two.png',contentType:'image/png',data:'c2Vjb25kLWltYWdlLWJ5dGVz'}}}}, ]); - process.stdout.write(JSON.stringify(await store.snapshot())); + const snapshot=await store.snapshot(); + process.stdout.write(JSON.stringify({{first:{{isBlob:snapshot[0].attachment.blob instanceof Blob, + text:await snapshot[0].attachment.blob?.text()}},second:snapshot[1].attachment.data}})); }})(); """ output = run_node(script) - assert output[0]["attachment"]["data"] == "first-image-bytes" - assert output[1]["attachment"]["data"] == "second-image-bytes" + assert output["first"] == {"isBlob": True, "text": "first-image-bytes"} + assert output["second"] == "c2Vjb25kLWltYWdlLWJ5dGVz" def test_issue_sync_store_hydrates_one_capture_by_key_without_scanning_all_records(): diff --git a/tests/test_issue_attachment_ui.py b/tests/test_issue_attachment_ui.py index 1389a65..9e89811 100644 --- a/tests/test_issue_attachment_ui.py +++ b/tests/test_issue_attachment_ui.py @@ -18,15 +18,19 @@ def run_node(script: str) -> str: ).stdout -def test_mobile_attachment_prepares_comment_and_reuses_confirmed_upload(): +def test_mobile_attachment_prepares_comment_and_reuses_confirmed_binary_upload(): script = f""" const attachment = require({json.dumps(str(ATTACHMENT))}); const calls = []; -const file = {{name:'checkout.png', type:'image/png', size:8}}; +const file = new Blob(['png-bytes'],{{type:'image/png'}}); file.name='checkout.png'; const controller = attachment.create({{ createOperationId: () => 'attachment-comment-471', - readDataUrl: async selected => {{ calls.push('read:' + selected.name); return 'data:image/png;base64,iVBORw0KGgo='; }}, - upload: async payload => {{ calls.push(payload); return {{markdown:'![checkout.png](https://forge.example/a.png)'}}; }}, + readDataUrl: async () => {{ throw new Error('base64 conversion must not run'); }}, + upload: async payload => {{ calls.push({{ + repository:payload.repository,number:payload.number,filename:payload.filename, + content_type:payload.content_type,isBlob:payload.blob instanceof Blob, + text:await payload.blob.text(),operation_id:payload.operation_id + }}); return {{markdown:'![checkout.png](https://forge.example/a.png)'}}; }}, }}); controller.select(file); (async()=>{{ @@ -40,19 +44,17 @@ controller.select(file); "Layout breaks\n\n![checkout.png](https://forge.example/a.png)" ) assert output["second"] == output["first"] - assert output["calls"] == [ - "read:checkout.png", - { - "repository": "stackchain/api", - "number": 17, - "filename": "checkout.png", - "content_type": "image/png", - "data": "iVBORw0KGgo=", - "operation_id": "attachment-comment-471", - }, - ] + assert output["calls"] == [{ + "repository": "stackchain/api", + "number": 17, + "filename": "checkout.png", + "content_type": "image/png", + "isBlob": True, + "text": "png-bytes", + "operation_id": "attachment-comment-471", + }] assert output["state"] == { - "name": "checkout.png", "size": 8, "uploaded": True + "name": "checkout.png", "size": 9, "uploaded": True } @@ -82,26 +84,50 @@ controller.select({{name:'first.png',type:'image/png',size:8}}); ] -def test_selected_screenshot_serializes_for_durable_issue_capture_without_uploading(): +def test_selected_screenshot_serializes_as_binary_blob_without_base64_expansion(): script = f""" const attachment = require({json.dumps(str(ATTACHMENT))}); const calls=[]; +const file=new Blob(['binary-image'],{{type:'image/webp'}}); +file.name='phone.webp'; const controller=attachment.create({{ - readDataUrl:async file=>{{calls.push('read:'+file.name);return 'data:image/webp;base64,UklGRg==';}}, + readDataUrl:async()=>{{throw new Error('base64 conversion must not run');}}, upload:async()=>{{calls.push('upload');}}, }}); -controller.select({{name:'phone.webp',type:'image/webp',size:8}}); -controller.serialize().then(value=>process.stdout.write(JSON.stringify({{value,calls}}))); +controller.select(file); +controller.serialize().then(async value=>process.stdout.write(JSON.stringify({{ + filename:value.filename,contentType:value.contentType, + isBlob:value.blob instanceof Blob,size:value.blob.size,text:await value.blob.text(),calls +}}))); """ output = json.loads(run_node(script)) assert output == { - "value": { - "filename": "phone.webp", - "contentType": "image/webp", - "data": "UklGRg==", - }, - "calls": ["read:phone.webp"], + "filename": "phone.webp", + "contentType": "image/webp", + "isBlob": True, + "size": 12, + "text": "binary-image", + "calls": [], + } + + +def test_binary_screenshot_builds_multipart_body_with_original_bytes(): + script = f""" +const attachment=require({json.dumps(str(ATTACHMENT))}); +const blob=new Blob(['original-bytes'],{{type:'image/png'}}); +const form=attachment.multipart({{filename:'phone.png',contentType:'image/png',blob}}); +const file=form.get('file'); +(async()=>process.stdout.write(JSON.stringify({{ + filename:file.name,type:file.type,size:file.size,text:await file.text() +}})))(); +""" + + assert json.loads(run_node(script)) == { + "filename": "phone.png", + "type": "image/png", + "size": 14, + "text": "original-bytes", } @@ -220,7 +246,7 @@ process.stdout.write(JSON.stringify({{invalid,selected,removed:{{hidden:preview. } -def test_issue_comment_actions_upload_before_posting_and_clear_after_acceptance(): +def test_issue_comment_actions_upload_binary_multipart_before_posting_and_clear_after_acceptance(): source = DASHBOARD.read_text() assert "issueAttachment.mount({" in source @@ -229,6 +255,7 @@ def test_issue_comment_actions_upload_before_posting_and_clear_after_acceptance( assert "issueAttachmentController.prepareComment(selectedIssue, body)" in source assert source.count("issueAttachmentController.clear();") >= 2 assert "'Idempotency-Key': payload.operation_id" in source + assert "body: issueAttachment.multipart(payload)" in source def test_closing_issue_sheet_cannot_carry_a_screenshot_to_another_issue(): diff --git a/tests/test_issue_attachments.py b/tests/test_issue_attachments.py index 10ff6f4..d518952 100644 --- a/tests/test_issue_attachments.py +++ b/tests/test_issue_attachments.py @@ -58,6 +58,32 @@ async def test_attachment_endpoint_uploads_valid_screenshot_to_assigned_issue(mo ] +@pytest.mark.anyio +async def test_attachment_endpoint_accepts_binary_multipart_without_base64_expansion(monkeypatch): + calls = [] + + async def upload(repository, number, filename, content_type, content): + calls.append((repository, number, filename, content_type, content)) + return { + "name": filename, + "url": "https://forge.example/attachments/checkout.png", + "size": len(content), + } + + monkeypatch.setattr(main.gitea_proxy, "upload_assigned_issue_attachment", upload) + transport = httpx.ASGITransport(app=main.app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + response = await client.post( + "/api/v1/repos/stackchain/api/issues/17/attachments", + files={"file": ("checkout.png", PNG_BYTES, "image/png")}, + ) + + assert response.status_code == 201 + assert calls == [ + ("stackchain/api", 17, "checkout.png", "image/png", PNG_BYTES) + ] + + @pytest.mark.anyio async def test_attachment_endpoint_replays_confirmed_upload_for_same_key(monkeypatch): calls = [] diff --git a/tests/test_issue_outbox.py b/tests/test_issue_outbox.py index 1557a28..7678a33 100644 --- a/tests/test_issue_outbox.py +++ b/tests/test_issue_outbox.py @@ -234,20 +234,27 @@ const calls=[];let uploadAttempts=0; const outbox=createIssueOutbox({{ storage,getOwnerLogin:()=>'timmy',createOperationId:()=>'image-op', fetchJson:async(url,options={{}})=>{{ - calls.push({{url,key:options.headers?.['Idempotency-Key'],body:options.body&&JSON.parse(options.body)}}); + const form=options.body instanceof FormData ? options.body : null; + const file=form?.get('file'); + calls.push({{url,key:options.headers?.['Idempotency-Key'],multipart:Boolean(form), + filename:file?.name,text:file?await file.text():null}}); if(url.endsWith('/issues'))return{{repository:'o/r',number:7,title:'Visual bug'}}; if(url.endsWith('/attachments') && uploadAttempts++ === 0){{const error=new Error('offline');error.status=503;throw error;}} if(url.endsWith('/attachments'))return{{markdown:'![screen.png]()'}}; return{{id:8}}; }}, }}); -const queued=outbox.enqueue({{repository:'o/r',title:'Visual bug',attachment:{{filename:'screen.png',contentType:'image/png',data:'abc'}}}}); +const queued=outbox.enqueue({{repository:'o/r',title:'Visual bug',attachment:{{filename:'screen.png',contentType:'image/png',data:'b2ZmbGluZQ=='}}}}); (async()=>{{await outbox.flush('timmy');const partial=outbox.list()[0];const result=await outbox.retry(queued.id,'timmy');process.stdout.write(JSON.stringify({{calls,partial,result,remaining:outbox.list()}}));}})(); """ output = run_node(script) assert output["partial"]["deliveredIssue"]["number"] == 7 assert [call["url"] for call in output["calls"]].count("api/v1/repos/o/r/issues") == 1 + uploads = [call for call in output["calls"] if call["url"].endswith("/attachments")] + assert uploads[-1]["multipart"] is True + assert uploads[-1]["filename"] == "screen.png" + assert uploads[-1]["text"] == "offline" assert output["calls"][-1]["url"] == "api/v1/repos/o/r/issues/7/comments" assert output["result"]["confirmed"][0]["number"] == 7 assert output["remaining"] == [] @@ -452,14 +459,14 @@ Promise.resolve().then(async () => {{ assert output["items"][0]["title"] == "Keep this" -def test_durable_screenshot_is_mirrored_before_a_payload_free_local_index_is_committed(): +def test_durable_screenshot_is_mirrored_as_blob_before_a_payload_free_local_index_is_committed(): script = f""" const createIssueOutbox = require({json.dumps(str(OUTBOX))}); const values = new Map(); const events = []; const snapshots = []; const storage = {{ getItem:key => values.get(key) || null, setItem:(key,value) => {{ - if (value.includes('base64-screenshot-bytes')) throw new Error('screenshot leaked into localStorage'); + if (value.includes('binary-screenshot-bytes')) throw new Error('screenshot leaked into localStorage'); events.push('local-index'); values.set(key,value); }}, }}; @@ -471,26 +478,35 @@ const outbox = createIssueOutbox({{ }}, }}); (async () => {{ + const blob=new Blob(['binary-screenshot-bytes'],{{type:'image/webp'}}); const result = await outbox.enqueueDurably({{ repository:'stackchain/dashboard', title:'Mobile layout', - attachment:{{filename:'phone.webp',contentType:'image/webp',data:'base64-screenshot-bytes'}}, + attachment:{{filename:'phone.webp',contentType:'image/webp',blob}}, }}); + const durable=snapshots[0][0].attachment; process.stdout.write(JSON.stringify({{ - events, mirrored:snapshots[0][0], local:outbox.list()[0], result:result.item, - raw:values.get('stackchain.issue-outbox.v1'), + events, mirrored:{{filename:durable.filename,contentType:durable.contentType, + isBlob:durable.blob instanceof Blob,size:durable.blob?.size,text:await durable.blob?.text()}}, + local:outbox.list()[0], result:result.item, raw:values.get('stackchain.issue-outbox.v1'), }})); }})(); """ output = run_node(script) assert output["events"] == ["indexeddb", "local-index", "sync"] - assert output["mirrored"]["attachment"]["data"] == "base64-screenshot-bytes" + assert output["mirrored"] == { + "filename": "phone.webp", + "contentType": "image/webp", + "isBlob": True, + "size": 23, + "text": "binary-screenshot-bytes", + } assert output["local"]["attachment"] == { "filename": "phone.webp", "contentType": "image/webp", "stored": True, } - assert "base64-screenshot-bytes" not in output["raw"] + assert "binary-screenshot-bytes" not in output["raw"] assert output["result"]["attachment"] == output["local"]["attachment"] diff --git a/tests/test_request_boundary.py b/tests/test_request_boundary.py index f460ed2..1144a85 100644 --- a/tests/test_request_boundary.py +++ b/tests/test_request_boundary.py @@ -116,7 +116,7 @@ def test_request_limits_are_route_specific_and_cover_api_mutations(): main.request_body_limit( "POST", "/api/v1/repos/stackchain/project/issues/17/attachments" ) - == 3 * 1024 * 1024 + == 2 * 1024 * 1024 + 64 * 1024 ) assert main.request_body_limit("POST", "/unrelated/attachments") is None assert main.request_body_limit("POST", "/unrelated") is None -- 2.43.0