Fix strict model auth and browser fetch cancellation#7
Fix strict model auth and browser fetch cancellation#7godnight10061 wants to merge 11 commits intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and reliability of streaming operations, particularly concerning browser-based transports and proxy mechanisms. It introduces more granular control over streaming timeouts, ensures deterministic behavior for userscript proxy jobs, and improves error handling and fallback strategies for various network and authentication failures. These changes aim to provide a more stable and resilient streaming experience, especially in challenging environments like Windows. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several significant improvements to streaming reliability and transport fallback logic, including making userscript proxy job timeouts deterministic, adding a timeout for initial stream data, and improving transport selection on Windows. However, a security audit identified critical vulnerabilities in the administrative dashboard and API handling. Specifically, there are stored XSS vulnerabilities in the dashboard due to unescaped rendering of model names and API key names, and administrative endpoints lack CSRF protection, potentially allowing unauthorized actions. Additionally, while the PR adds robust error handling for proxy timeouts and non-reCAPTCHA 403s and includes good tests, there is an area of code duplication in the streaming logic that could be refactored for better maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to streaming reliability, timeout handling, and transport fallback mechanisms. The changes are well-structured and address important issues like deterministic timeouts for proxy jobs and preventing infinite keep-alives on stuck streams. The addition of comprehensive tests for the new logic is also a great enhancement. I've identified a couple of opportunities to improve maintainability by refactoring duplicated code blocks, which I've detailed in the specific comments.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the reliability and security of the application. The introduction of deterministic timeouts, fallback logic for streaming transports, and robust CSRF/XSS protection on the dashboard are all critical enhancements. The code is well-structured and the addition of comprehensive tests is commendable.
I've found one high-severity issue related to timeout calculations and one medium-severity suggestion for refactoring duplicated code.
Additionally, while you've added CSRF protection to the API key management endpoints, I noticed that other state-changing endpoints like /add-auth-token and /delete-auth-token are still missing this protection. It would be great to secure them as well to prevent Cross-Site Request Forgery vulnerabilities across the entire dashboard.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to streaming reliability, transport fallback logic, and dashboard security. Key changes include adding a timeout for streams that don't produce data, making userscript proxy timeouts deterministic, and enhancing fallback mechanisms between different browser transports (Userscript proxy, Chrome, and Camoufox). The dashboard has also been hardened against XSS and CSRF attacks through HTML escaping and CSRF protection for API key management. However, I identified one remaining instance of unescaped output in the dashboard that could lead to XSS if the upstream model data is malicious. Additionally, there is a suggestion to improve the timeout implementation's robustness.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to streaming timeouts, transport fallback logic, and dashboard security. The changes effectively address potential infinite keep-alive streams by adding a 'first delta' timeout and make proxy job timeouts deterministic. The transport fallback mechanisms are now more robust, handling various error scenarios like non-reCAPTCHA 403s and proxy timeouts. Security is enhanced with CSRF protection on the dashboard and systematic HTML escaping to prevent XSS. The addition of comprehensive unit tests for the new logic is a great practice. My review includes one suggestion for minor code simplification in an error handling path, but overall, the changes are excellent and well-implemented.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the reliability and security of the application. The introduction of CSRF protection and HTML escaping on the dashboard is a critical security enhancement. The improvements to streaming timeouts, particularly the 'no delta' timeout and the deterministic proxy job timeouts, will greatly enhance reliability and prevent hanging requests. The transport fallback logic is also much more robust now. The addition of a comprehensive test suite to validate these changes is highly appreciated. I've found a couple of minor opportunities for refactoring to reduce code duplication, but overall this is a very high-quality contribution.
godnight10061
force-pushed
the
pr/stream-first-delta-timeout
branch
from
fcf6326 to
a22ab18
Compare
godnight10061
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant number of improvements across security, robustness, and functionality. The addition of CSRF protection and HTML escaping on the dashboard is a critical security enhancement. The streaming logic has been made much more robust with better transport selection, error handling for various 403 scenarios, and timeouts to prevent hangs. The fixes for strict model authentication and browser fetch cancellation are also valuable. The code is well-structured, and the inclusion of a comprehensive test suite for the new features and fixes is excellent. I have one suggestion for refactoring a piece of duplicated logic to improve maintainability.
Note: Security Review did not run due to the size of the PR.
| auth_for_browser = str(current_token or "").strip() | ||
| try: | ||
| cand = str(EPHEMERAL_ARENA_AUTH_TOKEN or "").strip() | ||
| except Exception: | ||
| cand = "" | ||
| if cand: | ||
| try: | ||
| if ( | ||
| is_probably_valid_arena_auth_token(cand) | ||
| and not is_arena_auth_token_expired(cand, skew_seconds=0) | ||
| and ( | ||
| (not auth_for_browser) | ||
| or (not is_probably_valid_arena_auth_token(auth_for_browser)) | ||
| or is_arena_auth_token_expired(auth_for_browser, skew_seconds=0) | ||
| ) | ||
| ): | ||
| auth_for_browser = cand | ||
| except Exception: | ||
| auth_for_browser = cand |
There was a problem hiding this comment.
This block of logic for selecting the best authentication token for browser-based transports appears to be duplicated in a few places (here for non-streaming requests, and inside _try_chrome_fetch and _try_camoufox_fetch in the streaming logic). To improve maintainability and reduce code duplication, consider refactoring this logic into a separate helper function.
For example:
def _get_auth_for_browser_transport(current_token: str) -> str:
"""Selects the best auth token for browser transports, preferring a valid ephemeral token."""
auth_for_browser = str(current_token or "").strip()
try:
cand = str(EPHEMERAL_ARENA_AUTH_TOKEN or "").strip()
except Exception:
cand = ""
if not cand:
return auth_for_browser
try:
is_cand_valid = (
is_probably_valid_arena_auth_token(cand)
and not is_arena_auth_token_expired(cand, skew_seconds=0)
)
if not is_cand_valid:
return auth_for_browser
is_current_invalid = (
(not auth_for_browser)
or (not is_probably_valid_arena_auth_token(auth_for_browser))
or is_arena_auth_token_expired(auth_for_browser, skew_seconds=0)
)
if is_current_invalid:
return cand
except Exception:
# Fallback to candidate on parsing error
return cand
return auth_for_browserYou could then call this helper function in all three locations where this logic is used.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the security and robustness of the application. It introduces crucial security measures like CSRF protection and output escaping to prevent XSS, fixes several bugs related to transport selection and timeouts, and improves session management. The changes to the userscript proxy and browser fetch transports make them more resilient to errors and upstream issues. The addition of a comprehensive suite of tests for the new functionality is also highly commendable. I have one minor suggestion for improvement.
Note: Security Review did not run due to the size of the PR.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
For clarity and to avoid unintentionally catching unrelated errors, it's better to catch the specific asyncio.QueueEmpty exception here instead of the general Exception.
| except Exception: | |
| pass | |
| except asyncio.QueueEmpty: | |
| pass |
1 participant
Summary
Tests