feat: Badge identity delivery — kya_web_fetch + kya_getHeaders (KYA-55) - #27
Conversation
SSRF protection was defined locally in ucp-manifest.ts. Extract to src/lib/url-safety.ts so kya_web_fetch and future outbound fetch tools can reuse it. Zero functional change — import replaces inline. Adds 16 tests covering RFC1918, localhost, IPv6 private, link-local, AWS metadata (169.254.x), and malformed URL cases. Part of: KYA-55 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Returns { headers: { "Kya-Token": token } } for agents using
Playwright (setExtraHTTPHeaders) or Chrome extensions (document.cookie).
Returns NO_IDENTITY error when no identity established.
Part of: KYA-55 (Path 2/2.5 identity delivery)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Primary identity delivery path. Wraps fetch with: - Kya-Token header auto-injection - Auto-declare (browse_declared event, fire-and-forget) - SSRF protection via isPublicOrigin() - HTTPS-only, 5MB body cap, 30s timeout - Manual redirects (prevents token leak to redirect targets) - Method allowlist: GET, HEAD, OPTIONS - Response header filtering (strips set-cookie) 29 tests covering identity, URL validation, SSRF, methods, fetch behavior, truncation, timeouts, auto-declare, and header override prevention. Part of: KYA-55 (Path 1 identity delivery) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Register kya_web_fetch and kya_getHeaders in index.ts. Deprecate kya_reportBadgeOutcome and kya_reportBadgeNotPresented to no-ops — outcomes tracked server-side via verify endpoint, not-presented event no longer scored. Both log once-per-session stderr warning. Remove unused imports. Update kya_reportBadgePresented description to stop referencing the now-deprecated kya_reportBadgeOutcome. Part of: KYA-55 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Version 2.4.0 → 2.5.0 (minor: new tools, deprecations, no breaking changes). CHANGELOG documents kya_web_fetch, kya_getHeaders, deprecations. .gitignore adds .env patterns. Part of: KYA-55 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds web-fetch and header helper tools, deprecates two outcome-reporting tools to no-ops, extracts SSRF URL-safety logic into a shared module, implements per-merchant badge-token enrollment/caching with enroll-on-identity side-effect, bumps package version and changelog, and adds comprehensive Vitest tests for new modules. Changes
Sequence DiagramsequenceDiagram
actor Agent as Agent/Client
participant MCP as MCP Server
participant Storage as Storage
participant URLValidator as URL Safety
participant Fetch as HTTP Fetch
participant API as Remote API
Agent->>MCP: call kya_web_fetch(url, method?, headers?)
MCP->>Storage: getCachedBadgeToken / getStoredConsentKey
Storage-->>MCP: token or null
alt no token
MCP-->>Agent: { error: "NO_IDENTITY", code: "NO_IDENTITY" }
else token available
MCP->>URLValidator: isPublicOrigin(url)
URLValidator-->>MCP: true/false
alt blocked
MCP-->>Agent: { error: "BLOCKED_URL", code: "BLOCKED_URL" }
else allowed
MCP->>Fetch: fetch(url, { headers with Kya-Token, redirect: "manual", timeout: 30s })
Fetch-->>MCP: Response (status, headers, body, location?)
MCP->>MCP: parse/truncate body (5MB), filter headers
MCP-->>Agent: WebFetchSuccess { status, headers, body, truncated, url }
par fire-and-forget telemetry
MCP->>Storage: getOrCreateInstallId()
MCP->>API: POST /api/badge/report (browse_declared) (5s timeout)
API-->>MCP: logged (errors to stderr only)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/tools/webFetch.ts (1)
22-24: Centralize the release version.
BADGE_VERSIONnow duplicatesversion: "2.5.0"insrc/index.ts:18-21. This will eventually drift and send stalebadge_versiontelemetry unless it comes from one shared source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/webFetch.ts` around lines 22 - 24, Replace the duplicated BADGE_VERSION constant with a single shared source: export the app version from the existing version symbol in src/index.ts (e.g., export const version = "...") or create a central constant (e.g., APP_VERSION) and import it into src/tools/webFetch.ts; then use that imported symbol in place of BADGE_VERSION so badge_version telemetry always comes from the single authoritative version constant (update the BADGE_VERSION reference in webFetch.ts to use the imported version and remove the hardcoded "2.5.0").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/url-safety.ts`:
- Around line 13-22: The check in isPublicOrigin uses origin.startsWith instead
of the parsed URL protocol, so malformed schemes like "ftp:" or "file:" slip
through when VITEST is set; update isPublicOrigin to parse the URL once (using
new URL(origin)), read url.protocol (e.g., "https:" / "http:"), and replace the
startsWith check with a protocol comparison that enforces "https:" normally and
only relaxes to allow "http:" when process.env.VITEST is set; keep the existing
try/catch that returns false for invalid origins and use the parsed hostname
variable as before.
- Around line 24-42: The SSRF guard misses full IPv4 loopback and IPv6 fe80::/10
ranges; update the IPv4 check in src/lib/url-safety.ts to reject all 127.0.0.0/8
by adding a condition that returns false when the first octet (variable a
derived from ipv4Match) equals 127, and broaden the IPv6 bracketed check
(variable inner) to match the entire fe80::/10 range by rejecting addresses
whose hex prefix starts with "fe8", "fe9", "fea" or "feb" (use
inner.toLowerCase()). Keep existing checks for fc*/fd* and ::1, and add
regression tests asserting that "127.0.0.2" (or any 127.*) and "[fe90::1]" are
treated as unsafe.
In `@src/tools/getHeaders.test.ts`:
- Around line 14-18: The stderr spy created as stderrSpy =
vi.spyOn(process.stderr, "write") is not restored; vi.clearAllMocks() only
clears call history, so add an afterAll hook that calls stderrSpy.mockRestore()
(i.e., afterAll(() => stderrSpy.mockRestore())) to restore process.stderr.write
and avoid leaking the mocked implementation into other tests; keep the existing
afterEach clearing but add this afterAll cleanup referencing stderrSpy and
mockRestore.
In `@src/tools/webFetch.test.ts`:
- Around line 230-237: The test must cover case-insensitive header names: add
one or two tests that call webFetch (same as existing test using mockFetch and
mockResponse) passing agent headers with different casing like "kya-token" and
"KYA-TOKEN" and assert the server token still wins; locate the existing test for
webFetch and either extend it or add new it() blocks that find the fetch call
(using mockFetch.mock.calls and the URL check as in the current test) and verify
by normalizing header names (e.g., lowercasing keys from fetchCall[1].headers)
that the value for "kya-token" equals the expected server token
("pk_test_abc123"), ensuring all case permutations cannot override the server
token.
- Around line 35-47: The mockResponse helper incorrectly sets ok for 3xx codes;
update the mockResponse function so its ok property matches Fetch spec (true
only for 200–299) by computing ok as status >= 200 && status < 300 (or status <=
299), keeping the rest of the mock behavior (headers, text, url) unchanged so
tests reflect real Response.ok semantics.
In `@src/tools/webFetch.ts`:
- Around line 98-102: The request header construction in src/tools/webFetch.ts
currently does `...(headers ?? {})` then sets "Kya-Token": token, which allows
callers to inject case-variant headers (e.g. "kya-token" or "KYA-TOKEN") that
will be normalized and concatenated by Fetch; fix by filtering incoming headers
case-insensitively before spreading: remove any header whose lowercased name
equals "kya-token" (use the same approach as the response header filtering used
around lines 133-141) and then set the canonical "Kya-Token": token so our token
always wins; update the logic that builds requestHeaders (the block creating
requestHeaders) accordingly and add/adjust tests to cover
lower/upper/case-variant header names.
- Around line 120-131: The current block uses response.text() and string.length
which buffers the entire body and counts UTF-16 code units instead of bytes;
replace that logic in the function that reads the response (the body/truncated
handling around response.text(), MAX_BODY_BYTES, and truncated) with a streaming
reader: use response.body?.getReader() to read Uint8Array chunks, sum
value.byteLength to enforce MAX_BODY_BYTES, push bytes into a buffer (or decode
incrementally with a TextDecoder) up to the byte limit, set truncated = true
when the limit would be exceeded and stop reading further, and then produce the
final body string from the collected bytes; ensure the fallback catch logic
remains if response.body is absent or reading fails.
- Around line 105-117: The catch block that classifies timeouts only checks for
AbortError so undici's TimeoutError (thrown during headers/body reads) is
misclassified; update the error handling around the fetch (the
variables/constructs: response, err, AbortSignal.timeout, FETCH_TIMEOUT_MS) to
treat both AbortError and TimeoutError as timeouts by checking err.name ===
"AbortError" || err.name === "TimeoutError" (or err.constructor?.name ===
"TimeoutError"), and return the same { error: "Request timed out", code:
"TIMEOUT" } for either case; optionally, if undici's TimeoutError class can be
imported in your environment, use instanceof TimeoutError for a stronger check.
---
Nitpick comments:
In `@src/tools/webFetch.ts`:
- Around line 22-24: Replace the duplicated BADGE_VERSION constant with a single
shared source: export the app version from the existing version symbol in
src/index.ts (e.g., export const version = "...") or create a central constant
(e.g., APP_VERSION) and import it into src/tools/webFetch.ts; then use that
imported symbol in place of BADGE_VERSION so badge_version telemetry always
comes from the single authoritative version constant (update the BADGE_VERSION
reference in webFetch.ts to use the imported version and remove the hardcoded
"2.5.0").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 96e49c5f-e7e3-4f75-abdc-fd7d8f23d387
📒 Files selected for processing (11)
.gitignoreCHANGELOG.mdpackage.jsonsrc/index.tssrc/lib/ucp-manifest.tssrc/lib/url-safety.test.tssrc/lib/url-safety.tssrc/tools/getHeaders.test.tssrc/tools/getHeaders.tssrc/tools/webFetch.test.tssrc/tools/webFetch.ts
| export function isPublicOrigin(origin: string): boolean { | ||
| let hostname: string; | ||
| try { | ||
| hostname = new URL(origin).hostname; | ||
| } catch { | ||
| return false; | ||
| } | ||
|
|
||
| // Block non-https (except in tests) | ||
| if (!origin.startsWith("https://") && !process.env.VITEST) return false; |
There was a problem hiding this comment.
Use the parsed protocol here, not a raw string prefix.
With process.env.VITEST set, inputs like ftp://example.com and file:///tmp/x pass this gate because the condition only rejects non-HTTPS when the env var is absent. If the intended exception is “allow http: in tests”, compare url.protocol and only relax that case.
Suggested fix
export function isPublicOrigin(origin: string): boolean {
- let hostname: string;
+ let url: URL;
try {
- hostname = new URL(origin).hostname;
+ url = new URL(origin);
} catch {
return false;
}
+ const { hostname, protocol } = url;
// Block non-https (except in tests)
- if (!origin.startsWith("https://") && !process.env.VITEST) return false;
+ if (protocol !== "https:" && !(process.env.VITEST && protocol === "http:")) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/url-safety.ts` around lines 13 - 22, The check in isPublicOrigin uses
origin.startsWith instead of the parsed URL protocol, so malformed schemes like
"ftp:" or "file:" slip through when VITEST is set; update isPublicOrigin to
parse the URL once (using new URL(origin)), read url.protocol (e.g., "https:" /
"http:"), and replace the startsWith check with a protocol comparison that
enforces "https:" normally and only relaxes to allow "http:" when
process.env.VITEST is set; keep the existing try/catch that returns false for
invalid origins and use the parsed hostname variable as before.
| // Block localhost and loopback | ||
| if (hostname === "localhost" || hostname === "127.0.0.1" || hostname === "::1") return false; | ||
| if (hostname.endsWith(".localhost")) return false; | ||
|
|
||
| // Block private/reserved IPv4 ranges | ||
| const ipv4Match = hostname.match(/^(\d+)\.(\d+)\.(\d+)\.(\d+)$/); | ||
| if (ipv4Match) { | ||
| const [, a, b] = ipv4Match.map(Number); | ||
| if (a === 10) return false; // 10.0.0.0/8 | ||
| if (a === 172 && b >= 16 && b <= 31) return false; // 172.16.0.0/12 | ||
| if (a === 192 && b === 168) return false; // 192.168.0.0/16 | ||
| if (a === 169 && b === 254) return false; // 169.254.0.0/16 (link-local + metadata) | ||
| if (a === 0) return false; // 0.0.0.0/8 | ||
| } | ||
|
|
||
| // Block IPv6 loopback/link-local | ||
| if (hostname.startsWith("[")) { | ||
| const inner = hostname.slice(1, -1).toLowerCase(); | ||
| if (inner === "::1" || inner.startsWith("fe80:") || inner.startsWith("fc") || inner.startsWith("fd")) return false; |
There was a problem hiding this comment.
The shared SSRF guard still allows part of loopback/link-local space.
127.0.0.2/127.255.255.255 and IPv6 link-local addresses like [fe90::1] still return true here. Since both src/lib/ucp-manifest.ts and kya_web_fetch now rely on this helper, that leaves a shared SSRF bypass. Expand the IPv4 check to all of 127.0.0.0/8 and the IPv6 check to the full fe80::/10 range, then add a regression case for each.
Suggested fix
if (ipv4Match) {
const [, a, b] = ipv4Match.map(Number);
+ if (a === 127) return false; // 127.0.0.0/8
if (a === 10) return false; // 10.0.0.0/8
if (a === 172 && b >= 16 && b <= 31) return false; // 172.16.0.0/12
if (a === 192 && b === 168) return false; // 192.168.0.0/16
if (a === 169 && b === 254) return false; // 169.254.0.0/16 (link-local + metadata)
if (a === 0) return false; // 0.0.0.0/8
}
// Block IPv6 loopback/link-local
if (hostname.startsWith("[")) {
const inner = hostname.slice(1, -1).toLowerCase();
- if (inner === "::1" || inner.startsWith("fe80:") || inner.startsWith("fc") || inner.startsWith("fd")) return false;
+ const firstHextet = Number.parseInt(inner.split(":")[0] ?? "", 16);
+ if (inner === "::1") return false;
+ if (firstHextet >= 0xfe80 && firstHextet <= 0xfebf) return false; // fe80::/10
+ if (inner.startsWith("fc") || inner.startsWith("fd")) return false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/url-safety.ts` around lines 24 - 42, The SSRF guard misses full IPv4
loopback and IPv6 fe80::/10 ranges; update the IPv4 check in
src/lib/url-safety.ts to reject all 127.0.0.0/8 by adding a condition that
returns false when the first octet (variable a derived from ipv4Match) equals
127, and broaden the IPv6 bracketed check (variable inner) to match the entire
fe80::/10 range by rejecting addresses whose hex prefix starts with "fe8",
"fe9", "fea" or "feb" (use inner.toLowerCase()). Keep existing checks for
fc*/fd* and ::1, and add regression tests asserting that "127.0.0.2" (or any
127.*) and "[fe90::1]" are treated as unsafe.
| const stderrSpy = vi.spyOn(process.stderr, "write").mockImplementation(() => true); | ||
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the test file
find . -name "getHeaders.test.ts" -type fRepository: kyalabs-Io/badge-server
Length of output: 97
🏁 Script executed:
#!/bin/bash
# List the tools directory to understand structure
ls -la src/tools/ 2>/dev/null || echo "Tools dir not found"Repository: kyalabs-Io/badge-server
Length of output: 587
🏁 Script executed (no clone):
#!/bin/bash
# Research Vitest's vi.clearAllMocks() behavior
cat > /tmp/vitest_research.txt << 'EOF'
Vitest documentation on vi.clearAllMocks():
- clearAllMocks(): Clears all mock call history and partially restores spy functionality
- Does NOT restore the original implementation when used with mockImplementation()
- mockRestore(): Restores the spy to its original pre-spy state
Key difference:
- clearAllMocks() removes call history and resets return values but KEEPS the spy active
- mockRestore() removes the spy completely and restores the original function
For spies created with vi.spyOn().mockImplementation(), the original function is replaced.
After clearAllMocks(), the spy remains in place with the mock implementation.
EOF
cat /tmp/vitest_research.txt
Length of output: 664
🏁 Script executed:
#!/bin/bash
# Read the test file
cat -n src/tools/getHeaders.test.tsRepository: kyalabs-Io/badge-server
Length of output: 1721
🏁 Script executed:
#!/bin/bash
# Check if there are other test files that might be affected by the unrestored stderr spy
find . -name "*.test.ts" -type f | head -20Repository: kyalabs-Io/badge-server
Length of output: 392
🏁 Script executed:
#!/bin/bash
# Check if any other test files use process.stderr
grep -r "process.stderr" . --include="*.test.ts" --include="*.ts" | head -20Repository: kyalabs-Io/badge-server
Length of output: 1571
🏁 Script executed (no clone):
#!/bin/bash
# Verify Vitest's actual behavior with afterEach and spies
cat > /tmp/vitest_cleanup.txt << 'EOF'
Vitest spy cleanup behavior:
IMPORTANT: When a spy is created at describe-block scope (not in beforeEach):
- The spy is created ONCE when the describe block is parsed
- afterEach runs after EACH test, but the spy remains active
- When the describe block ENDS, the variable goes out of scope, but the spy on the actual method does NOT automatically restore
CRITICAL ISSUE: process.stderr.write is a GLOBAL object
- The spy modifies the actual process.stderr.write method
- This modification persists across tests and potentially across test files
- Without mockRestore(), subsequent tests may receive the mocked behavior instead of the real one
BEST PRACTICE FIX:
Option 1: afterAll(() => stderrSpy.mockRestore()) - Restore after all tests in describe
Option 2: Move spy creation to beforeEach and restore in afterEach
Option 3: Use vi.stubGlobal() which auto-restores on test cleanup
This is a genuine test isolation concern.
EOF
cat /tmp/vitest_cleanup.txt
Length of output: 960
Restore the stderr spy after all tests complete.
vi.clearAllMocks() clears call history but does not restore this spy. Since process.stderr is a global object, the mocked implementation persists and can affect stderr calls in other tests. Add afterAll(() => stderrSpy.mockRestore()) to properly clean up the spy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/getHeaders.test.ts` around lines 14 - 18, The stderr spy created as
stderrSpy = vi.spyOn(process.stderr, "write") is not restored;
vi.clearAllMocks() only clears call history, so add an afterAll hook that calls
stderrSpy.mockRestore() (i.e., afterAll(() => stderrSpy.mockRestore())) to
restore process.stderr.write and avoid leaking the mocked implementation into
other tests; keep the existing afterEach clearing but add this afterAll cleanup
referencing stderrSpy and mockRestore.
| function mockResponse(body: string, init?: { status?: number; headers?: Record<string, string> }) { | ||
| const status = init?.status ?? 200; | ||
| const headers = new Map(Object.entries(init?.headers ?? { "content-type": "text/html" })); | ||
| return { | ||
| ok: status >= 200 && status < 400, | ||
| status, | ||
| headers: { | ||
| entries: () => headers.entries(), | ||
| get: (k: string) => headers.get(k.toLowerCase()) ?? null, | ||
| }, | ||
| text: () => Promise.resolve(body), | ||
| url: "https://example.com", | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
git ls-files | head -20Repository: kyalabs-Io/badge-server
Length of output: 406
🏁 Script executed:
#!/bin/bash
fd "webFetch" --type fRepository: kyalabs-Io/badge-server
Length of output: 115
🏁 Script executed:
#!/bin/bash
cat -n src/tools/webFetch.test.ts | head -100Repository: kyalabs-Io/badge-server
Length of output: 3908
🏁 Script executed:
#!/bin/bash
cat -n src/tools/webFetch.test.ts | sed -n '100,200p'Repository: kyalabs-Io/badge-server
Length of output: 4590
🏁 Script executed:
#!/bin/bash
cat -n src/tools/webFetch.tsRepository: kyalabs-Io/badge-server
Length of output: 6857
🌐 Web query:
Fetch API Response.ok specification MDN
💡 Result:
The Response.ok read-only property of the Fetch API's Response interface is a boolean indicating whether the HTTP response is successful, i.e., if the status code is in the range 200-299. It returns true for status codes 200 through 299, and false otherwise (including status 0 for opaque responses). Example from MDN: fetch(myRequest).then((response) => { console.log(response.ok); // true if successful }); This is defined in the Fetch Standard, referenced in MDN documentation.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Response/ok
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Response
- 3: https://fetch.spec.whatwg.org/
The mock incorrectly marks 3xx responses as ok: true.
Per the Fetch API spec, Response.ok is true only for status codes 200–299. The current mock marks all 2xx and 3xx as ok, which diverges from real Fetch behavior. This can mask future regressions if code depends on response.ok to validate redirect responses.
Fix
return {
- ok: status >= 200 && status < 400,
+ ok: status >= 200 && status < 300,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/webFetch.test.ts` around lines 35 - 47, The mockResponse helper
incorrectly sets ok for 3xx codes; update the mockResponse function so its ok
property matches Fetch spec (true only for 200–299) by computing ok as status >=
200 && status < 300 (or status <= 299), keeping the rest of the mock behavior
(headers, text, url) unchanged so tests reflect real Response.ok semantics.
| it("does not allow agent to override Kya-Token via headers param", async () => { | ||
| mockFetch.mockResolvedValue(mockResponse("ok")); | ||
| await webFetch("https://example.com", "GET", { "Kya-Token": "evil" }); | ||
| const fetchCall = mockFetch.mock.calls.find( | ||
| (c: any[]) => c[0] === "https://example.com" | ||
| ); | ||
| // Our token wins, not the agent's | ||
| expect(fetchCall![1].headers["Kya-Token"]).toBe("pk_test_abc123"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the webFetch.test.ts file and check its existence
find . -name "webFetch.test.ts" -type fRepository: kyalabs-Io/badge-server
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Get the relevant lines from the test file
if [ -f "src/tools/webFetch.test.ts" ]; then
echo "=== Lines 230-237 ==="
sed -n '230,237p' src/tools/webFetch.test.ts
echo ""
echo "=== Context around line 230 (225-250) ==="
sed -n '225,250p' src/tools/webFetch.test.ts
fiRepository: kyalabs-Io/badge-server
Length of output: 1602
🏁 Script executed:
#!/bin/bash
# Check the webFetch implementation
if [ -f "src/tools/webFetch.ts" ]; then
echo "=== webFetch.ts implementation ==="
cat -n src/tools/webFetch.ts
fiRepository: kyalabs-Io/badge-server
Length of output: 6892
🏁 Script executed:
#!/bin/bash
# Check if there are any other tests for case-insensitive header handling
grep -n -i "kya-token\|kya.token" src/tools/webFetch.test.ts | head -20Repository: kyalabs-Io/badge-server
Length of output: 577
🏁 Script executed:
#!/bin/bash
# Search for any case normalization in the webFetch implementation
grep -n -i "lowercase\|toLocaleCase\|normalize" src/tools/webFetch.tsRepository: kyalabs-Io/badge-server
Length of output: 171
Add case-insensitive override test for Kya-Token.
HTTP header names are case-insensitive (RFC 7230), but the current test only checks the exact "Kya-Token" spelling. The implementation merges agent headers and server headers using plain object spread, which is case-sensitive. An agent passing "kya-token", "KYA-TOKEN", or other case variants would create separate object keys, potentially allowing the agent's value to coexist with or override the server's token depending on merge order and fetch implementation behavior. Add tests for lowercase and mixed-case variants to ensure all case permutations are protected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/webFetch.test.ts` around lines 230 - 237, The test must cover
case-insensitive header names: add one or two tests that call webFetch (same as
existing test using mockFetch and mockResponse) passing agent headers with
different casing like "kya-token" and "KYA-TOKEN" and assert the server token
still wins; locate the existing test for webFetch and either extend it or add
new it() blocks that find the fetch call (using mockFetch.mock.calls and the URL
check as in the current test) and verify by normalizing header names (e.g.,
lowercasing keys from fetchCall[1].headers) that the value for "kya-token"
equals the expected server token ("pk_test_abc123"), ensuring all case
permutations cannot override the server token.
| let response: Response; | ||
| try { | ||
| response = await fetch(url, { | ||
| method: resolvedMethod, | ||
| headers: requestHeaders, | ||
| redirect: "manual", | ||
| signal: AbortSignal.timeout(FETCH_TIMEOUT_MS), | ||
| }); | ||
| } catch (err) { | ||
| if (err instanceof Error && err.name === "AbortError") { | ||
| return { error: "Request timed out", code: "TIMEOUT" }; | ||
| } | ||
| return { error: "Failed to fetch URL", code: "FETCH_ERROR" }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if webFetch.ts exists and examine its content
fd -t f "webFetch.ts" || echo "File not found with fd"Repository: kyalabs-Io/badge-server
Length of output: 88
🏁 Script executed:
#!/bin/bash
# List TypeScript files in src/tools to understand structure
ls -la src/tools/ 2>/dev/null || echo "src/tools not found"Repository: kyalabs-Io/badge-server
Length of output: 587
🏁 Script executed:
#!/bin/bash
# Search for fetch-related files more broadly
find . -name "*fetch*" -o -name "*web*" 2>/dev/null | head -20Repository: kyalabs-Io/badge-server
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Read webFetch.ts with line numbers to examine lines 105-117
cat -n src/tools/webFetch.ts | sed -n '95,125p'Repository: kyalabs-Io/badge-server
Length of output: 1088
🏁 Script executed:
#!/bin/bash
# Get total line count to understand file size
wc -l src/tools/webFetch.tsRepository: kyalabs-Io/badge-server
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Check package.json for Node version and dependencies
cat package.json | head -50Repository: kyalabs-Io/badge-server
Length of output: 1201
🏁 Script executed:
#!/bin/bash
# Search for any timeout-related handling or tests
rg -i "timeout|aborterror|timeouterror" --type tsRepository: kyalabs-Io/badge-server
Length of output: 4606
🏁 Script executed:
#!/bin/bash
# Read the webFetch.test.ts file to see timeout tests
cat -n src/tools/webFetch.test.tsRepository: kyalabs-Io/badge-server
Length of output: 12338
🏁 Script executed (no clone):
#!/bin/bash
# Research AbortSignal.timeout() behavior in Node.js
node --version 2>/dev/null || echo "Node not available locally"
Length of output: 38
🌐 Web query:
AbortSignal.timeout() Node.js 20 TimeoutError AbortError behavior
💡 Result:
In Node.js 20, AbortSignal.timeout(delay) creates an AbortSignal that automatically aborts after the specified milliseconds by internally using setTimeout to call abort on the signal with a TimeoutError DOMException as the reason (signal.reason). This feature was added in Node.js v16.14.0 via commit 62c4b4c and is stable in v20 (LTS). When passed to abortable APIs: - Timers.promises (setTimeout, setImmediate, etc.): Rejects with AbortError if aborted, ignoring the reason. Check err.name === 'AbortError'. TimeoutError is not directly thrown by the timer; it's only the abort reason. - Fetch (powered by undici): Rejects with AbortError in most cases (spec-compliant behavior for fetch), even when triggered by AbortSignal.timeout. Some undici issues (#2171, #3296) note inconsistent TimeoutError vs AbortError depending on timing (headers vs body), but generally expects AbortError. Recent blog post (2025) shows TimeoutError example, possibly fixed or specific conditions. To distinguish timeout from manual abort, check if (err.name === 'AbortError' && err.cause?.name === 'TimeoutError'). Official Node.js docs do not explicitly detail error types for AbortSignal.timeout consumers, aligning with Web API spec where abort reasons may not propagate to AbortError in fetch/timers. Example: try { await fetch(url, { signal: AbortSignal.timeout(5000) }); } catch (err) { if (err.name === 'AbortError' && err.cause?.name === 'TimeoutError') { console.log('Request timed out'); } } For timers: const { setTimeout } = require('timers/promises'); try { await setTimeout(5000, null, { signal: AbortSignal.timeout(3000) }); } catch (err) { if (err.name === 'AbortError') console.log('Aborted (check signal.reason for TimeoutError)'); } Node.js 20 behavior matches spec: signal aborts with TimeoutError reason, but consuming APIs throw AbortError.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout_static
- 2: lib: add AbortSignal.timeout nodejs/node#40899
- 3: https://nodejs.org/docs/latest-v20.x/api/globals.html
- 4: https://blog.appsignal.com/2025/02/12/managing-asynchronous-operations-in-nodejs-with-abortcontroller.html
- 5: nodejs/node@62c4b4c85b
- 6: (regression?) timeouts while reading a body no longer abort with a TimeoutError nodejs/undici#3296
- 7: AbortSignal.timeout inconsistently leads to TimeoutError or AbortError nodejs/undici#2171
- 8: https://nodejs.org/en/blog/release/v20.0.0
🌐 Web query:
Node.js undici fetch AbortSignal.timeout() error types 2024 2025
💡 Result:
In Node.js undici fetch with AbortSignal.timeout, the primary error type thrown on timeout is DOMException [AbortError]: "The operation was aborted." This occurs because AbortSignal.timeout aborts the signal with a TimeoutError DOMException as its reason, but fetch rejects with AbortError per the Fetch spec when the signal aborts during the request. A DOMException [TimeoutError] can occur in specific timing scenarios (e.g., connect timeout or internal undici timeouts during headers/body reading), but this is inconsistent and considered spec-compliant behavior where AbortError is expected for signal aborts. No changes specific to 2024-2025 alter this fundamental behavior; recent issues discuss related bugs but confirm AbortError as the standard outcome for AbortSignal.timeout in fetch. Undici's own errors (e.g., ConnectTimeoutError) are separate from fetch signal aborts and use custom UndiciError subclasses.
Citations:
- 1: AbortSignal.timeout inconsistently leads to TimeoutError or AbortError nodejs/undici#2171
- 2: (regression?) timeouts while reading a body no longer abort with a TimeoutError nodejs/undici#3296
- 3: https://stackoverflow.com/questions/75969669/abortsignal-timeout-in-fetch-request-always-responds-with-aborterror-but-not-t
- 4: https://undici-docs.vramana.dev/docs/api/Errors
- 5: https://github.com/nodejs/undici/blob/3372aed902951f135d710d0fb430f39b8571e547/README.md
- 6: https://undici-docs.vramana.dev/docs/
🏁 Script executed:
#!/bin/bash
# Search for other AbortSignal.timeout usage
rg "AbortSignal\.timeout" --type ts -B 3 -A 5Repository: kyalabs-Io/badge-server
Length of output: 1023
🏁 Script executed:
#!/bin/bash
# Check if TimeoutError is handled anywhere in codebase
rg "TimeoutError" --type tsRepository: kyalabs-Io/badge-server
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check error handling in ucp-manifest.ts for AbortSignal.timeout
cat -n src/lib/ucp-manifest.ts | sed -n '1,50p'Repository: kyalabs-Io/badge-server
Length of output: 1866
🏁 Script executed:
#!/bin/bash
# Search for the try-catch around ucp-manifest fetch
rg -B 5 -A 10 "ucp-manifest" src/lib/ucp-manifest.ts | head -50Repository: kyalabs-Io/badge-server
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Get full context of ucp-manifest fetch call
grep -n "AbortSignal.timeout" src/lib/ucp-manifest.ts -B 10 -A 10Repository: kyalabs-Io/badge-server
Length of output: 789
TimeoutError from AbortSignal.timeout() will be misclassified as FETCH_ERROR.
AbortSignal.timeout() is designed to abort with AbortError per the Fetch spec; however, undici (Node.js's fetch implementation) can throw TimeoutError in timing-dependent scenarios during headers or body reading (nodejs/undici#2171, #3296). The current code only checks for AbortError, so actual timeouts surfacing as TimeoutError will fall through and return FETCH_ERROR instead of TIMEOUT.
Suggested fix
- let response: Response;
+ let response: Response;
+ const signal = AbortSignal.timeout(FETCH_TIMEOUT_MS);
try {
response = await fetch(url, {
method: resolvedMethod,
headers: requestHeaders,
redirect: "manual",
- signal: AbortSignal.timeout(FETCH_TIMEOUT_MS),
+ signal,
});
} catch (err) {
- if (err instanceof Error && err.name === "AbortError") {
+ if (
+ signal.aborted ||
+ (err instanceof Error &&
+ (err.name === "AbortError" || err.name === "TimeoutError"))
+ ) {
return { error: "Request timed out", code: "TIMEOUT" };
}
return { error: "Failed to fetch URL", code: "FETCH_ERROR" };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let response: Response; | |
| try { | |
| response = await fetch(url, { | |
| method: resolvedMethod, | |
| headers: requestHeaders, | |
| redirect: "manual", | |
| signal: AbortSignal.timeout(FETCH_TIMEOUT_MS), | |
| }); | |
| } catch (err) { | |
| if (err instanceof Error && err.name === "AbortError") { | |
| return { error: "Request timed out", code: "TIMEOUT" }; | |
| } | |
| return { error: "Failed to fetch URL", code: "FETCH_ERROR" }; | |
| let response: Response; | |
| const signal = AbortSignal.timeout(FETCH_TIMEOUT_MS); | |
| try { | |
| response = await fetch(url, { | |
| method: resolvedMethod, | |
| headers: requestHeaders, | |
| redirect: "manual", | |
| signal, | |
| }); | |
| } catch (err) { | |
| if ( | |
| signal.aborted || | |
| (err instanceof Error && | |
| (err.name === "AbortError" || err.name === "TimeoutError")) | |
| ) { | |
| return { error: "Request timed out", code: "TIMEOUT" }; | |
| } | |
| return { error: "Failed to fetch URL", code: "FETCH_ERROR" }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/webFetch.ts` around lines 105 - 117, The catch block that
classifies timeouts only checks for AbortError so undici's TimeoutError (thrown
during headers/body reads) is misclassified; update the error handling around
the fetch (the variables/constructs: response, err, AbortSignal.timeout,
FETCH_TIMEOUT_MS) to treat both AbortError and TimeoutError as timeouts by
checking err.name === "AbortError" || err.name === "TimeoutError" (or
err.constructor?.name === "TimeoutError"), and return the same { error: "Request
timed out", code: "TIMEOUT" } for either case; optionally, if undici's
TimeoutError class can be imported in your environment, use instanceof
TimeoutError for a stronger check.
| // 8. Read body with size cap | ||
| let body: string; | ||
| let truncated = false; | ||
| try { | ||
| body = await response.text(); | ||
| if (body.length > MAX_BODY_BYTES) { | ||
| body = body.slice(0, MAX_BODY_BYTES); | ||
| truncated = true; | ||
| } | ||
| } catch { | ||
| body = ""; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate webFetch.ts and understand the code structure
find . -name "webFetch.ts" -o -name "webFetch.js" | head -20Repository: kyalabs-Io/badge-server
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Read webFetch.ts to understand the code structure
wc -l ./src/tools/webFetch.tsRepository: kyalabs-Io/badge-server
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Read the entire webFetch.ts file
cat -n ./src/tools/webFetch.tsRepository: kyalabs-Io/badge-server
Length of output: 6857
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or related code that might show how this is used
# or what Node.js version is targeted
grep -r "MAX_BODY_BYTES\|response\.text\|getReader" --include="*.ts" --include="*.js" --include="*.json" | head -20Repository: kyalabs-Io/badge-server
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Check package.json for Node.js version requirements
cat package.json | grep -A 5 -B 5 "node\|engines" | head -30Repository: kyalabs-Io/badge-server
Length of output: 784
🏁 Script executed:
#!/bin/bash
# Check if there are tests for webFetch
find . -name "*webFetch*.test.*" -o -name "*webFetch*.spec.*" | head -10Repository: kyalabs-Io/badge-server
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Read the test file to understand usage patterns
cat -n ./src/tools/webFetch.test.tsRepository: kyalabs-Io/badge-server
Length of output: 12338
The 5MB limit is enforced after the full response is already buffered, and body.length counts UTF-16 code units rather than bytes.
response.text() reads the entire response before truncation occurs (lines 124–127). More critically, JavaScript string .length measures UTF-16 code units, not bytes. A response containing multi-byte UTF-8 characters (emoji, CJK) can exceed the advertised 5MB byte limit while still passing the code-unit-based check. For example, a 5.3MB response of 4-byte UTF-8 characters would be decoded into fewer than 5,242,880 UTF-16 code units and would not be truncated.
The proposed streaming fix using response.body?.getReader() correctly counts actual bytes via value.byteLength and truncates at the proper boundary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/webFetch.ts` around lines 120 - 131, The current block uses
response.text() and string.length which buffers the entire body and counts
UTF-16 code units instead of bytes; replace that logic in the function that
reads the response (the body/truncated handling around response.text(),
MAX_BODY_BYTES, and truncated) with a streaming reader: use
response.body?.getReader() to read Uint8Array chunks, sum value.byteLength to
enforce MAX_BODY_BYTES, push bytes into a buffer (or decode incrementally with a
TextDecoder) up to the byte limit, set truncated = true when the limit would be
exceeded and stop reading further, and then produce the final body string from
the collected bytes; ensure the fallback catch logic remains if response.body is
absent or reading fails.
…YA-98) The #1 E2E blocker. getHeaders() and webFetch() were injecting the consent key (pk_* / OAuth token) as Kya-Token header. The verify endpoint expects kya_* opaque tokens from /api/badge/enroll. Fix: - New module badge-token.ts: per-merchant badge token cache + enroll API - getAgentIdentity: calls enroll after identity established (fire-and-forget) - webFetch: looks up cached badge token by merchant, enrolls on-the-fly if needed - getHeaders: reads from badge token cache (last enrolled merchant) Also updates NEXT_STEP_TEXT to reference 2.0 tools (KYA-105). 178 tests passing (10 new in badge-token.test.ts). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Enroll endpoint now requires auth. badge-token.ts must send the consent key as Authorization: Bearer header when calling enroll. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/tools/webFetch.test.ts (1)
41-53:⚠️ Potential issue | 🟡 MinorMake
mockResponse.okmatch real Fetch behavior.
Response.okis true only for 2xx responses. Treating 3xx as ok makes the redirect-path assertions less trustworthy.🧩 Proposed fix
return { - ok: status >= 200 && status < 400, + ok: status >= 200 && status < 300, status, headers: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/webFetch.test.ts` around lines 41 - 53, The mockResponse helper currently sets ok for 2xx–3xx; update mockResponse so Response.ok mirrors real Fetch by making ok true only for 2xx (i.e., status >= 200 && status < 300). Locate the mockResponse function and change the ok calculation accordingly (keep the rest of the return shape the same so tests using headers, text, and url behave unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/badge-token.test.ts`:
- Around line 48-55: The enroll API assertion is missing verification that the
request includes the consent key; after parsing opts.body (where
mockFetch.mock.calls[0] is unpacked into url and opts and assigned to body), add
an assertion that body.consent exists and equals the expected consent value used
by the test (or at minimum is truthy), so update the assertions around mockFetch
/ body to include expect(body.consent)... using the same expected consent
variable from the test setup.
In `@src/lib/badge-token.ts`:
- Around line 22-23: getCachedBadgeToken() returns a cached token but never
updates the process-global lastEnrolledMerchant, causing subsequent calls to
kya_getHeaders / getHeaders to pick the wrong merchant; update
lastEnrolledMerchant whenever getCachedBadgeToken() returns a cached token (set
lastEnrolledMerchant = merchant argument or derived merchant id inside
getCachedBadgeToken), and likewise ensure any other cache-hit paths in
getBadgeToken/getCachedBadgeToken and the no-arg getHeaders/kya_getHeaders
fallback refresh lastEnrolledMerchant before returning so the global pointer
matches the token being returned; longer-term, consider changing
getHeaders/kya_getHeaders to require explicit merchant context instead of
relying on lastEnrolledMerchant.
- Around line 35-47: The enroll request is currently sent without the consent
key even though consentKey is required; update the POST to
`${apiUrl}/api/badge/enroll` to include the consent key (consentKey returned
from getStoredConsentKey) in the payload so the server can authenticate the
enroll call—i.e., modify the JSON body passed to fetch (the JSON.stringify call
in the fetch within this block) to include consent_key: consentKey (or add an
appropriate header if your API expects it) alongside merchant and install_id.
In `@src/tools/getAgentIdentity.ts`:
- Line 163: The message advertises using kya_getHeaders before the badge-token
enrollment completes, causing callers of kya_getAgentIdentity to get NO_IDENTITY
if the cache isn’t populated yet; fix by ensuring the enrollment is awaited:
either have kya_getAgentIdentity await the background enrollment Promise before
returning (so the cache is populated) or change kya_getHeaders to await a shared
in-flight enrollment Promise (e.g., badgeEnrollmentPromise /
startBadgeEnrollment()) before returning headers; update the code paths that
kick off enrollment (the background call around lines ~296-300) to store and
reuse a single Promise so both kya_getAgentIdentity and kya_getHeaders can await
it.
---
Duplicate comments:
In `@src/tools/webFetch.test.ts`:
- Around line 41-53: The mockResponse helper currently sets ok for 2xx–3xx;
update mockResponse so Response.ok mirrors real Fetch by making ok true only for
2xx (i.e., status >= 200 && status < 300). Locate the mockResponse function and
change the ok calculation accordingly (keep the rest of the return shape the
same so tests using headers, text, and url behave unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8672f480-a98b-4023-a9b0-842a60a65e0f
📒 Files selected for processing (7)
src/lib/badge-token.test.tssrc/lib/badge-token.tssrc/tools/getAgentIdentity.tssrc/tools/getHeaders.test.tssrc/tools/getHeaders.tssrc/tools/webFetch.test.tssrc/tools/webFetch.ts
✅ Files skipped from review due to trivial changes (1)
- src/tools/getHeaders.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tools/getHeaders.test.ts
- src/tools/webFetch.ts
| // Verify the enroll API was called correctly | ||
| expect(mockFetch).toHaveBeenCalledTimes(1); | ||
| const [url, opts] = mockFetch.mock.calls[0]; | ||
| expect(url).toContain("/api/badge/enroll"); | ||
| expect(opts.method).toBe("POST"); | ||
| const body = JSON.parse(opts.body); | ||
| expect(body.merchant).toBe("etsy.com"); | ||
| expect(body.install_id).toBe("inst-aaaa-bbbb-cccc-dddddddddddd"); |
There was a problem hiding this comment.
Assert that /api/badge/enroll actually carries the consent key.
This only checks the body today, so the test still passes if the enroll call is anonymous.
🧪 Proposed assertion
const [url, opts] = mockFetch.mock.calls[0];
expect(url).toContain("/api/badge/enroll");
expect(opts.method).toBe("POST");
+ expect(opts.headers.Authorization).toBe("Bearer pk_test_abc123");
const body = JSON.parse(opts.body);
expect(body.merchant).toBe("etsy.com");
expect(body.install_id).toBe("inst-aaaa-bbbb-cccc-dddddddddddd");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Verify the enroll API was called correctly | |
| expect(mockFetch).toHaveBeenCalledTimes(1); | |
| const [url, opts] = mockFetch.mock.calls[0]; | |
| expect(url).toContain("/api/badge/enroll"); | |
| expect(opts.method).toBe("POST"); | |
| const body = JSON.parse(opts.body); | |
| expect(body.merchant).toBe("etsy.com"); | |
| expect(body.install_id).toBe("inst-aaaa-bbbb-cccc-dddddddddddd"); | |
| // Verify the enroll API was called correctly | |
| expect(mockFetch).toHaveBeenCalledTimes(1); | |
| const [url, opts] = mockFetch.mock.calls[0]; | |
| expect(url).toContain("/api/badge/enroll"); | |
| expect(opts.method).toBe("POST"); | |
| expect(opts.headers.Authorization).toBe("Bearer pk_test_abc123"); | |
| const body = JSON.parse(opts.body); | |
| expect(body.merchant).toBe("etsy.com"); | |
| expect(body.install_id).toBe("inst-aaaa-bbbb-cccc-dddddddddddd"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/badge-token.test.ts` around lines 48 - 55, The enroll API assertion
is missing verification that the request includes the consent key; after parsing
opts.body (where mockFetch.mock.calls[0] is unpacked into url and opts and
assigned to body), add an assertion that body.consent exists and equals the
expected consent value used by the test (or at minimum is truthy), so update the
assertions around mockFetch / body to include expect(body.consent)... using the
same expected consent variable from the test setup.
| /** Track the last enrolled merchant for getHeaders() (no merchant context). */ | ||
| let lastEnrolledMerchant: string | null = null; |
There was a problem hiding this comment.
lastEnrolledMerchant goes stale on cache hits.
When Line 33 returns a cached token, the “active” merchant is not refreshed, but Lines 80-81 still use lastEnrolledMerchant as the no-arg source of truth. After a multi-merchant session, getCachedBadgeToken() can hand kya_getHeaders the wrong merchant token even without any async race. At minimum refresh it on cache hits; longer-term this lookup should take explicit merchant context instead of a process-global pointer.
🩹 Minimum fix for the stale cache-hit case
const cached = badgeTokenCache.get(merchant);
- if (cached) return cached;
+ if (cached) {
+ lastEnrolledMerchant = merchant;
+ return cached;
+ }Also applies to: 30-33, 61-62, 75-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/badge-token.ts` around lines 22 - 23, getCachedBadgeToken() returns a
cached token but never updates the process-global lastEnrolledMerchant, causing
subsequent calls to kya_getHeaders / getHeaders to pick the wrong merchant;
update lastEnrolledMerchant whenever getCachedBadgeToken() returns a cached
token (set lastEnrolledMerchant = merchant argument or derived merchant id
inside getCachedBadgeToken), and likewise ensure any other cache-hit paths in
getBadgeToken/getCachedBadgeToken and the no-arg getHeaders/kya_getHeaders
fallback refresh lastEnrolledMerchant before returning so the global pointer
matches the token being returned; longer-term, consider changing
getHeaders/kya_getHeaders to require explicit merchant context instead of
relying on lastEnrolledMerchant.
| const AGENT_TYPE = "badge-mcp"; | ||
| const NEXT_STEP_TEXT = | ||
| "When visiting a merchant, report your badge presentation using kya_reportBadgePresented with the merchant domain and your verification token."; | ||
| "Your identity is attached automatically via kya_web_fetch. Use kya_web_fetch instead of web_fetch when shopping — it injects your Kya-Token header and records your visit. For browser automation, use kya_getHeaders to get headers for setExtraHTTPHeaders."; |
There was a problem hiding this comment.
Don’t advertise kya_getHeaders before the badge-token enroll finishes.
Line 163 says the next step is kya_getHeaders, but Line 300 fires enrollment in the background. A caller can successfully complete kya_getAgentIdentity(...) and immediately get NO_IDENTITY because the cache is still empty. Either await this here or have kya_getHeaders await a shared in-flight enrollment.
Also applies to: 296-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/getAgentIdentity.ts` at line 163, The message advertises using
kya_getHeaders before the badge-token enrollment completes, causing callers of
kya_getAgentIdentity to get NO_IDENTITY if the cache isn’t populated yet; fix by
ensuring the enrollment is awaited: either have kya_getAgentIdentity await the
background enrollment Promise before returning (so the cache is populated) or
change kya_getHeaders to await a shared in-flight enrollment Promise (e.g.,
badgeEnrollmentPromise / startBadgeEnrollment()) before returning headers;
update the code paths that kick off enrollment (the background call around lines
~296-300) to store and reuse a single Promise so both kya_getAgentIdentity and
kya_getHeaders can await it.
- Filter case-variant kya-token headers before spreading in webFetch so agents can't override the server-injected Kya-Token (Badge #6) - Update lastEnrolledMerchant on cache hit in getCachedBadgeToken so no-arg getHeaders() returns the correct merchant context (Badge #11) 178/178 tests passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tools/webFetch.ts (1)
61-82: Minor: Step numbering in comments is inconsistent.Comments jump from "1." (line 61) to "3." (line 69), "4." (line 76), "5." (line 81), then two "7."s (lines 104, 119). Consider renumbering for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/webFetch.ts` around lines 61 - 82, The inline step numbering in the fetch validation comments is inconsistent (e.g., the block around URL parsing/validation using parsed, the scheme check using isHttps/isTestHttp, the SSRF check using isPublicOrigin, and the merchant extraction using merchant) and later duplicated "7." steps; renumber the comment steps sequentially to reflect the actual flow (1, 2, 3, 4, 5, ...) around those symbols so future readers can follow the validation stages in order and remove the duplicate "7." labels.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/tools/webFetch.ts`:
- Around line 61-82: The inline step numbering in the fetch validation comments
is inconsistent (e.g., the block around URL parsing/validation using parsed, the
scheme check using isHttps/isTestHttp, the SSRF check using isPublicOrigin, and
the merchant extraction using merchant) and later duplicated "7." steps;
renumber the comment steps sequentially to reflect the actual flow (1, 2, 3, 4,
5, ...) around those symbols so future readers can follow the validation stages
in order and remove the duplicate "7." labels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9afa85ba-1e7d-459f-af2d-80cae096bbb6
📒 Files selected for processing (2)
src/lib/badge-token.tssrc/tools/webFetch.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/badge-token.ts
1 participant
Summary
kya_web_fetch— fetch with automatic Kya-Token header injection + auto-declare (browse_declared event). SSRF protection, HTTPS-only, 5MB body cap, 30s timeout, manual redirects. Primary identity delivery path (Path 1).kya_getHeaders— returns{ "Kya-Token": token }for PlaywrightsetExtraHTTPHeadersor Chrome extensiondocument.cookie. Identity delivery for browser automation (Path 2/2.5).kya_reportBadgeOutcome+kya_reportBadgeNotPresentedto no-ops — outcomes now tracked server-side via verify endpoint; not-presented event no longer scored.isPublicOrigin()to sharedsrc/lib/url-safety.tswith 16 SSRF tests (RFC1918, localhost, IPv6, link-local, AWS metadata).Security
isPublicOrigin()blocks RFC1918, localhost, loopback, link-local (169.254.x), IPv6 privateredirect: "manual"prevents Kya-Token leaking to redirect targetsset-cookiefrom response headersKya-Tokenvia headers param (our token wins)console.login production paths (stderr only)Test plan
npm run build— zero TypeScript errorsconsole.login production pathsLinear: KYA-55
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Deprecations
Security
Tests
Chores