enh: cors middleware & html fallback - #537
Closed
taylorwilsdon wants to merge 6 commits into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a public Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CORSMiddleware
participant WellKnownCacheControl
participant SessionMiddleware
participant ServerApp
rect rgba(200,200,255,0.5)
Client->>CORSMiddleware: HTTP request
end
rect rgba(200,255,200,0.5)
CORSMiddleware->>WellKnownCacheControl: pass request
end
rect rgba(255,200,200,0.5)
WellKnownCacheControl->>SessionMiddleware: pass request
SessionMiddleware->>ServerApp: pass request
ServerApp-->>SessionMiddleware: response
SessionMiddleware-->>WellKnownCacheControl: response
WellKnownCacheControl-->>CORSMiddleware: response
CORSMiddleware-->>Client: response (CORS headers applied)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/server.py`:
- Around line 45-50: The CORS middleware setup in cors_middleware (using
CORSMiddleware with allow_origins=["*"] and allow_credentials=True) violates
Starlette validation and will raise at runtime; update cors_middleware to either
(A) set allow_credentials=False when using allow_origins=["*"], or (B) replace
the wildcard with an explicit allowlist of trusted origins (e.g.,
["https://example.com"]) and keep allow_credentials=True, and then restrict
where this middleware is applied so credentialed CORS is not the global
outermost layer (move or apply the cors_middleware only to the OAuth endpoints
or route group rather than adding it as the global outer middleware).
In `@gmail/gmail_tools.py`:
- Around line 174-189: The plain text is considered non-low-value only via
markers/footers/suffix heuristics, so very short plain text still beats HTML;
add a minimum meaningful plain-text length constant (e.g.,
MIN_MEANINGFUL_PLAIN_LENGTH) and treat plain text shorter than that as low-value
by incorporating len(plain_lower) < MIN_MEANINGFUL_PLAIN_LENGTH into the
plain_is_low_value determination (or into the use_html decision) so that
use_html becomes true when html_text exists and plain text is present but
beneath the minimum meaningful length; update references to
LOW_VALUE_TEXT_PLACEHOLDERS, LOW_VALUE_TEXT_FOOTER_MARKERS and
LOW_VALUE_TEXT_HTML_DIFF_MIN accordingly when locating plain_is_low_value and
use_html.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
core/server.pygmail/gmail_tools.pytests/core/test_well_known_cache_control_middleware.py
Comment on lines
+174
to
189
| plain_is_low_value = plain_lower and ( | ||
| any(marker in plain_lower for marker in LOW_VALUE_TEXT_PLACEHOLDERS) | ||
| or ( | ||
| any(marker in plain_lower for marker in LOW_VALUE_TEXT_FOOTER_MARKERS) | ||
| and len(html_lower) >= len(plain_lower) + LOW_VALUE_TEXT_HTML_DIFF_MIN | ||
| ) | ||
| or ( | ||
| len(html_lower) >= len(plain_lower) + LOW_VALUE_TEXT_HTML_DIFF_MIN | ||
| and html_lower.endswith(plain_lower) | ||
| ) | ||
| ) | ||
|
|
||
| # Detect useless fallback: HTML comments in text, or HTML is 50x+ longer | ||
| use_html = html_stripped and ( | ||
| not text_stripped | ||
| or "<!--" in text_stripped | ||
| or len(html_stripped) > len(text_stripped) * 50 | ||
| # Prefer plain text, but fall back to HTML when plain text is empty or clearly low-value. | ||
| use_html = html_text and ( | ||
| not text_stripped or "<!--" in text_stripped or plain_is_low_value | ||
| ) |
Contributor
There was a problem hiding this comment.
Add an explicit “minimum meaningful plain-text length” fallback condition.
Current logic only flags low-value plain text via marker/footer/suffix heuristics. Very short non-empty text/plain (e.g., 1–20 chars) can still win over rich HTML content, which misses the “below minimal meaningful length” requirement.
💡 Proposed adjustment
LOW_VALUE_TEXT_HTML_DIFF_MIN = 80
+LOW_VALUE_TEXT_MIN_MEANINGFUL_LEN = 40
@@
- plain_is_low_value = plain_lower and (
- any(marker in plain_lower for marker in LOW_VALUE_TEXT_PLACEHOLDERS)
+ plain_is_low_value = bool(plain_lower) and (
+ (
+ len(plain_lower) < LOW_VALUE_TEXT_MIN_MEANINGFUL_LEN
+ and len(html_lower) >= len(plain_lower) + LOW_VALUE_TEXT_HTML_DIFF_MIN
+ )
+ or any(marker in plain_lower for marker in LOW_VALUE_TEXT_PLACEHOLDERS)
or (
any(marker in plain_lower for marker in LOW_VALUE_TEXT_FOOTER_MARKERS)
and len(html_lower) >= len(plain_lower) + LOW_VALUE_TEXT_HTML_DIFF_MIN
)
or (
len(html_lower) >= len(plain_lower) + LOW_VALUE_TEXT_HTML_DIFF_MIN
and html_lower.endswith(plain_lower)
)
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gmail/gmail_tools.py` around lines 174 - 189, The plain text is considered
non-low-value only via markers/footers/suffix heuristics, so very short plain
text still beats HTML; add a minimum meaningful plain-text length constant
(e.g., MIN_MEANINGFUL_PLAIN_LENGTH) and treat plain text shorter than that as
low-value by incorporating len(plain_lower) < MIN_MEANINGFUL_PLAIN_LENGTH into
the plain_is_low_value determination (or into the use_html decision) so that
use_html becomes true when html_text exists and plain text is present but
beneath the minimum meaningful length; update references to
LOW_VALUE_TEXT_PLACEHOLDERS, LOW_VALUE_TEXT_FOOTER_MARKERS and
LOW_VALUE_TEXT_HTML_DIFF_MIN accordingly when locating plain_is_low_value and
use_html.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/server.py (1)
44-46: Clarify scope in comments to match behavior.The comment says this is for “OAuth endpoints,” but the middleware is added globally in
http_app. Updating the wording will prevent future confusion.Also applies to: 104-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/server.py` around lines 44 - 46, The comment currently states the CORS middleware is "for OAuth endpoints" but the middleware is mounted globally on http_app; update the comment(s) near where CORS is configured (the code that adds the CORS middleware to http_app) to clarify that this CORS policy applies application-wide (or specify the actual scoped endpoints if you change behavior), and keep the note about allow_credentials=True vs allow_origins=["*"]; adjust both occurrences of the explanatory comment so they accurately reflect that the middleware is global rather than limited to OAuth endpoints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test_cors.py`:
- Around line 6-40: The test currently performs import-time execution and calls
sys.exit in test_cors.py; refactor it into a proper pytest test function (e.g.,
test_cors_middleware_present) that sets required env vars via a fixture or
monkeypatch (instead of global os.environ mutating at import), imports server
and calls server.http_app() inside the test, inspects app.user_middleware for a
middleware with cls.__name__ == "CORSMiddleware", and replace print/ sys.exit
logic with plain assertions (assert any(...)). Ensure env changes are scoped to
the test and not performed at import time so test collection isn't aborted.
---
Nitpick comments:
In `@core/server.py`:
- Around line 44-46: The comment currently states the CORS middleware is "for
OAuth endpoints" but the middleware is mounted globally on http_app; update the
comment(s) near where CORS is configured (the code that adds the CORS middleware
to http_app) to clarify that this CORS policy applies application-wide (or
specify the actual scoped endpoints if you change behavior), and keep the note
about allow_credentials=True vs allow_origins=["*"]; adjust both occurrences of
the explanatory comment so they accurately reflect that the middleware is global
rather than limited to OAuth endpoints.
Comment on lines
+6
to
+40
| sys.path.insert(0, ".") | ||
|
|
||
| # Set required env vars before importing | ||
| import os | ||
|
|
||
| os.environ["MCP_ENABLE_OAUTH21"] = "true" | ||
| os.environ["WORKSPACE_MCP_STATELESS_MODE"] = "true" | ||
| os.environ["GOOGLE_OAUTH_CLIENT_ID"] = "test" | ||
| os.environ["GOOGLE_OAUTH_CLIENT_SECRET"] = "test" | ||
|
|
||
| from core.server import server | ||
|
|
||
| # Get the HTTP app | ||
| app = server.http_app() | ||
|
|
||
| # Check middleware stack | ||
| print("Middleware stack:") | ||
| for i, m in enumerate(app.user_middleware): | ||
| if hasattr(m, "cls"): | ||
| print(f" {i}: {m.cls.__name__}") | ||
| else: | ||
| print(f" {i}: {type(m).__name__}") | ||
|
|
||
| print("\nChecking for CORSMiddleware...") | ||
| has_cors = any( | ||
| hasattr(m, "cls") and m.cls.__name__ == "CORSMiddleware" | ||
| for m in app.user_middleware | ||
| ) | ||
| print(f"CORS middleware found: {has_cors}") | ||
|
|
||
| if has_cors: | ||
| print("✅ CORS middleware is configured!") | ||
| else: | ||
| print("❌ CORS middleware is NOT configured!") | ||
| sys.exit(1) |
Contributor
There was a problem hiding this comment.
Avoid import-time execution and sys.exit in test modules.
This file runs assertions at import time and exits the process on failure (Line 40), which can break/abort test collection and leak env mutations into unrelated tests. Please convert it to a real test function with scoped env setup and plain assertions.
✅ Suggested rewrite
-#!/usr/bin/env python3
-"""Quick test to verify CORS middleware is configured correctly."""
-
-import sys
-
-sys.path.insert(0, ".")
-
-# Set required env vars before importing
-import os
-
-os.environ["MCP_ENABLE_OAUTH21"] = "true"
-os.environ["WORKSPACE_MCP_STATELESS_MODE"] = "true"
-os.environ["GOOGLE_OAUTH_CLIENT_ID"] = "test"
-os.environ["GOOGLE_OAUTH_CLIENT_SECRET"] = "test"
-
-from core.server import server
-
-# Get the HTTP app
-app = server.http_app()
-
-# Check middleware stack
-print("Middleware stack:")
-for i, m in enumerate(app.user_middleware):
- if hasattr(m, "cls"):
- print(f" {i}: {m.cls.__name__}")
- else:
- print(f" {i}: {type(m).__name__}")
-
-print("\nChecking for CORSMiddleware...")
-has_cors = any(
- hasattr(m, "cls") and m.cls.__name__ == "CORSMiddleware"
- for m in app.user_middleware
-)
-print(f"CORS middleware found: {has_cors}")
-
-if has_cors:
- print("✅ CORS middleware is configured!")
-else:
- print("❌ CORS middleware is NOT configured!")
- sys.exit(1)
+from starlette.middleware.cors import CORSMiddleware
+
+
+def test_http_app_has_cors_middleware(monkeypatch):
+ monkeypatch.setenv("MCP_ENABLE_OAUTH21", "true")
+ monkeypatch.setenv("WORKSPACE_MCP_STATELESS_MODE", "true")
+ monkeypatch.setenv("GOOGLE_OAUTH_CLIENT_ID", "test")
+ monkeypatch.setenv("GOOGLE_OAUTH_CLIENT_SECRET", "test")
+
+ from core.server import server
+
+ app = server.http_app()
+ assert any(
+ getattr(m, "cls", None) is CORSMiddleware for m in app.user_middleware
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test_cors.py` around lines 6 - 40, The test currently performs import-time
execution and calls sys.exit in test_cors.py; refactor it into a proper pytest
test function (e.g., test_cors_middleware_present) that sets required env vars
via a fixture or monkeypatch (instead of global os.environ mutating at import),
imports server and calls server.http_app() inside the test, inspects
app.user_middleware for a middleware with cls.__name__ == "CORSMiddleware", and
replace print/ sys.exit logic with plain assertions (assert any(...)). Ensure
env changes are scoped to the test and not performed at import time so test
collection isn't aborted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
& better fallback for html
Closes #536
Summary by CodeRabbit
New Features
Bug Fixes
Tests