[type:feature] add http record and replay plugin [#6108] - #6393
Open
hengyuss wants to merge 6 commits into
Open
Conversation
- 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
force-pushed
the
feat/record_and_replay_plugin
branch
from
June 22, 2026 07:47
1bb57e9 to
d657aa9
Compare
Aias00
requested changes
Aug 2, 2026
Aias00
left a comment
Contributor
There was a problem hiding this comment.
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) includingAuthorization,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 stripsContent-Length/Hostand re-sends the rest, includingAuthorization, to an admin-suppliedtargetHost. 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
RecordServerHttpResponseoverrides onlywriteWith, notwriteAndFlushWith, andcollect()is only called inside thewriteWithpath. SSE/chunked responses and error/empty-body paths that never callwriteWithproduce orphaned records (request tapped, response never collected, nothing uploaded). Consider movingcollect()to adoFinallyonchain.execute(mutatedExchange)and/or overridingwriteAndFlushWith.startReplaysubmits all records to a pool withAbortPolicy; ifsubmitthrowsRejectedExecutionExceptionit escapesreplayRecord's try/catch, leavingprogress.totalset but some records never run, soisFinished()is never true and the progress API reports a stuck replay until the 1h Caffeine eviction. Please account for rejected submits.- Bootstrap
application.ymlshipsshenyu.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 phoneslocalhost:9095withadmin/123456. Also the@ConditionalOnPropertykey (shenyu.plugins.http-record.enabled) doesn't match the yml key (shenyu.http-record.enabled), so the ymlenabled:trueis a no-op. Please don't ship credential/server defaults, and make the conditional key match. /record/upload-recordtakes@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/batchIntervalMsare read once before theconsume()loop andstart()is idempotent, so admin-side tuning of batch params doesn't take effect until restart.maxBodySizeisInteger(boxed) andnew 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.savedoesn't apply the sametaskIdpath-traversal guard thatloadRecordsdoes (asymmetry, admin-controlledtaskId).uploadreturns a plainStringwhile sibling endpoints returnShenyuAdminResult.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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.