Skip to content

fix: harden provider runtime edge cases#32

Merged
moose-lab merged 1 commit into
mainfrom
hotfix/provider-runtime-edge-cases
May 30, 2026
Merged

fix: harden provider runtime edge cases#32
moose-lab merged 1 commit into
mainfrom
hotfix/provider-runtime-edge-cases

Conversation

@moose-lab

Copy link
Copy Markdown
Owner

Summary

  • Block IPv4-mapped IPv6 private hosts in provider base URL validation.
  • Classify provider HTTP 403 as forbidden while keeping 401 as auth_failed.
  • Preserve existing versioned provider paths such as /v2 and clean up pending API-mode user messages when a session is killed before the provider response returns.

Root Cause

  • The provider runtime only checked plain IPv4 and a narrow set of IPv6 private patterns, so ::ffff:* addresses could bypass the private-host block.
  • Provider 403 responses were grouped with 401 auth failures, masking permission/model-tier failures as invalid keys.
  • API-mode session turns inserted the user message before the provider call and did not roll that message back if the session was killed while awaiting the provider.

Validation

  • git diff --check
  • bun run test (164/164 passed on the hotfix branch)
  • bun run typecheck
  • bun run build

@claude

claude Bot commented May 30, 2026

Copy link
Copy Markdown

Claude finished @moose-lab's task in 3m 0s —— View job


PR Review: fix: harden provider runtime edge cases

Tasks

  • Gather context and understand the request
  • Read changed source files
  • Read changed test files
  • Analyze changes and form findings
  • Post review

Findings

  • [should-fix] Emit/DB divergence on session killapi-session-runtime.ts:97–112: The user-message event is emitted unconditionally at line 97, before the provider call. When the session is killed and the DB row is deleted (lines 110–112), no compensating event is emitted. A client watching the stream will have received the { type: "message", role: "user" } event; on reconnect it loads from DB and sees nothing. Whether this matters depends on whether killed-session streams are torn down before the check fires, but the silent divergence is worth addressing — either defer the emit until after the kill-check, or emit a { type: "revoke_message" } tombstone event. Fix this →

  • [nit] parseIpv4MappedIpv6 relies on URL constructor normalizationapi-provider-runtime.ts:80: The two prefix strings ("::ffff:", "0:0:0:0:0:ffff:") only cover post-normalization forms. This is safe because all callers pass url.hostname which the URL constructor already normalizes, but the function's contract isn't expressed anywhere. A brief inline comment (e.g. // expects post-URL-parse normalized hostname) would make the dependency explicit and prevent misuse if the function is called elsewhere.

  • [nit] Duplicated classifyProviderFailure — both api-provider-runtime.ts:579 and api-provider-models.ts:262 contain identical copies of this function and were both updated in sync here. This PR is a good opportunity to extract to a shared helper to eliminate future drift risk.

  • [nit] Missing test for hex-form IPv4-mapped IPv6::ffff:c0a8:101 (hex for 192.168.1.1) is a valid input that exercises the high/low word-split path in parseIpv4MappedIpv6 but has no test. The logic is correct, but coverage is incomplete.


Assessment

The three core fixes are all correct:

  • IPv4-mapped IPv6 blocking: parseIpv4MappedIpv6 correctly handles both dotted-decimal and hex-word forms. The parseIpv6FirstGroup rewrite replacing the regex on the raw string is strictly better — the bitmask approach ((firstGroup & 0xfe00) === 0xfc00 for fc00::/7, (firstGroup & 0xffc0) === 0xfe80 for fe80::/10) is the canonical way to test these ranges and avoids false positives from the old regex on edge addresses like fcc0::, fe90::, etc.

  • 403 → forbidden: The split is semantically correct. 401 = invalid/missing credential; 403 = valid credential but insufficient permission or model-tier restriction. Masking them into the same kind was lossy.

  • User-message rollback on session kill: The lastInsertRowid-scoped DELETE is the right approach — it avoids a TOCTOU where a second message with the same content could be incorrectly deleted. The existing test update confirms the expected new behavior.

  • Versioned path regex: /\/v\d+(?:beta)?$/ correctly covers /v1, /v2, /v1beta (Google), etc. and is a clear improvement over the prior endsWith("/v1") check.

Verdict: APPROVE


@moose-lab
moose-lab merged commit 291b50b into main May 30, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant