Skip to content

[codex] Center Windows Hello prompt without visible parent#149

Draft
SirAndrosBot wants to merge 1 commit into
masterfrom
codex/issue-54-hidden-main-prompt
Draft

[codex] Center Windows Hello prompt without visible parent#149
SirAndrosBot wants to merge 1 commit into
masterfrom
codex/issue-54-hidden-main-prompt

Conversation

@SirAndrosBot

Copy link
Copy Markdown
Collaborator

What changed

  • Added UIContext.PromptParentWindowHandle for Windows Hello prompt parenting.
  • Kept the existing ParentWindowHandle unchanged for normal UI context behavior and foreground restoration.
  • When the current context parent is visible and not minimized, Windows Hello still receives that original KeePass/KeePass prompt HWND.
  • When the parent HWND is hidden, minimized, or unavailable, the plugin creates a short-lived KeePass-process helper form centered on the screen under the cursor and passes that HWND to CNG.
  • Disposes the helper when the pushed UI context exits.
  • Marshals helper creation/disposal to the captured WinForms UI thread when the current caller is on another thread.
  • Added Win32 helpers for IsWindowVisible and IsIconic.

Why

Issue #54 reports that when KeePass is minimized or hidden in the tray, the Windows Hello dialog can appear at the top-left of the screen. The existing code always passes the current UI context parent HWND to NCRYPT_WINDOW_HANDLE_PROPERTY. If that parent is hidden/minimized, Windows has no useful visible anchor for prompt placement.

The fix keeps normal visible-parent behavior unchanged and adds a fallback parent only for the hidden/minimized case. This avoids the unsafe approaches rejected in earlier foreground work: it does not search for or foreground Windows credential dialogs, does not parent the prompt to arbitrary external windows, and does not call the existing EnsureForeground path that can minimize KeePass windows.

Refs #54

Review notes

The reviewer raised a thread-affinity issue in the first pass. The patch now captures the original WinForms parent control and uses Control.Invoke for helper form creation/disposal when needed. The second review found no blocking issues.

Residual risk remains UI/manual: helper ownership and prompt placement need real Windows Hello validation on a non-RDP desktop with KeePass hidden, minimized, and in tray.

Validation

  • Worker subagent implemented the fix in an isolated branch.
  • Reviewer subagent reviewed the patch twice; first pass found thread-affinity risk, second pass found no blocking issues.
  • git diff --check passed with only Git LF-to-CRLF working-copy warnings.
  • MSBuild.exe src\KeePassWinHello.csproj /t:Rebuild /p:Configuration=Release /p:DefineConstants=MONO /p:FrameworkPathOverride=C:\Windows\Microsoft.NET\Framework\v4.0.30319 /p:ReferencePath=C:\proj\KeePassWinHello\lib passed with 0 warnings and 0 errors.

Manual test needed

  • KeePass minimized, hidden, and tray unlock paths.
  • Multi-monitor behavior, especially cursor on a non-primary screen.
  • Persistent-key creation from Options with visible parent.
  • Secure desktop enabled/disabled.
  • Elevated KeePass foreground behavior.

@sonarqubecloud

Copy link
Copy Markdown

@SirAndrosBot

Copy link
Copy Markdown
Collaborator Author

Follow-up from issue triage: this helper-parent change appears to cover the same root cause reported in #86 as well.

#86 is the global Auto-Type shortcut case where KeePass is locked/minimized and Windows Hello opens without focus. A separate worker reproduced the same patch for #86, and review found it materially identical to this PR: when the KeePass/key-prompt parent is hidden or minimized, CNG receives a poor parent HWND. This PR replaces only the CNG prompt parent with a short-lived KeePass-owned helper window in that hidden/minimized case.

I am leaving the PR as \Refs #54\ in the body rather than changing it to close #86 too, because #86 still needs real non-RDP Windows Hello validation through the global Auto-Type shortcut path before calling it fixed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants