Skip to content

Commit d59a1e5

Browse files
kevin1chunclaude
andcommitted
security: comprehensive hardening — URL validation, order bounds, redaction, rate limiting
TypeScript URL safety (HIGH): - Apply safeSegment() to all 22 URL builders that interpolate parameters (was only applied to tags() and marketHours(), matching Python parity) - Narrow SAFE_PATH_SEGMENT regex: remove : and @ characters Order safety (HIGH + MEDIUM): - Default time_in_force to "gfd" instead of "gtc" in TypeScript client and MCP tool (matches Python fix from previous commit) - Fix trailing_peg payload: omit unused fields instead of undefined - Add bounds validation to all order methods (both SDKs): quantity, price, stop_price, trail_amount, limit_price must be positive + finite - Fix crypto order division-by-zero: limitPrice validated > 0 before division in both SDKs Encryption key validation (MEDIUM): - ROBINHOOD_TOKEN_KEY env var now validated: must decode to exactly 32 bytes (both Python and TypeScript) Redaction hardening (MEDIUM): - Add password, secret, account_number to SENSITIVE_KEYS (both SDKs) - Add "Authorization: Bearer <token>" plain-text pattern detection - Both redactTokens() and redact_tokens() now catch bearer headers MCP tool input hardening (MEDIUM): - Add .positive() to all Zod z.number() schemas for order prices/quantities - Add .max(200) to symbol string inputs to prevent oversized requests Token refresh rate limiting (MEDIUM): - Add 5-second minimum interval between refresh attempts (both SDKs) - Prevents DoS via repeated 401s exhausting the refresh endpoint Tests: - 24 new path traversal rejection tests for TypeScript URL builders - TypeScript: 168 tests passing - Python: 88 tests passing - PII scan: clean (no emails, tokens, credentials in codebase) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent a05e407 commit d59a1e5

File tree

14 files changed

+203
-44
lines changed

14 files changed

+203
-44
lines changed

python/src/robinhood_agents/_auth.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
_CLIENT_ID = "c82SH0WZOsabOXGP2sxqcj34FxkvfnWRZBKlBjFS"
2727
_EXPIRATION_TIME = 734000
28+
_MIN_REFRESH_INTERVAL = 5.0 # seconds
2829

2930

3031
@dataclass
@@ -35,6 +36,7 @@ class AuthState:
3536
store: TokenStore
3637
_refresh_lock: asyncio.Lock = field(default_factory=asyncio.Lock, repr=False)
3738
_refresh_task: asyncio.Task[str | None] | None = field(default=None, repr=False)
39+
_last_refresh_at: float = field(default=0.0, repr=False)
3840

3941

4042
async def _refresh_tokens(state: AuthState) -> str | None:
@@ -109,10 +111,15 @@ def _create_refresh_callback(
109111
"""
110112

111113
async def _refresh() -> str | None:
114+
# Rate limit: refuse to refresh if the last attempt was too recent
115+
if time() - state._last_refresh_at < _MIN_REFRESH_INTERVAL:
116+
return None
117+
112118
async with state._refresh_lock:
113119
# If another coroutine already refreshed while we waited, return that result
114120
if state._refresh_task is not None:
115121
return await state._refresh_task
122+
state._last_refresh_at = time()
116123
state._refresh_task = asyncio.ensure_future(_refresh_tokens(state))
117124
try:
118125
return await state._refresh_task

python/src/robinhood_agents/_client.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -551,6 +551,18 @@ async def order_stock(
551551
self._require_auth()
552552
sym = symbol.strip().upper()
553553

554+
# Validate numeric bounds
555+
import math
556+
557+
if quantity <= 0 or not math.isfinite(quantity):
558+
raise ValueError("quantity must be a positive finite number")
559+
if limit_price is not None and (limit_price <= 0 or not math.isfinite(limit_price)):
560+
raise ValueError("limit_price must be a positive finite number")
561+
if stop_price is not None and (stop_price <= 0 or not math.isfinite(stop_price)):
562+
raise ValueError("stop_price must be a positive finite number")
563+
if trail_amount is not None and (trail_amount <= 0 or not math.isfinite(trail_amount)):
564+
raise ValueError("trail_amount must be a positive finite number")
565+
554566
# Validate mutually exclusive order params
555567
if trail_amount is not None and (limit_price is not None or stop_price is not None):
556568
msg = "Cannot combine trail_amount with limit_price or stop_price"
@@ -657,9 +669,17 @@ async def order_option(
657669
account_number: str | None = None,
658670
) -> OptionOrder:
659671
self._require_auth()
672+
import math
673+
660674
if not legs:
661675
msg = "At least one leg is required"
662676
raise ValueError(msg)
677+
if price <= 0 or not math.isfinite(price):
678+
raise ValueError("price must be a positive finite number")
679+
if quantity <= 0:
680+
raise ValueError("quantity must be a positive integer")
681+
if stop_price is not None and (stop_price <= 0 or not math.isfinite(stop_price)):
682+
raise ValueError("stop_price must be a positive finite number")
663683

664684
resolved_legs = []
665685
for leg in legs:
@@ -749,6 +769,13 @@ async def order_crypto(
749769
limit_price: float | None = None,
750770
) -> CryptoOrder:
751771
self._require_auth()
772+
import math
773+
774+
if amount_or_quantity <= 0 or not math.isfinite(amount_or_quantity):
775+
raise ValueError("amount_or_quantity must be a positive finite number")
776+
if limit_price is not None and (limit_price <= 0 or not math.isfinite(limit_price)):
777+
raise ValueError("limit_price must be a positive finite number")
778+
752779
s = symbol.strip().upper()
753780

754781
raw = await request_get(self._session, urls.crypto_currency_pairs(), data_type="results")

python/src/robinhood_agents/_redact.py

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,13 @@
88
# Matches JWT-shaped strings: three dot-separated base64url segments, each 20+ chars.
99
_JWT_PATTERN = re.compile(r"\beyJ[A-Za-z0-9_-]{20,}\.[A-Za-z0-9_-]{20,}\.[A-Za-z0-9_-]{20,}\b")
1010

11+
# Matches "Authorization: Bearer <token>" in plain-text headers/logs.
12+
_BEARER_HEADER_PATTERN = re.compile(r"\bBearer\s+[A-Za-z0-9_.-]{10,}\b", re.IGNORECASE)
13+
1114
# Matches values of known sensitive keys in JSON-serialized strings.
1215
_SENSITIVE_KEY_PATTERN = re.compile(
13-
r'"(access_token|refresh_token|device_token|bearer_token|authorization)":\s*"([^"]*)"',
16+
r'"(access_token|refresh_token|device_token|bearer_token|authorization'
17+
r'|password|secret|account_number)":\s*"([^"]*)"',
1418
re.IGNORECASE,
1519
)
1620

@@ -21,6 +25,9 @@
2125
"device_token",
2226
"bearer_token",
2327
"token",
28+
"password",
29+
"secret",
30+
"account_number",
2431
}
2532
)
2633

@@ -29,6 +36,7 @@ def redact_tokens(input_str: str) -> str:
2936
"""Redact JWT tokens and known sensitive key values from a string."""
3037
result = _SENSITIVE_KEY_PATTERN.sub(rf'"\1":"{_REDACTED}"', input_str)
3138
result = _JWT_PATTERN.sub(_REDACTED, result)
39+
result = _BEARER_HEADER_PATTERN.sub(f"Bearer {_REDACTED}", result)
3240
return result
3341

3442

python/src/robinhood_agents/_token_store.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,11 @@ def _resolve_encryption_key() -> bytes:
161161
# 1. Env var
162162
env_key = os.environ.get("ROBINHOOD_TOKEN_KEY", "").strip()
163163
if env_key:
164-
return base64.b64decode(env_key)
164+
key = base64.b64decode(env_key)
165+
if len(key) != _KEY_BYTES:
166+
msg = f"ROBINHOOD_TOKEN_KEY must decode to {_KEY_BYTES} bytes (got {len(key)})"
167+
raise ValueError(msg)
168+
return key
165169

166170
# 2. Keychain
167171
try:

typescript/__tests__/client/auth.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ describe("logout", () => {
8888
it("clears access token and onUnauthorized", async () => {
8989
const session = mockSession();
9090
const store = mockStore();
91-
const state: AuthState = { tokens: sampleTokens, store, refreshing: null };
91+
const state: AuthState = { tokens: sampleTokens, store, refreshing: null, lastRefreshAt: 0 };
9292

9393
await logout(session, state);
9494

@@ -99,7 +99,7 @@ describe("logout", () => {
9999
it("attempts to revoke token at Robinhood", async () => {
100100
const session = mockSession();
101101
const store = mockStore();
102-
const state: AuthState = { tokens: sampleTokens, store, refreshing: null };
102+
const state: AuthState = { tokens: sampleTokens, store, refreshing: null, lastRefreshAt: 0 };
103103

104104
await logout(session, state);
105105

@@ -112,7 +112,7 @@ describe("logout", () => {
112112
it("deletes from store", async () => {
113113
const session = mockSession();
114114
const store = mockStore();
115-
const state: AuthState = { tokens: sampleTokens, store, refreshing: null };
115+
const state: AuthState = { tokens: sampleTokens, store, refreshing: null, lastRefreshAt: 0 };
116116

117117
await logout(session, state);
118118

typescript/__tests__/client/urls.test.ts

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,4 +100,45 @@ describe("URL builders", () => {
100100
it("top100", () =>
101101
expect(urls.top100()).toBe("https://api.robinhood.com/midlands/tags/tag/100-most-popular/"));
102102
});
103+
104+
describe("safeSegment rejects path traversal", () => {
105+
const cases: Array<[string, (arg: string) => string]> = [
106+
["challenge", urls.challenge],
107+
["pathfinderInquiry", urls.pathfinderInquiry],
108+
["pushPromptStatus", urls.pushPromptStatus],
109+
["account", urls.account],
110+
["portfolio", urls.portfolio],
111+
["portfolioHistoricals", urls.portfolioHistoricals],
112+
["quote", urls.quote],
113+
["instrument", urls.instrument],
114+
["fundamental", urls.fundamental],
115+
["stockHistoricalsFor", urls.stockHistoricalsFor],
116+
["news", urls.news],
117+
["ratings", urls.ratings],
118+
["optionChain", urls.optionChain],
119+
["optionMarketData", urls.optionMarketData],
120+
["optionOrder", urls.optionOrder],
121+
["cryptoQuote", urls.cryptoQuote],
122+
["cryptoHistoricals", urls.cryptoHistoricals],
123+
["cryptoOrder", urls.cryptoOrder],
124+
["stockOrder", urls.stockOrder],
125+
["cancelStockOrder", urls.cancelStockOrder],
126+
["cancelOptionOrder", urls.cancelOptionOrder],
127+
["cancelCryptoOrder", urls.cancelCryptoOrder],
128+
];
129+
130+
for (const [name, fn] of cases) {
131+
it(`${name} rejects ../bad`, () => {
132+
expect(() => fn("../bad")).toThrow(/Invalid/);
133+
});
134+
}
135+
136+
it("rejects colon in segment", () => {
137+
expect(() => urls.account("foo:bar")).toThrow(/Invalid/);
138+
});
139+
140+
it("rejects @ in segment", () => {
141+
expect(() => urls.account("foo@bar")).toThrow(/Invalid/);
142+
});
143+
});
103144
});

typescript/__tests__/server/tools.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ describe("Tool handlers return MCP content format", () => {
147147

148148
const accountsData = await callTool(tools, "robinhood_get_accounts");
149149
expect(accountsData.accounts).toEqual([
150-
{ url: "https://api.robinhood.com/accounts/ABC123/", account_number: "ABC123", type: "cash" },
150+
{ url: "https://api.robinhood.com/accounts/ABC123/", account_number: "[REDACTED]", type: "cash" },
151151
]);
152152

153153
const accountData = await callTool(tools, "robinhood_get_account", { info_type: "user" });

typescript/src/client/auth.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,14 @@ export interface LoginResult {
1818
method: "keychain" | "encrypted_file" | "token";
1919
}
2020

21+
const MIN_REFRESH_INTERVAL_MS = 5_000;
22+
2123
/** State held per-client for token management. */
2224
export interface AuthState {
2325
tokens: TokenData;
2426
store: TokenStore;
2527
refreshing: Promise<string | null> | null;
28+
lastRefreshAt: number;
2629
}
2730

2831
/**
@@ -93,6 +96,11 @@ function createRefreshCallback(state: AuthState): () => Promise<string | null> {
9396
if (state.refreshing) {
9497
return state.refreshing;
9598
}
99+
// Rate limit: refuse to refresh if the last attempt was too recent
100+
if (Date.now() - state.lastRefreshAt < MIN_REFRESH_INTERVAL_MS) {
101+
return null;
102+
}
103+
state.lastRefreshAt = Date.now();
96104
state.refreshing = refreshTokens(state).finally(() => {
97105
state.refreshing = null;
98106
});
@@ -123,6 +131,7 @@ export async function restoreSession(
123131
tokens,
124132
store,
125133
refreshing: null,
134+
lastRefreshAt: 0,
126135
};
127136

128137
// Register 401 callback for automatic token refresh

typescript/src/client/client.ts

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -535,6 +535,23 @@ export class RobinhoodClient {
535535
this.requireAuth();
536536
const sym = symbol.trim().toUpperCase();
537537

538+
// Validate numeric bounds
539+
if (quantity <= 0 || !Number.isFinite(quantity)) {
540+
throw new Error("quantity must be a positive finite number");
541+
}
542+
if (opts?.limitPrice != null && (opts.limitPrice <= 0 || !Number.isFinite(opts.limitPrice))) {
543+
throw new Error("limitPrice must be a positive finite number");
544+
}
545+
if (opts?.stopPrice != null && (opts.stopPrice <= 0 || !Number.isFinite(opts.stopPrice))) {
546+
throw new Error("stopPrice must be a positive finite number");
547+
}
548+
if (
549+
opts?.trailAmount != null &&
550+
(opts.trailAmount <= 0 || !Number.isFinite(opts.trailAmount))
551+
) {
552+
throw new Error("trailAmount must be a positive finite number");
553+
}
554+
538555
// Validate mutually exclusive order params
539556
if (opts?.trailAmount != null && (opts?.limitPrice != null || opts?.stopPrice != null)) {
540557
throw new Error("Cannot combine trailAmount with limitPrice or stopPrice");
@@ -586,19 +603,22 @@ export class RobinhoodClient {
586603
quantity: String(quantity),
587604
type: orderType,
588605
trigger,
589-
time_in_force: isFractional ? "gfd" : (opts?.timeInForce ?? "gtc"),
606+
time_in_force: isFractional ? "gfd" : (opts?.timeInForce ?? "gfd"),
590607
extended_hours: opts?.extendedHours ?? false,
591608
ref_id: crypto.randomUUID(),
592609
};
593610

594611
if (opts?.limitPrice != null) payload.price = String(opts.limitPrice);
595612
if (opts?.stopPrice != null) payload.stop_price = String(opts.stopPrice);
596613
if (opts?.trailAmount != null) {
597-
payload.trailing_peg = {
598-
type: opts.trailType ?? "percentage",
599-
percentage: opts.trailType === "amount" ? undefined : String(opts.trailAmount),
600-
price: opts.trailType === "amount" ? { amount: String(opts.trailAmount) } : undefined,
601-
};
614+
const pegType = opts.trailType ?? "percentage";
615+
const peg: Record<string, unknown> = { type: pegType };
616+
if (pegType === "amount") {
617+
peg.price = { amount: String(opts.trailAmount) };
618+
} else {
619+
peg.percentage = String(opts.trailAmount);
620+
}
621+
payload.trailing_peg = peg;
602622
}
603623

604624
// Market buys get a 5% price collar
@@ -664,6 +684,15 @@ export class RobinhoodClient {
664684
if (legs.length === 0) {
665685
throw new Error("At least one leg is required");
666686
}
687+
if (price <= 0 || !Number.isFinite(price)) {
688+
throw new Error("price must be a positive finite number");
689+
}
690+
if (quantity <= 0 || !Number.isFinite(quantity)) {
691+
throw new Error("quantity must be a positive finite number");
692+
}
693+
if (opts?.stopPrice != null && (opts.stopPrice <= 0 || !Number.isFinite(opts.stopPrice))) {
694+
throw new Error("stopPrice must be a positive finite number");
695+
}
667696

668697
// Resolve each leg's option instrument
669698
const resolvedLegs = [];
@@ -754,6 +783,15 @@ export class RobinhoodClient {
754783
},
755784
): Promise<CryptoOrder> {
756785
this.requireAuth();
786+
787+
// Validate numeric bounds
788+
if (amountOrQuantity <= 0 || !Number.isFinite(amountOrQuantity)) {
789+
throw new Error("amountOrQuantity must be a positive finite number");
790+
}
791+
if (opts?.limitPrice != null && (opts.limitPrice <= 0 || !Number.isFinite(opts.limitPrice))) {
792+
throw new Error("limitPrice must be a positive finite number");
793+
}
794+
757795
const s = symbol.trim().toUpperCase();
758796

759797
// Look up the currency pair

typescript/src/client/token-store.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,11 @@ async function resolveEncryptionKey(): Promise<Buffer> {
102102
// 1. Env var
103103
const envKey = process.env.ROBINHOOD_TOKEN_KEY?.trim();
104104
if (envKey) {
105-
return Buffer.from(envKey, "base64");
105+
const key = Buffer.from(envKey, "base64");
106+
if (key.length !== KEY_BYTES) {
107+
throw new Error(`ROBINHOOD_TOKEN_KEY must decode to ${KEY_BYTES} bytes (got ${key.length})`);
108+
}
109+
return key;
106110
}
107111

108112
// 2. Keychain

0 commit comments

Comments
 (0)