Repository navigation
Conversation
There was a problem hiding this comment.
🤖 AI Code Review — 💬 Needs discussion
Confidence: low · Findings: 6
AI judgment: NEEDS DISCUSSION · Evidence: COMPLETE · Authority: advisory
Evidence complete · 10/10 changed files accounted for
Other findings (6)
- [low] Multipart uploads are fully written to disk before the size limit is enforced
- [low] Temp-file cleanup misses CancelledError (BaseException) paths
- [low] click_send upload path stalls ~30s when 'File upload pending' never appears, and flag reset is skipped on error
- [medium] Shared per-driver
_file_upload_activeflag is a cross-request race under concurrency⚠️ unverified - [low] Untrusted multipart filename flows into browser automation with no sanitization
⚠️ unverified
🔮 Devil's Advocate (2 turns): 1 downgraded · 3 missed risks found · 2 upheld
GitWire AI Review Gate (bundle-driven v2) · Structured schema · Scope-validated · Devil's Advocate · AI recommendation — maintainers and repository policy retain authority
Finding details (not annotatable inline)
-
[MEDIUM] Shared per-driver
_file_upload_activeflag is a cross-request race under concurrencyFinding #2 treats the flag as a per-request concern, but it lives on the shared driver object. If the server serves more than one request (or more than one chat) against the same browser instance, request A's upload sets the flag True and request B's plain-text send — arriving before A's click_send resets it — is misrouted through the upload-wait/CDP path, or A's state is cleared by B. The review flagged the reset-on-error gap but missed the inherent shared-mutable-state design flaw that the reset fix cannot solve.
-
[LOW] Untrusted multipart filename flows into browser automation with no sanitization
In a Web2API bridge the original client-supplied filename is typically interpolated into DOM selectors or JS snippets used to locate the uploaded chip on the ChatGPT page. If that's the case here, a filename containing quotes, backslashes, or HTML breaks the selector or injects script into the logged-in page context (self-XSS at worst, broken uploads for legitimate exotic filenames at best). The review scrutinized the upload bytes but never the upload name — the classic blind spot in file-upload reviews.
-
[LOW] Rejected uploads may leak temp files on the 400/413 path, not just the cancellation path
Finding #1 covers only the CancelledError leak, but validate_upload_files runs after materialization: if it rejects a file and surfaces a 400/413 without raising through the same except-Exception cleanup block (e.g., by returning an error response directly), every rejected upload permanently leaks its temp file. This is arguably the HIGHEST-frequency leak path — it triggers on routine policy rejections, not rare cancellations — and the review's proposed try/finally fix only helps if the validation failure actually propagates as an exception through that block. Worth verifying; if validation raises, this merges into finding #1's fix.
| ) | ||
| path = Path(handle.name) | ||
| try: | ||
| while chunk := await part.read_chunk(): |
There was a problem hiding this comment.
[LOW] Multipart uploads are fully written to disk before the size limit is enforced
_parse_chat_request streams every file part to a NamedTemporaryFile with no bound, and validate_upload_files (which enforces W2A_MAX_UPLOAD_BYTES) runs only after the entire body has been materialized. A single authenticated request with one oversized part (or a very large number of parts, since there is no count cap) fills the temp filesystem before the server returns 400. Fix: enforce the limit inside the read_chunk loop — track bytes written per part (and total), abort and delete the partial file as soon as it exceeds max_upload_bytes(), and cap the number of file parts. Also note handle.write(chunk) is synchronous and blocks the event loop per chunk for large uploads; consider asyncio.to_thread for the write.
| raise UploadError("Multipart requests must include a JSON 'messages' field") | ||
| if upload_paths: | ||
| validate_upload_files(upload_paths) | ||
| return body, upload_paths |
There was a problem hiding this comment.
[LOW] Temp-file cleanup misses CancelledError (BaseException) paths
Both cleanup sites use except Exception, which does not catch asyncio.CancelledError (a BaseException since Python 3.8). If the client disconnects or the handler task is cancelled while multipart parts are being streamed, the except Exception: block in _parse_chat_request is skipped and already-written temp files leak in the temp directory; the outer except Exception as e: in _handle_chat has the same gap. Fix: perform the unlink loops in a try/finally (or catch BaseException, clean up, and re-raise) so temp files are removed on cancellation.
| # With attachments, ChatGPT starts the upload on the first send | ||
| # click. The button remains enabled, but the UI briefly exposes | ||
| # "File upload pending" and ignores that click as a submission. | ||
| # Wait for that transition and click again only after it clears. |
There was a problem hiding this comment.
[LOW] click_send upload path stalls ~30s when 'File upload pending' never appears, and flag reset is skipped on error
In the _file_upload_active branch of click_send, the wait loop only breaks after a pending→ready transition (elif saw_pending: break). If the pending state never appears (upload already finished at attach time, or UI wording differs), all 60 iterations sleep 0.5s, adding a fixed ~30s delay to every file-upload send before it (correctly) skips the second click. Additionally, d._file_upload_active = False is only reached on the success path; on the (json.JSONDecodeError, KeyError, TypeError) path the flag stays True, so the next non-upload message is sent via the CDP coordinate-click path instead of the normal JS dispatch. Fix: break the wait on an observed user message/new conversation (or a shorter deadline), and reset _file_upload_active in a finally block.
Summary
DOM.setFileInputFileschat_completion.filesW2A_MAX_UPLOAD_BYTES(default 50 MiB per file)Verification
ruff check src/chatgpt_web2api tests/test_file_upload.py tests/test_e2e_file_upload.pypytest -q— 698 passed, 32 e2e deselectedW2A_E2E_RUN=1 W2A_E2E_PACE=0 W2A_CDP_PORT=9223 pytest tests/test_e2e_file_upload.py -v -s— 1 passedNotes
The live E2E uses a real
.logattachment and verifies that ChatGPT returns the expected sentinel response. The first send click initiates the web upload; the implementation waits forFile upload pendingto clear and then submits the prompt.