Conversation
Gateway now proxies /webhook/* requests to WebhookServer (port 8080), allowing external webhooks via ngrok tunnel (bound to gateway port 3000) to reach webhook endpoints correctly. - Add webhook_proxy_addr field to GatewayState - Add webhook_proxy_handler for reverse proxying requests - Add with_webhook_proxy() method to GatewayChannel - Safe error handling without unwrap() - Add unit tests for webhook proxy functionality Fixes nearai#738
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a crucial enhancement to the system's networking capabilities by integrating a reverse proxy within the Gateway. This change specifically addresses issues where webhook requests, originating from a tunnel, failed to reach the dedicated WebhookServer. By establishing a clear forwarding mechanism, the system now reliably routes these requests, preventing service disruptions and improving the overall robustness of webhook handling. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a reverse proxy to the gateway for webhook requests, significantly improving tunnel support. However, a critical path traversal vulnerability was identified in the webhook_proxy_handler due to unsanitized user input being used to construct the proxy destination URI, which could allow access to unintended internal endpoints. Additionally, there are opportunities to enhance the performance and robustness of the new proxy handler and reduce code duplication in the tests for better maintainability.
- Add path traversal protection (block '..' in path) - Use static reqwest::Client for connection pool reuse - Handle resp_builder error instead of unwrap() - Extract test helper function for GatewayState
Add missing webhook_proxy_addr field to GatewayState in: - tests/openai_compat_integration.rs (2 locations) - tests/ws_gateway_integration.rs
zmanian
left a comment
There was a problem hiding this comment.
Review
Adds a reverse proxy on the gateway at `/webhook/{*path}` that forwards requests to the webhook server (typically port 8080), enabling tunnel providers (bound to the gateway port) to reach webhook endpoints.
Implementation
- `webhook_proxy_handler` reads the body (up to 15MB), copies headers (except Host), and forwards via a static `reqwest::Client`.
- Path traversal check: `path.contains("..")`
- `webhook_proxy_addr: Option` added to `GatewayState`, set via `with_webhook_proxy()` builder method.
- Returns 503 when not configured, 502 on upstream failure.
Concerns
-
Same concern as PR #778 (which includes this): `path.contains("..")` may miss URL-encoded traversal. Since the path comes from Axum's router which decodes, this is likely fine, but document the assumption.
-
Body buffering: Reads the entire request body into memory before forwarding. For webhook payloads this is fine (they're small), but the 15MB limit is generous. Consider lowering to 1MB (matching the gateway's `DefaultBodyLimit`).
-
No auth forwarding awareness: The proxy forwards all headers except Host. If the webhook endpoint requires the gateway auth token, the proxy would need to inject it. Currently the webhook endpoints use their own per-channel secrets, so this is probably fine.
-
Overlap with PR #778: This change appears to be a subset of PR #778 (the larger refactor). Are these coordinated? If #778 merges first, this PR may need rebasing or closing.
Tests are thorough -- proxy forwarding, 503 when unconfigured, different paths.
@zmanian @ilblackdragon — Thanks for the thorough reviews. After looking at #778, I can see it includes the same webhook proxy implementation and also moves the initialization into the module-owned factory pattern, which is cleaner than my approach of passing the address through main.rs. |
|
I'd like to pause and discuss the broader architectural implications before this PR gets reviewed/merged. While implementing this fix, I realized there's an underlying architectural question that might need clarification from the maintainers. My Question: The tunnel currently binds to Gateway (3000), but webhooks are handled by WebhookServer (8080). This mismatch caused #738, which this PR addresses with a reverse proxy. But I'm wondering: what was the original design intent?
Alternative approaches I considered:
I went with the reverse proxy approach because it enables both use cases (remote Web UI + webhooks) with a single tunnel, which seems practical. But I want to make sure this aligns with the project's long-term vision before committing to this architecture. |
|
Closing — superseded by #778 (merged). |
Summary
Closes #738.
/webhook/*requests to WebhookServer (port 8080)Changes
src/channels/web/server.rswebhook_proxy_addrfield to GatewayState; addwebhook_proxy_handlerand/webhook/{*path}route; add unit testssrc/channels/web/mod.rswebhook_proxy_addrto state initialization; addwith_webhook_proxy()builder methodsrc/main.rssrc/channels/web/test_helpers.rswebhook_proxy_addrto test statesrc/channels/web/ws.rswebhook_proxy_addrto test stateTest plan
cargo check— compilescargo test --lib— all tests passcargo test --lib web::server::tests::test_webhook— webhook proxy tests pass/webhook/slackto webhook server on port 8080