Support Microsoft Ads refresh token rotation

[AnastasiaKhukhra]

• Added Microsoft Ads refresh token rotation support.
• When Microsoft returns a new refresh token, the connector sends it through updateCredentials.
• Backend stores the rotated token as generated_refresh_token in connector_source_credentials.
• The original user-provided refresh token is not overwritten.
• On the next run, backend injects generated_refresh_token as runtime-only GeneratedRefreshToken, and Microsoft Ads uses it before falling back to the original refresh token.
• Added backend handling for connector updateCredentials messages with an allowlist: only generated_refresh_token can be saved from runtime messages.
• Saved rotated tokens even if the connector run fails after token refresh.
• Preserved/copied generated_refresh_token for externalized secret configs, but kept it hidden from API responses.
• Runtime token persistence requires _secrets_id or _source_credential_id; legacy inline configs without credential references are skipped quietly.

[zapolsky]

🔴 Blocked issues — see inline comments. Posted by automated OWOX review (multi-agent + comprehensive + architecture passes). Both findings were verified by reading the actual connector platform / persistence code.

[zapolsky]

[Bug — token rotation is silently broken] GeneratedRefreshToken is injected as a bare string here (and identically at lines 102 and 113), but the Microsoft Ads connector reads it as this.config.GeneratedRefreshToken?.value (packages/connectors/src/Sources/MicrosoftAds/Source.js:607). The connector platform's AbstractConfig.addParameter (packages/connectors/src/Core/AbstractConfig.js:77) assigns a bare string straight to this[name] — it does not wrap it in { value: ... }. So this.config.GeneratedRefreshToken.value === undefined, and the connector falls back to the original (already single-use-invalidated) refresh token. The rotated token is effectively dropped on every subsequent run, so the whole feature is a no-op (and auth will break once the provider invalidates the original token).

Fix: inject the { value } shape used by every other param, in all three branches: result.GeneratedRefreshToken = { value: generatedRefreshToken }; (and resolvedConfig.GeneratedRefreshToken = { value: generatedRefreshToken };, and { GeneratedRefreshToken: { value: generatedRefreshToken } }).

[zapolsky]

Refuted on re-verification — please disregard this finding (and do not apply its suggested fix).

The trace above stops two hops short. The full chain: injection happens before DTO construction (connector-source-config.service.ts:33-43 — injectSecrets / injectOAuthCredentials first, then new SourceConfigDto(...)) → the SourceConfigDto constructor runs every top-level value through recursiveValueTransform (packages/connectors/src/Core/Dto/ConfigDto.js:26), which wraps bare strings into { value: ... } → that already-wrapped shape is what gets serialized into OW_CONFIG (connector-process-spawner.service.ts:48) → the runner (connector-runner.js:109-116) hands pre-wrapped values to AbstractConfig.addParameter, so it has nothing left to wrap. At runtime this.config.GeneratedRefreshToken?.value (externalized path) and this.config.AuthType?.items?.GeneratedRefreshToken?.value (OAuth path) both resolve correctly — the feature is live.

Applying the suggested fix (result.GeneratedRefreshToken = { value: ... } in the injector) would double-wrap ({value: {value: 'tok'}}) and actually break the feature.

The real, verified bug in this area is different: the in-run update writes top-level while the OAuth path reads AuthType.items.* first — see the new inline comment on packages/connectors/src/Sources/MicrosoftAds/Source.js:319.

[zapolsky]

[Security — unconstrained write to encrypted credentials] message.credentials is typed z.record(z.string(), z.unknown()) (message-credentials-update.schema.ts:7) — i.e. arbitrary keys/values — and is persisted wholesale via updateCredentialFields → existing.credentials = { ...existing.credentials, ...updates } (connector-source-credentials.service.ts:237), with no allow-list. A misbehaving or compromised connector process (it parses arbitrary external HTTP responses) can therefore emit e.g. {type:'updateCredentials', credentials:{ ClientSecret: '...' }} and overwrite any field in the encrypted ConnectorSourceCredentials row. The rotation feature only ever needs generated_refresh_token.

Fix: restrict the persisted keys to an explicit allow-list (just generated_refresh_token) before calling updateCredentialFields, and tighten the schema from z.record(z.string(), z.unknown()) to the single expected field.

[zapolsky]

🔵 Review notes (non-blocking)

Suggestions and questions — none of these block merge (the blockers are in the inline review).

• apps/backend/src/data-marts/services/connector/connector-source-credentials.service.ts:237 — Concurrency / lost update: updateCredentialFields does an unguarded read-modify-write and the entity has no @VersionColumn. If two runs of the same data mart overlap, both rotate the provider's single-use refresh token and the loser's save clobbers the winner's — persisting an already-invalidated token and permanently breaking the credential. If concurrent same-mart runs are possible, add optimistic locking (@VersionColumn) or do the merge inside a row-locked transaction.
• apps/backend/src/data-marts/services/connector/connector-executor.service.spec.ts — No test asserts that a CREDENTIALS_UPDATE message triggers updateCredentialFields, nor covers getCredentialIdForConfig / findSourceCredentialId (_secrets_id precedence vs nested _source_credential_id discovery, and the no-credential-id warn path). This is the most security-sensitive new logic — please add coverage.
• packages/connectors/src/Sources/MicrosoftAds/Source.js — Connector (deferred): out of scope for the backend/NestJS rules — please run connector-conventions. Two notes: _updateCredentials logs the rotated token to stdout (this is the IPC channel; the backend doesn't raw-log it on the happy path, so not a leak there), and Source.js:607 reads GeneratedRefreshToken?.value, which must match whatever shape the backend injection ends up using (see the blocking inline comment).
• Changeset: this is a user-facing connector change (Microsoft Ads offline_access scope + refresh-token rotation) but there's no .changeset/*.md entry — please add one ('owox': minor).

_{Automated OWOX review · multi-agent + comprehensive + architecture passes.}

[zapolsky]

🔴 Blocked issues — see inline comments (3 new, adversarially verified with evidence chains). These should be addressed before merge, but this review does not formally block; a human owns the merge decision. Note: the earlier inline comment on connector-credential-injector.service.ts:227 (bare-string injection) was re-verified and is refuted — see the reply in that thread. Posted by automated OWOX review.

[zapolsky]

[Bug — verified] Stale generated_refresh_token permanently shadows a fresh user-provided token; nothing ever clears it.

Evidence chain: Source.js:273 always prefers generatedRefreshToken || originalRefreshToken with no invalid_grant fallback (lines 309–315 only retry across scopes) → this preservation branch (lines 212–217) re-adds generated_refresh_token on every user secret save — incoming secrets always omit it, because mask() never exposes it (connector-secret.service.ts:495-497) and injectSecretsAtPaths skips it (connector-secret.service.ts:171-173), so it can never be re-extracted from a user edit → the only writer is updateCredentialFields after a successful run (connector-executor.service.ts:336-344), so once runs fail the row can never heal itself.

Trigger: the generated token is revoked at Microsoft (consent revoked, password reset, 90-day inactivity) → runs fail → user re-authorizes and pastes a fresh RefreshToken → the dead generated token is preserved and still preferred → permanent failure with no user-visible cause (the field is hidden from the UI by design) and no remedy short of deleting the configuration item or editing the DB row.

Fix: clear generated_refresh_token here when an incoming refresh-token secret value differs from the stored one (i.e. the user re-credentialed; a ********** mask doesn't count). Defense in depth: in Source.js getAccessToken, on invalid_grant with the generated token retry once with the original token.

[zapolsky]

[Bug — verified] Rotated token is discarded when the run fails — persistence must not be gated on success.

Evidence chain: Microsoft rotates the refresh token at the token exchange near the start of the run (Source.js:294-306, emit at 316-318) → credentialUpdates is accumulated (connector-executor.service.ts:261-263) → but persisted only inside if (success) (lines 335-344). Any later failure — storage error, API quota, abort, non-zero exit (catch at line 354) — silently drops the freshest token.

Impact: the whole feature exists to survive the sliding inactivity window; a data mart failing for two weeks near token expiry loses exactly the tokens that would have kept the credential alive. Correctness currently rests on Microsoft's undocumented family-of-tokens semantics (superseded tokens staying valid); if MS tightens rotation, every failed run permanently bricks the credential.

Fix: persist credentialUpdates regardless of run outcome — eagerly in the CREDENTIALS_UPDATE message handler (the way REQUESTED_DATE already persists at lines 240-259), or in the finally block. Credential persistence is a side effect that is valid the moment the message is received, not a run outcome.

[zapolsky]

[Bug — verified] Write/read asymmetry: the in-run token update writes to a slot the OAuth read path never prefers, so subsequent token exchanges in the same run reuse the pre-rotation token.

Evidence chain: for OAuth-credentialed configs the backend injects the token under AuthType (connector-credential-injector.service.ts:111-114) → SourceConfigDto.recursiveValueTransform (packages/connectors/src/Core/Dto/ConfigDto.js:8-27) shapes it into AuthType.items.GeneratedRefreshToken.{value} at runtime → the read at Source.js:273 prefers AuthType?.items?.GeneratedRefreshToken?.value → but this setter writes only top-level this.config.GeneratedRefreshToken → the second getAccessToken() call (Source.js:381, :619 — e.g. after the 60-min access-token expiry) redeems the stale pre-rotation token again.

The externalized _secrets_id path injects top-level (connector-credential-injector.service.ts:226-228) where write and read agree — which is exactly the only path the new unit test covers, so the asymmetry is invisible to the test suite. If Microsoft enforces strict one-time rotation for the token family, the second exchange fails with invalid_grant mid-run; combined with the if (success) gating the rotated token is then also discarded.

Fix: mirror the read precedence here — when this.config.AuthType?.items?.GeneratedRefreshToken exists, update it too; or flip the read precedence at line 273 so the top-level (in-run, always freshest) slot wins.

[zapolsky]

🔵 Review notes (non-blocking) — follow-up pass

Deep multi-agent re-review of the same head. New findings only — the earlier notes (lost-update race, executor test coverage, changeset) still stand and are not repeated here.

• Correction: the earlier inline blocker on connector-credential-injector.service.ts:227 ("GeneratedRefreshToken injected as a bare string → feature is a no-op") is refuted — SourceConfigDto.recursiveValueTransform (ConfigDto.js:26) wraps bare strings into {value} before OW_CONFIG serialization. Full chain in the thread reply. Do not apply its suggested fix (it would double-wrap and break the feature).
• connector-secret.service.ts:734-736 — Copy flow can persist a plaintext generated_refresh_token in the data-mart definition. mergeDefinitionSecretsFromSource inlines the token unconditionally, but extractAndSaveSecrets early-returns for items carrying _source_credential_id (:239) before the delete (:301) — and maskRecursively (:345-368) only masks spec secret-field names, so a definition containing it inline is returned unmasked by mask(). Narrow scenario (dual-marker item), but the masking layer has zero defense-in-depth for this field. Guard the assignment + strip/mask generated_refresh_token unconditionally in mask().
• connector-process-spawner.service.ts:75-80 — stdout chunk-split can leak the raw token into persisted run logs. A JSON line split across a 64KB pipe-chunk boundary fails JSON.parse on both halves → raw text becomes an UNKNOWN message → pushed into configLogs (connector-executor.service.ts:285-293) → persisted into DataMartRun.logs (API-visible). Pre-existing parser weakness, but this PR is the first to put a secret on stdout. Also connector-message-parser.service.ts:13-18 logs the full raw message on schema failure. Fix: line-buffer stdout in the spawner (split on \n, keep the tail); redact raw lines containing "updateCredentials".
• connector-secret.service.ts:580 — Dual-marker configs write rotations to one credentials row but read from another. mergeDefinitionSecrets unconditionally re-attaches the previous _secrets_id, so an item switched from manual entry to the OAuth flow carries both markers. getCredentialIdForConfig (connector-executor.service.ts:391-397) prefers _secrets_id for persistence, while the runtime read prefers the OAuth row's injection — rotations land in the stale row, and a manual-era generated token can shadow a freshly authorized OAuth token. Drop the stale _secrets_id when an item transitions to OAuth.
• connector-executor.service.ts:338 — saveConnectorCredentials resolves the credential id from the original config, but refreshCredentialsForConfig (:302) may return a config pointing at a new credential row (Facebook/TikTok-style refreshCredentials creates one — connector.service.ts:260-269). Updates would land on the orphaned row. Moot for MicrosoftAds today (AbstractSource.refreshCredentials returns null), but a landmine for the generic mechanism — pass refreshedConfig.
• Legacy inline-secret configs (no _secrets_id / _source_credential_id): the feature is silently dead for them. No injection (connector-credential-injector.service.ts:196-200) and every rotation is dropped with a warn (connector-executor.service.ts:392-401) — on every run, since offline_access now makes Microsoft issue rotations for these configs too. If this degradation is accepted, document it; otherwise a migration is needed.
• generated_refresh_token is now defined in four places (injector :7, secret service :10, inline literal in credentials service :214-216, hardcoded in Source.js) plus the implicit snake_case→GeneratedRefreshToken rename convention duplicated backend/connector-side. Export one constant from @owox/connectors Core (backend already imports Core). Worth deciding now — while the storage shape is unreleased — whether connector-written fields should live under a reserved _runtime.* namespace; the allowlist from the earlier security comment then falls out for free.
• Source.js:334-341 — _updateCredentials hand-rolls the stdout JSON transport inside a Source; every other structured message goes through the Config layer (NodeJsConfig.updateCurrentStatus & siblings). Add updateCredentials(credentials) to AbstractConfig (no-op) + NodeJsConfig, and call this.config.updateCredentials({...}).
• Two rotation mechanisms now coexist undocumented: pre-run pull (AbstractSource.refreshCredentials → new credential row) vs mid-run push (updateCredentials message → in-place mutation). A short comment on both stating when to use which will save the next OAuth-connector author a wrong-mechanism PR.
• Minor: Microsoft returns a refresh_token on nearly every exchange with offline_access → one extra credentials-row save per run; skip the emit when the new token equals the current one. And a tiny schema spec asserting toFormattedString prints credential keys only, never values would pin the log-hygiene property.

_{Automated OWOX review · worktree multi-agent passes · all blockers adversarially verified.}

[max-voloshyn]

🔴 Blocking: This preserves the previous generated_refresh_token whenever regular secrets are updated, but Microsoft Ads now prefers generated_refresh_token over the configured RefreshToken. If a user replaces the refresh token, the stale generated token is kept and continues to be used, so the credential update cannot take effect. Please drop or regenerate generated_refresh_token when the underlying refresh-token secret changes.
🤖 Reviewed by Codex (GPT-5.5)

[max-voloshyn]

🔴 Blocking: Captured credential updates are saved only when the overall configuration finishes successfully. Microsoft Ads emits this update immediately after the token endpoint returns a rotated refresh token, before later data fetch/storage work can fail; if that later work fails, the new token is discarded and the next run starts from stale credentials. Persist captured credential updates independently of the run success status once the connector process has emitted them.
🤖 Reviewed by Codex (GPT-5.5)

[max-voloshyn]

🔴 Blocking: saveConnectorCredentials swallows persistence failures after logging. If updateCredentialFields fails, the run is still marked successful even though the rotated refresh token was not persisted, leaving the next run to fail with no visible signal in run status. Let this error fail the configuration or add it to configErrors so the run does not report success when credential persistence failed.
🤖 Reviewed by Codex (GPT-5.5)

[max-voloshyn]

🟡 Non-blocking: This uses the original config to locate the credential row even though refreshCredentialsForConfig may return refreshedConfig with a replacement _source_credential_id. If a connector refreshes to a new credential id and then emits updateCredentials, the update will be written to the stale credential row. Use refreshedConfig when resolving the credential id or include the target id in the update message.
🤖 Reviewed by Codex (GPT-5.5)

[zapolsky]

✅ No blocking issues found — looks ready to approve (a maintainer should give the formal approval).

Full worktree re-review of head 7e3765a (5 parallel specialist passes + fresh-eyes generalist; the raw JSON SQL and the stdout line-buffer were also exercised empirically against better-sqlite3). This is a re-review: the head changed substantially since the last pass (4a6e831), and the rework is solid.

Prior blockers — all resolved ✅

The five previously-flagged blockers (and the overlapping Codex/@max-voloshyn findings) are genuinely fixed in this head, so the earlier CHANGES_REQUESTED is now stale:

• Unconstrained write to encrypted credentials → fixed: getAllowedCredentialUpdates + updateCredentialFields hard-restrict the persisted key to generated_refresh_token (string, non-empty).
• Success-gated token loss → fixed: persistence now runs in the finally block regardless of run outcome (connector-executor.service.ts:404).
• Stale generated_refresh_token shadows a fresh user token → fixed: shouldPreserveGeneratedRefreshToken drops the generated token when the user's refresh token changes, plus an invalid_grant fallback to the original token in Source.js.
• Credential-save failure silently reported as success → fixed: a save failure now sets success=false and adds a run error.
• Source.js write/read slot asymmetry → fixed: _setGeneratedRefreshToken writes the top-level slot that getAccessToken reads first.

Verified clean as part of this pass: no token reaches run logs / API responses / persisted definitions (CREDENTIALS_UPDATE is accumulated in-memory and never pushed to configLogs; mask()/extractAndSaveSecrets strip the field at every depth; the [CREDENTIALS] formatter emits key names only); the JSON_SET/JSON_EXTRACT compare-and-swap is correct on both sqlite and mysql with no injection surface (path is a constant, value is bound); the stdout line-buffer + oversized-line truncation state machine is correct; and the cross-package version-skew leak path is contained by the fixed changeset version group (connectors + backend ship at the same version). CI parity: tsc -b and eslint are clean on all PR-touched files (the only tsc errors are pre-existing, in untouched project-member-api-keys / permissions-*.e2e-spec files — not attributable to this PR).

🔵 Review notes (non-blocking)

• apps/backend/src/data-marts/services/connector/connector-executor.service.ts:550 — 4096-char cap can silently drop a legitimately large rotated token. getAllowedCredentialUpdates returns {} when generated_refresh_token.length > GENERATED_REFRESH_TOKEN_MAX_LENGTH (4096); the caller then early-returns with only a logger.warn (:448), no run-visible signal. The token length here is controlled by Microsoft Entra, not us, and some tenant/claim configurations push refresh tokens past 4 KB. If that happens, the rotation is silently lost and the credential decays at the end of the sliding inactivity window — the exact failure this feature exists to prevent. Consider raising the cap (e.g. 16 KB) and/or surfacing an over-cap drop as a run error rather than a silent warn.
• apps/backend/src/data-marts/connector-types/connector-message/services/connector-message-parser.service.ts:7 (and the other const { GENERATED_REFRESH_TOKEN_CREDENTIAL_FIELD } = Core sites) — the feature hard-depends on a build-time constant with no fail-loud guard. If @owox/connectors is ever loaded un-rebuilt (a build/version skew where the constant resolves to undefined), rotation silently no-ops (getAllowedCredentialUpdates returns {} for every message) and redactCredentialUpdateMessage weakens (it would test message.includes(undefined) for the non-updateCredentials path). The fixed changeset group prevents this on supported upgrades, so this is a footgun, not a live bug — but a one-line module-load assertion (if (!GENERATED_REFRESH_TOKEN_CREDENTIAL_FIELD) throw …) turns a silent degradation into a loud failure for the price of one line.

_{Automated OWOX review · worktree multi-agent passes · all candidate blockers adversarially verified (0 confirmed, prior 5 confirmed fixed).}