Skip to content

fix(config): nostr keys and webhook URLs show in a plain text box - #1513

Open
SebTardif wants to merge 12 commits into
openclaw:mainfrom
SebTardif:fix/f083-mask-config-secrets
Open

SebTardif wants to merge 12 commits into
openclaw:mainfrom
SebTardif:fix/f083-mask-config-secrets

Conversation

@SebTardif

@SebTardif SebTardif commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes: the config editor shows a Nostr private key and a Google Chat webhook URL in a normal text box.

User Impact

User impact: those fields use the same password box as other secrets, and a stored value is not preloaded. A stored secret object that declares properties shows how many entries are configured. Replace all starts blank. Clear all asks before it removes the entries.

Why This Change Was Made

The schema editor and the no-schema fallback share ConfigPathSensitivity. An exact path segment named nsec, privateKey, webhookUrl, or webhookUrls takes the existing sensitive field. webhookUrlExtra stays a normal field. A sensitive string array uses a password box. The stored item is kept by the editor, not on the control. A blank box keeps that item.

A sensitive array of objects stays off the page. The page shows how many entries are configured. Replace all opens a blank JSON box and the patch contains only that new array. Clear all asks, then sends an empty array. Cancel, or a save that does not use those actions, leaves the stored array out of the patch. The no-schema fallback uses the same editor.

A sensitive object uses that same hidden editor, including when its schema declares properties. Child titles and stored children stay off the page. An object that is not itself sensitive still expands. A sensitive string stays a password field. Replace all checks the expected JSON kind before it stages a value. A wrong kind leaves the stored value in place and keeps Save disabled. Cancelling that replacement, or cancelling Clear all, clears the blocking validation state.

Evidence

Terminal output from commit 4184c01c.

Passed!  - Failed: 0, Passed: 15, Skipped: 0, Total: 15, Duration: 38 ms
channels.nostr.nsec = masked
channels.nostr.nsecExtra = plain
channels.googlechat.webhookUrl = masked
channels.googlechat.webhook = plain
channels.discord.token = masked

Head 92b1cd635d83f3f038f42f18ceea4d6559b21c5b. ConfigPathSensitivityTests: 22 passed, including channels.slack.webhookUrls, channels.googlechat.webhookUrlExtra as plain, and SchemaEditor_HidesStoredValuesInSensitiveArrays. The parent capture below is 29faa6b2.

The running Config page on this head, after Display Name was changed to after-edit, Save, and Refresh:

Config Channels on this head: Display Name after-edit, two masked Webhook Urls, and Webhook Url Extra benign-near-match

Display Name, both Webhook Urls rows, Webhook Url Extra, and Show JSON are inside this frame. The two stored array items show Leave blank to keep existing value and do not show the stored URLs. Webhook Url Extra shows benign-near-match. Show JSON is visible, so the JSON preview is collapsed. The stored array strings are not on screen.

Redacted patch trace from the same run:

config.patch
baseHashMatched: true
arrayPreserved: true
displayName: after-edit
extra: benign-near-match
config.get after Refresh: displayName after-edit, both array boxes still blank, extra still benign-near-match

This was a loopback stub on 127.0.0.1:18794, not a product OpenClaw gateway. The earlier full-frame capture 6aa74002 is parent 7e488a40. That run did not include an empty array entry.

Head 92b1cd635d83f3f038f42f18ceea4d6559b21c5b keeps a loaded empty string in the array. The same page, after Display Name was changed to after-edit and Save:

Config page keeping two blank webhook rows, including a stored empty entry

Both rows show Leave blank to keep existing value. The stored URL is not on screen. Show JSON is visible.

config.patch
arrayPreserved: true
emptyKept: true
displayName: after-edit

This was a loopback stub on 127.0.0.1:18795, not a product OpenClaw gateway.

Head 26cf9b9fd8fe22ad05550afe215a7b5d3b2c4999 was the read-only note. Head b3c02a5bc0f9a3e37da143b860a0bae4d08e85ce replaces that dead end with Replace all and Clear all. The stored objects are still not copied into the editor.

Webhook Urls note on the previous head: stored values stay hidden and cannot be edited on this page

Change Type

  • Bug fix
  • Feature
  • Refactor
  • Docs or instructions
  • Tests or validation
  • Security hardening
  • Chore or infrastructure

Scope

  • Tray or WinUI UX
  • Windows node capability
  • Local MCP or winnode
  • Gateway, connection, or pairing
  • Setup or onboarding
  • Permissions, privacy, or security
  • Tests, CI, or docs

Required proof pools

  • windows-winui-interactive: isolated tray on head b2c836d4b9dae9d24fa430e0c8633cc2ac206ac3, Config page. That head is an empty retrigger, and the tree matches 0a3a1c67c876c281efba0739bc088bd9f23e0437. A sensitive array of objects and a sensitive object that declares properties each show a count. The capture applies a replacement, saves it, confirms clear, and cancels a clear. Stored values stay off the page.

Validation

  • Head b2c836d4b9dae9d24fa430e0c8633cc2ac206ac3. Empty retrigger. The tree matches 0a3a1c67c876c281efba0739bc088bd9f23e0437.
  • ./build.ps1: exit 0.
  • Shared: 4107 passed, 32 skipped, 0 failed, 4139 total.
  • Local LF Tray: 3101 passed, 5 failed, 3106 total. Those five compare multiline source to a hardcoded CRLF snippet. That mismatch is #1518.
  • That Tray run includes UseHiddenObjectEditor_SensitiveObjectWithProperties_StaysHidden, ArrayItemKinds_RejectsTheWrongJsonKind, ArrayItemKinds_AcceptsObjectAndStringReplacements, SensitiveObject_UnrelatedEditAndCancel_PreserveStoredObject, and SensitiveObject_ReplaceSendsOnlyTheNewObject_ClearSendsEmptyObject. webhookUrlExtra stays plain.

Head 3f81db4d114cba09c3bb2cb66c8fb113bf511152:

  • ./build.ps1: exit 0.
  • Shared: 4107 passed, 32 skipped, 0 failed, 4139 total.
  • Local LF Tray: 3102 passed, 5 failed, 3107 total. The same five CRLF source-contract checks as #1518.
  • SensitiveArray_RejectedRetryThenCanceledClear_RestoresCommittedReplacement passed. SchemaEditor_HidesStoredValuesInSensitiveArrays passed.

Real Behavior Proof

  • Behavior or issue addressed: a sensitive object whose schema declares properties was expanded into child controls, and Replace all accepted the wrong JSON kind.
  • Real environment tested: Windows isolated tray on head b2c836d4b9dae9d24fa430e0c8633cc2ac206ac3 (empty retrigger, same tree as 0a3a1c67c876c281efba0739bc088bd9f23e0437). Loopback stub ws://127.0.0.1:18796. Registry isLocal false. Fake shared token. Deep link openclaw://hub/config. ./build.ps1, Shared, and Tray for this head are in Validation above.
  • Exact steps or command run after this patch: Opened Channels with Show JSON collapsed. Applied ["not-an-object"] to Webhook URLs. Opened Clear all and chose Cancel. Applied [{"url":"https://example.invalid/hook-dummy"}] to Webhook URLs and {"id":"replacement-dummy"} to Secret Bundle, then saved. After reconnect, confirmed Clear all on Webhook URLs and saved again.
  • Evidence after fix: The first patch set webhookUrls to the dummy array and secretBundle to {"id":"replacement-dummy"}. The second patch set webhookUrls to [] and left secretBundle as that dummy object. The stored marker was absent from both changed fields. Display Name stayed after-edit. Webhook URL Extra stayed benign-near-match.
  • Observed result after fix: Secret Bundle and Webhook URLs each opened as "1 entry is configured. Stored values stay hidden." The child title Bundle Id was not on the page. The invalid item showed "Item 1: Must be a JSON object." and "Fix validation errors before saving", with Save disabled. Cancel on the Clear all dialog cleared that error and left both counts at one entry, with Save still disabled. After the dummy apply, both fields said "Replacement is ready to save." and Save Changes was enabled. Save showed "Saving configuration" and "Gateway restarting". Reload showed "Gateway reconnected" with both counts still one entry. The confirmed clear showed "This array will be cleared on save.", then reload showed Webhook URLs as "0 entries are configured. Stored values stay hidden." and Secret Bundle still as one entry.
  • Screenshot or artifact links verified? Yes. The ten frames below are the b2c836d4b9dae9d24fa430e0c8633cc2ac206ac3 tree, captured on 0a3a1c67. The three frames after those are head a18cd305, where Apply, the confirmation Clear all, and Save were not clicked. The pictures in Evidence are earlier heads. The password-field capture 6aa74002 on 7e488a40 was not recaptured.

Channels on 0a3a1c67: Webhook URLs and Secret Bundle each show one hidden entry, Show JSON collapsed

Invalid replace on 0a3a1c67: a string item is rejected and Save stays disabled

Clear all confirmation on 0a3a1c67, with the rejected replacement still blocking save

Clear all cancelled on 0a3a1c67: validation is clean and both counts stay at one entry

Replacements ready on 0a3a1c67: Save Changes is enabled for the dummy array and dummy object

Save on 0a3a1c67: Saving configuration and Gateway restarting

Reload on 0a3a1c67: Gateway reconnected, both fields still one hidden entry

Confirmed clear ready on 0a3a1c67: Webhook URLs will be cleared and Secret Bundle stays one entry

Clear save on 0a3a1c67: Saving configuration while Secret Bundle stays a count

Reload after clear on 0a3a1c67: Webhook URLs shows zero entries and Secret Bundle stays one entry

Channels on a18cd305: one hidden webhook entry, plain Webhook URL Extra, and Show JSON

Replace all on a18cd305: the JSON box starts empty

Clear all on a18cd305: confirmation before the stored entries are removed

  • Not verified / blocked: This gateway was a loopback stub on 127.0.0.1:18796. The rejected-retry then canceled-clear restore was not opened on the Config page. The session test keeps the committed replacement, and Cancel clear stages that replacement again instead of dropping it.

Security Impact

  • New permissions or capabilities? No
  • Secrets or tokens handling changed? Yes
  • New or changed network calls? No
  • Command or tool execution surface changed? No
  • Data access scope changed? No
  • If any answer is Yes, explain the risk and mitigation: Stored objects are not copied into the editor, including a sensitive object whose schema declares properties. Replace all sends only the newly entered JSON after the item kind checks. Clear all sends an empty array or an empty object after confirmation. A save that does not use those actions leaves the stored value out of the patch. A rejected replacement keeps Save disabled.

Compatibility and Migration

  • Backward compatible? Yes
  • Config or environment changes? No
  • Migration needed? No

Review Conversations

  • I replied to or resolved every bot review conversation addressed by this PR.
  • I left unresolved only conversations that still need maintainer judgment.

Config paths whose segment is nsec, and paths that contain webhookUrl,
now use the existing password field that does not preload the stored value.

- Add ConfigPathSensitivity and route SchemaConfigEditor.IsSensitive through it
- Cover nostr nsec, googlechat webhookUrl, and the existing secret names

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 24, 2026
@clawsweeper

clawsweeper Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 30, 2026, 4:49 PM ET / 20:49 UTC (Revision 25).

ClawSweeper review

What this changes

The Windows Config editor masks Nostr keys and webhook URLs, hides stored sensitive arrays and objects, and adds blank replacement and confirmed-clear controls.

Merge readiness

⛔ Blocked before merge - 3 items remain

Current main still exposes these config fields through ordinary controls. The branch addresses that behavior, and its latest commit appears to repair the prior staged-replacement finding. The remaining blocker is current-head running-UI proof of that final cancel-and-save sequence.

Priority: P2
Reviewed head: 3f81db4d114cba09c3bb2cb66c8fb113bf511152

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The implementation addresses the known finding and has useful UI evidence, but proof of the latest changed interaction stops at a model test.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: Earlier isolated WinUI screenshots and a loopback Config save trace support the hidden-field and replace/clear flow. The latest head changes cancel handling, while the PR body says its valid replacement, invalid retry, canceled clear, and subsequent save sequence was not run in the Config page; the new session test is supplemental. No stored-data format changes are introduced. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: Earlier isolated WinUI screenshots and a loopback Config save trace support the hidden-field and replace/clear flow. The latest head changes cancel handling, while the PR body says its valid replacement, invalid retry, canceled clear, and subsequent save sequence was not run in the Config page; the new session test is supplemental. No stored-data format changes are introduced. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 8 items Current main remains affected: The current-main sensitivity check covers token, secret, password, and API-key names, but not the reported privateKey or webhookUrl segments; object fields also enter the ordinary JSON editor.
Introduced secret-safe editor: The PR routes classified sensitive objects and arrays to hidden-value controls and provides blank Replace all and confirmed Clear all actions.
Prior finding addressed in source: Canceling Clear all now restages a committed replacement. The exact latest-commit patch was verified through the GitHub commit endpoint because the local partial checkout could not read the previous head's file blobs.
Findings None None.
Security None None.

How this fits together

The Windows tray Config page receives configuration and schema data from the Gateway, builds editable controls, and sends saved changes back to the Gateway. This PR changes how that page displays and edits values classified as sensitive.

flowchart LR
  A[Gateway config] --> C[Config page]
  B[Config schema] --> C
  C --> D{Sensitive field?}
  D -->|Yes| E[Hidden value editor]
  D -->|No| F[Ordinary controls]
  E --> G[Validated changes]
  F --> G
  G --> H[Gateway config save]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: Earlier isolated WinUI screenshots and a loopback Config save trace support the hidden-field and replace/clear flow. The latest head changes cancel handling, while the PR body says its valid replacement, invalid retry, canceled clear, and subsequent save sequence was not run in the Config page; the new session test is supplemental. No stored-data format changes are introduced. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - The running Config page has not demonstrated on the current head that a valid secret replacement survives an invalid retry and canceled Clear all through Save and reload.
  • Complete next step (P2) - Provide a redacted current-head isolated Config-page capture or runtime trace showing valid Replace all, an invalid retry, canceled Clear all, and Save/reload retaining the replacement; update the PR body to trigger re-review, or ask a maintainer to comment @clawsweeper re-review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Code and test delta production +540/-35 lines; tests +343 lines; 6 files The substantial production growth implements the maintainer-requested hidden editing flow and has focused model and source-contract coverage.

Merge-risk options

Maintainer options:

  1. Prove the final edit sequence (recommended)
    Capture a redacted current-head Config-page run showing valid Replace all, invalid retry, canceled Clear all, then Save and reload with the replacement retained.

Technical review

Best possible solution:

Keep the agreed hidden-value editing contract and verify the final replacement decision reaches the Gateway save request on the current head.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: current main does not classify the reported path segments as sensitive and sends them to ordinary controls. I did not run the Windows app in this read-only review.

Is this the best way to solve the issue?

Yes, the branch follows the maintainer's bounded hidden-value editing decision. Its final cancel-and-save behavior still needs current-head UI confirmation.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 04a880fe9488.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: The PR addresses a concrete Config editor secret-display bug with limited product scope.
  • merge-risk: 🚨 other: The final edit-session sequence could leave an intended credential replacement out of the save request if the UI and model diverge.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Earlier isolated WinUI screenshots and a loopback Config save trace support the hidden-field and replace/clear flow. The latest head changes cancel handling, while the PR body says its valid replacement, invalid retry, canceled clear, and subsequent save sequence was not run in the Config page; the new session test is supplemental. No stored-data format changes are introduced. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. Earlier isolated WinUI screenshots and a loopback Config save trace support the hidden-field and replace/clear flow. The latest head changes cancel handling, while the PR body says its valid replacement, invalid retry, canceled clear, and subsequent save sequence was not run in the Config page; the new session test is supplemental. No stored-data format changes are introduced.

Evidence

What I checked:

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • shanselman: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add redacted current-head WinUI evidence for the replacement, rejected retry, canceled clear, Save, and reload sequence.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (24 earlier review cycles; latest 8 shown)
  • reviewed 2026-09-28T23:01:12.421Z sha a18cd30 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-29T00:40:58.911Z sha a18cd30 :: blocked before merge. :: [P2] Clear hidden validation errors when cancelling Clear all
  • reviewed 2026-09-29T01:35:46.515Z sha a18cd30 :: needs changes before merge. :: [P2] Clear hidden validation errors when cancelling Clear all
  • reviewed 2026-09-29T04:06:17.441Z sha a18cd30 :: needs changes before merge. :: [P2] Clear abandoned validation errors when Clear all is cancelled
  • reviewed 2026-09-29T15:26:11.590Z sha adc98c4 :: needs real behavior proof before merge. :: [P2] Route sensitive objects before expanding schema properties
  • reviewed 2026-09-29T16:22:32.654Z sha 0a3a1c6 :: needs real behavior proof before merge. :: [P2] Restore the staged replacement when Clear all is canceled
  • reviewed 2026-09-29T16:37:25.196Z sha 0a3a1c6 :: needs changes before merge. :: [P2] Restore the staged replacement after canceling Clear all
  • reviewed 2026-09-29T16:59:50.639Z sha b2c836d :: blocked before merge. :: [P2] Restore the staged value after a rejected retry and canceled clear

@shanselman

Copy link
Copy Markdown
Collaborator

Hi @SebTardif, Copilot here helping Scott triage your OpenClaw fixes. Thank you for the careful follow-ups and for keeping the repairs on the original PR branches. There are now 23 open PRs from this batch, and we want to give each one a fair, timely review rather than let useful fixes get lost in the queue.

Could you focus on closing ClawSweeper's current-head, changed-behavior proof requests on a smaller set before adding more follow-ups? A concise proof on the PR itself is ideal: exact head SHA, the smallest full required build/Shared/Tray and focused test counts, and a redacted real-path observation of the behavior the patch changes. Please mark anything unavailable as Not verified / blocked rather than treating a unit test or a green label as a substitute. You do not need to re-explain the same Gateway 2026.9.6 setup failure on every PR; #1498 tracks that shared baseline separately.

For this PR, the next decisive item is an isolated running Config page view showing stored Nostr nsec and Google Chat webhookUrl in password-style controls without preloading plaintext; the helper tests alone do not show the live UI. For the other PRs, please follow their particular ClawSweeper request (for example, a real Gateway/token handoff or strict MXC when authorization changes). We will take the ones that clear the required proof and checks with >=90% independent confidence, and give focused feedback on the others. No need to race the bot or produce broad unrelated proof. Thanks for collaborating with us.

@karkarl

karkarl commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Global triage: HOLD_FOR_AUTHOR. Take confidence 15%; recommendation confidence 99%; effort small code fix plus medium proof; risk medium.

The real Nostr credential path remains unmasked. The classifier recognizes an exact nsec segment, but the supported Gateway schema uses channels.nostr.privateKey. That path still routes through the ordinary TextBox. The attached loopback proof modeled nsec, so it did not exercise the product contract.

Please classify the exact privateKey segment, add channels.nostr.privateKey regression coverage, and rerun windows-winui-interactive against the current supported Gateway. Show the Nostr private key and Google Chat webhook URL as password controls, preserve existing secrets while editing an unrelated field, then save/reload. Do not capture secret values.

The failed E2E lanes reproduce on the exact base in shared Gateway restart setup; CI Gate is derivative. Core, Tray, and UI/accessibility checks passed at this head, but the required exact build, Shared, and Tray closeout results are not reported in the PR body.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. labels Sep 25, 2026
The editor treated an exact nsec segment as secret, but the Gateway
property is channels.nostr.privateKey. That stored value still loaded
into a plain text box.

Match an exact privateKey segment the same way as nsec. A longer name
such as privateKeyExtra stays plain.

Validation: ./build.ps1 exit 0. Shared 4107 passed, 32 skipped, 0 failed
(4139 total). ConfigPathSensitivityTests 19 passed. Tray 3086 passed
and 5 failed (3091 total). Those five are the existing LF source checks.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Sep 25, 2026
@shanselman shanselman added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 25, 2026
@shanselman

Copy link
Copy Markdown
Collaborator

Maintainer review at exact head 7e488a40754685220cdfa5b0b5b7fb2d06fe95ad: do not merge yet.

The current-head screenshot and redacted patch trace are useful proof for the two scalar fields shown: channels.nostr.privateKey and channels.googlechat.webhookUrl render as password controls, and an unrelated save preserves their stored values. The required build, focused config contracts, and full Tray suite also pass locally.

There is one concrete in-scope blocker before this can reach the requested >=90% landing confidence:

  • ConfigPathSensitivityTests asserts channels.slack.webhookUrls is sensitive, but SchemaConfigEditor.RenderField dispatches arrays and objects before consulting isSensitive. Existing string-array items are copied into ordinary TextBox.Text, complex arrays/objects are serialized into plaintext JSON editors, and the no-schema array fallback also prints the stored array. This means a path the new test declares protected is still exposed by the actual editor. The structured autoreview independently reported this as P2 with 98% confidence; the security and rubber-duck reviews reached the same conclusion.

Please make sensitivity control rendering, not only classification:

  1. Ensure sensitive arrays/objects never preload stored contents into a TextBox or JSON editor. Preserve the current blank/unmodified secret semantics during unrelated saves.
  2. Add a rendering/source contract covering both schema and no-schema fallback paths, asserting that stored webhook values never enter visible plaintext controls.
  3. Keep the new matching exact. Add a benign near-match such as webhookUrlExtra to the negative cases. If this PR is intentionally scalar-only, narrow the title/body/tests and track the remaining array exposure explicitly, but the present webhookUrls = sensitive claim cannot remain without working UI behavior.
  4. Broader follow-up: the Gateway schema has authoritative sensitive metadata for fields such as Google Chat serviceAccount, Synology incomingUrl, and TLS key/passphrase values, while ConfigPage.GetSchemaRoot currently drops uiHints. Path heuristics cannot be comprehensive; please either consume that metadata in a focused follow-up or document the compatibility boundary.

Validation on this exact head:

  • ./build.ps1: passed.
  • Focused ConfigPathSensitivityTests + ChannelsPageDiscordConfigContractTests: 22 passed.
  • Full Tray: 3091 passed, 0 failed.
  • Full Shared: 4103 passed, 35 skipped, 1 failed repeatedly in the unrelated McpHttpServerTests.Dispose_DuringInFlightHandler_DoesNotSurfaceObjectDisposedException; the focused test passes alone.
  • Exact-head CI: Core/Tray/UI passed; setup/revocation/network E2E and CI Gate remain red.
  • Latest main advanced to ecd6aefc (fix(tests): tray source checks fail when the checkout is LF #1518, fix(tests): tray source checks fail when the checkout is LF) and its CI is not green, so the requested current-main-green merge gate is also unmet.

No maintainer patch or commit was added, so Seb’s authorship and branch remain unchanged.

@shanselman shanselman removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 25, 2026
@clawsweeper clawsweeper Bot added merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 25, 2026
A sensitive string array, including webhookUrls, uses a password box and does not copy the stored value into it. A blank box keeps the existing item. The schema JSON view and the fallback array view do the same.

./build.ps1 exit 0. Shared tests: 4107 passed, 32 skipped. Tray tests: 3087 passed, 5 failed on the LF source-contract mismatch tracked in openclaw#1518. ConfigPathSensitivityTests: 20 passed.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label Sep 26, 2026
@clawsweeper clawsweeper Bot added merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. proof: sufficient Contributor real behavior proof is sufficient. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Sep 29, 2026
@shanselman shanselman added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 29, 2026
@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. label Sep 29, 2026
@shanselman shanselman removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 29, 2026
@shanselman

Copy link
Copy Markdown
Collaborator

@SebTardif, thank you. The a18cd305 screenshots address the agreed hidden-array / blank Replace all / confirmed Clear all UI contract. I checked the remaining findings against the public head a18cd30510dbb4ff7754c6f84dd822ff6bdc6617, not our unpushed maintainer experiment.

Please address two bounded gaps on this original branch:

  1. Sensitive schema objects can still prefill a plaintext editor. In SchemaConfigEditor.xaml.cs, RenderField calculates isSensitive at line 185, but its object branch at lines 233-236 unconditionally calls RenderJsonObjectField. That renderer initializes TextBox.Text from stored object JSON at lines 678-680. Handle a sensitive object before generic object dispatch using the same preserve/explicit-replace/confirmed-clear principle. Add a regression with a helper-classified sensitive path whose schema is type: object; stored dummy values must not enter the control, and an unrelated edit/cancel must preserve the value. Cover the corresponding schema-less path without broadening unrelated editor behavior.

  2. Replace all accepts invalid item kinds. ValidateValue at lines 890-902 has no object/string checks for JsonElement, while lines 949-954 pass each array element back as a JsonElement. For an array with items.type: object, ["not-an-object"] is therefore accepted. Validate the expected item JSON kind before staging/save; add negative tests for wrong object/string item kinds and positive tests for valid replacements. Invalid input must leave the original value intact and keep Save disabled; cancelling the invalid replacement must clear the blocking validation state.

Please keep this correction in the existing editor/classifier/model/test scope. Our broader local preview-redaction/localization/fixture experiment was stopped and has not been pushed; it is not a request to reproduce that expansion. No redesign or new service is needed for this follow-up.

After the fix, include the exact head, focused regression results plus the required build/Shared/Tray floor, and a redacted isolated UI proof that actually applies/saves a replacement and exercises confirmed clear/cancel with dummy values. Existing green CI is useful but does not cover these two paths. We will re-review this original PR once those targeted fixes are present.

Sensitive schema objects, and schema-less sensitive objects, use the same
blank replace and confirmed clear editor as complex arrays. Stored values
stay off the control. Replace all checks the JSON kind and each array item
kind before staging. Invalid input keeps the original value and blocks Save.
Cancelling Clear all drops the abandoned validation error.

./build.ps1 exit 0. Shared 4107 passed, 32 skipped. Tray 3100 passed, 5
failed on the known LF source contracts (openclaw#1518). Focused editor and model
tests passed.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper clawsweeper Bot added merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient Contributor real behavior proof is sufficient. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 29, 2026
A classified object with a properties map was expanded into child
controls before the hidden editor ran. Decide that case first, keep
the stored children off the page, and cover the properties schema.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@SebTardif

Copy link
Copy Markdown
Contributor Author

@shanselman

Please address two bounded gaps on this original branch:

RenderSchemaNode sends any object schema with properties to RenderObjectSection first.

A sensitive object stays hidden when its schema declares properties. Bundle Id was not on the page. Saving the dummy replacement and the confirmed clear both reached the patch. Cancelling Clear all cleared the validation error from the rejected item, and the stored marker was absent from the changed fields.

Head 0a3a1c67c876c281efba0739bc088bd9f23e0437. ./build.ps1 exited 0. Shared was 4107 passed, 32 skipped, and 0 failed. Local LF Tray was 3101 passed and 5 failed. Those five are the #1518 CRLF source checks. That Tray run includes UseHiddenObjectEditor_SensitiveObjectWithProperties_StaysHidden, ArrayItemKinds_RejectsTheWrongJsonKind, and ArrayItemKinds_AcceptsObjectAndStringReplacements.

On the isolated Config page, Webhook URLs and Secret Bundle each opened as one hidden entry. Saving [{"url":"https://example.invalid/hook-dummy"}] and {"id":"replacement-dummy"} patched those values. After the confirmed clear, Webhook URLs saved as [] and reloaded as "0 entries are configured. Stored values stay hidden." Secret Bundle stayed one entry. The frames are in Real Behavior Proof.

CI on this head is still running.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Sep 29, 2026
Shared failed only ExtractTarBz2Async_CancellationIsBoundedAndKillsExtractor
because the extractor process was still running. That test passed locally.
Revocation recovery failed in wsl-create: the Ubuntu-24.04 install exited -1
with no output after 900 seconds. The tree matches 0a3a1c6.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@SebTardif

Copy link
Copy Markdown
Contributor Author

@shanselman

The red checks on the previous head are not this config change. Shared failed one cancellation test because the piper extractor process was still running, and that same test passed locally. Revocation recovery stopped while creating the WSL instance: the Ubuntu install exited -1 with no output after 900 seconds. Head b2c836d4b9dae9d24fa430e0c8633cc2ac206ac3 is an empty retrigger of the same tree as 0a3a1c67c876c281efba0739bc088bd9f23e0437.

CI on this head is still running.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. and removed proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. labels Sep 29, 2026
@SebTardif

Copy link
Copy Markdown
Contributor Author

@shanselman

Head b2c836d4b9dae9d24fa430e0c8633cc2ac206ac3 failed two timing checks. Each job had one failure, and both tests passed locally.

CredentialReplacementOperations_WaitForAutomaticLifecycleLease threw TimeoutException. That test gives the setup task 2 seconds after the lifecycle lease is released. Dispose_VoiceCancellationCallbackCanWaitForReentrantControllerWork returned false from its 2 second voice-callback wait. The same tree passed the Tray job on 0a3a1c67. Both tests sit outside the config editor.

This pull already used its empty retrigger, so this head stays.

…celed

A valid Replace all stayed on the edit session after a later invalid
draft was rejected. Canceling Clear all cleared the error and left
the pending change removed, so a later save kept the stored secret.
Cancel clear now stages the committed replacement again.

SensitiveArray_RejectedRetryThenCanceledClear_RestoresCommittedReplacement
passed. ./build.ps1 exit 0. Shared 4107 passed, 32 skipped. Tray 3102
passed and 5 failed, the LF source-contract mismatch in openclaw#1518.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper clawsweeper Bot added proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient Contributor real behavior proof is sufficient. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 30, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal priority bug or improvement with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants