refactor: shared hardened JSON string-map parser for BYOK header/body parsing#6528
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors duplicated BYOK JSON string-map parsing while preserving header and body-field validation policies.
Changes:
- Adds a shared hardened parser.
- Retains policy-specific validation.
- Expands body-field parsing tests.
Show a summary per file
| File | Description |
|---|---|
containers/api-proxy/providers/copilot-byok.js |
Consolidates parsing and validation logic. |
containers/api-proxy/copilot-byok.test.js |
Adds body-field edge-case coverage. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| const lowerName = name.toLowerCase(); | ||
|
|
||
| if (typeof parsed !== 'object' || Array.isArray(parsed) || parsed === null) { | ||
| console.warn('AWF_BYOK_EXTRA_BODY_FIELDS: expected a JSON object; ignoring extra body fields'); | ||
| return {}; | ||
| } | ||
| // Prevent prototype pollution / special keys in header maps. | ||
| if (lowerName === '__proto__' || lowerName === 'constructor' || lowerName === 'prototype') { | ||
| return `AWF_BYOK_EXTRA_HEADERS: "${name}" is not an allowed header name; skipping`; |
|
@copilot address review feedback |
Fixed in the latest commit — all declarations and conditionals inside the |
|
✅ Copilot review passed with no inline comments. @copilot Add the |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
❌ Security Guard failed. Please review the logs for details. |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
✅ Smoke Claude passed |
|
✅ Build Test Suite completed successfully! |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
✅ Contribution Check completed successfully! Contribution check complete: the PR includes focused tests for the new parsing behavior, keeps the refactor inside the existing |
|
📰 VERDICT: Smoke Docker Sbx has concluded. All systems operational. This is a developing story. 🎤 |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
❌ Security Guard failed. Please review the logs for details. |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
✅ Smoke Claude passed |
|
✅ Contribution Check completed successfully! Code review complete: the PR appears to follow the applicable CONTRIBUTING.md guidelines. It includes focused tests for the new shared parser behavior, the description is clear and references a related issue, and the changes stay within the expected container/provider file organization. No comment needed. |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
🔌 Smoke Services — All services reachable! ✅ |
|
✅ Allowed domain ( Overall: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "example.com"See Network Configuration for more information.
|
Smoke Test: Claude Engine Validation
Overall result: PASS
|
Smoke Test Results
Overall status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
|
Smoke test results:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
|
Smoke Test: Copilot BYOK (Direct) Mode ✅ PASS Test Results:
Running in direct BYOK mode with
|
🔬 Smoke Test Results
Overall: PARTIAL — MCP connectivity confirmed ✅; pre-computed test data ( PR #6528 author:
|
|
Running in direct BYOK mode (AWF_AUTH_TYPE=github-oidc + AWF_AUTH_AZURE_* + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) authenticated via Microsoft Entra Overall: PASS
|
|
|
Smoke Test: API Proxy OpenTelemetry Tracing
All structurally verifiable scenarios pass. The OTEL tracing integration is correctly wired end-to-end.
|
Chroot Version Comparison Results
Overall: ❌ FAILED — Node.js version mismatch (host
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Smoke Test: Services Connectivity
Overall: FAIL —
|
🔬 Smoke Test Results
Overall:
|
parseByokExtraHeaders()andparseByokExtraBodyFields()duplicated the same security-sensitive parsing scaffold (JSON parse, object-shape checks, reserved-key guards, string-value enforcement), creating drift risk. This change consolidates that common path while preserving header-specific and body-field-specific policy checks.Shared parsing scaffold
parseJsonStringMap(...)incontainers/api-proxy/providers/copilot-byok.js.Header-specific policy preserved
parseByokExtraHeaders()now layers on:__proto__,constructor,prototype)authorization,x-api-key, etc.)isValidHeaderName)Body-field policy preserved
parseByokExtraBodyFields()now layers on:Focused test coverage updates
containers/api-proxy/copilot-byok.test.jsto cover body-field behavior parity for whitespace input, non-object JSON, and reserved key rejection.