Skip to content

Enforce Firefox native messaging output limits + Windows harness fix - #21

Merged
SunkenInTime merged 3 commits into
mainfrom
devin/pr18-windows-harness-fix
Sep 10, 2026
Merged

SunkenInTime merged 3 commits into
mainfrom
devin/pr18-windows-harness-fix

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

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):

  • Split the single 1 GiB native-message ceiling into MAX_NATIVE_INPUT_MESSAGE_BYTES (1 GiB browser→host) and MAX_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_messages now returns NativeOutputOutcome (CleanEof/Fatal) over an mpsc channel; on Fatal the supervisor kills and reaps the host, prints the diagnostic, and exits 1. forward_stderr watches a fatal_output flag so shutdown no longer waits on descendants retaining stderr.
  • File-upload enrichment is preflighted: pending_file_payload canonicalizes + 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 with take(byte_len). Forged _firefoxFilePayloads keys are stripped before re-enrichment.
  • Equivalent POSIX + Windows fixtures and tests for exact limits, oversized headers, delayed hosts, later frames, enrichment overflow, and directionality.

New commit on top (79e910c, Windows harness only — tests/test-native-host.mjs):

  • runBridge measures durationMilliseconds on the child 'exit' event instead of 'close'. The C# fixture's SpawnDescendantThatRetainsStderr uses Process.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 — exit is the correct signal for "bridge terminated promptly".
  • rustCanonicalPath uses fs.realpathSync.native on Windows: GetFinalPathNameByHandle expands 8.3 short names (e.g. ADMINI~1 → Administrator), matching Rust fs::canonicalize and eliminating a 5-byte projected-length drift in oversized-file-upload.
  • The final fs.rmSync(temp, …) now uses maxRetries/retryDelay so a still-running fixture.exe can't surface EPERM and mask the real assertion error.

Verification

  • Windows Server 2022 (stable-msvc Rust 1.98.1, Node 20): cargo test --locked 23/23, clippy -D warnings clean, cargo fmt --check clean, npm test fully green — including output-above-limit-header-then-stderr-descendant (~160ms) and oversized-file-upload (exact projected-length match).
  • Linux: cargo test 25/25, npm test green.

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

2bTwist and others added 3 commits August 29, 2026 17:26
- 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-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a4455cc8-b079-4363-b21b-cc6e15e35a3d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

Safe to merge for the reviewed response-limit, file-enrichment, and fatal-shutdown behavior.

What we checked:

  • Oversized upstream output frame: The bridge compares the upstream frame length with the output limit before allocating the payload. A violating frame becomes fatal rather than being sent to Firefox. T-Rex
  • Post-enrichment output overflow: The bridge treats enrichment overflow as fatal and checks the serialized enriched response before writing a native frame. The enlarged message is not emitted to Firefox. T-Rex
  • Oversized file-enrichment read: The bridge projects base64 expansion from validated metadata before the read phase. It rejects over-budget responses before read_to_end or base64 encoding is reached. T-Rex
  • Fatal-output shutdown stall: On fatal output, the supervisor kills and waits for the direct child, then skips joining the stderr thread. A descendant retaining stderr therefore cannot defer the bridge exit. T-Rex
  • The bridge enforces size checks before allocation, rejecting frames larger than the maximum and validating a boundary where exactly 1 MiB is accepted while 1 MiB plus one byte or a complete oversized frame is rejected. T-Rex
  • The current bridge rejects enriching a maximum-size 1 MiB getInfo frame into a 1,048,609-byte message, exits nonzero, and emits no Firefox native-message frame, with the focused enrichment harness and tests passing. T-Rex
  • The preflight uses file metadata and Base64 expansion to project the serialized response size before entering the file-reading loop, and oversized uploads are skipped with no Firefox output when the projection is too large. T-Rex

Summary

  • This change makes the Firefox and Zen native bridge enforce Firefox’s response-size limit while retaining the larger browser-to-host request allowance. It also rejects oversized file uploads before reading their contents and exits promptly after fatal host output failures.
  • Focused checks confirmed that valid 1 MiB responses pass, oversized headers and complete frames produce no Firefox output, enrichment overflow is rejected, oversized uploads are stopped before file reads, and an inherited stderr pipe does not delay fatal shutdown.

Reviews (1) · Last reviewed commit: "Fix Windows native-host test harness"

@SunkenInTime
SunkenInTime merged commit b9147b0 into main Sep 10, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants