Repository navigation
fix: verify aws region - #3412
Conversation
|
@strandly-the-agent review |
|
Assessment: Approve Clean, well-scoped security fix. The shared Review notes
Nicely done — refactoring the two paths onto one helper closes the drift that let this gap exist. |
strandly-the-agent
left a comment
There was a problem hiding this comment.
TL;DR: Approve — at 43a3af6967e5a8e4ad959d93f7289f50e1e92bf4, every region-to-URL path is validated before token or client setup; no actionable findings.
✅ Python focused suites: 242 passed
✅ TypeScript Mantle suite: 31 passed, no type errors
✅ Cold Python/Node probes rejected prefix/control/newline/NUL/Unicode/100k inputs; the unvalidated report payload resolves to attacker.com
✅ git diff --check; exact-head GitHub CI completed green
portaudio.h
Review breakdown
- Correctness/security: All three manual region-interpolated URL sinks are covered. Python Mantle validates explicit, attached-session, and default-session regions; TypeScript validates explicit and environment regions; Nova validates before endpoint/SigV4 setup.
- Regex semantics: Both SDKs use full-string ASCII allowlists. Direct probes confirmed the shipped patterns reject URL delimiters, prefix/suffix payloads, line terminators, NUL, Unicode digits, and oversized input.
- API/compatibility: Helpers remain private/internal and package exports are unchanged; no API-review label is needed.
- Tests: Existing coverage proves the security behavior. A few possible mutation-hardening additions did not reveal a product defect, so I left them as non-blocking rather than add review noise.
- Pass caveat: The advanced adversarial model was unavailable and two default retries returned empty, so I independently ran the cold security probes above.
Description
The region is interpolated straight into
https://bedrock-mantle.{region}.api.aws/...with no validation. Because the sibling code mints a fresh AWS bearer token and attaches it as the request's auth header, a region likex@attacker.com:443/#re-points the host toattacker.comand ships a live credential there on the first request.Was already fixed this for Nova Sonic (#2990) but not propagated it to the Mantle path. This centralizes that validation as a shared
validate_regionhelper, applies it at each SDK's region-resolution chokepoint (so config-, session-, and env-sourced regions are all covered), and refactors Nova Sonic to reuse it so the two paths can't drift again.Two constraints worth flagging for review:
\A...\Z, not^...$— Python's$matches before a trailing newline, so^...$would letus-east-1\nthrough under.match()/.search().\A...\Zis fully self-anchored regardless of which method a future caller uses. The TS literal keeps/^...$/(JS has no\A\Z, and JS$without themflag is already end-of-string).[0-9]rather than\d— Python's\dalso matches Unicode digits (us-east-١would pass), so the two SDKs' patterns are now byte-identical ASCII, honoring the cross-SDK parity requirement.Related Issues
Prior art: #2990
Documentation PR
No documentation changes — internal validation hardening with no public API surface change.
Type of Change
Bug fix
Testing
Ran the Python model suites (
test__validation,test_openai,test_openai_responses) and the Nova Sonic region tests under thebidiextras, plus the TypeScript Mantle suite — together these cover the config, boto3-session, and env-var resolution paths in both SDKs, valid regions (includingus-gov-east-1), and the malformed/Unicode/newline rejection cases. Also drove both SDKs end-to-end against the real public API (OpenAIModel(bedrock_mantle_config=...)/resolveMantleRegion) with the report'sx@attacker.com:443/#payload and confirmed it is rejected before any token is minted, while a legitimate region still builds the correct URL.hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.