feat: Update plugin system to support HTTP headers #566
Conversation
Signed-off-by: Dylan Kilkenny <[email protected]>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces HTTP header propagation through the plugin execution pipeline. Headers are extracted from incoming HTTP requests at the route layer, threaded through the Rust services layer, serialized as JSON, passed as command-line arguments to the TypeScript executor, and finally injected into the PluginContext for access by user plugins. Documentation and examples are updated to reflect this feature and remove SOROBAN_RPC_URL from required environment variables. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant RouteHandler as Route Handler<br/>(plugin.rs)
participant PluginService as Plugin Service<br/>(mod.rs)
participant PluginRunner as Plugin Runner<br/>(runner.rs)
participant ScriptExecutor as Script Executor<br/>(script_executor.rs)
participant TypeScriptExec as TypeScript Executor<br/>(executor.ts)
participant UserPlugin as User Plugin<br/>Handler
Client->>RouteHandler: HTTP Request<br/>(with headers)
activate RouteHandler
RouteHandler->>RouteHandler: extract_headers()<br/>HashMap<String, Vec<String>>
RouteHandler->>PluginService: plugin_call()<br/>with extracted headers
deactivate RouteHandler
activate PluginService
PluginService->>PluginService: Serialize headers<br/>to JSON string
PluginService->>PluginRunner: run()<br/>+ headers_json param
deactivate PluginService
activate PluginRunner
PluginRunner->>ScriptExecutor: execute_typescript()<br/>+ headers_json param
deactivate PluginRunner
activate ScriptExecutor
ScriptExecutor->>TypeScriptExec: CLI args<br/>+ headersJson
deactivate ScriptExecutor
activate TypeScriptExec
TypeScriptExec->>TypeScriptExec: parseHeaders()<br/>from headersJson
TypeScriptExec->>UserPlugin: handler(context)<br/>context.headers
deactivate TypeScriptExec
activate UserPlugin
UserPlugin->>UserPlugin: Access headers<br/>via context
UserPlugin-->>Client: Response
deactivate UserPlugin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
examples/channels-plugin-example/README.md (1)
280-314: Docs match the new mount behavior; header text could be tweakedThe description and env var usage now correctly reflect mounting the full plugin directory into the container. You might optionally rename or soften the “Local Plugin Development (Swap Built Output Only)” heading to avoid implying that only
dist/is overlaid, since the entire package is now mounted.plugins/tests/lib/plugin.test.ts (1)
281-339: Header-shape tests are fine; consider an end-to-end assertionThese tests nicely capture the intended
Record<string, string[]>shape and safe access patterns forheaders. For even stronger coverage, you could add (or rely on) an integration-style test that sends actual HTTP headers through the plugin route and asserts thatPluginContext.headersis populated with normalized lowercase keys as expected, exercising the full Rust→TS path.src/api/routes/plugin.rs (1)
141-169: Integration test doesn't verify header propagation.The test
test_plugin_call_extracts_headersuses a mock handler (mock_plugin_call) that doesn't actually verify the headers were extracted and passed through. The test only confirms the endpoint returns success with headers present, but doesn't validate the headers reached the plugin call request.This is acceptable for a route-level test since the unit tests (
test_extract_headers_unit, etc.) cover the extraction logic, but consider noting this limitation or adding an integration test that verifies end-to-end header propagation if coverage is a concern.plugins/tests/lib/executor.test.ts (1)
10-24: Test strategy note: Local function copy may drift from implementation.Since
parseHeadersis not exported, tests use a local copy. This is acceptable for validating the expected behavior/contract, but be aware that if the implementation inexecutor.tschanges, these tests may not catch regressions.Consider exporting
parseHeadersfor direct testing, or add a comment noting this is an intentional contract test rather than a direct unit test.src/services/plugins/mod.rs (1)
232-234: Consider logging serialization failures instead of silently defaulting.The
unwrap_or_default()will produce an empty string if serialization fails, which might mask issues. WhileHashMap<String, Vec<String>>serialization should never fail in practice, a warning log could help debugging edge cases.let headers_json = plugin_call_request .headers - .map(|h| serde_json::to_string(&h).unwrap_or_default()); + .map(|h| serde_json::to_string(&h).unwrap_or_else(|e| { + tracing::warn!("Failed to serialize headers: {}", e); + String::new() + }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
docs/1.2.x/plugins/channels.mdx(0 hunks)docs/1.2.x/plugins/index.mdx(17 hunks)docs/plugins/channels.mdx(0 hunks)docs/plugins/index.mdx(17 hunks)examples/channels-plugin-example/README.md(1 hunks)examples/channels-plugin-example/channel/package.json(1 hunks)examples/channels-plugin-example/docker-compose.plugin-dev.yaml(1 hunks)examples/channels-plugin-example/docker-compose.yaml(0 hunks)examples/launchtube-plugin-example/README.md(0 hunks)openapi.json(1 hunks)plugins/lib/executor.ts(4 hunks)plugins/lib/plugin.ts(6 hunks)plugins/tests/lib/executor.test.ts(1 hunks)plugins/tests/lib/plugin.test.ts(1 hunks)src/api/controllers/plugin.rs(3 hunks)src/api/routes/plugin.rs(3 hunks)src/models/plugin.rs(2 hunks)src/services/plugins/mod.rs(13 hunks)src/services/plugins/runner.rs(5 hunks)src/services/plugins/script_executor.rs(8 hunks)
💤 Files with no reviewable changes (4)
- docs/plugins/channels.mdx
- examples/launchtube-plugin-example/README.md
- examples/channels-plugin-example/docker-compose.yaml
- docs/1.2.x/plugins/channels.mdx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-05T04:46:17.576Z
Learnt from: sammccord
Repo: OpenZeppelin/openzeppelin-relayer PR: 457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.576Z
Learning: In the OpenZeppelin Relayer CDP signer tests, the mocks simulate the CDP service layer (which returns bytes) rather than the external CDP API layer (which returns base64), which is why tests use raw bytes.
Applied to files:
docs/1.2.x/plugins/index.mdxdocs/plugins/index.mdx
📚 Learning: 2025-09-05T04:46:17.576Z
Learnt from: sammccord
Repo: OpenZeppelin/openzeppelin-relayer PR: 457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.576Z
Learning: The CDP SDK in the OpenZeppelin Relayer consistently returns decoded bytes from signing operations, not base64 strings. The codebase intentionally uses EncodedSerializedTransaction for architectural consistency even when it requires encoding/decoding round-trips.
Applied to files:
plugins/lib/plugin.ts
🧬 Code graph analysis (4)
plugins/tests/lib/plugin.test.ts (1)
plugins/lib/kv.ts (1)
key(133-136)
plugins/lib/executor.ts (2)
src/models/rpc/json_rpc.rs (1)
result(112-119)plugins/lib/plugin.ts (1)
runUserPlugin(538-566)
src/api/routes/plugin.rs (1)
src/api/controllers/plugin.rs (1)
call_plugin(36-113)
plugins/lib/plugin.ts (1)
plugins/lib/kv.ts (2)
PluginKVStore(28-88)DefaultPluginKVStore(96-277)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: msrv
- GitHub Check: Analyze (rust)
- GitHub Check: semgrep/ci
🔇 Additional comments (29)
openapi.json (1)
15-15: Version bump aligns with new behaviorBumping the OpenAPI
info.versionto1.2.0to reflect the headers-related feature is appropriate and keeps the spec metadata in sync with runtime behavior.examples/channels-plugin-example/channel/package.json (1)
15-18: No issues with the dependency block changeThe simplified dependencies configuration is straightforward; nothing here suggests runtime or tooling problems for the example.
examples/channels-plugin-example/docker-compose.plugin-dev.yaml (1)
5-6: Full plugin directory mount matches the intended dev workflowMounting
${CHANNELS_PLUGIN_LOCAL}over the installed@openzeppelin/relayer-plugin-channelspath is consistent with the updated README and should make local plugin development smoother.src/models/plugin.rs (1)
1-1: PluginCallRequest.headers wiring is consistent with the designUsing
Option<HashMap<String, Vec<String>>>plus#[serde(default, skip_deserializing)]cleanly distinguishes client-providedparamsfrom server-injectedheaders, and the structure aligns with theRecord<string, string[]>expectation on the TS side. No issues from the model layer perspective.Also applies to: 23-30
src/api/controllers/plugin.rs (1)
180-183: Test updates for new headers field are correctInitializing
PluginCallRequestwithheaders: Nonein controller tests is the right minimal change and keeps existing behavior unchanged while accommodating the new field.Also applies to: 200-203, 229-232
src/services/plugins/script_executor.rs (2)
41-48: execute_typescript header-plumbing looks soundExtending
execute_typescriptwith aheaders_json: Option<String>and threading it through as an extra argv entry is consistent with the documented argv layout and doesn’t affect existing call sites (now updated to passNone). Usingunwrap_or_default()is fine here as long as the TS executor treats an empty string as “no headers”, which matches the intended design.Also applies to: 66-74, 199-206, 242-249, 275-282, 324-331, 364-371
382-442: New headers integration test gives good end-to-end coverage
test_execute_typescript_with_headerseffectively validates that JSON-encoded headers reach the TShandler(context)as aheaders: Record<string, string[]>, that logging still works, and thatparamsare preserved. This is a solid high-level test of the new behavior.src/api/routes/plugin.rs (2)
21-33: Header extraction looks correct for multi-value support.The implementation correctly handles multi-value headers by using
entry().or_default().push(). Note that headers with non-UTF8 values are silently skipped (line 25), which is reasonable since HTTP header values should generally be ASCII/UTF8.Security consideration: All HTTP headers including potentially sensitive ones (
Cookie,Authorization, internal proxy headers likeX-Forwarded-For,X-Real-IP) are forwarded to plugins. Ensure plugin authors are aware they receive the full header set and handle sensitive data appropriately.Consider whether header filtering/allowlisting would be beneficial for security. You may want to document which headers are passed to plugins so users understand the security implications.
37-46: LGTM!The handler correctly extracts the
HttpRequest, converts the JSON body, attaches extracted headers, and delegates to the controller. The pattern follows actix-web conventions.plugins/tests/lib/executor.test.ts (1)
26-127: LGTM!Comprehensive test coverage for header parsing including edge cases (empty, invalid JSON, special characters, Unicode) and Rust serialization format compatibility. The tests validate the expected contract for the header parsing behavior.
plugins/lib/executor.ts (3)
35-62: LGTM!The CLI argument extraction is correctly updated to include
headersJsonas the 7th argument (index 7). The return object properly includes the new field, and validation only covers required arguments, which is appropriate sincehttpRequestIdandheadersJsonare optional.
75-88: LGTM!The
parseHeadersfunction correctly handles the optional nature of headers with graceful fallback toundefinedon parse errors. This aligns with the test expectations and the Rust serialization format.
101-111: LGTM!The main function correctly extracts, parses, and forwards headers to
runUserPlugin. The integration is clean and follows the established pattern for other optional parameters likehttpRequestId.src/services/plugins/runner.rs (3)
33-60: LGTM!The trait signature is correctly extended with
headers_json: Option<String>. The parameter placement afterhttp_request_idand beforestateis logical and consistent with the implementation.
103-112: LGTM!The
headers_jsonparameter is correctly threaded through toScriptExecutor::execute_typescript. The timeout handling and error paths remain unchanged, which is appropriate since headers are just passed through.
201-212: LGTM!Test correctly updated to include
Nonefor the newheaders_jsonparameter, maintaining existing test behavior.src/services/plugins/mod.rs (4)
236-248: LGTM!The runner invocation correctly includes
headers_jsonin the expected parameter position, maintaining consistency with the trait signature.
412-436: LGTM!Existing tests correctly updated to handle the new 8-parameter runner signature and include
headers: NoneinPluginCallRequest. This maintains backward compatibility testing.
893-968: LGTM!Excellent test for header propagation. The test correctly:
- Creates a headers map with multiple header types
- Captures the
headers_jsonparameter passed to the runner- Verifies the JSON is valid and contains expected values
This provides good confidence in the serialization and propagation path.
970-1019: LGTM!Good complementary test verifying that
headers_jsonisNonewhen no headers are provided, ensuring the optional nature of headers is preserved throughout the call chain.docs/1.2.x/plugins/index.mdx (3)
11-19: LGTM! Clear feature documentation.The HTTP Headers feature is well-documented and appropriately placed in the feature list with a concise description.
430-480: Excellent documentation of the HTTP Headers feature.The section provides comprehensive coverage including format specification, usage patterns, common use cases, and important limitations. The examples demonstrate proper TypeScript patterns with optional chaining and array access.
546-549: Clear backwards compatibility documentation.The pattern detection rules are clearly documented, helping users understand which handler pattern provides access to headers and KV storage.
plugins/lib/plugin.ts (5)
197-201: LGTM! Correct type definition for HTTP headers.The
PluginHeaderstype properly represents multi-value HTTP headers asRecord<string, string[]>, which aligns with standard HTTP header handling. The JSDoc comment clearly explains the structure.
206-211: LGTM! Consistent interface extension.The
headersproperty is added toPluginContextas a required field, which is consistent with thekvproperty design. The implementation ensures headers are always provided (defaulting to{}when not supplied), preventing undefined access issues.
294-300: LGTM! Well-designed function signature extension.The
headersparameter is correctly added as optional at the end of the parameter list, maintaining backwards compatibility. The JSDoc is appropriately updated to document the new parameter.
336-343: LGTM! Correct header propagation logic.The implementation correctly provides headers to modern context handlers while excluding them from legacy handlers. The
headers ?? {}default ensures the context always has a valid headers object, preventing undefined access issues.
538-550: LGTM! Complete header propagation chain.The
runUserPluginfunction correctly accepts and forwards headers toloadAndExecutePlugin, completing the header propagation chain. The optional parameter design is consistent with the rest of the implementation.docs/plugins/index.mdx (1)
1-551: LGTM! Documentation is consistent.This file contains identical changes to
docs/1.2.x/plugins/index.mdx, maintaining consistency between versioned and current documentation. All the same quality observations apply: clear feature documentation, comprehensive examples, and accurate backwards compatibility notes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #566 +/- ##
=======================================
- Coverage 93.1% 92.3% -0.9%
=======================================
Files 244 250 +6
Lines 94702 89389 -5313
=======================================
- Hits 88236 82547 -5689
- Misses 6466 6842 +376
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
LGTM
Question around doc changes
docs/1.2.x/plugins/index.mdx
Outdated
| - ***Docker Integration***: Seamless development and deployment | ||
| - ***Comprehensive Error Handling***: Detailed logging and debugging capabilities | ||
|
|
||
| - **_Handler Pattern_**: Simple export-based plugin development |
There was a problem hiding this comment.
If these changes are going to be part of 1.3 probably we do not need to update these docs, just development versions as you already did.
cc @tirumerla to provide his 2 cents
There was a problem hiding this comment.
We don't have to update it for 1.2.x since the root files will be part of 1.3.x
Signed-off-by: Dylan Kilkenny <[email protected]>
Signed-off-by: Dylan Kilkenny <[email protected]>
Signed-off-by: Dylan Kilkenny <[email protected]>
Signed-off-by: Dylan Kilkenny <[email protected]>
| } | ||
| ``` | ||
|
|
||
| ## Fair Use Policy |
dylankilkenny
deleted the
plat-6970-forward-request-headers-to-relayer-plugin
branch
Summary
Testing Process
Checklist
Note
If you are using Relayer in your stack, consider adding your team or organization to our list of Relayer Users in the Wild!
Summary by CodeRabbit
New Features
Documentation
Configuration
Other
✏️ Tip: You can customize this high-level summary in your review settings.