Visitar URL original
fix(auth): let Ctrl+C abort the login callback wait on Windows by benjaminrigaud-gg · Pull Request #1446 · GitGuardian/ggshield · GitHub
Skip to content

fix(auth): let Ctrl+C abort the login callback wait on Windows - #1446

Merged
benjaminrigaud-gg merged 2 commits into
mainfrom
benjaminrigaud/end-987-ctrlc-does-not-abort-ggshield-auth-login-on-windows
Sep 18, 2026
Merged

benjaminrigaud-gg merged 2 commits into
mainfrom
benjaminrigaud/end-987-ctrlc-does-not-abort-ggshield-auth-login-on-windows

Conversation

@benjaminrigaud-gg

@benjaminrigaud-gg benjaminrigaud-gg commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Context

On Windows, ggshield auth login (browser flow) cannot be interrupted with Ctrl+C while it waits for the browser callback. The command stays blocked until the login completes in the browser or the terminal is closed. Linux and macOS abort as expected.

Root cause: the local callback server waits in handle_request() with no timeout, so select() blocks until the redirect arrives. Python runs signal handlers only between bytecodes. On POSIX, SIGINT interrupts poll() with EINTR, so the handler runs at once. On Windows, Ctrl+C is delivered on a separate console thread that only sets a flag, WinSock select() is never woken, and the KeyboardInterrupt (mapped to click.Abort) only fires once the callback arrives. Known CPython limitation, see python/cpython#133223.

What has been done

  • Give the callback server a 0.5s timeout (CALLBACK_POLL_INTERVAL_SECONDS). The wait loop already re-enters handle_request() until the callback is processed, so the only change is that a pending interrupt is now raised within that bound.

  • Give the request handler the same timeout. A peer that connects but never sends a request line (browser pre-connect, port scan) used to block readline() with no timeout, one level below the server's select(). The stdlib closes such a connection on the read timeout and the wait resumes.

  • Two regression tests in tests/unit/verticals/auth/test_oauth.py, both marked enable_socket because unit tests run with --disable-socket:

    • flip the finished flag from another thread and check the wait loop notices;
    • raise SIGINT from a helper thread (this trips Python's flag without waking select(), the same shape as Windows delivery) and check click.Abort is raised within the bound. If the interrupt never fires, a 5s safety net ends the loop so the test fails with DID NOT RAISE instead of hanging, and both timers are cancelled so a stray SIGINT cannot hit a later test;
    • connect a silent peer first, then flip the finished flag, and check the loop still returns promptly.

    Both fail without the timeout and pass with it on Linux. The windows-2022 legs run them with real Windows signal machinery.

  • Changelog fragment.

Validation

  • uv run pytest --disable-socket tests/unit/verticals/auth tests/unit/cmd/auth: 182 passed.
  • Red/green checked: with the previous oauth.py, the two new tests fail (loop still alive after 3s; abort only after the 5s safety connection).
  • Probe scripts on Linux: KeyboardInterrupt after 3.00s without the timeout (only once a connection arrived), 0.50s with it.
  • Windows VM (Server 2022, Python 3.13.15), real console Ctrl+C with a silent peer connected to the callback port: without the handler timeout the process was still running 10s after Ctrl+C; with it, exit after 0.30s and 0.30s. Plain Ctrl+C: 0.33s.
  • Manual, on Windows: run ggshield auth login, do not complete the browser login, press Ctrl+C. The command should exit within about half a second. On Linux/macOS it still aborts immediately.

PR check list

  • As much as possible, the changes include tests (unit and/or functional)
  • If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

@linear

linear Bot commented Sep 3, 2026

Copy link
Copy Markdown

END-987

@codecov

codecov Bot commented Sep 3, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.06%. Comparing base (bd30431) to head (2b86a46).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1446      +/-   ##
==========================================
+ Coverage   93.97%   94.06%   +0.08%     
==========================================
  Files         201      201              
  Lines       12771    12777       +6     
==========================================
+ Hits        12002    12019      +17     
+ Misses        769      758      -11     
Flag Coverage Δ
unittests 94.06% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@benjaminrigaud-gg

Copy link
Copy Markdown
Contributor Author

QA on a Windows VM (Windows Server 2022 Standard, build 20348, Python 3.13.15)

Compared the released 1.54.0 against this branch's wheel in the same venv. The driver starts python -m ggshield auth login in its own console, waits 5s for it to reach the callback wait, then sends a real CTRL_C_EVENT to that console and times the exit.

Build In-process SIGINT probe Real console Ctrl+C
1.54.0 click.Abort only once a connection arrived (7.99s) still running 10s after Ctrl+C, killed
this branch click.Abort after 0.51s exited after 0.35s, 3.34s, 0.80s, 0.63s, 0.64s (5 runs), prints Aborted!

Notes: the 3.34s run overlapped with the browser launch on a loaded 4-core VM. One run exited with STATUS_CONTROL_C_EXIT (0xC000013A) instead of click's exit code 1, still within a second. The five Windows unit-test legs (Python 3.9 to 3.13) are green in CI.

@benjaminrigaud-gg
benjaminrigaud-gg marked this pull request as ready for review September 3, 2026 09:20
@benjaminrigaud-gg
benjaminrigaud-gg requested a review from a team as a code owner September 3, 2026 09:20

@clement-tourriere clement-tourriere left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, the poll approach is good. But I think there is 2 things to change before merge, see comments.

Comment thread ggshield/verticals/auth/oauth.py
Comment thread tests/unit/verticals/auth/test_oauth.py Outdated
The local OAuth callback server waited in handle_request() with no
timeout, so select() blocked until the browser redirect arrived. Python
runs signal handlers only between bytecodes, and on Windows Ctrl+C just
sets a flag that cannot wake select(), so the KeyboardInterrupt (and the
click.Abort it maps to) only fired once the login completed. POSIX was
unaffected because SIGINT interrupts poll() with EINTR.

Give the server a 0.5s timeout so the wait loop returns to Python
regularly and a pending interrupt is raised within that bound.

Two regression tests cover it: one flips the finished flag from another
thread and checks the loop notices, the other raises SIGINT from a helper
thread (which trips the flag without waking select(), the same shape as
Windows delivery) and checks click.Abort is raised within the bound. Both
fail without the timeout and run on the Windows CI legs too.
@benjaminrigaud-gg
benjaminrigaud-gg force-pushed the benjaminrigaud/end-987-ctrlc-does-not-abort-ggshield-auth-login-on-windows branch from ba64f2f to fdded12 Compare September 14, 2026 12:45
A peer that connects to the callback port but never sends a request line
(browser pre-connect, port scan) left the handler blocked in readline()
with no timeout. The server timeout only covers select() before accept(),
so the wait loop was wedged again and a Windows Ctrl+C went unnoticed,
one level deeper than the previous fix.

Give the request handler the same poll interval; the stdlib closes the
connection on the read timeout and the loop resumes.

Also make the SIGINT regression test fail with DID NOT RAISE instead of
hanging when the interrupt never fires, and cancel its timers so a stray
SIGINT cannot land in a later test.
@benjaminrigaud-gg benjaminrigaud-gg self-assigned this Sep 18, 2026
@benjaminrigaud-gg
benjaminrigaud-gg merged commit 1b53c31 into main Sep 18, 2026
75 of 76 checks passed
@benjaminrigaud-gg
benjaminrigaud-gg deleted the benjaminrigaud/end-987-ctrlc-does-not-abort-ggshield-auth-login-on-windows branch September 18, 2026 13:39
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