Duansg commented on PR #4281:
URL: https://github.com/apache/hertzbeat/pull/4281#issuecomment-5242822055
Swapping the parameter definitions for non-static monitors drops validation
of the app's own params — including credential encryption
validate() previously always resolved definitions from monitor.getApp().
This PR changes it to:
String parameterDefinitionApp = isStatic ? monitor.getApp() :
monitor.getScrape();
List<ParamDefineInfo> paramDefines =
appService.getAppParamDefines(parameterDefinitionApp);
...
checkJobFields(parameterDefinitionApp);
The motivation is clear and correct — __sd_token__ lives in app-http_sd.yml,
so without this it never enters the loop and the mask-restore branch can't
fire. But this is a swap rather than an addition, and the submitted payload
contains both sets of params. monitor-new.component.ts:285:
params: info.params.concat(info.advancedParams).concat(info.sdParams),
So for any non-static monitor, the loop now iterates the sd definitions only
and never sees the app's own params. Three things follow.
1. App credentials are stored in plaintext and returned unmasked. The loop
body ends with:
if (param != null && StringUtils.hasText(param.getParamValue())) {
paramValidatorManager.validate(paramDefine, param);
}
PasswordParamValidator is the only call site of AesUtil.aesEncode under
hertzbeat-manager/src/main, and it is also what sets
param.setType(CommonConstants.PARAM_TYPE_PASSWORD). If the app's password
params are never visited:
- the value is persisted in cleartext, and
- Param.type is never set to PARAM_TYPE_PASSWORD, so the new masking in
MonitorParam.fromEntity — which keys off exactly that — never applies either.
Create a mysql (or ssh, redis, oracle …) monitor with scrape=http_sd and its
password goes to the database in the clear and comes back out of the API in the
clear. That inverts the goal of this PR on the very path it was extended to
cover.
2. Required-param validation is lost for the app's fields (port, username,
and so on) on non-static monitors. The pre-existing if (!isStatic &&
"host".equals(field)) continue; is itself evidence that the loop was always
meant to run over the app's definitions, with host as the single
discovery-supplied exception.
3. checkJobFields now checks the wrong template. It walks job.getMetrics()
field names through JexlCheckerUtil.verifyKeywords / verifyStartCharacter /
verifySpaces. Passing http_sd instead of the real app means the app template's
field names are no longer keyword-checked on this path.
Suggested fix
Concatenate rather than replace — keep monitor.getApp() definitions and
append the monitor.getScrape() ones when the monitor is not static, and pass
monitor.getApp() to checkJobFields as before (optionally checking the scrape
define too). The sd definitions use __sd_* / __nacos_sd_* style field names
with no overlap against any app define, so a plain concatenation needs no
dedupe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
