[DSIP-105][API][UI] Add Property.sensitive with API/UI masking

[det101]

Summary

Implements #18586 (subtask of DSIP-105 / #17937): add Property.sensitive and mask sensitive values as ****** on API/UI, with keep-original merge on write/start.

DB and in-process objects stay plaintext. HTTP responses are copy-then-masked in controllers (no ResponseBodyAdvice).

In scope (#18586)

• Property.sensitive (default false; missing JSON field is false)
• HTTP mask in controllers via typed SensitivePropertyUtils.mask(...) overloads (WorkflowDefinition / DagData / WorkflowInstance / TaskDefinition / TaskInstance / view-variables localParams). Covers query, version lists, and create/update replies. Copies are masked; Service/DAO objects are not mutated.
• Write-path merge: only ****** means keep the DB original; empty / null is a real empty value
• Create rejects ****** (no previous value). Update rejects true → false with placeholder-only ****** (would persist the mask). false → true with ****** is keep-original when an existing value exists
• UI: Sensitive checkbox on workflow global params and task localParams; reload echoes ******
• Start / backfill: API restoreStartParams replaces ****** with the definition global plaintext before the command is sent. Master does not interpret ******; the DB never stores the placeholder

Out of scope (follow-up subtasks)

• [DSIP-105][Feature][API] Encrypt sensitive definition params with PasswordUtils #18587: definition-time encrypt/decrypt via PasswordUtils
• [DSIP-105][Feature][Worker] Mask sensitive parameter values in task logs #18588: Worker stdout dynamic redaction + cleanup

Related

• Fixes [DSIP-105][Feature][API] Add Property.sensitive and mask values in API/UI #18586
• Refs [DSIP-105][Feature][Parent] Sensitive Variable Support — Masking in API/UI & Encrypted Storage #17937 (parent DSIP; do not close)

Test plan

• Unit: PropertySensitiveUtilsTest, SensitivePropertyUtilsTest
• Local standalone: create workflow with sensitive global + local params
• API query returns masked ****** (globalParamList / localParams / view-variables)
• Create/update Result is masked without mutating the persisted object
• UI save modal shows Sensitive checked and value ****** after reload
• CI

Verification screenshots

UI — reload workflow: sensitive global param masked as ******

UI — save modal: Sensitive checkbox available

API — same workflow masked on query / view-variables

=== Workflow Definition Query (masked) ===
globalParamList: [{"prop": "db_pwd", "direct": "IN", "type": "VARCHAR", "value": "******", "sensitive": true}]
task localParams: [{"prop": "token", "direct": "IN", "type": "VARCHAR", "value": "******", "sensitive": true}]

=== view-variables (masked) ===
{
  "globalParams": [
    {
      "prop": "db_pwd",
      "direct": "IN",
      "type": "VARCHAR",
      "value": "******",
      "sensitive": true
    }
  ],
  "localParams": {
    "shell_node_1": {
      "localParamsList": [
        {
          "prop": "token",
          "direct": "IN",
          "type": "VARCHAR",
          "value": "******",
          "sensitive": true
        }
      ],
      "taskType": "SHELL"
    }
  }
}

Screenshot branch (fork only, not part of review diff): det101:pr1-17937-verification-screenshots

[det101]

UI / API verification screenshots (PR1)

1) Reload existing sensitive workflow — global params masked

Save modal shows:

• db_pwd value = ******
• Sensitive checkbox checked

2) Create / save modal — Sensitive checkbox present

3) API mask proof (same workflow)

=== Workflow Definition Query (masked) ===
globalParamList: [{"prop": "db_pwd", "direct": "IN", "type": "VARCHAR", "value": "******", "sensitive": true}]
task localParams: [{"prop": "token", "direct": "IN", "type": "VARCHAR", "value": "******", "sensitive": true}]

=== view-variables (masked) ===
{
  "globalParams": [
    {
      "prop": "db_pwd",
      "direct": "IN",
      "type": "VARCHAR",
      "value": "******",
      "sensitive": true
    }
  ],
  "localParams": {
    "shell_node_1": {
      "localParamsList": [
        {
          "prop": "token",
          "direct": "IN",
          "type": "VARCHAR",
          "value": "******",
          "sensitive": true
        }
      ],
      "taskType": "SHELL"
    }
  }
}

[det101]

Verification screenshots (hosted on fork evidence branch)

Reload existing sensitive workflow — value masked + Sensitive checked

Save modal — Sensitive checkbox present

API mask proof

=== Workflow Definition Query (masked) ===
globalParamList: [{"prop": "db_pwd", "direct": "IN", "type": "VARCHAR", "value": "******", "sensitive": true}]
task localParams: [{"prop": "token", "direct": "IN", "type": "VARCHAR", "value": "******", "sensitive": true}]

=== view-variables (masked) ===
{
  "globalParams": [
    {
      "prop": "db_pwd",
      "direct": "IN",
      "type": "VARCHAR",
      "value": "******",
      "sensitive": true
    }
  ],
  "localParams": {
    "shell_node_1": {
      "localParamsList": [
        {
          "prop": "token",
          "direct": "IN",
          "type": "VARCHAR",
          "value": "******",
          "sensitive": true
        }
      ],
      "taskType": "SHELL"
    }
  }
}

[SbloodyS]

I found two blocking issues:

1. [P1] Sensitive values are still exposed by the definition-version APIs

The new masking is applied to the current workflow/task definition query paths, but the version-list endpoints still return the persisted entities directly:

• GET /projects/{projectCode}/task-definition/{code}/versions returns TaskDefinitionLog.taskParams.
• GET /projects/{projectCode}/workflow-definition/{code}/versions returns WorkflowDefinitionLog.globalParams.

Both SQL projections include these fields, and the service methods put the records into the response without masking them. Therefore, a parameter with sensitive=true can still be read in plaintext by querying its version history, which violates the acceptance criterion that sensitive values must never be returned through external APIs.

Please either mask the returned version entities using the same deep-copy strategy or use dedicated summary DTOs that omit these fields. Regression tests should cover both version endpoints.

2. [P1] Updating an old workflow instance can restore the secret from the wrong task version

WorkflowInstanceServiceImpl.mergeSensitiveLocalParams() resolves ****** using taskDefinitionDao.queryByCodes(), which loads the current task definitions. However, the workflow-instance detail shown to the user may have been generated from an older workflow/task version.

If the task's sensitive value changed after that instance was created, editing and saving the old instance will silently replace ****** with the latest task definition's value instead of preserving the value belonging to the displayed instance version.

Please resolve each original value using the matching task definition code and version associated with the instance/submitted task definition, rather than querying only the current definition by code. A regression test should cover updating an old instance after its task secret has changed in a newer version.

[det101]

@SbloodyS Thanks for the review. Both P1s are addressed, with a few extra hardening changes.

1. Version-list APIs
   GET .../task-definition/{code}/versions and GET .../workflow-definition/{code}/versions now mask sensitive taskParams / globalParams. Following the deep-copy approach you suggested, masking is applied to JSON copies (copyAndMask*) so MyBatis-mapped log entities are not mutated in place. Regression tests cover both endpoints and assert the original records stay unmasked.

2. Old workflow instance update
   WorkflowInstanceServiceImpl.mergeSensitiveLocalParams no longer uses taskDefinitionDao.queryByCodes(). It restores ****** from TaskDefinitionLog by code + version. To avoid N+1 queries it uses a single queryByTaskDefinitions call, with a List of keys (TaskDefinition.equals() ignores code/version, so a Set would collapse distinct versions). A regression test updates an old instance after the current task secret has changed and asserts the matching version value is kept.

3. Extra hardening

   • maskWorkflowDefinition / maskTaskDefinition now clear the cached globalParamMap / taskParamMap, because setGlobalParams does not rebuild the map and getters could still return plaintext.
   • Workflow definition update still merges against the current definition via queryByCodes(), because that path edits the live definition rather than an old instance.

[SbloodyS]

Disabling sensitive can silently overwrite the real value with ******

There is still a data-loss path when an existing sensitive parameter is changed to non-sensitive:

1. The API returns the parameter as value = "******", sensitive = true.
2. The user unchecks Sensitive without editing the value.
3. The UI submits value = "******", sensitive = false.
4. findInvalidSensitivePlaceholderProp() skips the parameter because it is no longer sensitive.
5. mergeSensitiveValuePlaceholder() also skips it for the same reason.
6. The update therefore persists the literal ****** and permanently overwrites the original value.

This affects both workflow global parameters and task localParams. Besides losing the secret, subsequent task executions will receive ****** instead of the configured credential.

Please reject ****** when an existing sensitive parameter is being changed to non-sensitive, requiring the user to enter an explicit replacement value. This validation must be enforced by the backend rather than only by the UI. Restoring the original value while setting sensitive = false would not be safe either, because the next read would expose that value as non-sensitive.

[det101]

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

[SbloodyS]

Sensitive global parameters are exposed by create/update responses

The read/query endpoints now mask sensitive parameters, but the write endpoints still return the in-memory WorkflowDefinition containing the merged plaintext globalParams:

• WorkflowDefinitionServiceImpl#createWorkflowDefinition() returns the object created from the submitted plaintext parameters.
• WorkflowDefinitionServiceImpl#updateWorkflowDefinition() returns the object after mergeGlobalParams() has restored the real value.
• WorkflowInstanceServiceImpl#updateWorkflowInstance() also returns a WorkflowDefinition containing the merged plaintext value.

The corresponding controllers serialize these objects directly in their Result<WorkflowDefinition> responses. Therefore, creating a sensitive global parameter or updating it with a new value returns the real value through an external API, which violates the acceptance criterion that sensitive parameters must only be returned as ******.

Please return a masked deep copy from these external write paths, for example by applying copyAndMaskWorkflowDefinition() after persistence. Do not mask the object before it has finished being persisted or reuse a masked object for execution.

Please add regression coverage for:

1. Creating a workflow with a sensitive global parameter.
2. Updating a workflow with a new sensitive value.
3. Updating a workflow instance containing a sensitive global parameter.

Each response should contain ******, while the persisted/internal value must remain unchanged and usable for execution.

[det101]

@SbloodyS Thank you — you are right that create/update responses were still serializing the merged plaintext WorkflowDefinition.

We did not only wrap those three service returns. Query-time masking was too easy to miss on write endpoints (and on any new Result that carries Property), so masking now happens at the HTTP outbound boundary:

• SensitivePropertyResponseAdvice copy-then-masks Result.data when it contains workflow/task/instance Property payloads. That includes createWorkflowDefinition, updateWorkflowDefinition, updateWorkflowInstance, version lists, and the existing query/view-variables paths.
• Query/write services keep returning plaintext. Persistence and in-process objects are not mutated; only a JSON copy is masked before Jackson writes the body.
• Unrelated Result payloads (login, cluster, user, …) are left unchanged.

Acceptance is unchanged: API/UI still show ******; the metadata DB is still plaintext in this PR (encryption remains #18587).

[det101]

@SbloodyS Thank you — the create/update plaintext leak is addressed at the HTTP outbound boundary (SensitivePropertyResponseAdvice copy-then-masks Result.data after persistence). Service / persisted objects stay plaintext for execution.

We also added the three regressions you asked for:

1. Creating a workflow with a sensitive global parameter
2. Updating a workflow with a new sensitive value
3. Updating a workflow instance that already has a sensitive global parameter

Each asserts the HTTP Result contains ****** while the object passed to saveWorkflowDefine remains the real value.

[SbloodyS]

mask(WorkflowInstance) masks globalParams, varPool, and dagData, but leaves commandParam unchanged. The trigger transformer restores ****** to plaintext, WorkflowManualTrigger serializes those values into commandParams, and RunWorkflowCommandHandler persists that JSON on the workflow instance. As a result, GET /workflow-instances/{id} can still expose sensitive start-parameter values through commandParam. Please mask ICommandParam.commandParams in the response copy and add regression coverage for this endpoint.

[SbloodyS]

Please change your PR's description since the implementation has been changed.

[ruanwenjun]

mask(WorkflowInstance) masks globalParams, varPool, and dagData, but leaves commandParam unchanged. The trigger transformer restores ****** to plaintext, WorkflowManualTrigger serializes those values into commandParams, and RunWorkflowCommandHandler persists that JSON on the workflow instance. As a result, GET /workflow-instances/{id} can still expose sensitive start-parameter values through commandParam. Please mask ICommandParam.commandParams in the response copy and add regression coverage for this endpoint.

+1, need to mask the commandParams at controller， but the command params doesn't have type, need to find the type from localparam/global param.

[det101]

@SbloodyS Thanks for catching these.

1. commandParam leak — addressed in ea46d67873. mask(WorkflowInstance) now parses ICommandParam, masks commandParams on the response copy, and leaves the persisted/in-process value unchanged. Added SensitivePropertyUtilsTest.maskWorkflowInstanceMasksCommandParam.

2. PR description — updated to match the current design: controller typed mask(...) (no ResponseBodyAdvice), create/update placeholder rules, and API restoreStartParams (Master does not interpret ******).

Please take another look when you have a chance.

[SbloodyS]

The PR description matches the current controller masking implementation and correctly states that encryption is out of scope. However, its false → true plus ****** behavior conflicts with #18586, which requires that case to be rejected. Please align the issue's acceptance criteria with the intended behavior.

[SbloodyS]

I don't think it's a good way to abuse @SuppressWarnings("unchecked"). We should try to avoid this problem in the official code.

[det101]

Thanks for the comment.

@SuppressWarnings("unchecked") is here because the existing service method only returns a raw Result. getData() is Object, so masking the page requires (PageInfo<TaskDefinitionLog>) result.getData(). The same pattern is used in queryWorkflowDefinitionVersions and queryTaskListPaging.

I see two ways to remove it. Which do you prefer?

1. Change the service interface and implementation to Result<PageInfo<...>>, so the controller can use result.getData() without a cast. This matches the data the method already returns, but it touches the interface, implementation, and callers/tests.

2. Keep the raw Result and check with instanceof in the controller before masking. This avoids a service signature change. On Java 8, instanceof does not narrow the type, so a cast is still required after the check, and the controller code is more verbose.

I will update all three paging endpoints the same way once you confirm.

[SbloodyS]

It is the best way to return a concrete type in the service.

[det101]

Updated to option 1: typed the three service methods as Result<PageInfo<...>> (queryTaskDefinitionVersions, queryWorkflowDefinitionVersions, queryTaskListPaging) so the controllers can call result.getData() without @SuppressWarnings("unchecked"). This matches the existing paging API style in this module.

[SbloodyS]

GET /workflow-instances/{id} and GET /workflow-instances/{id}/view-variables still return values in plaintext.

[det101]

Thanks for the catch.

The plaintext came from start/command merge: Map-style startParams build Property objects without sensitive=true, then Master overwrites same-named workflow globals with that whole object, so instance globalParams / commandParam kept the value but lost the flag. Response masking only looks at isSensitive(), so the values leaked.

Fix (write path, before commandParam is persisted) — in restoreStartParams (used by start / backfill transformers):

1. Same name as a workflow global — copy all attributes from the global (sensitive, direct, type, …) and only override value. (****** still restores the global plaintext for execution.)
2. Not matching any global — treat as a normal start param (sensitive=false); it is not masked as sensitive.

After that, Master’s existing whole-object merge keeps the inherited sensitive flag on instance globals, so GET /workflow-instances/{id} and view-variables mask correctly again via the existing controller mask.

Also masked commandParam on monitor listCommand / listErrorCommand the same way as other response paths.

Verified with a Map-style start ({"pwd":"..."} without sensitive) on local standalone: instance detail and view-variables return ****** with sensitive:true, no plaintext.