fix: address code review findings — security, correctness, formatting

- Fix token refresh body encoding: use urlencode() instead of manual
  string concatenation to prevent corruption of tokens with special chars
- Fix refresh concurrency guard: share awaitable across concurrent 401s
  instead of returning None (matches TypeScript SDK behavior)
- Set restrictive file permissions (0600) on encrypted token files
- Apply _safe_segment validation to all URL builders that interpolate
  user-supplied parameters (defense-in-depth against path traversal)
- Default order_stock time_in_force to "gfd" instead of "gtc" for safety
- Use asyncio.to_thread for sync keyring calls to avoid blocking event loop
- Fix trailing_peg payload to omit unused fields instead of sending nulls
- Fix ruff formatting issues that caused CI failure
- Add pagination + untrusted URL rejection tests for _http.py
- Add parametrized path traversal tests for all URL builders

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: kevin1chun

python/src/robinhood_agents/_auth.py

@@ -6,9 +6,11 @@
 
 from __future__ import annotations
 
-from dataclasses import dataclass
+import asyncio
+from dataclasses import dataclass, field
 from time import time
 from typing import TYPE_CHECKING
+from urllib.parse import urlencode
 
 import httpx
 
@@ -31,7 +33,8 @@ class AuthState:
 
     tokens: TokenData
     store: TokenStore
-    refreshing: bool = False
+    _refresh_lock: asyncio.Lock = field(default_factory=asyncio.Lock, repr=False)
+    _refresh_task: asyncio.Task[str | None] | None = field(default=None, repr=False)
 
 
 async def _refresh_tokens(state: AuthState) -> str | None:
@@ -60,7 +63,7 @@ async def _refresh_tokens(state: AuthState) -> str | None:
                     "Content-Type": "application/x-www-form-urlencoded; charset=utf-8",
                     "X-Robinhood-API-Version": "1.431.4",
                 },
-                content="&".join(f"{k}={v}" for k, v in body.items()),
+                content=urlencode(body),
             )
     except Exception:
         return None
@@ -99,16 +102,22 @@ async def _refresh_tokens(state: AuthState) -> str | None:
 def _create_refresh_callback(
     state: AuthState,
 ) -> Callable[[], Awaitable[str | None]]:
-    """Create a 401-refresh callback with a concurrency guard."""
+    """Create a 401-refresh callback with a concurrency guard.
+
+    Concurrent 401s coalesce onto a single refresh attempt — all waiters
+    get the same result, matching the TypeScript SDK's behaviour.
+    """
 
     async def _refresh() -> str | None:
-        if state.refreshing:
-            return None  # Another refresh is in progress
-        state.refreshing = True
+        async with state._refresh_lock:
+            # If another coroutine already refreshed while we waited, return that result
+            if state._refresh_task is not None:
+                return await state._refresh_task
+            state._refresh_task = asyncio.ensure_future(_refresh_tokens(state))
         try:
-            return await _refresh_tokens(state)
+            return await state._refresh_task
         finally:
-            state.refreshing = False
+            state._refresh_task = None
 
     return _refresh
 
@@ -152,7 +161,12 @@ async def logout(session: Session, state: AuthState | None) -> None:
             async with httpx.AsyncClient(timeout=5.0) as client:
                 await client.post(
                     "https://api.robinhood.com/oauth2/revoke_token/",
-                    content=f"client_id={_CLIENT_ID}&token={state.tokens.access_token}",
+                    content=urlencode(
+                        {
+                            "client_id": _CLIENT_ID,
+                            "token": state.tokens.access_token,
+                        }
+                    ),
                     headers={"Content-Type": "application/x-www-form-urlencoded; charset=utf-8"},
                 )
         except Exception:

python/src/robinhood_agents/_client.py

@@ -544,7 +544,7 @@ async def order_stock(
         stop_price: float | None = None,
         trail_amount: float | None = None,
         trail_type: str | None = None,
-        time_in_force: str | None = None,
+        time_in_force: str = "gfd",
         extended_hours: bool = False,
         account_number: str | None = None,
     ) -> StockOrder:
@@ -588,7 +588,7 @@ async def order_stock(
             "quantity": str(quantity),
             "type": order_type,
             "trigger": trigger,
-            "time_in_force": "gfd" if is_fractional else (time_in_force or "gtc"),
+            "time_in_force": "gfd" if is_fractional else time_in_force,
             "extended_hours": extended_hours,
             "ref_id": str(uuid.uuid4()),
         }
@@ -599,11 +599,12 @@ async def order_stock(
             payload["stop_price"] = str(stop_price)
         if trail_amount is not None:
             t_type = trail_type or "percentage"
-            payload["trailing_peg"] = {
-                "type": t_type,
-                "percentage": str(trail_amount) if t_type != "amount" else None,
-                "price": {"amount": str(trail_amount)} if t_type == "amount" else None,
-            }
+            peg: dict[str, object] = {"type": t_type}
+            if t_type == "amount":
+                peg["price"] = {"amount": str(trail_amount)}
+            else:
+                peg["percentage"] = str(trail_amount)
+            payload["trailing_peg"] = peg
 
         # Market buys get a 5% price collar
         if order_type == "market" and side == "buy" and trigger == "immediate":

python/src/robinhood_agents/_session.py

@@ -16,11 +16,13 @@
 _DEFAULT_TIMEOUT = 16.0
 
 # Trusted Robinhood origins for redirect safety.
-_TRUSTED_ORIGINS = frozenset({
-    "https://api.robinhood.com",
-    "https://nummus.robinhood.com",
-    "https://robinhood.com",
-})
+_TRUSTED_ORIGINS = frozenset(
+    {
+        "https://api.robinhood.com",
+        "https://nummus.robinhood.com",
+        "https://robinhood.com",
+    }
+)
 
 
 async def _safe_fetch(
@@ -104,7 +106,11 @@ async def _fetch_with_retry(
     ) -> httpx.Response:
         """Fetch with single-retry on 401."""
         resp = await _safe_fetch(
-            self._client, method, url, headers=headers, content=content,
+            self._client,
+            method,
+            url,
+            headers=headers,
+            content=content,
             timeout=timeout,
         )
 
@@ -114,7 +120,11 @@ async def _fetch_with_retry(
                 self._access_token = new_token
                 headers = {**headers, "Authorization": f"Bearer {new_token}"}
                 resp = await _safe_fetch(
-                    self._client, method, url, headers=headers, content=content,
+                    self._client,
+                    method,
+                    url,
+                    headers=headers,
+                    content=content,
                     timeout=timeout,
                 )
 
@@ -123,7 +133,9 @@ async def _fetch_with_retry(
     async def get(self, url: str, params: dict[str, str] | None = None) -> httpx.Response:
         target = f"{url}?{urlencode(params)}" if params else url
         return await self._fetch_with_retry(
-            "GET", target, headers=self._auth_headers(self._headers),
+            "GET",
+            target,
+            headers=self._auth_headers(self._headers),
         )
 
     async def post(
@@ -153,16 +165,20 @@ async def post(
                 [(k, str(v)) for k, v in (body or {}).items()],
             )
 
-        req_timeout = (
-            httpx.Timeout(timeout) if timeout and timeout != self._timeout else None
-        )
+        req_timeout = httpx.Timeout(timeout) if timeout and timeout != self._timeout else None
         return await self._fetch_with_retry(
-            "POST", url, headers=headers, content=content, timeout=req_timeout,
+            "POST",
+            url,
+            headers=headers,
+            content=content,
+            timeout=req_timeout,
         )
 
     async def delete(self, url: str) -> httpx.Response:
         return await self._fetch_with_retry(
-            "DELETE", url, headers=self._auth_headers(self._headers),
+            "DELETE",
+            url,
+            headers=self._auth_headers(self._headers),
         )
 
     async def close(self) -> None:

python/src/robinhood_agents/_token_store.py

@@ -15,9 +15,11 @@
 
 from __future__ import annotations
 
+import asyncio
 import base64
 import json
 import os
+import stat
 from dataclasses import dataclass
 from pathlib import Path
 from time import time
@@ -111,13 +113,18 @@ async def delete(self) -> None: ...
 
 
 class KeychainTokenStore:
-    """Store tokens in the OS keychain via the ``keyring`` library."""
+    """Store tokens in the OS keychain via the ``keyring`` library.
+
+    Keyring calls are synchronous and may block (especially on macOS with
+    authorization prompts), so they are dispatched to a thread via
+    ``asyncio.to_thread`` to avoid blocking the event loop.
+    """
 
     async def load(self) -> TokenData | None:
         try:
             import keyring
 
-            raw = keyring.get_password(KEYRING_SERVICE, KEYRING_TOKENS)
+            raw = await asyncio.to_thread(keyring.get_password, KEYRING_SERVICE, KEYRING_TOKENS)
             if raw:
                 data = json.loads(raw)
                 return TokenData.from_dict(data)
@@ -128,13 +135,15 @@ async def load(self) -> TokenData | None:
     async def save(self, tokens: TokenData) -> None:
         import keyring
 
-        keyring.set_password(KEYRING_SERVICE, KEYRING_TOKENS, json.dumps(tokens.to_dict()))
+        await asyncio.to_thread(
+            keyring.set_password, KEYRING_SERVICE, KEYRING_TOKENS, json.dumps(tokens.to_dict())
+        )
 
     async def delete(self) -> None:
         try:
             import keyring
 
-            keyring.delete_password(KEYRING_SERVICE, KEYRING_TOKENS)
+            await asyncio.to_thread(keyring.delete_password, KEYRING_SERVICE, KEYRING_TOKENS)
         except Exception:
             pass
 
@@ -170,7 +179,9 @@ def _resolve_encryption_key() -> bytes:
         import keyring
 
         keyring.set_password(
-            KEYRING_SERVICE, KEYRING_ENCRYPTION_KEY, base64.b64encode(key).decode(),
+            KEYRING_SERVICE,
+            KEYRING_ENCRYPTION_KEY,
+            base64.b64encode(key).decode(),
         )
     except Exception:
         pass  # Key lives only in memory this session
@@ -232,6 +243,7 @@ async def save(self, tokens: TokenData) -> None:
         path = Path(self._file_path)
         path.parent.mkdir(parents=True, exist_ok=True)
         path.write_text(json.dumps(blob), "utf-8")
+        path.chmod(stat.S_IRUSR | stat.S_IWUSR)  # 0600 — owner only
 
     async def delete(self) -> None:
         Path(self._file_path).unlink(missing_ok=True)