Use HTTP/2 to make requests from proxies

[antoniosarosi]

Google AI requires ALPN negotiation otherwise it returns HTTP 400. See PR #1318.

Promptfiddle uses a Node.js proxy server in order to inject API keys into the request. However, the proxy was written using Express.js and http-proxy-middleware, which does not support HTTP/2 and ALPN by default.

This is a rewrite of the proxy using the Node.js http2 module.

VSCode extensions uses a local proxy running on Express as well, same change applies to that proxy.

---

Important

Rewrites proxy servers in server.js and extension.ts to use HTTP/2 for ALPN support, replacing http-proxy-middleware.

• Behavior:
  • Rewrites proxy server in server.js to use Node.js http2 module instead of http-proxy-middleware.
  • Updates extension.ts to use http2 for proxying requests, ensuring compatibility with ALPN requirements.
• Headers:
  • Removes host and origin headers from upstream requests in both server.js and extension.ts.
  • Injects API keys into requests only for allowed origins using API_KEY_INJECTION_ALLOWED.
• Error Handling:
  • Adds error handling for missing baml-original-url header in both server.js and extension.ts.
  • Logs proxy request errors and sends 500 Internal Server Error response on failure.

^{This description was created by for 20cf0fc. It will automatically update as commits are pushed.}

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
baml	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 27, 2025 2:03pm

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 95177ff in 1 minute and 22 seconds

More details

• Looked at 174 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. typescript/fiddle-proxy/server.js:98

• Draft comment:
  Consider adding error handling for req.pipe(proxyReq) to manage cases where the request body cannot be piped correctly.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. typescript/fiddle-proxy/server.js:110

• Draft comment:
  Consider adding error handling for proxyReq.on('data', ...) to manage cases where the response data is not received correctly.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The code already has comprehensive error handling. The 'data' event in Node.js streams doesn't typically need explicit error handling because: 1) Stream errors are caught by the 'error' event handler 2) Write errors would be caught by the response stream's error handler 3) The try-catch block would catch any synchronous errors. Adding more error handling would be redundant.
  I might be overlooking some edge cases in HTTP/2 streaming that could cause data corruption without triggering the error event. There could be specific HTTP/2 stream error states that need special handling.
  The existing error handlers are sufficient - the HTTP/2 stream errors would be caught by the proxyReq.on('error') handler, and Node.js's stream implementation is robust enough to handle data streaming issues.
  The comment should be deleted as it suggests adding unnecessary error handling when the code already has comprehensive error handling in place.

Workflow ID: wflow_TDGrM4RVpAuyQFiV

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

Destructuring in the for loop is incorrect. It should be for (const [header, value] of Object.entries(allowedHeaders)) {. This issue is also present in other parts of the code where destructuring is used in a for loop.

[ellipsis-dev]

Consider adding error handling for http2.connect to manage cases where the connection cannot be established.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 9d6c1e5 in 32 seconds

More details

• Looked at 12 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. typescript/fiddle-proxy/server.js:107

• Draft comment:
  Setting 'Access-Control-Allow-Origin' to '*' can lead to security vulnerabilities by allowing any domain to access the resources. Consider restricting this to specific domains.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  This is a proxy server specifically designed to handle cross-origin requests to API providers. The use of '*' for CORS is intentional here as it's meant to be accessible from any origin. The server has other security measures in place, like the API_KEY_INJECTION_ALLOWED whitelist, which is the real security boundary. The CORS setting is not the security vulnerability here.
  The comment raises a valid general security concern about CORS. In many applications, unrestricted CORS can indeed be dangerous.
  However, in this specific case, the unrestricted CORS is an intentional and necessary part of the proxy server's design. The security is handled through other mechanisms like the origin whitelist.
  The comment should be deleted as it raises a concern that doesn't apply to this specific use case where unrestricted CORS is intentional and necessary.

Workflow ID: wflow_KUgYrnGzvhKWVXhS

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 08b6cc6 in 25 seconds

More details

• Looked at 235 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. typescript/vscode-ext/packages/vscode/src/extension.ts:263

• Draft comment:
  Consider using a proper logging library instead of console.error for better error tracking and management.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The code uses console.log for error logging, which is not ideal for production environments. A proper logging mechanism should be used.

2. typescript/vscode-ext/packages/vscode/src/extension.ts:268

• Draft comment:
  Consider using a proper logging library instead of console.error for better error tracking and management.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The code uses console.log for error logging, which is not ideal for production environments. A proper logging mechanism should be used.

3. typescript/vscode-ext/packages/vscode/src/extension.ts:198

• Draft comment:
  Consider using a proper logging library instead of console.error for better error tracking and management.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The code uses console.log for error logging, which is not ideal for production environments. A proper logging mechanism should be used.

4. typescript/vscode-ext/packages/vscode/src/extension.ts:199

• Draft comment:
  Consider using a proper logging library instead of console.error for better error tracking and management.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The code uses console.log for error logging, which is not ideal for production environments. A proper logging mechanism should be used.

Workflow ID: wflow_9v8suRqLFIMIrPsN

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 20cf0fc in 24 seconds

More details

• Looked at 96 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_DB3wI7jDbe4b2Kpx

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.