Skip to content

[type:feature] add http record and replay plugin [#6108] - #6393

Open
hengyuss wants to merge 6 commits into
apache:masterfrom
hengyuss:feat/record_and_replay_plugin
Open

[type:feature] add http record and replay plugin [#6108]#6393
hengyuss wants to merge 6 commits into
apache:masterfrom
hengyuss:feat/record_and_replay_plugin

Conversation

@hengyuss

Copy link
Copy Markdown
Contributor
  • Add shenyu-plugin-record module: intercepts HTTP request/response, captures body, headers, and metadata within a configurable time window
  • Add admin record service: receives recorded data via disruptor, persists to database, supports task replay
  • Add BodyWriter with max size limit to prevent OOM in gateway
  • Add HttpRecordCollector with batch upload and CRC32 checksum
  • Add MySQL/H2/OceanBase schema and upgrade SQL scripts
  • Add comprehensive unit tests for plugin, collector, and admin
  • Updated the corresponding frontend pages

Make sure that:

  • You have read the contribution guidelines.
  • You submit test cases (unit or integration tests) that back your changes.
  • Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

- Add shenyu-plugin-record module: intercepts HTTP request/response,
  captures body, headers, and metadata within a configurable time window
- Add admin record service: receives recorded data via disruptor,
  persists to database, supports task management and replay
- Add BodyWriter with max size limit to prevent OOM in gateway
- Add HttpRecordCollector with batch upload and CRC32 checksum
- Add MySQL/H2/OceanBase schema and upgrade SQL scripts
- Add comprehensive unit tests for plugin, collector, and admin
@hengyuss
hengyuss force-pushed the feat/record_and_replay_plugin branch from 1bb57e9 to d657aa9 Compare June 22, 2026 07:47

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-merge review of #6393 (http record & replay plugin). CI is green and the unit coverage is solid, but before merging a security-sensitive traffic-recording feature I'd like to see a few things addressed.

Blocker

  • The plugin records every request/response header verbatim (RecordUtils.getHeaders) including Authorization, Cookie, Set-Cookie, X-Access-Token, and persists them as plaintext JSONL at the default path /tmp/shenyu-records (world-readable on Linux). Replay (HttpRecordServiceImpl.buildReplayHeaders) only strips Content-Length/Host and re-sends the rest, including Authorization, to an admin-supplied targetHost. There is no redaction config, no at-rest encryption, and no access control on the storage dir beyond admin Shiro. For a feature whose purpose is capturing real traffic, shipping with no secret-handling hardening and no opt-out is a data-breach risk. Please add a configurable sensitive-header denylist (redact on record, keep on replay if opted in), a non-world-readable default storage path, and a loud docs/README warning that recorded data contains credentials.

Should-fix

  • RecordServerHttpResponse overrides only writeWith, not writeAndFlushWith, and collect() is only called inside the writeWith path. SSE/chunked responses and error/empty-body paths that never call writeWith produce orphaned records (request tapped, response never collected, nothing uploaded). Consider moving collect() to a doFinally on chain.execute(mutatedExchange) and/or overriding writeAndFlushWith.
  • startReplay submits all records to a pool with AbortPolicy; if submit throws RejectedExecutionException it escapes replayRecord's try/catch, leaving progress.total set but some records never run, so isFinished() is never true and the progress API reports a stuck replay until the 1h Caffeine eviction. Please account for rejected submits.
  • Bootstrap application.yml ships shenyu.http-record.{enabled:true, serverLists:http://localhost:9095, username:admin, password:123456} by default, and the DB rows enable plugin 67 (enabled=1) on fresh install and upgrade. So every gateway with the starter phones localhost:9095 with admin/123456. Also the @ConditionalOnProperty key (shenyu.plugins.http-record.enabled) doesn't match the yml key (shenyu.http-record.enabled), so the yml enabled:true is a no-op. Please don't ship credential/server defaults, and make the conditional key match.
  • /record/upload-record takes @RequestBody byte[] with no request-size limit; a large/malicious batch can OOM the admin (the CRC check runs after the full body is in memory).
  • batchSize/batchIntervalMs are read once before the consume() loop and start() is idempotent, so admin-side tuning of batch params doesn't take effect until restart. maxBodySize is Integer (boxed) and new BodyWriter(maxBodySize) will NPE if admin pushes an explicit null — please null-guard it.
  • No e2e test for the record→upload→store→load→replay round-trip (the feature's core contract), and no large-body or streaming test through the plugin path.

Nits

  • LocalJsonFileHttpRecordRepository.save doesn't apply the same taskId path-traversal guard that loadRecords does (asymmetry, admin-controlled taskId).
  • upload returns a plain String while sibling endpoints return ShenyuAdminResult.
  • BodyWriter.output() sets recorded body to "Write failed: ..." on exception, leaking error text into recorded data.

One open question on ordering: HTTP_RECORD(215) sits between URI(205)/WEB_CLIENT(210) and MODIFY_RESPONSE(220), so it records post-routing/pre-modify-response traffic. If the intent is to capture raw client traffic, the order should be < 200; if post-transform is intended, 215 is fine but misses MODIFY_RESPONSE output. Please confirm the intended capture point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants