fix(federated): prevent masked credentials from corrupting stored secrets

[nmgarza5]

Description

When an admin edits a federated connector, the GET /api/federated/{id} endpoint returns credentials with apply_mask=True, replacing secrets with •••••••••••• (U+2022 BULLET). The edit form loaded these masked values into state. On save, PUT /api/federated/{id} stored the masked string as the real credentials — permanently corrupting both client_id and client_secret. This caused Slack OAuth to fail with "Invalid client_id parameter" because the URL contained masked bullets instead of the real client ID.

Frontend fix

Edit form no longer pre-fills credential fields with masked values in edit mode. A credentialsModified flag tracks whether the user has actually typed into any credential field:

• If credentials are not modified: null is passed to updateFederatedConnector, which serializes as undefined (omitted from the JSON body). The Pydantic model defaults credentials to None. The API endpoint passes None to the DB function. The DB function's if credentials is not None: guard skips the entire credential update block — existing credentials are untouched.
• If credentials are modified: The new values are sent and validated normally.

Backend fix (defense-in-depth)

Both create_federated_connector and update_federated_connector now reject any credential value containing the mask character (U+2022 BULLET), preventing masked placeholders from ever being persisted.

Linear: CS-55

How Has This Been Tested?

Additional Options

• [Optional] Please cherry-pick this PR to the latest release version.
• [Optional] Override Linear Check

[github-actions]

Preview Deployment

Status	Preview	Commit	Updated
✅	https://onyx-preview-evculxiqj-danswer.vercel.app	7b30842	2026-04-02 20:19:00 UTC

[greptile-apps]

Greptile Summary

This PR fixes a credential-corruption bug where the federated connector edit form pre-populated credential fields with masked values returned by the API (GET /api/federated/{id} uses apply_mask=True), and those masked strings were subsequently saved back to the database on form submit.

Changes:

• Frontend (primary fix): FederatedConnectorForm no longer initialises credential fields with API data in edit mode. A credentialsModified flag gates required-field validation, credential-validation API calls, and whether the credential payload is included in the PUT request — sending null (serialised as undefined/omitted) when the user hasn't typed anything, which causes the backend's if credentials is not None: guard to skip the entire credential update block.
• Backend (defense-in-depth): _reject_masked_credentials() is called in both create_federated_connector and update_federated_connector to reject credential values matching either masked format produced by mask_string: the bullet-character short-string format ("••••••••••••") and the xxxx...xxxx long-string format.
• Constants: MASK_CREDENTIAL_CHAR and MASK_CREDENTIAL_LONG_RE extracted to constants.py to make both the guard and tests share a single source of truth.
• Tests: Unit tests cover all four masking scenarios, the clean-credentials path, and non-string field types.

Two minor gaps in the backend guard are noted: the "*****" placeholder used by mask_credential_dict for integer/float fields is not detected, and nested credential dicts are not recursively checked. Neither affects current Slack OAuth federated connectors (flat string credentials), but are worth hardening for future connector schemas.

Confidence Score: 5/5

• Safe to merge — the primary bug fix is robust and correctly addresses both the immediate corruption vector and the defense-in-depth layer.
• All previously raised concerns (long-string mask detection, test using artificial mask format) have been resolved in this revision. Remaining findings are P2 edge cases (integer mask format, non-recursive nested dict check) that don't affect the current Slack OAuth use case. The frontend and backend fixes are logically sound and well-tested.
• No files require special attention; the minor gaps in backend/onyx/db/federated.py are P2 quality suggestions only.

Important Files Changed

Filename	Overview
backend/onyx/configs/constants.py	Adds MASK_CREDENTIAL_CHAR (U+2022 bullet) and MASK_CREDENTIAL_LONG_RE (regex for "xxxx...xxxx" 11-char mask format) constants; both are correctly derived from mask_string() implementation in encryption.py.
backend/onyx/db/federated.py	Adds _reject_masked_credentials() guard covering both short-string (bullet) and long-string (xxxx...xxxx) mask formats; minor gap: does not detect the "***" integer mask or recurse into nested credential dicts, but neither affects current federated OAuth use cases.
backend/tests/unit/federated_connector/test_reject_masked_credentials.py	Well-structured unit tests covering fully-masked, long-string masked (real "1234...7890" format), partial masking, and clean credential paths; tests now use the actual mask_string output format.
web/src/components/admin/federated/FederatedConnectorForm.tsx	Frontend fix correctly prevents credential pre-population in edit mode; credentialsModified flag gates validation, required-field checks, and credential payload inclusion; placeholder UX is clear and helpful.

Sequence Diagram

sequenceDiagram
    participant Admin
    participant FederatedForm as FederatedConnectorForm (FE)
    participant API as PUT /api/federated/{id}
    participant DB as update_federated_connector (DB)

    Admin->>FederatedForm: Opens edit page
    Note over FederatedForm: credentials initialised to {}<br/>(NOT pre-filled with masked values)
    Note over FederatedForm: credentialsModified = false

    alt User does NOT touch credential fields
        Admin->>FederatedForm: Saves (config-only change)
        FederatedForm->>API: PUT { credentials: undefined, config: {...} }
        API->>DB: update(credentials=None)
        Note over DB: if credentials is not None → SKIPPED<br/>Existing secrets preserved ✅
    else User types new credential values
        Admin->>FederatedForm: Types into credential inputs
        Note over FederatedForm: credentialsModified = true
        Admin->>FederatedForm: Saves
        FederatedForm->>API: PUT { credentials: {client_id, client_secret}, config: {...} }
        API->>DB: update(credentials={...})
        DB->>DB: _reject_masked_credentials()<br/>• Rejects "••••••••••••" (bullet char)<br/>• Rejects "xxxx...xxxx" (11-char format)
        Note over DB: Credentials validated & persisted ✅
    end

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: backend/onyx/db/federated.py
Line: 47-68

Comment:
**`"*****"` integer-mask format not covered**

`mask_credential_dict` (in `encryption.py`) renders `int` and `float` credential values as the string `"*****"` (five asterisks, no bullet chars, only 5 chars). `_reject_masked_credentials` currently only detects the bullet-char format and the `xxxx...xxxx` (11-char) format produced by `mask_string`, so a masked numeric field would silently pass through.

For the Slack OAuth use-case all credential values are strings, so this is not a current regression. But if a future federated connector schema includes a numeric field, a masked `"*****"` value would bypass the guard.

Consider extending the check:

```python
MASK_NUMERIC_PLACEHOLDER = "*****"

def _reject_masked_credentials(credentials: dict[str, Any]) -> None:
    for key, val in credentials.items():
        if isinstance(val, str) and (
            MASK_CREDENTIAL_CHAR in val
            or MASK_CREDENTIAL_LONG_RE.match(val)
            or val == MASK_NUMERIC_PLACEHOLDER
        ):
            raise ValueError(
                f"Credential field '{key}' contains masked placeholder characters. Please provide the actual credential value."
            )
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: backend/onyx/db/federated.py
Line: 47-68

Comment:
**`_reject_masked_credentials` is not recursive**

`mask_credential_dict` recurses into nested dicts and lists, so a masked credential payload can contain nested masked values (e.g. `{"oauth": {"client_id": "abcd...wxyz"}}`). The current `_reject_masked_credentials` only iterates the top-level keys, so nested masked values would pass unchecked.

For the current federated connector schemas (flat Slack OAuth dicts) this is not an issue, but it creates a gap for future schemas that use nested credential structures. Consider a recursive traversal (analogous to how `mask_credential_dict` recurses).

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (2): Last reviewed commit: "fix(federated): address PR review commen..." | Re-trigger Greptile}

[github-actions]

🖼️ Visual Regression Report

Project	Changed	Added	Removed	Unchanged	Report
admin	0	0	0	165	✅ No changes
exclusive	0	0	0	8	✅ No changes