fix(interactive-review): Windows 호환성 및 타임아웃 개선#9
fix(interactive-review): Windows 호환성 및 타임아웃 개선#9YoungjaeDev wants to merge 2 commits intoteam-attention:mainfrom
Conversation
- Windows에서 SIGHUP/SIGPIPE 에러 수정 (Unix-only 시그널) - 한글 출력 깨짐 수정 (UTF-8 인코딩) - 타임아웃 파라미터 추가 (기본 10분, 1분~1시간 조절 가능)
Summary of ChangesHello @YoungjaeDev, 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! 이 풀 리퀘스트는 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. 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 PR effectively improves Windows compatibility and timeout functionality. However, a critical Cross-Site Scripting (XSS) vulnerability was identified in the review UI generation logic, which is accessible through the start_review tool modified in this PR. Implementing proper input escaping and output sanitization is recommended to ensure the safety of the interactive review process. Additionally, consider refining the user-facing timeout messages and modernizing the Windows encoding fix.
|
|
||
|
|
||
| async def start_review_impl(content: str, title: str = "Review") -> dict[str, Any]: | ||
| async def start_review_impl(content: str, title: str = "Review", timeout: int = 600) -> dict[str, Any]: |
There was a problem hiding this comment.
The start_review tool is vulnerable to Cross-Site Scripting (XSS). The title and content parameters are used to generate an interactive HTML UI without proper sanitization or escaping.
- The
titleis embedded directly into the HTML template (inweb_ui.py), allowing for HTML injection. - The
contentis rendered usingmarked.jswithout sanitization, which can execute arbitrary JavaScript if the content contains malicious HTML/JS. - The
contentis also embedded into a<script>tag as a JSON string without escaping</script>, which allows an attacker to break out of the script tag and execute arbitrary code.
Since this tool handles content that may be generated by an LLM (which can be influenced by untrusted external data via prompt injection), this poses a significant risk to the user. Although this PR primarily focuses on timeout and Windows compatibility, it modifies the start_review_impl function which is the entry point for these unsanitized inputs. It is highly recommended to escape the title and use a sanitization library like DOMPurify for the rendered markdown content.
| if sys.platform == 'win32': | ||
| sys.stdout = io.TextIOWrapper(sys.stdout.buffer, encoding='utf-8') | ||
| sys.stderr = io.TextIOWrapper(sys.stderr.buffer, encoding='utf-8') |
There was a problem hiding this comment.
Python 3.7 이상 버전에서는 sys.stdout.reconfigure()와 sys.stderr.reconfigure()를 사용할 수 있습니다. 이 방법은 스트림을 다시 래핑하는 것보다 더 직접적이고 현대적인 방식으로 텍스트 스트림의 인코딩을 변경합니다. 만약 Python 3.7 이전 버전과의 호환성을 유지해야 한다면 현재 구현 방식도 괜찮습니다.
| if sys.platform == 'win32': | |
| sys.stdout = io.TextIOWrapper(sys.stdout.buffer, encoding='utf-8') | |
| sys.stderr = io.TextIOWrapper(sys.stderr.buffer, encoding='utf-8') | |
| if sys.platform == 'win32': | |
| sys.stdout.reconfigure(encoding='utf-8') | |
| sys.stderr.reconfigure(encoding='utf-8') |
| ) | ||
|
|
||
| if not result_received: | ||
| minutes = timeout // 60 |
There was a problem hiding this comment.
타임아웃 메시지에 더 정확한 시간을 표시하기 위해, 분 단위로 내림하는 대신 반올림하는 것을 고려해 보세요. 예를 들어, 110초 타임아웃의 경우 1분보다는 2분으로 표시하는 것이 사용자에게 더 직관적일 것입니다.
| minutes = timeout // 60 | |
| minutes = round(timeout / 60) |
| return { | ||
| "status": "timeout", | ||
| "message": "Review timed out after 5 minutes" | ||
| "message": f"Review timed out after {minutes} minutes" |
There was a problem hiding this comment.
타임아웃 메시지의 문법을 개선하기 위해, 'minute'의 복수형을 올바르게 처리하도록 변경하는 것이 좋습니다.
| "message": f"Review timed out after {minutes} minutes" | |
| "message": f"Review timed out after {minutes} minute{'s' if minutes != 1 else ''}" |
- reconfigure() 사용으로 현대화 (Python 3.7+) - 타임아웃 반올림 처리 (110초 -> 2분) - minute 단수/복수형 처리
리뷰 피드백 반영 완료적용된 수정:
XSS 취약점 관련XSS 취약점은 |
1 participant
문제
Windows에서 interactive-review 실행 시 3가지 문제 발생:
AttributeError: module 'signal' has no attribute 'SIGHUP'- Unix 전용 시그널 사용수정
hasattr()체크로 Unix 전용 시그널(SIGHUP, SIGPIPE) 조건부 등록timeout파라미터 추가 (기본 10분, 1분~1시간 조절 가능)테스트
Windows 11에서 한글 콘텐츠로 테스트 완료. 기존 Unix/Linux/macOS 동작에 영향 없음.