Enforce Firefox native messaging output limits + Windows harness fix - #21
Conversation
- Measure bridge termination on 'exit' instead of 'close': a fixture descendant that inherits the std handles keeps the harness pipes open for its full lifetime, masking how quickly the bridge actually exited. - Canonicalize the oversized-upload oracle path with fs.realpathSync.native so 8.3 short names expand the same way Rust's fs::canonicalize expands them. - Retry temp-dir cleanup so a still-running fixture executable cannot turn the real assertion failure into an EPERM. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Summary
Lands PR #18's native-messaging output-limit enforcement plus a follow-up fix for three Windows-only failures found when the new harness actually ran on Windows.
Commits from #18 (unchanged, authored by @2bTwist):
MAX_NATIVE_INPUT_MESSAGE_BYTES(1 GiB browser→host) andMAX_NATIVE_OUTPUT_MESSAGE_BYTES(1 MiB host→browser, Firefox's documented limit), checked both on the raw frame and again after file-payload enrichment.forward_native_messagesnow returnsNativeOutputOutcome(CleanEof/Fatal) over an mpsc channel; onFatalthe supervisor kills and reaps the host, prints the diagnostic, and exits 1.forward_stderrwatches afatal_outputflag so shutdown no longer waits on descendants retaining stderr.pending_file_payloadcanonicalizes + opens the file up front (O_NONBLOCK|O_NOFOLLOW on unix — FIFOs and swapped paths can't block or leak), projects the base64 length before reading, and bounds each read withtake(byte_len). Forged_firefoxFilePayloadskeys are stripped before re-enrichment.New commit on top (
79e910c, Windows harness only —tests/test-native-host.mjs):runBridgemeasuresdurationMillisecondson the child'exit'event instead of'close'. The C# fixture'sSpawnDescendantThatRetainsStderrusesProcess.Start, which on .NET Framework inherits all inheritable handles, so the descendant kept the harness pipes open ~3.2s even though the bridge exited in ~160ms —exitis the correct signal for "bridge terminated promptly".rustCanonicalPathusesfs.realpathSync.nativeon Windows:GetFinalPathNameByHandleexpands 8.3 short names (e.g.ADMINI~1→Administrator), matching Rustfs::canonicalizeand eliminating a 5-byte projected-length drift inoversized-file-upload.fs.rmSync(temp, …)now usesmaxRetries/retryDelayso a still-runningfixture.execan't surface EPERM and mask the real assertion error.Verification
cargo test --locked23/23, clippy-D warningsclean,cargo fmt --checkclean,npm testfully green — includingoutput-above-limit-header-then-stderr-descendant(~160ms) andoversized-file-upload(exact projected-length match).cargo test25/25,npm testgreen.Merging this branch also merges PR #18's commits verbatim, so GitHub will mark #18 merged automatically.
Link to Devin session: https://app.devin.ai/sessions/5e91ca6247894f6280ead28231ec9a4c
Open in Devin Desktop: https://app.devin.ai/desktop/session/5e91ca6247894f6280ead28231ec9a4c?variant=devin
Requested by: @SunkenInTime