Skip to content

fix(gateway): safely deliver local file URLs from image tags (QQBot Windows) - #43332

Open
k176060444-lgtm wants to merge 2 commits into
NousResearch:mainfrom
k176060444-lgtm:work/pr-local-file-qqbot-20260610
Open

fix(gateway): safely deliver local file URLs from image tags (QQBot Windows)#43332
k176060444-lgtm wants to merge 2 commits into
NousResearch:mainfrom
k176060444-lgtm:work/pr-local-file-qqbot-20260610

Conversation

@k176060444-lgtm

@k176060444-lgtm k176060444-lgtm commented Jun 10, 2026

Copy link
Copy Markdown

Summary

Safely deliver local file:// image URLs from explicit markdown/HTML image tags on QQBot (Windows) and other platforms, rebuilt cleanly on current upstream/main during conflict resolution on 2026-08-12.

Current head SHA: 2aaf21f62da1bd3f4cd50aac0627ee2ba30c06c0

Behavior Contract

  • Post-stream delivery remains explicit-only (#20834): bare local paths in an already-streamed reply are treated as text the user has seen, never auto-promoted to attachments; extract_local_files() is NOT called in the post-stream path.
  • Explicit MEDIA: / file:// may be re-sent in later turns (#73771): cross-turn history dedup was removed on main; a MEDIA: directive or explicit file:// image tag in a later turn is the model deliberately attaching a file (including a user-requested resend) and must deliver again.
  • Canonical dedup applies only within a single response: MEDIA:/a.png and file:///C:/a.png referencing the same file deliver once per response; foo.png vs foo.png.backup.png (different files) both deliver; HTTP(S) URLs use exact-string comparison.
  • Background file:// delivery routes through send_multiple_images()send_image_file() (decoded local path), never passed as a literal pathname to the HTTP-only send_image().
  • Windows production encoded-drive URIs accepted: the delivery batching code wraps local paths with the default urllib.parse.quote() (safe=/), which on Windows percent-encodes the drive letter and backslashes into the authority segment (e.g. file://C%3A%5Cpath%5Cimage.png). _normalize_file_url() decodes such URIs back to the local drive path and they still pass through the existing validate_media_delivery_path() security chain (container→host translation, symlink resolve, denylist/credential protection, strict mode).
  • Unsafe URIs rejected: UNC, non-local authority, encoded-UNC, and drive-relative file:// URIs are refused by _normalize_file_url().

Changes (4 files, +1157/-56)

gateway/platforms/base.py (+188/-36)

  • Add _normalize_file_url() to parse and validate file:// URIs (case-insensitive scheme, Windows two/three-slash variants, production default-quote() encoded-drive authority form, rejects UNC / non-local authority / drive-relative).
  • Extend extract_images() to support file:// markdown/HTML image tags with mask-based false-positive prevention (fences/blockquotes/JSON/data-src).
  • send_multiple_images() routes local file:// URIs through _normalize_file_url()send_image_file() (case-insensitive scheme; FILE:// also works).

gateway/run.py (+59/-20)

  • Post-stream _deliver_media_from_response() captures adapter.extract_images(cleaned) and merges explicit file:// image tags with MEDIA: paths into one send_multiple_images() call.
  • Same-response canonical dedup via namespace-tuple keys (("local", normcase(path)) / ("url", url)).
  • No cross-turn history dedup (history_media_paths / _history_local_keys removed — follows current main #73771 semantics).
  • Background task image delivery uses send_multiple_images() with _image_dedup_keys preventing MEDIA:path + file:// tag double-send within one response.

tests/gateway/test_background_command.py (+210/-0)

  • Parametrized: two-slash and three-slash Windows file:// URIs route through send_multiple_imagessend_image_file; uppercase-scheme variant.
  • Regression: MEDIA:path + file:// tag for the same file sends once per response.
  • Windows-only three_slash_windows variant skipped on non-Win32.

tests/gateway/test_tts_media_routing.py (+700/-0)

  • Coverage: file:// markdown/HTML extraction; HTML bare-path rejection (C:\..., /tmp/..., smb://); JSON-embedded and data-src non-detection; UNC / non-local authority / drive-relative rejection; Windows two/three-slash, encoded-drive (file://C%3A%5C...) and case-insensitive scheme; same-response MEDIA:path + file:// dedup; substring-safe dedup; send_multiple_images round-trip.
  • Removed old cross-turn history-dedup tests (test_post_stream_history_dedup_*) that relied on the removed history_media_paths contract.
  • Added explicit-resend contract tests (test_post_stream_explicit_resend_allowed_in_later_turn, test_post_stream_file_url_resend_allowed_in_later_turn) proving a later turn repeating the same MEDIA: / file:// still delivers.
  • Added P1 regression tests using the production default quote() encoding: file://C%3A%5C... normalizes to the Windows drive path and routes to send_image_file() (never send_image()).
  • Windows-only test_post_stream_file_url_windows_path_delivered correctly skipped on non-Win32; resend tests run on all platforms.

Test Results

Command Result
pytest tests/gateway/test_tts_media_routing.py (Windows) 33 passed, 3 failed
pytest tests/gateway/test_background_command.py (Windows) 11 passed
Broad gateway media/tts/background/platform suite (Windows) 550 passed, 17 failed
Same broad suite on clean upstream/main baseline (Windows) 521 passed, 17 failed — identical failure list
ruff check (all 4 changed files) All checks passed
git diff --check clean
Conflict markers / CRLF scan none / LF-only

The 17 broad-suite failures are pre-existing Windows platform-path assertion issues (also failing identically on clean main); this PR adds no new failures and passes 29 more tests than baseline. No CI checks are currently reported for this PR head; the repository's required aggregated status check therefore remains unsatisfied.

Verification

  • py_compile: 4 files, all OK
  • mergeable: MERGEABLE; PR is OPEN and not a draft

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/qqbot QQ Bot adapter labels Jun 10, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Verification Review

Solid implementation. A few observations:

  1. validate_media_delivery_path as the security boundary: The file:// URI acceptance is gated on both extension filtering (FILE_LIKE_EXTS) and validate_media_delivery_path(). This is the correct layering — extension check is a fast pre-filter, path validation is the real guard. As long as validate_media_delivery_path enforces a directory boundary (not just existence), the attack surface is well-contained.

  2. Masked-content protection is thorough: Code fences, inline code, blockquotes, and JSON-embedded text are all masked before scanning. The real_match re-extraction from original content at the same offset avoids path corruption from masking. This handles the common case where LLMs include file:// paths in code examples.

  3. Post-stream local_files = [] is the right call: Disabling extract_local_files in post-stream delivery (referencing bug(gateway): post-stream media delivery can upload bare local paths not intentionally present in the visible reply #20834) prevents bare-path false positives. Only explicit image tags and MEDIA directives produce attachments now — clean separation.

  4. Minor: removal logic has redundant re-validation: The _image_keys dict rebuild re-normalizes and re-validates URLs that were already validated in the image extraction loop. Not a bug, just ~15 lines of duplicated work per invocation. Acceptable for correctness insurance.

LGTM overall — well-tested with 15+ test cases covering Windows/POSIX/percent-encoded/UNC/code-block/inline-code scenarios.

@liuhao1024

Copy link
Copy Markdown
Contributor

Verification Review — reviewed full diff, no issues found.

The file:// URI handling in extract_images is thorough:

  • _normalize_file_url() covers Windows drive letters (2 and 3 slashes), POSIX absolute, percent-encoding, backslash normalization, and quote stripping
  • UNC paths (file://server/share/...) correctly rejected at both netloc and decoded-// layers
  • validate_media_delivery_path() gate ensures only existing files within allowed roots are promoted
  • Extension allowlist (FILE_LIKE_EXTS) prevents non-image files from becoming attachments
  • Masked-span approach (_mask_protected_spans + _mask_json_string_media) prevents file:// URIs in fenced code / inline code / blockquotes from being extracted
  • Span-based deletion is precise — accepted tags removed, rejected tags preserved, code examples survive
  • Post-stream delivery change (L12361-12380) stops bare local path auto-promotion (bug(gateway): post-stream media delivery can upload bare local paths not intentionally present in the visible reply #20834 root cause)
  • 25+ test cases covering: POSIX paths, Windows paths, UNC rejection, percent-encoding, fenced code masking, inline code masking, blockquote masking, extension filtering, mixed image+PDF, case-insensitive scheme, HTML img with extra attributes, post-stream integration

Clean implementation with strong defense-in-depth.

@k176060444-lgtm
k176060444-lgtm marked this pull request as ready for review June 10, 2026 22:47
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the careful Windows file:// parsing and validation work. The underlying extraction gap still exists on current main (gateway/platforms/base.py:3324-3346), but two current-main paths need reconciliation before salvage.

Problems

  • The post-stream local_files = [] change removes the active bare-deliverable path at gateway/run.py:13250-13252. That behavior is deliberate delivery-mode functionality from f2fdb9a178a0b646d0803ab0789914657dc8c361; please preserve it or add a narrower guard for the reported false-positive case.
  • gateway/run.py:13475-13483 sends extracted background-task images through send_image. For QQBot, _is_url() accepts only HTTP(S) (gateway/platforms/qqbot/adapter.py:3135-3136), so a newly extracted file:// URI would be treated as a literal local pathname rather than reaching send_image_file.

Suggested changes

  • Rebase the behavior on the current post-stream delivery contract and add a focused regression test for the retained bare-path behavior.
  • Route background-task file:// images through send_multiple_images or decoded send_image_file, with a QQBot-oriented regression test.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 14, 2026
@k176060444-lgtm

Copy link
Copy Markdown
Author

@teknium1 — Addressed both concerns from your review. Diff contained to 3 files (+158/−45).

Item 1: post-stream bare-path contract
Restored current-main extract_local_files + filter_local_delivery_paths in _deliver_media_from_response. The PR's local_files = [] / image_delivery replacement is reverted. Bare local images and non-image files now follow the existing deliverable-mode routing (images → send_multiple_images, documents → send_document). Added two regression tests:

  • test_post_stream_bare_local_image_still_delivered
  • test_post_stream_bare_local_pdf_still_delivered_as_document

Item 2: background-task file:// QQBot routing
Replaced the per-image send_image() loop in _run_background_task with a single send_multiple_images(images=...) call. Existing BasePlatformAdapter.send_multiple_images decodes file:// URIs and dispatches to send_image_file, bypassing QQBot's _is_url (http(s) only) without touching QQBot adapter code. Added test_qqbot_background_task_file_url_routes_to_send_image_file which uses a real adapter subclass and real extract_images to verify the full round-trip.

Validation:

  • pytest tests/gateway/test_tts_media_routing.py — 16/16 passed
  • pytest tests/gateway/test_background_command.py — 23/23 passed
  • ruff check — clean
  • git diff --check — clean

Commit: fcf3cbd38660b073b18c1a8d6c52c1311c15c1e8

The changes were kept limited to the reviewed concerns and the PR scope was not expanded.

@k176060444-lgtm
k176060444-lgtm force-pushed the work/pr-local-file-qqbot-20260610 branch from fcf3cbd to 26472cd Compare July 14, 2026 12:31
@k176060444-lgtm

Copy link
Copy Markdown
Author

@teknium1 — Rebased PR #43332 on current main. The previous branch had CRLF churn (~21000 lines of noise); this clean rebuild contains only functional changes (+678/−54) and is now mergeable.

Item 1: post-stream bare-path contract — Restored current-main extract_local_files + filter_local_delivery_paths. Both extract_images results (explicit file:///HTTP tags) AND local_files (bare paths) feed into send_multiple_images with deduplication. Bare-path delivery mode is preserved. Two new regression tests: test_post_stream_bare_local_image_still_delivered, test_post_stream_bare_local_pdf_still_delivered_as_document.

Item 2: background-task file:// routing_run_background_task now calls send_multiple_images(images=...) instead of looping send_image(image_url=...). The base adapter decodes file:// to send_image_file, bypassing QQBot _is_url without touching QQBot code. New test: test_qqbot_background_task_file_url_routes_to_send_image_file.

Validation:

  • pytest tests/gateway/test_tts_media_routing.py — 17/17 passed
  • pytest tests/gateway/test_background_command.py — 23/23 passed
  • ruff check — clean
  • git diff --check — clean
  • Zero CRLF churn

Commit: 26472cd0fecd547fc75a913cf22be75477dd058a

The changes were kept limited to the reviewed concerns and the PR scope was not expanded.

@alt-glitch alt-glitch removed sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 14, 2026
@k176060444-lgtm

Copy link
Copy Markdown
Author

Live QQBot Windows E2E validation

Validated the current PR commits on a real QQBot Windows deployment after
cherry-picking them onto the locally deployed branch and restarting the
gateway.

Environment:

  • PR head: 5675ebc7da47ef8e6e57894ffc4f7e79547ab409
  • Deployed integration head: 74002151a1
  • Related automated tests: 44/44 passed
    • tests/gateway/test_tts_media_routing.py: 20/20
    • tests/gateway/test_background_command.py: 24/24

Results:

Scenario Result
Post-stream Markdown file:// image PASS — one native image received with caption test
Post-stream bare local image path PASS — one native image received
Post-stream bare local PDF path PASS — one document attachment received
Existing MEDIA: image path PASS — one native image received
Background-task Markdown file:// image PASS — ack sent no image; task completion delivered exactly one native image with caption bg

For the image cases, the live QQBot logs confirmed local-image upload through
send_multiple_images / send_image_file; the PDF retained the existing
document-delivery path. No literal file:// pathname upload failures were
observed.

This validates both review fixes in the real target environment:

  • retained post-stream bare-path delivery;
  • background-task file:// images no longer pass through QQBot's HTTP-only
    send_image URL path.

@k176060444-lgtm

Copy link
Copy Markdown
Author

Follow-up from live /background E2E

One additional issue was exposed during the live QQBot Windows verification.

In the first run, the /background command prompt itself contained an existing
bare local image path. The immediate acknowledgement echoed that path in its
prompt preview, and the normal response pipeline promoted it through
extract_local_files() before the background task completed.

The task finalizer then delivered the intended file:// Markdown image, so the
same image was uploaded twice:

  1. acknowledgement path — bare-path extraction, empty caption;
  2. background completion path — intended file:// image, caption bg.

An isolated rerun without a bare path in the acknowledgement preview delivered
exactly one image at task completion, confirming that the background
file:// routing implemented by this PR works as intended.

This is a separate, pre-existing acknowledgement/media-extraction issue rather
than a regression introduced by #43332. It is tracked independently in:

A focused fix is being handled separately in:

No scope change to this PR is required.

@alt-glitch alt-glitch added platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 16, 2026
@k176060444-lgtm
k176060444-lgtm marked this pull request as draft July 29, 2026 01:09
@k176060444-lgtm
k176060444-lgtm force-pushed the work/pr-local-file-qqbot-20260610 branch from 5675ebc to 43593d6 Compare July 29, 2026 10:11
@k176060444-lgtm
k176060444-lgtm marked this pull request as ready for review July 29, 2026 10:36
@k176060444-lgtm
k176060444-lgtm marked this pull request as draft July 29, 2026 11:04
@k176060444-lgtm
k176060444-lgtm marked this pull request as ready for review July 29, 2026 11:38
@k176060444-lgtm
k176060444-lgtm marked this pull request as draft August 1, 2026 10:49
@k176060444-lgtm
k176060444-lgtm force-pushed the work/pr-local-file-qqbot-20260610 branch from 2a112e4 to 6412d65 Compare August 1, 2026 11:06
@k176060444-lgtm
k176060444-lgtm marked this pull request as ready for review August 1, 2026 11:23
… main

Transplant PR NousResearch#43332 net behavior onto latest upstream/main:

- Add _normalize_file_url() to parse/validate file:// URIs (POSIX,
  Windows drive-letter, percent-encoding; reject UNC/drive-relative)
- Extend extract_images() to accept file:// markdown/HTML image tags
  with mask-based false-positive prevention; keep upstream's
  validate_media_delivery_path / container->host translation chain
- Post-stream delivery stays EXPLICIT-ONLY (NousResearch#20834): MEDIA: and
  file:// tags deliver; bare local paths are never auto-uploaded
- Same-response dedup via namespace-tuple keys (MEDIA + file:// +
  HTTP exact URL); cross-response explicit resend still delivers
- Keep upstream thread_metadata propagation in _deliver_media_from_response
- Batch background image delivery via send_multiple_images (file:// aware)
- Tests: keep upstream queued-delivery suite intact, add PR's file://
  regression coverage (POSIX/Windows/unicode/space paths, dedup,
  bare-path rejection, resend)
_normalize_file_url() rejected the URI form produced by the gateway
delivery batching code, which wraps local paths with the default
urllib.parse.quote() (safe='/').  On Windows that percent-encodes the
drive letter and backslashes into the authority segment
(file://C%3A%5Cdir%5Ca.png), which urlparse then treats as a bogus
UNC host, so local images silently fell back to send_image() with the
raw file:// URI and were never delivered.

Decode the authority segment and accept it only when it resolves to an
absolute drive path (C:/...).  UNC hosts, encoded UNC authorities,
drive-relative paths and all other non-drive authorities stay rejected.
Re-check the drive-letter form after percent-decoding so the encoded
three-slash variant (file:///C%3A/dir/a.png) normalizes identically to
the plain form.

Also update the post-stream docstring to describe the actual explicit
attachment contract (MEDIA: + file:// + http(s) image tags) and drop a
dead unquote import from send_multiple_images.

Adds regression coverage using the production default quote encoding:
- _normalize_file_url accepts file://C%3A%5C... and decodes to C:/...
- send_multiple_images routes it to send_image_file (never send_image)
@k176060444-lgtm
k176060444-lgtm force-pushed the work/pr-local-file-qqbot-20260610 branch from 30e8a7a to 2aaf21f Compare August 12, 2026 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/qqbot QQ Bot adapter platform/windows Native Windows-specific behavior or breakage sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants