Compare commits
2 Commits
dfad8d3072
...
hermes/her
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
27c8cb3597 | ||
|
|
4a33d6924e |
778
skills/polymarket-browse/reviews/2026-03-25.md
Normal file
778
skills/polymarket-browse/reviews/2026-03-25.md
Normal file
@@ -0,0 +1,778 @@
|
|||||||
|
# Polymarket-Browse Skill Review
|
||||||
|
|
||||||
|
**Date:** 2026-03-25
|
||||||
|
**Reviewer:** Hermes Agent (Shoko)
|
||||||
|
**Version Reviewed:** Current HEAD
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 1. Current State of SKILL.md
|
||||||
|
|
||||||
|
### 1.1 Overview
|
||||||
|
The SKILL.md is well-structured with clear sections:
|
||||||
|
- Installation instructions (Hermes Agent + OpenClaw)
|
||||||
|
- Usage with argument reference
|
||||||
|
- Output format examples
|
||||||
|
- Game categories table
|
||||||
|
- Filters explanation
|
||||||
|
- Pagination and rate limiting notes
|
||||||
|
- Odds format documentation
|
||||||
|
|
||||||
|
### 1.2 Strengths
|
||||||
|
- Clear argument documentation with defaults
|
||||||
|
- Good output format examples showing both match and non-match markets
|
||||||
|
- Filters section is detailed and explains tradeable vs non-tradeable logic
|
||||||
|
- Game categories table is easy to reference
|
||||||
|
- Rate limiting and backoff strategy documented
|
||||||
|
|
||||||
|
### 1.3 Issues/Gaps in SKILL.md
|
||||||
|
|
||||||
|
| Issue | Severity | Notes |
|
||||||
|
|-------|----------|-------|
|
||||||
|
| No troubleshooting section | Low | API errors, partial fetches, common issues not documented |
|
||||||
|
| No examples for --search | Low | Only mentioned in passing, no concrete example |
|
||||||
|
| No mention of required dependencies | Low | Assumes curl is available (it is on Linux) |
|
||||||
|
| No changelog | Low | Hard to track what changed between versions |
|
||||||
|
| Telegram section minimal | Low | Doesn't explain HTML parse_mode limitations |
|
||||||
|
| No credits/author info | Low | Who built this? |
|
||||||
|
|
||||||
|
### 1.4 Recommendations for SKILL.md
|
||||||
|
|
||||||
|
1. **Add troubleshooting section:**
|
||||||
|
- Partial fetch warnings (API errors/timeout)
|
||||||
|
- What to do if no markets appear
|
||||||
|
- Explaining why some matches disappear after they start
|
||||||
|
|
||||||
|
2. **Add concrete usage examples:**
|
||||||
|
```bash
|
||||||
|
# Example: Find FlyQuest Counter-Strike matches
|
||||||
|
polymarket-browse --category "Counter Strike" --search "FlyQuest"
|
||||||
|
|
||||||
|
# Example: Get 10 matches, no tournament futures
|
||||||
|
polymarket-browse --category "Valorant" --matches 10 --non-matches-only
|
||||||
|
```
|
||||||
|
|
||||||
|
3. **Add HTML escape notes for Telegram:**
|
||||||
|
- `<` `>` `&` `>` `<` need to be escaped in Telegram messages
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 2. Current State of browse.py
|
||||||
|
|
||||||
|
### 2.1 Code Organization
|
||||||
|
|
||||||
|
The script is organized into logical sections with clear headers:
|
||||||
|
|
||||||
|
```
|
||||||
|
CONFIG
|
||||||
|
FETCH
|
||||||
|
FILTERS
|
||||||
|
FORMATTING
|
||||||
|
BROWSE
|
||||||
|
FORMAT
|
||||||
|
DISPLAY
|
||||||
|
TELEGRAM
|
||||||
|
MAIN
|
||||||
|
```
|
||||||
|
|
||||||
|
**Issues:**
|
||||||
|
- Lines are excessively long (erowse ~750 lines, some functions are very dense)
|
||||||
|
- `print_browse()` function is ~120 lines — too long to review mentally
|
||||||
|
- `send_to_telegram()` function is ~100 lines — also too long
|
||||||
|
- `format_detail_event()` has deeply nested list comprehensions
|
||||||
|
- No type hints anywhere
|
||||||
|
- No docstrings on main functions (only on helper functions)
|
||||||
|
|
||||||
|
### 2.2 Line Length Issues (CRITICAL)
|
||||||
|
|
||||||
|
The user specifically asked about this. Here are the longest lines:
|
||||||
|
|
||||||
|
| Line | Length | Issue |
|
||||||
|
|------|--------|-------|
|
||||||
|
| ~line 100 | ~180 chars | `fetch_page()` URL construction |
|
||||||
|
| ~line 160 | ~160 chars | `fetch_all_pages()` loop |
|
||||||
|
| ~line 210 | ~200 chars | `is_tradeable_event()` ML market checks |
|
||||||
|
| ~line 240 | ~180 chars | `is_tradeable_event()` datetime parsing |
|
||||||
|
| ~line 300 | ~180 chars | `get_match_time_status()` datetime math |
|
||||||
|
| ~line 380 | ~200 chars | `format_detail_event()` list comprehension |
|
||||||
|
| ~line 470 | ~220 chars | `print_browse()` event formatting |
|
||||||
|
| ~line 540 | ~180 chars | `send_to_telegram()` message building |
|
||||||
|
|
||||||
|
**Root cause:** The code was written for functionality, not readability. String concatenation and nested conditionals make lines very long.
|
||||||
|
|
||||||
|
### 2.3 Proposed Solutions for Line Length
|
||||||
|
|
||||||
|
**Option A: Refactor to shorter lines (Recommended)**
|
||||||
|
- Break long URL constructions into multiple lines
|
||||||
|
- Extract nested conditionals into helper variables
|
||||||
|
- Use intermediate variables for complex expressions
|
||||||
|
- Target: max 120 characters per line
|
||||||
|
|
||||||
|
**Option B: Add unit tests**
|
||||||
|
- Write unit tests that verify behavior without needing to read every line
|
||||||
|
- Tests serve as executable documentation
|
||||||
|
- Anyone can run `pytest` to verify correctness
|
||||||
|
- See Section 3 for details
|
||||||
|
|
||||||
|
**Option C: Both (Recommended)**
|
||||||
|
- Refactor for readability
|
||||||
|
- Add unit tests for critical paths
|
||||||
|
- This is the best approach
|
||||||
|
|
||||||
|
### 2.4 Function-by-Function Analysis
|
||||||
|
|
||||||
|
#### `fetch_page()` (~35 lines)
|
||||||
|
**What it does:** Fetches one page from Polymarket API with retry logic
|
||||||
|
**Issues:**
|
||||||
|
- URL construction is on one long line
|
||||||
|
- Exponential backoff is clear but verbose
|
||||||
|
- Could use `requests` library instead of curl subprocess
|
||||||
|
|
||||||
|
**Suggestions:**
|
||||||
|
- Break URL construction into multiple `params = {...}` style
|
||||||
|
- Consider using `httpx` or `requests` instead of curl subprocess
|
||||||
|
|
||||||
|
#### `fetch_all_pages()` (~25 lines)
|
||||||
|
**What it does:** Paginates through all results
|
||||||
|
**Issues:**
|
||||||
|
- `time.sleep(0.2)` is hardcoded — should be configurable
|
||||||
|
- No progress indicator for large fetches
|
||||||
|
|
||||||
|
**Suggestions:**
|
||||||
|
- Add progress callback option
|
||||||
|
- Make inter-page delay configurable
|
||||||
|
|
||||||
|
#### `is_tradeable_event()` (~70 lines)
|
||||||
|
**What it does:** Complex filter for tradeable match markets
|
||||||
|
**Issues:**
|
||||||
|
- This is the longest function at ~70 lines
|
||||||
|
- Multiple filter conditions stacked vertically (good) but with long lines (bad)
|
||||||
|
- Bare `except:` clauses that catch everything
|
||||||
|
|
||||||
|
**Suggestions:**
|
||||||
|
- Extract `is_bo2_tie()` check (already done — good)
|
||||||
|
- Extract datetime comparisons into helper functions
|
||||||
|
- Add early returns to reduce nesting
|
||||||
|
- Change bare `except:` to specific exceptions
|
||||||
|
|
||||||
|
#### `is_tradeable_market()` (~20 lines)
|
||||||
|
**What it does:** Filter for individual markets
|
||||||
|
**Issues:**
|
||||||
|
- Very similar to `is_tradeable_event()` — code duplication
|
||||||
|
- Could reuse logic from the event version
|
||||||
|
|
||||||
|
**Suggestions:**
|
||||||
|
- Consider unifying with `is_tradeable_event()`
|
||||||
|
|
||||||
|
#### `get_match_time_status()` / `get_match_time_str()` (~50 lines combined)
|
||||||
|
**What it does:** Time formatting for display
|
||||||
|
**Issues:**
|
||||||
|
- Duplicate logic — both functions do similar things
|
||||||
|
- WIB (UTC+7) is hardcoded — user is Indonesian, but this should be configurable
|
||||||
|
|
||||||
|
**Suggestions:**
|
||||||
|
- Consolidate into one function that returns both values
|
||||||
|
- Make timezone configurable
|
||||||
|
|
||||||
|
#### `print_browse()` (~120 lines)
|
||||||
|
**What it does:** Main display function for CLI output
|
||||||
|
**Issues:**
|
||||||
|
- ~120 lines is too long to review mentally
|
||||||
|
- Mixes display logic with data formatting
|
||||||
|
- Has its own datetime import (Python import inside function — anti-pattern)
|
||||||
|
|
||||||
|
**Suggestions:**
|
||||||
|
- Break into smaller functions:
|
||||||
|
- `format_match_line()`
|
||||||
|
- `format_non_match_line()`
|
||||||
|
- `print_match_section()`
|
||||||
|
- `print_non_match_section()`
|
||||||
|
|
||||||
|
#### `send_to_telegram()` (~100 lines)
|
||||||
|
**What it does:** Telegram integration
|
||||||
|
**Issues:**
|
||||||
|
- ~100 lines too long
|
||||||
|
- Complex chunking logic for Telegram 4096 char limit
|
||||||
|
- HTML escaping not handled
|
||||||
|
|
||||||
|
**Suggestions:**
|
||||||
|
- Extract chunking logic into separate function
|
||||||
|
- Add HTML escaping helper
|
||||||
|
- Consider using `python-telegram-bot` library instead of curl
|
||||||
|
|
||||||
|
#### `format_detail_event()` (~30 lines)
|
||||||
|
**What it does:** Formats event with all markets for detail view
|
||||||
|
**Issues:**
|
||||||
|
- List comprehension is deeply nested and hard to read
|
||||||
|
- ~15-line dict construction
|
||||||
|
|
||||||
|
**Suggestions:**
|
||||||
|
- Break the dict construction into multiple lines
|
||||||
|
- Extract market formatting into helper
|
||||||
|
|
||||||
|
### 2.5 Error Handling
|
||||||
|
|
||||||
|
| Issue | Severity | Notes |
|
||||||
|
|-------|----------|-------|
|
||||||
|
| Bare `except:` clauses | Medium | Catches KeyboardInterrupt, SystemExit |
|
||||||
|
| No logging | Low | Uses print statements |
|
||||||
|
| No structured errors | Low | Could benefit from custom exceptions |
|
||||||
|
|
||||||
|
### 2.6 Missing Features/Bugs
|
||||||
|
|
||||||
|
| Issue | Severity | Notes |
|
||||||
|
|-------|----------|-------|
|
||||||
|
| No test suite | High | Cannot verify correctness automatically |
|
||||||
|
| WIB hardcoded | Medium | Timezone should be configurable |
|
||||||
|
| No cache option | Low | Could cache results for repeated queries |
|
||||||
|
| `--detail` uses 1-indexed but docs unclear | Low | Works, but could be confusing |
|
||||||
|
| BO2 tie detection uses title match | Medium | Relies on "BO2" in title — fragile |
|
||||||
|
| `is_bo2_tie()` checks child_moneyline closed | Medium | API may not always set this flag |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 3. Recommended Improvements
|
||||||
|
|
||||||
|
### 3.1 Code Refactoring (Priority: HIGH)
|
||||||
|
|
||||||
|
**Goal:** Make browse.py reviewable by humans
|
||||||
|
|
||||||
|
**Specific changes:**
|
||||||
|
|
||||||
|
1. **Break `print_browse()` into helper functions:**
|
||||||
|
```python
|
||||||
|
def format_match_line(i, e, ml, outcomes, prices, vol, title, url, ...):
|
||||||
|
"""Format a single match event line."""
|
||||||
|
...
|
||||||
|
|
||||||
|
def print_match_section(match_events, ...):
|
||||||
|
"""Print the MATCH MARKETS section."""
|
||||||
|
...
|
||||||
|
```
|
||||||
|
|
||||||
|
2. **Break `send_to_telegram()` into helper functions:**
|
||||||
|
```python
|
||||||
|
def escape_html(text):
|
||||||
|
"""Escape HTML special characters."""
|
||||||
|
...
|
||||||
|
|
||||||
|
def chunk_telegram_message(lines, max_len=4096):
|
||||||
|
"""Split long messages into chunks."""
|
||||||
|
...
|
||||||
|
```
|
||||||
|
|
||||||
|
3. **Break long lines:**
|
||||||
|
- URL construction: use `params = {...}` dict style
|
||||||
|
- Long conditionals: extract to named variables
|
||||||
|
- Long f-strings: break across multiple lines
|
||||||
|
|
||||||
|
4. **Add type hints:**
|
||||||
|
```python
|
||||||
|
def fetch_page(q: str, page: int = 1, ...) -> Optional[dict]:
|
||||||
|
```
|
||||||
|
|
||||||
|
5. **Consolidate duplicate time functions:**
|
||||||
|
- `get_match_time_status()` and `get_match_time_str()` share logic
|
||||||
|
- Create one function returning both
|
||||||
|
|
||||||
|
### 3.2 Unit Tests (Priority: HIGH)
|
||||||
|
|
||||||
|
**Goal:** Enable human review via test execution, not line-by-line reading
|
||||||
|
|
||||||
|
**Proposed test structure:**
|
||||||
|
```
|
||||||
|
tests/
|
||||||
|
__init__.py
|
||||||
|
test_filters.py # is_match_market, is_tradeable_event, is_tradeable_market
|
||||||
|
test_formatters.py # format_odds, prob_to_cents, get_match_time_*
|
||||||
|
test_browse.py # Integration tests with mocked API
|
||||||
|
test_cli.py # Argument parsing tests
|
||||||
|
```
|
||||||
|
|
||||||
|
**Test examples:**
|
||||||
|
|
||||||
|
```python
|
||||||
|
# test_formatters.py
|
||||||
|
def test_prob_to_cents():
|
||||||
|
assert prob_to_cents(0.30) == 30
|
||||||
|
assert prob_to_cents(0.95) == 95
|
||||||
|
assert prob_to_cents(0.001) == 0
|
||||||
|
|
||||||
|
def test_format_odds():
|
||||||
|
assert format_odds(0.30) == "30c"
|
||||||
|
assert format_odds(0.95) == "95c"
|
||||||
|
|
||||||
|
# test_filters.py
|
||||||
|
def test_is_match_market_with_series():
|
||||||
|
e = {"seriesSlug": "csg", "gameId": "123", "title": "Team A vs Team B"}
|
||||||
|
assert is_match_market(e) == True
|
||||||
|
|
||||||
|
def test_is_match_market_vs_syntax():
|
||||||
|
e = {"title": "Team A vs Team B"}
|
||||||
|
assert is_match_market(e) == True
|
||||||
|
|
||||||
|
def test_is_match_market_non_match():
|
||||||
|
e = {"title": "Tournament Winner"}
|
||||||
|
assert is_match_market(e) == False
|
||||||
|
|
||||||
|
# test_filters.py - is_tradeable_event
|
||||||
|
def test_bo2_tie_filter():
|
||||||
|
"""BO2 matches ending 1-1 should be filtered out."""
|
||||||
|
e = create_bo2_event(ended_tie=True)
|
||||||
|
assert is_tradeable_event(e) == False
|
||||||
|
|
||||||
|
def test_converged_market_filter():
|
||||||
|
"""Market with bestBid >= 0.99 should be filtered."""
|
||||||
|
e = create_event_with_ml(bestBid=0.99, bestAsk=0.99)
|
||||||
|
assert is_tradeable_event(e) == False
|
||||||
|
```
|
||||||
|
|
||||||
|
**Mock API responses needed:**
|
||||||
|
- Store sample API responses in `tests/fixtures/` as JSON
|
||||||
|
- Use `responses` or `requests-mock` to mock HTTP calls
|
||||||
|
|
||||||
|
### 3.3 Documentation Improvements (Priority: MEDIUM)
|
||||||
|
|
||||||
|
1. Add troubleshooting section to SKILL.md
|
||||||
|
2. Add concrete usage examples
|
||||||
|
3. Add HTML escape notes for Telegram
|
||||||
|
4. Add changelog
|
||||||
|
5. Document the 1-indexed `--detail` argument more clearly
|
||||||
|
|
||||||
|
### 3.4 Configuration Options (Priority: LOW)
|
||||||
|
|
||||||
|
1. Make timezone (WIB) configurable via `--timezone` argument or env var
|
||||||
|
2. Make inter-page delay configurable
|
||||||
|
3. Add `--json` output option for programmatic use
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 4. Summary Table
|
||||||
|
|
||||||
|
| Category | Item | Priority | Effort |
|
||||||
|
|----------|------|----------|--------|
|
||||||
|
| **Code** | Refactor print_browse() into smaller functions | HIGH | Medium |
|
||||||
|
| **Code** | Refactor send_to_telegram() into smaller functions | HIGH | Medium |
|
||||||
|
| **Code** | Break long lines to max 120 chars | HIGH | Low |
|
||||||
|
| **Tests** | Add unit tests for filters | HIGH | Medium |
|
||||||
|
| **Tests** | Add unit tests for formatters | HIGH | Low |
|
||||||
|
| **Tests** | Add integration tests with mocked API | MEDIUM | Medium |
|
||||||
|
| **Docs** | Add troubleshooting section to SKILL.md | MEDIUM | Low |
|
||||||
|
| **Docs** | Add usage examples to SKILL.md | MEDIUM | Low |
|
||||||
|
| **Code** | Consolidate duplicate time functions | LOW | Low |
|
||||||
|
| **Code** | Add type hints | LOW | Medium |
|
||||||
|
| **Config** | Make timezone configurable | LOW | Low |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 5. Next Steps
|
||||||
|
|
||||||
|
1. **Immediate:** Create unit test structure under `tests/`
|
||||||
|
2. **Short-term:** Refactor `print_browse()` and `send_to_telegram()` into smaller functions
|
||||||
|
3. **Short-term:** Break long lines to max 120 characters
|
||||||
|
4. **Medium-term:** Add comprehensive unit tests
|
||||||
|
5. **Medium-term:** Update SKILL.md with troubleshooting and examples
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 6. Security Audit
|
||||||
|
|
||||||
|
### 6.1 Severity Classification
|
||||||
|
|
||||||
|
| Severity | Meaning |
|
||||||
|
|----------|---------|
|
||||||
|
| CRITICAL | Immediate action required. Users should stop using the skill until fixed. |
|
||||||
|
| HIGH | Serious vulnerability. Fix as soon as possible. |
|
||||||
|
| MEDIUM | Moderate issue. Fix in normal development cycle. |
|
||||||
|
| LOW | Minor issue. Fix when convenient. |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 6.2 CRITICAL: Telegram Bot Token Exposed in Process Listings
|
||||||
|
|
||||||
|
**Location:** Lines 598-606 (`send_to_telegram()`)
|
||||||
|
|
||||||
|
**Description:**
|
||||||
|
The Telegram bot token is passed as a subprocess argument to `curl`, making it visible in the process command line. On any Unix system, any user can view all running processes' command lines via `ps aux` or `/proc/*/cmdline`.
|
||||||
|
|
||||||
|
**Proof of Concept:**
|
||||||
|
```bash
|
||||||
|
# While the script is running (or just after it finishes):
|
||||||
|
ps aux | grep curl
|
||||||
|
|
||||||
|
# Output reveals token:
|
||||||
|
# curl -s https://api.telegram.org/bot123456789:ABCdefGHI/sendMessage -d chat_id=... -d text=...
|
||||||
|
```
|
||||||
|
|
||||||
|
**Impact:**
|
||||||
|
- Any user on a shared system can steal the bot token
|
||||||
|
- Attacker can send arbitrary messages from the bot
|
||||||
|
- Attacker can use the bot for spam, phishing, or social engineering
|
||||||
|
- If the bot is in group chats, attacker can extract member information
|
||||||
|
|
||||||
|
**Fix:**
|
||||||
|
Use curl with `--oauth2-bearer` or environment variable approach. The bot token should NEVER appear in the command line. Recommended fix:
|
||||||
|
|
||||||
|
```python
|
||||||
|
import os
|
||||||
|
|
||||||
|
def send_to_telegram(...):
|
||||||
|
bot_token = os.environ.get("BOT_TOKEN")
|
||||||
|
chat_id = os.environ.get("CHAT_ID")
|
||||||
|
if not bot_token or not chat_id:
|
||||||
|
return
|
||||||
|
|
||||||
|
env = os.environ.copy()
|
||||||
|
# Use a temp file or proper curl auth method
|
||||||
|
# Actually Telegram bot tokens cannot be passed via header
|
||||||
|
# Instead: use Python's requests library which handles this securely
|
||||||
|
# OR: write token to a file with 0600 perms and use curl's --config option
|
||||||
|
```
|
||||||
|
|
||||||
|
**Proper fix using Python `requests` library:**
|
||||||
|
```python
|
||||||
|
import requests
|
||||||
|
|
||||||
|
def send_to_telegram(...):
|
||||||
|
bot_token = os.environ.get("BOT_TOKEN")
|
||||||
|
chat_id = os.environ.get("CHAT_ID")
|
||||||
|
if not bot_token or not chat_id:
|
||||||
|
return
|
||||||
|
|
||||||
|
url = f"https://api.telegram.org/bot{bot_token}/sendMessage"
|
||||||
|
payload = {"chat_id": chat_id, "text": text, "parse_mode": "HTML"}
|
||||||
|
# Token stays in memory, never in command line
|
||||||
|
resp = requests.post(url, data=payload, timeout=10)
|
||||||
|
```
|
||||||
|
|
||||||
|
**Interim mitigation:** If you must use curl, write the token to a temporary file with mode 0600 and use curl's `--config` flag, then delete the file immediately after.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 6.3 HIGH: HTML Injection in Telegram Messages (XSS-adjacent)
|
||||||
|
|
||||||
|
**Location:** Lines 614-661 (`send_to_telegram()`)
|
||||||
|
|
||||||
|
**Description:**
|
||||||
|
Event titles and other data from the Polymarket API are inserted directly into Telegram messages with `parse_mode=HTML`. No HTML escaping is performed. Characters like `<`, `>`, `&` are not escaped.
|
||||||
|
|
||||||
|
**Attack scenario:**
|
||||||
|
1. Polymarket (or an attacker who compromises Polymarket data) includes a malicious title like:
|
||||||
|
- `<script>alert('XSS')</script>`
|
||||||
|
- `<img src=x onerror=alert(1)>`
|
||||||
|
- `Team A & Team B` (ampersand not escaped renders as `&amp;` or breaks parsing)
|
||||||
|
2. The bot sends this to Telegram
|
||||||
|
3. Telegram renders the HTML, potentially executing JavaScript in the context of the Telegram web client
|
||||||
|
|
||||||
|
**Note:** Telegram's HTML parser is restrictive (`<b>`, `<i>`, `<a>`, etc. only), so classic XSS is limited. However:
|
||||||
|
- Malformed HTML can crash the Telegram client
|
||||||
|
- Attribute-based injections in `<a>` tags could be possible
|
||||||
|
- The `<` and `>` characters themselves are illegal in Telegram HTML and will cause parse errors
|
||||||
|
|
||||||
|
**Current vulnerable code (simplified):**
|
||||||
|
```python
|
||||||
|
title = e.get("title", "?")
|
||||||
|
lines.append(f"<b>{i}.</b> <a href=\"{url}\">{title_clean}</a>")
|
||||||
|
# title_clean is title.split(" - ")[0].strip() -- no HTML escaping
|
||||||
|
```
|
||||||
|
|
||||||
|
**Fix:**
|
||||||
|
```python
|
||||||
|
import html
|
||||||
|
|
||||||
|
def escape_html(text):
|
||||||
|
"""Escape HTML special characters for Telegram."""
|
||||||
|
return (text
|
||||||
|
.replace("&", "&")
|
||||||
|
.replace("<", "<")
|
||||||
|
.replace(">", ">")
|
||||||
|
.replace('"', """))
|
||||||
|
|
||||||
|
title = e.get("title", "?")
|
||||||
|
title_escaped = escape_html(title_clean)
|
||||||
|
lines.append(f"<b>{i}.</b> <a href=\"{url}\">{title_escaped}</a>")
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 6.4 MEDIUM: Insufficient Input Sanitization on `--search` Parameter
|
||||||
|
|
||||||
|
**Location:** Line 39 (`fetch_page()`)
|
||||||
|
|
||||||
|
**Description:**
|
||||||
|
The `--search` argument is only sanitized with `.replace(' ', '%20')`. This only handles spaces. Other URL-sensitive characters (`#`, `?`, `&`, `%`, `+`, etc.) are not properly URL-encoded.
|
||||||
|
|
||||||
|
**Attack scenario:**
|
||||||
|
A user passes `--search "Team%20A"` expecting to search for "Team%20A" literally, but `%20` gets decoded to a space by the URL parser, searching for "Team A" instead.
|
||||||
|
|
||||||
|
More concerning: if the search term contains `#`, it could truncate the query:
|
||||||
|
```bash
|
||||||
|
--search "Team#A" -> URL becomes ?q=Team#A&limit=50...
|
||||||
|
# Everything after # is treated as URL fragment, not part of the query
|
||||||
|
```
|
||||||
|
|
||||||
|
**Current code:**
|
||||||
|
```python
|
||||||
|
url = (f"{base}?q={q.replace(' ', '%20')}&limit={PAGE_SIZE}&page={page}"
|
||||||
|
f"&search_profiles=false&search_tags=false"
|
||||||
|
f"&keep_closed_markets=0&events_status=active&cache=false")
|
||||||
|
```
|
||||||
|
|
||||||
|
**Fix:**
|
||||||
|
Use `urllib.parse.quote()` for proper URL encoding:
|
||||||
|
```python
|
||||||
|
from urllib.parse import quote
|
||||||
|
|
||||||
|
def fetch_page(q, page=1, ...):
|
||||||
|
base = "https://gamma-api.polymarket.com/public-search"
|
||||||
|
encoded_q = quote(q, safe='')
|
||||||
|
url = (f"{base}?q={encoded_q}&limit={PAGE_SIZE}&page={page}"
|
||||||
|
...)
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 6.5 MEDIUM: No Bounds Checking on `--detail` Argument
|
||||||
|
|
||||||
|
**Location:** Lines 778-785 (`main()`)
|
||||||
|
|
||||||
|
**Description:**
|
||||||
|
The `--detail N` argument is used to index into `result["match_events"]` without proper bounds checking. While there is a fallback (`idx = 0` if out of range), the logic silently defaults to index 0, which may not be what the user intended.
|
||||||
|
|
||||||
|
```python
|
||||||
|
idx = args.detail - 1 # User provides 1-indexed
|
||||||
|
if idx < 0 or idx >= len(result["match_events"]):
|
||||||
|
idx = 0 # Silently defaults to first event
|
||||||
|
detail_event = result["match_events"][idx]
|
||||||
|
```
|
||||||
|
|
||||||
|
**Impact:**
|
||||||
|
- Low security impact, but confusing UX
|
||||||
|
- User expects event #100 but gets event #1 silently
|
||||||
|
|
||||||
|
**Fix:** Warn user if index is out of range instead of silently defaulting.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 6.6 MEDIUM: Potential Denial of Service via Large API Response
|
||||||
|
|
||||||
|
**Location:** Lines 53, 79-81, 357-358, 393-403
|
||||||
|
|
||||||
|
**Description:**
|
||||||
|
The code loads the entire API response into memory via `json.loads()`. If a malicious or compromised Polymarket API returned an extremely large JSON payload (gigabytes), the script could exhaust system memory.
|
||||||
|
|
||||||
|
**Additional issue:** `fetch_all_pages()` loops up to 100 pages, each with up to 50 events. While bounded, there's no size limit on individual events or their nested `markets` arrays.
|
||||||
|
|
||||||
|
**Fix:**
|
||||||
|
```python
|
||||||
|
# Add response size limits
|
||||||
|
MAX_RESPONSE_SIZE = 10 * 1024 * 1024 # 10MB
|
||||||
|
if len(r.stdout) > MAX_RESPONSE_SIZE:
|
||||||
|
raise ValueError(f"API response too large: {len(r.stdout)} bytes")
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 6.7 LOW: Bare `except:` Clauses Swallow Security-Relevant Errors
|
||||||
|
|
||||||
|
**Location:** Lines 169, 183, 269, 308, 456
|
||||||
|
|
||||||
|
**Description:**
|
||||||
|
Bare `except: pass` catches ALL exceptions including `KeyboardInterrupt`, `SystemExit`, `MemoryError`, and `OSError`. This silently hides errors that might indicate security problems (e.g., timeouts could suggest a DoS attack in progress).
|
||||||
|
|
||||||
|
**Current code:**
|
||||||
|
```python
|
||||||
|
try:
|
||||||
|
end_dt = datetime.fromisoformat(end_str.replace('Z', '+00:00'))
|
||||||
|
...
|
||||||
|
except:
|
||||||
|
pass # Silently ignores ALL errors
|
||||||
|
```
|
||||||
|
|
||||||
|
**Fix:**
|
||||||
|
```python
|
||||||
|
try:
|
||||||
|
end_dt = datetime.fromisoformat(end_str.replace('Z', '+00:00'))
|
||||||
|
...
|
||||||
|
except (ValueError, TypeError):
|
||||||
|
pass # Only catch expected exceptions
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 6.8 LOW: No Rate Limiting on API Calls (DoS vector)
|
||||||
|
|
||||||
|
**Location:** Lines 74-76 (`fetch_all_pages()`)
|
||||||
|
|
||||||
|
**Description:**
|
||||||
|
An attacker who can pass arguments to this script (e.g., via a web wrapper) could repeatedly call the Polymarket API in a loop, potentially:
|
||||||
|
1. Getting the user's IP rate-limited or banned by Polymarket
|
||||||
|
2. Consuming the user's bandwidth
|
||||||
|
3. Causing resource exhaustion on the host
|
||||||
|
|
||||||
|
**Fix:** Add a cooldown between runs if called repeatedly, or use a token bucket rate limiter.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 6.9 Security Summary Table
|
||||||
|
|
||||||
|
| # | Issue | Severity | Exploitable Remotely | Fix Complexity |
|
||||||
|
|---|-------|----------|----------------------|----------------|
|
||||||
|
| 1 | Telegram bot token in process cmdline | CRITICAL | No (local access needed) | Easy |
|
||||||
|
| 2 | HTML injection in Telegram messages | HIGH | No (needs Polymarket compromise) | Easy |
|
||||||
|
| 3 | Insufficient `--search` sanitization | MEDIUM | Yes (any user input) | Easy |
|
||||||
|
| 4 | No bounds check on `--detail` | MEDIUM | Yes (any user input) | Trivial |
|
||||||
|
| 5 | Large API response can exhaust memory | MEDIUM | Yes (API or MITM) | Easy |
|
||||||
|
| 6 | Bare `except:` swallows errors | LOW | No | Trivial |
|
||||||
|
| 7 | No API rate limiting | LOW | Yes (with script access) | Medium |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 6.10 Recommended Immediate Actions
|
||||||
|
|
||||||
|
**If you are currently using this skill with a Telegram bot:**
|
||||||
|
|
||||||
|
1. **ROTATE YOUR BOT TOKEN IMMEDIATELY** — Go to @BotFather and use `/revoke` to invalidate the current token. Generate a new one.
|
||||||
|
|
||||||
|
2. **Do not use the `--telegram` flag** on shared/multi-user systems until Issue #1 is fixed.
|
||||||
|
|
||||||
|
3. **Do not use `--search` with untrusted input** until Issue #3 is fixed.
|
||||||
|
|
||||||
|
4. Monitor your Telegram bot's `getUpdates` for unauthorized access.
|
||||||
|
|
||||||
|
**Safe usage until fixes are deployed:**
|
||||||
|
```bash
|
||||||
|
# Without Telegram (safe):
|
||||||
|
polymarket-browse --category "Counter Strike" --limit 5
|
||||||
|
|
||||||
|
# With Telegram (UNSAFE on shared systems until fix is deployed):
|
||||||
|
# NOT RECOMMENDED until security issues are addressed
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 6.11 Fix Priority Order
|
||||||
|
|
||||||
|
1. **FIRST (CRITICAL):** Fix Telegram bot token exposure — use Python `requests` library instead of curl subprocess, OR use curl with `--config` file approach
|
||||||
|
2. **SECOND (HIGH):** Add HTML escaping for Telegram messages
|
||||||
|
3. **THIRD (MEDIUM):** Fix `--search` URL encoding with `urllib.parse.quote()`
|
||||||
|
4. **FOURTH (MEDIUM):** Add `--detail` bounds checking
|
||||||
|
5. **FIFTH (MEDIUM):** Add response size limits
|
||||||
|
6. **SIXTH (LOW):** Replace bare `except:` with specific exceptions
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Appendix A: Longest Lines in browse.py (for targeted refactoring)
|
||||||
|
|
||||||
|
| Line | Chars | Location | Content Summary |
|
||||||
|
|------|-------|----------|-----------------|
|
||||||
|
| 474 | 209 | `print_browse()` | Function signature |
|
||||||
|
| 564 | 152 | `print_detail()` | ML odds formatting |
|
||||||
|
| 571 | 136 | `print_detail()` | Market outcome formatting |
|
||||||
|
| 760 | 128 | `send_to_telegram()` | Telegram send call |
|
||||||
|
| 561 | 126 | `print_detail()` | Spread formatting |
|
||||||
|
| 736 | 122 | `send_to_telegram()` | Telegram API URL |
|
||||||
|
| 485 | 121 | `print_browse()` | Fetch stats line |
|
||||||
|
| 467 | 119 | `print_browse()` | Print category header |
|
||||||
|
| 728 | 112 | `send_to_telegram()` | Telegram send call |
|
||||||
|
| 569 | 110 | `print_detail()` | Market spread formatting |
|
||||||
|
|
||||||
|
**Key finding:** The `print_browse()` function signature itself (line 474) at 209 chars is the longest. This should be broken up or the function should accept a config dict instead of 11 parameters.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Appendix B: Duplicate Code Patterns
|
||||||
|
|
||||||
|
### B.1 Time formatting duplicated across 3 functions
|
||||||
|
|
||||||
|
| Function | Lines | Purpose |
|
||||||
|
|----------|-------|---------|
|
||||||
|
| `get_match_time_status()` | ~40 | Returns (status_str, urgency) tuple |
|
||||||
|
| `get_match_time_str()` | ~35 | Returns just status string |
|
||||||
|
| `get_start_time_wib()` | ~50 | Returns (abs_str, rel_str) tuple |
|
||||||
|
|
||||||
|
All three parse the same ISO datetime string and compute the same relative time logic. Should be consolidated into one function returning all needed values.
|
||||||
|
|
||||||
|
### B.2 `is_tradeable_event()` vs `is_tradeable_market()`
|
||||||
|
|
||||||
|
Both check convergence (bestBid >= 0.99, bestAsk <= 0.01) and acceptingOrders/closed status. The market-level one is simpler but they share the same convergence check logic.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Appendix C: Potential Bugs
|
||||||
|
|
||||||
|
### C.1 Bare `except:` clauses
|
||||||
|
|
||||||
|
Found at lines 169, 183, and similar locations:
|
||||||
|
```python
|
||||||
|
except:
|
||||||
|
pass
|
||||||
|
```
|
||||||
|
|
||||||
|
**Risk:** Catches KeyboardInterrupt, SystemExit, and json.JSONDecodeError. Should be:
|
||||||
|
```python
|
||||||
|
except (ValueError, TypeError):
|
||||||
|
pass
|
||||||
|
```
|
||||||
|
|
||||||
|
### C.2 Line 474: `print_browse()` signature is 209 characters
|
||||||
|
|
||||||
|
```python
|
||||||
|
def print_browse(match_events, non_match_events, category, total_raw, total_fetched, total_match, total_non_match, raw_mode=False, partial=False, non_matches_max=5, matches_only=False, non_matches_only=False):
|
||||||
|
```
|
||||||
|
|
||||||
|
**Issue:** 11 parameters is too many. Consider using a result dict or a config object.
|
||||||
|
|
||||||
|
**Fix options:**
|
||||||
|
1. Accept a `BrowseResult` namedtuple/dataclass
|
||||||
|
2. Split into `print_browse_header()` and `print_browse_sections()`
|
||||||
|
3. Use `**kwargs`
|
||||||
|
|
||||||
|
### C.3 Line 128 in `send_to_telegram()`: `bot_token=os.environ.get("BOT_` (truncated)
|
||||||
|
|
||||||
|
```python
|
||||||
|
bot_token=os.environ.get("BOT_TOKEN")
|
||||||
|
chat_id = os.environ.get("CHAT_ID")
|
||||||
|
```
|
||||||
|
|
||||||
|
This looks like a line that was cut off in the output but the actual code is fine. However, it highlights that the line at 582 is long.
|
||||||
|
|
||||||
|
### C.4 `chunk_len` variable unused
|
||||||
|
|
||||||
|
At line 681 in `send_to_telegram()`:
|
||||||
|
```python
|
||||||
|
chunk = []
|
||||||
|
chunk_len = 0 # NEVER USED
|
||||||
|
chunk_num = 1 # NEVER USED
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Appendix D: Missing Test Coverage
|
||||||
|
|
||||||
|
Functions that need tests but have none:
|
||||||
|
|
||||||
|
```
|
||||||
|
[x] fetch_page - needs mock curl response
|
||||||
|
[x] fetch_all_pages - needs mock paginated responses
|
||||||
|
[x] is_match_market - easy to test with dict inputs
|
||||||
|
[x] is_tradeable_event - complex, needs many test cases
|
||||||
|
[x] is_tradeable_market - similar to above
|
||||||
|
[x] is_bo2_tie - edge cases for BO2 detection
|
||||||
|
[x] get_ml_market - easy to test
|
||||||
|
[x] get_ml_volume - easy to test
|
||||||
|
[x] prob_to_cents - pure function, easy to test
|
||||||
|
[x] format_odds - pure function, easy to test
|
||||||
|
[x] format_spread - pure function, easy to test
|
||||||
|
[x] get_match_time_* - needs timezone mocking
|
||||||
|
[x] get_tournament - easy to test
|
||||||
|
[x] get_event_url - easy to test
|
||||||
|
[x] filter_events - easy to test
|
||||||
|
[x] sort_events - easy to test
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
*Report generated by Hermes Agent on 2026-03-25*
|
||||||
Reference in New Issue
Block a user