Skip to content

[type:fix] forward configured sign app parameter - #7199

Open
dengliming wants to merge 1 commit into
apache:masterfrom
dengliming:fix-6634-forward-app-param
Open

dengliming wants to merge 1 commit into
apache:masterfrom
dengliming:fix-6634-forward-app-param

Conversation

@dengliming

Copy link
Copy Markdown
Member

Fixes #6634.\n\nStore the verified app parameter on the exchange and have SignPlugin build the request carrying the APP_PARAM header before invoking the downstream chain. This covers both regular and request-body signing paths.\n\nTests: ./mvnw -q -pl shenyu-plugin/shenyu-plugin-security/shenyu-plugin-sign -am -DskipTests=false -Dcheckstyle.skip=false -Dtest=SignPluginTest,SignServiceVersionTwoTest -DfailIfNoTests=false test

@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.

Good fix — the original line was a no-op by construction.

exchange.getRequest().mutate().headers(...).build() builds a new request and discards it; the result was never assigned back to the exchange, so the appParam header never reached downstream plugins. Moving the value into exchange.getAttributes() and materialising the header on a properly mutated exchange in SignPlugin actually forwards it.

What I checked:

  • Both execution paths in SignPlugin#doExecute are covered: the no-body path via chain.execute(addAppParamHeader(exchange)), and the body-rewrite path via .map(this::addAppParamHeader) before flatMap(chain::execute). Easy to miss one; both are there.
  • ObjectUtils is already imported in SignPlugin (Spring's, already used for the rule-handler null check), so ObjectUtils.isEmpty(appParam) compiles and handles the blank-string case.
  • Constants.APP_PARAM is used consistently as both the attribute key and the header name, and addAppParamHeader returns the exchange unchanged when the attribute is absent — so plugins that never set it see no behaviour change.
  • Tests cover both sides: the service stores the attribute, and the plugin converts it into a request header on the exchange handed to the chain.

Non-blocking: storing an app parameter in a request header means it will be propagated to the upstream service. That looks intentional here (it's the point of the feature), but it's worth confirming this is desired for all deployments, since header injection is visible to backends.

This branch has not been deployed

No deployments
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.

[BUG] ComposableSignService.handleExchange silently discards the APP_PARAM header mutation

2 participants