det101 commented on PR #18585:
URL:
https://github.com/apache/dolphinscheduler/pull/18585#issuecomment-5488002657
@SbloodyS Thank you for the careful review — this is a real data-loss path,
and your analysis is correct.
Unchecking **Sensitive** while leaving `******` must not persist the mask.
Restoring the original value with `sensitive = false` would also be unsafe, as
you noted, because the next read would return the secret as non-sensitive. We
followed that guidance.
The backend now rejects this for both workflow global params and task
`localParams`. `findInvalidSensitivePlaceholderProp` treats `true → false` with
value `******` as invalid and returns `REQUEST_PARAMS_NOT_VALID_ERROR`, asking
the user to enter an explicit replacement. Toggle sequences are judged only by
the final `(sensitive, value)` pair, so uncheck → check again → save with `true
+ ******` still keeps the original secret.
On the UI we chose not to auto-clear `******` when the checkbox is
unchecked. Clearing it would make it too easy to save an empty string and wipe
the secret by accident. Leaving the mask in the field means save fails until
the user types a real new value, or checks **Sensitive** again (keep-original).
Empty string remains a real empty value; only `******` means keep-original.
Unit tests cover the reject path and the toggle-back keep-original path.
As a related hardening, start/backfill also skips a startParam whose value
is `******` instead of overlaying it onto workflow globals. That includes
map-format `{"pwd":"******"}`, which loses `sensitive=true`. Please let us know
if you would prefer a different approach here.
Thanks again for the detailed write-up.
--
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]
