unbridled-41 opened a new pull request, #5000:
URL: https://github.com/apache/rocketmq-dashboard/pull/5000
## Problem
`getGeneralSettings` (base `1ef5d860`, `SettingsService.java:112-127`)
redacts `dingtalkWebhook`/`smsWebhook` to the literal `"******"` for non-admin
sessions — that is the #2339/#2341 redaction doing its job on the read side.
The **write side was never taught about it**:
- The console echoes every field it received:
`GeneralSettingsTab.buildPayload` (`GeneralSettingsTab.tsx:54`/`:58`) always
includes both webhooks, and every save re-reads the current (redacted) record
first — `loadFreshSettings` then `saveGeneralSettings(buildPayload({ ...base,
...patch }))` (`:104-144`). The form is even pre-filled with `"******"`.
- `saveGeneralSettings` (`SettingsService.java:130-166`, base) preserves
blank `apiKey`/`dingtalkSigningSecret` but has **no handling for the webhook
marker**, so `"******"` goes straight to
`settingsRepository.saveGeneralSettings`.
Result: any save from a non-admin session — even one that only changes the
session timeout — silently overwrites both webhook URLs with six asterisks.
Every subsequent DingTalk/SMS notification then fails against a URL that is
literally `******`, with no error surfaced to the writer (the save succeeds).
## Root cause
The redaction is not round-trip safe: the GET redacts, the client echoes,
and the POST does not recognise the marker the way it already recognises a
blank secret.
## Fix
In `saveGeneralSettings`, when the incoming webhook equals
`REDACTED_NOTIFICATION_WEBHOOK`, restore the stored value instead of persisting
the marker (mirroring the existing blank-secret preservation). A webhook
genuinely retyped to a different URL still replaces the stored one — pinned by
a dedicated test.
## Scoring (AGENTS.md)
PRIORITY **62** = impact 28 (silent, destructive corruption of the
production notification config; alerting dies) + reach 10 (settings save path)
+ reproducibility 14 (deterministic for any non-admin save; admin saves are
unaffected because the API returns unredacted values to them) + maintainability
value 10 (completes the #2339 redaction). FIX_CONFIDENCE **90**: server-side
sentinel check, same shape as the two existing preservation branches; covered
against the false-positive case (a real URL is never `******`-equal).
## Tests
- New:
`saveGeneralSettingsShouldKeepExistingWebhooksWhenTheRedactedMarkerIsEchoedTest`
— pre-fix fails on `expected:
"https://oapi.dingtalk.com/robot/send?access_token=secret"` (the marker was
persisted); new
`saveGeneralSettingsShouldReplaceAWebhookThatWasActuallyRetypedTest` guards the
marker against locking a real retype out.
- Post-fix: `mvn -o -Dtest=SettingsServiceTest test` -> **Tests run: 45,
Failures: 0 - BUILD SUCCESS**; `SettingsControllerTest` -> **23/23**.
## Risk
Low. One writer-side branch, no schema/API change. A deployment whose stored
webhook is *already* the literal `"******"` (i.e. already corrupted by this
bug) keeps it until an operator retypes a real URL — the retype path is
explicitly tested to keep working.
--
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]
