Skip to content

Review and Verification of PR #577: Matrix Media Caching Gate#685

Open
badMade wants to merge 4 commits into
mainfrom
review-pr-577-12782419627848629418
Open

Review and Verification of PR #577: Matrix Media Caching Gate#685
badMade wants to merge 4 commits into
mainfrom
review-pr-577-12782419627848629418

Conversation

@badMade
Copy link
Copy Markdown
Owner

@badMade badMade commented Jun 1, 2026

PR #577 Code Review & Verification

I have reviewed and verified the changes in PR #577, which addresses security and resource management concerns in the Matrix adapter.

Changes Verified:

  1. Matrix Media Authorization Gate:
    • Moved _resolve_message_context before media downloading.
    • Added an authorization check using the gateway runner's _is_user_authorized method.
    • If unauthorized, a media-less MessageEvent is still passed through to handle DM pairing flows, but no files are downloaded or cached.
  2. Media Filename Preservation:
    • Introduced original_body to ensure that when body is cleared (e.g., for images with filename-like bodies), the original filename is still available for the caching logic to determine the correct file extension.
  3. Runner Bugfix:
    • Fixed a NameError in gateway/run.py where team_id was being used without being defined. I improved the fix by safely retrieving team_id from the source object via getattr(source, "team_id", ""), ensuring the team-based authorization logic remains functional.

Verification Results:

  • Matrix Tests: All 144 tests in tests/gateway/test_matrix.py passed, including the new security tests:
    • test_group_media_without_mention_is_not_downloaded
    • test_unauthorized_media_is_not_downloaded_before_gateway_auth
  • Runner Authorization Tests: Verified that _is_user_authorized still works correctly for other platforms:
    • tests/gateway/test_wecom_callback.py (Passed)
    • tests/gateway/test_discord_bot_auth_bypass.py (Passed)
    • tests/gateway/test_unauthorized_dm_behavior.py (Passed)

Conclusion:

The changes are sound and significantly improve the security posture of the Matrix platform integration by preventing the processing of media from unauthorized users. The fix in gateway/run.py also resolves a regression that would have caused crashes during authorization checks.

Fixes #683


PR created automatically by Jules for task 12782419627848629418 started by @badMade

- Matrix: resolve message context and check authorization before downloading/caching media.
- Matrix: use original_body for filename preservation during caching.
- Runner: fix NameError by safely retrieving team_id from source in _is_user_authorized.
- Tests: add coverage for unauthorized media download prevention.

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
@google-labs-jules
Copy link
Copy Markdown

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@google-labs-jules google-labs-jules Bot mentioned this pull request Jun 1, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 1, 2026

🔎 Lint report: review-pr-577-12782419627848629418 vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 8254 on HEAD, 8262 on base (✅ -8)

🆕 New issues (45):

Rule Count
invalid-argument-type 34
unresolved-attribute 7
unsupported-operator 4
First entries
run_agent.py:7146: [invalid-argument-type] invalid-argument-type: Argument to function `_codex_cloudflare_headers` is incorrect: Expected `str`, found `Unknown | str | dict[str, str] | ... omitted 4 union elements`
run_agent.py:9727: [invalid-argument-type] invalid-argument-type: Argument to function `github_model_reasoning_efforts` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
run_agent.py:13108: [invalid-argument-type] invalid-argument-type: Argument to bound method `SessionDB.update_token_counts` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
run_agent.py:9978: [unresolved-attribute] unresolved-attribute: Attribute `lower` is not defined on `dict[Unknown, Unknown] & ~AlwaysFalsy`, `int & ~AlwaysFalsy`, `dict[Unknown | str, Unknown | str | dict[str, str]] & ~AlwaysFalsy` in union `(str & ~AlwaysFalsy) | (Unknown & ~AlwaysFalsy) | (dict[Unknown, Unknown] & ~AlwaysFalsy) | ... omitted 3 union elements`
run_agent.py:5851: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["/"]` and `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
run_agent.py:8837: [invalid-argument-type] invalid-argument-type: Argument to bound method `ContextCompressor.update_model` is incorrect: Expected `int`, found `Divergent | Unknown | str | ... omitted 3 union elements`
run_agent.py:7763: [invalid-argument-type] invalid-argument-type: Argument to function `get_provider_request_timeout` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
run_agent.py:5423: [invalid-argument-type] invalid-argument-type: Argument to function `parse_rate_limit_headers` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
run_agent.py:9700: [invalid-argument-type] invalid-argument-type: Argument to function `lmstudio_model_reasoning_options` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
run_agent.py:11554: [unresolved-attribute] unresolved-attribute: Attribute `strip` is not defined on `dict[Unknown, Unknown] & ~AlwaysFalsy`, `int & ~AlwaysFalsy`, `dict[Unknown | str, Unknown | str | dict[str, str]] & ~AlwaysFalsy` in union `(str & ~AlwaysFalsy) | (Unknown & ~AlwaysFalsy) | (dict[Unknown, Unknown] & ~AlwaysFalsy) | ... omitted 3 union elements`
run_agent.py:11483: [invalid-argument-type] invalid-argument-type: Argument to function `_fixed_temperature_for_model` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
run_agent.py:3397: [invalid-argument-type] invalid-argument-type: Argument to function `get_provider_stale_timeout` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
tests/run_agent/test_provider_attribution_headers.py:155: [unsupported-operator] unsupported-operator: Operator `not in` is not supported between objects of type `Literal["X-OpenRouter-Cache"]` and `Unknown | str | dict[str, str] | ... omitted 4 union elements`
run_agent.py:9114: [invalid-argument-type] invalid-argument-type: Argument to function `get_transport` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
run_agent.py:13571: [invalid-argument-type] invalid-argument-type: Argument to function `_is_oauth_token` is incorrect: Expected `str`, found `str | dict[Unknown | str, Unknown | str | dict[str, str]] | Any | ... omitted 4 union elements`
tests/agent/test_codex_cloudflare_headers.py:163: [unresolved-attribute] unresolved-attribute: Attribute `startswith` is not defined on `dict[str, str]` in union `Unknown | str | Divergent | dict[str, str]`
run_agent.py:13024: [invalid-argument-type] invalid-argument-type: Argument to function `normalize_usage` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
run_agent.py:8920: [invalid-argument-type] invalid-argument-type: Argument to function `build_anthropic_client` is incorrect: Expected `str`, found `Divergent | Unknown | str | ... omitted 3 union elements`
run_agent.py:12322: [invalid-argument-type] invalid-argument-type: Argument to function `apply_anthropic_cache_control_long_lived` is incorrect: Expected `bool`, found `int | Divergent | Unknown | ... omitted 3 union elements`
run_agent.py:9552: [invalid-argument-type] invalid-argument-type: Argument to function `get_provider_profile` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
run_agent.py:4875: [invalid-argument-type] invalid-argument-type: Argument to function `save_trajectory` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
run_agent.py:12328: [invalid-argument-type] invalid-argument-type: Argument to function `apply_anthropic_cache_control` is incorrect: Expected `bool`, found `int | Divergent | Unknown | ... omitted 3 union elements`
run_agent.py:13112: [invalid-argument-type] invalid-argument-type: Argument to bound method `SessionDB.update_token_counts` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
tests/run_agent/test_provider_attribution_headers.py:90: [unresolved-attribute] unresolved-attribute: Attribute `startswith` is not defined on `dict[str, str]` in union `Unknown | str | Divergent | dict[str, str]`
cli.py:8659: [invalid-argument-type] invalid-argument-type: Argument to function `estimate_usage_cost` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown, Unknown] | int | dict[Unknown | str, Unknown | str | dict[str, str]]`
... and 20 more

✅ Fixed issues (43):

Rule Count
invalid-argument-type 31
unresolved-attribute 6
unsupported-operator 4
unresolved-reference 1
unresolved-import 1
First entries
run_agent.py:13069: [invalid-argument-type] invalid-argument-type: Argument to function `estimate_usage_cost` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:9535: [invalid-argument-type] invalid-argument-type: Argument to function `_get_anthropic_max_output` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:13071: [invalid-argument-type] invalid-argument-type: Argument to function `estimate_usage_cost` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
tests/agent/test_codex_cloudflare_headers.py:163: [unresolved-attribute] unresolved-attribute: Attribute `startswith` is not defined on `dict[str, str]` in union `Unknown | str | dict[str, str]`
run_agent.py:5851: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["/"]` and `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:13805: [invalid-argument-type] invalid-argument-type: Argument to function `_pool_may_recover_from_rate_limit` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:8920: [invalid-argument-type] invalid-argument-type: Argument to function `build_anthropic_client` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:9114: [invalid-argument-type] invalid-argument-type: Argument to function `get_transport` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:13571: [invalid-argument-type] invalid-argument-type: Argument to function `_is_oauth_token` is incorrect: Expected `str`, found `str | dict[Unknown | str, Unknown | str | dict[str, str]] | Any | ... omitted 3 union elements`
run_agent.py:4285: [invalid-argument-type] invalid-argument-type: Argument to `AIAgent.__init__` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:4875: [invalid-argument-type] invalid-argument-type: Argument to function `save_trajectory` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:12328: [invalid-argument-type] invalid-argument-type: Argument to function `apply_anthropic_cache_control` is incorrect: Expected `bool`, found `int | str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | dict[Unknown, Unknown]`
run_agent.py:11483: [invalid-argument-type] invalid-argument-type: Argument to function `_fixed_temperature_for_model` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
tests/run_agent/test_provider_attribution_headers.py:90: [unresolved-attribute] unresolved-attribute: Attribute `startswith` is not defined on `dict[str, str]` in union `Unknown | str | dict[str, str]`
run_agent.py:13112: [invalid-argument-type] invalid-argument-type: Argument to bound method `SessionDB.update_token_counts` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:3397: [invalid-argument-type] invalid-argument-type: Argument to function `get_provider_stale_timeout` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:5851: [unresolved-attribute] unresolved-attribute: Attribute `split` is not defined on `dict[Unknown | str, Unknown | str | dict[str, str]]`, `int`, `dict[Unknown, Unknown]` in union `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:9552: [invalid-argument-type] invalid-argument-type: Argument to function `get_provider_profile` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:9727: [invalid-argument-type] invalid-argument-type: Argument to function `github_model_reasoning_efforts` is incorrect: Expected `str | None`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
run_agent.py:9978: [unresolved-attribute] unresolved-attribute: Attribute `lower` is not defined on `dict[Unknown | str, Unknown | str | dict[str, str]] & ~AlwaysFalsy`, `int & ~AlwaysFalsy`, `dict[Unknown, Unknown] & ~AlwaysFalsy` in union `(str & ~AlwaysFalsy) | (Unknown & ~AlwaysFalsy) | (dict[Unknown | str, Unknown | str | dict[str, str]] & ~AlwaysFalsy) | ... omitted 3 union elements`
tests/run_agent/test_provider_attribution_headers.py:156: [unsupported-operator] unsupported-operator: Operator `not in` is not supported between objects of type `Literal["X-OpenRouter-Cache-TTL"]` and `Unknown | str | dict[str, str] | ... omitted 3 union elements`
run_agent.py:5423: [invalid-argument-type] invalid-argument-type: Argument to function `parse_rate_limit_headers` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
gateway/run.py:5544: [unresolved-reference] unresolved-reference: Name `team_id` used when not defined
tests/agent/test_codex_cloudflare_headers.py:181: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["originator"]` and `(Unknown & ~AlwaysFalsy) | (str & ~AlwaysFalsy) | (dict[str, str] & ~AlwaysFalsy) | ... omitted 3 union elements`
tests/run_agent/test_provider_attribution_headers.py:155: [unsupported-operator] unsupported-operator: Operator `not in` is not supported between objects of type `Literal["X-OpenRouter-Cache"]` and `Unknown | str | dict[str, str] | ... omitted 3 union elements`
... and 18 more

Unchanged: 4319 pre-existing issues carried over.

Diagnostics are surfaced as warnings — this check never fails the build.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request prevents downloading and caching attacker-controlled media for unauthorized users in the Matrix platform adapter. It resolves the message context earlier in _handle_media_message, checks authorization using a new _get_gateway_authorizer helper, and passes a media-less event if unauthorized. It also adds corresponding unit tests to verify this behavior. There are no review comments, and I have no additional feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@badMade badMade marked this pull request as ready for review June 1, 2026 22:29
Copilot AI review requested due to automatic review settings June 1, 2026 22:29
@badMade
Copy link
Copy Markdown
Owner Author

badMade commented Jun 1, 2026

@claude FIX failing checks
Auto-merge / Auto-merge on review + passing checks (pull_request_review)
Auto-merge / Auto-merge on review + passing checks (pull_request_review)Cancelled after 12s
Docker Build and Publish / build-amd64 (pull_request)
Docker Build and Publish / build-amd64 (pull_request)Failing after 6m
Tests / test (pull_request)

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR strengthens the Matrix gateway adapter’s security and resource usage by ensuring media is not downloaded/cached until after mention/DM context is resolved and the gateway authorization allowlist check has passed.

Changes:

  • Move Matrix mention/thread/DM gating (_resolve_message_context) ahead of any media download/cache operations.
  • Add a gateway-runner authorization pre-check for Matrix media events, forwarding a media-less event to support unauthorized DM pairing flows without persisting bytes.
  • Preserve original media filenames (original_body) for cache naming/extension decisions, and fix a NameError in _is_user_authorized by safely deriving team_id.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
gateway/platforms/matrix.py Reorders context resolution before downloads, adds gateway authorization pre-check, and preserves original body for cache naming.
gateway/run.py Fixes undefined team_id usage in _is_user_authorized by safely retrieving it from source.
tests/gateway/test_matrix.py Adds regressions asserting non-mentioned group media and unauthorized media do not trigger downloads.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 1, 2026

Auto-merge: checks failing

The following checks did not pass:

  • copilot (failure)

Please fix the failing checks before this PR can be merged.

View workflow run

- Matrix: resolve message context and check authorization before downloading/caching media.
- Matrix: use original_body for filename preservation during caching.
- Runner: fix NameError by safely retrieving team_id from source in _is_user_authorized.
- Session: restore missing get_terminal_cwd helper to fix run_agent.py ImportErrors.
- Tests: add coverage for unauthorized media download prevention.

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
@badMade
Copy link
Copy Markdown
Owner Author

badMade commented Jun 2, 2026

@copilot resolve the merge conflicts in this pull request

…sions

- Matrix: resolve message context and check authorization before downloading/caching media.
- Matrix: use original_body for filename preservation during caching.
- Runner: fix NameError by safely retrieving team_id from source in _is_user_authorized.
- Session: restore missing get_terminal_cwd helper to fix run_agent.py ImportErrors.
- Auth: restore missing gateway/proxy_scope_auth.py to fix API server tests.
- API Server: fix non-ASCII key comparison in _check_auth using bytes.
- Approval: restore missing run_id helpers in tools/approval.py.
- Tests: restore missing _FakeProviderMemoryManager and fix outdated memory context and ddgs test cases.

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
@badMade
Copy link
Copy Markdown
Owner Author

badMade commented Jun 3, 2026

@copilot resolve the merge conflicts in this pull request

…sions

- Matrix: resolve message context and check authorization before downloading/caching media.
- Matrix: use original_body for filename preservation during caching.
- Runner: fix NameError by safely retrieving team_id from source in _is_user_authorized.
- Session: restore missing get_terminal_cwd helper to fix run_agent.py ImportErrors.
- Auth: restore missing gateway/proxy_scope_auth.py to fix API server tests.
- API Server: fix non-ASCII key comparison in _check_auth using bytes.
- Approval: restore missing run_id helpers in tools/approval.py.
- Tests: restore missing _FakeProviderMemoryManager and fix outdated memory context and ddgs test cases.

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@jules code review

2 participants