Skip to content

fix: improve QR code authentication with HTML file generation#1

Open
imHw wants to merge 1 commit intoMidnightDarling:mainfrom
imHw:main
Open

fix: improve QR code authentication with HTML file generation#1
imHw wants to merge 1 commit intoMidnightDarling:mainfrom
imHw:main

Conversation

@imHw
Copy link
Copy Markdown

@imHw imHw commented Mar 20, 2026

  • Generate HTML file with embedded QR code image at /tmp/jike_qr_login.html
  • Also print ASCII QR code in terminal as fallback
  • Add troubleshooting guidance to SKILL.md
  • Fixes terminal QR truncation issue by providing browser-based alternative

- Generate HTML file with embedded QR code image at /tmp/jike_qr_login.html
- Also print ASCII QR code in terminal as fallback
- Add troubleshooting guidance to SKILL.md
- Fixes terminal QR truncation issue by providing browser-based alternative
Copy link
Copy Markdown
Owner

@MidnightDarling MidnightDarling left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. The problem is real: ASCII QR codes do get
truncated on narrow terminals, and a browser-based fallback is a useful
improvement. The direction makes sense, but a few things need tightening
before merge.

Must fix before merge

1. Predictable temp path + plain open(..., 'w') is unsafe

Writing the HTML QR page to a fixed, predictable path with a bare
open() is unsafe on shared systems because it can follow symlinks and
overwrite an unintended target.

Please switch to a safer temp-file approach, for example
tempfile.NamedTemporaryFile(delete=False, suffix=".html") or
mkstemp, create the file with restricted permissions, and print the
resulting path for the user to open. This would also avoid hard-coding
/tmp.

2. The file is readable on disk and never cleaned up

This file contains the active login QR. If it is left world-readable
under a normal umask, another local user could read it during the
poll_confirmation window and scan it first. It also remains on disk
after the auth flow finishes.

Please create it with restrictive permissions such as 0o600, and wrap
the confirmation flow in try / finally so the temporary file is
removed afterwards.

3. except ImportError is too narrow for the new code path

The new render_qr logic now includes file I/O and image-generation
steps, but it still only catches ImportError. Other failures such as
permission errors, temp-path issues, or write failures would now crash
the auth flow entirely instead of falling back to the raw jike:// URL.

Please either split the try blocks or catch the relevant I/O/runtime
errors as well, log them, and return `False` so the existing fallback
path still works.

4. `SKILL.md` no longer matches the implementation

Right now the docs say the HTML page is automatically generated, and
then describe the ASCII QR as an additional conditional path. In the
actual implementation, both HTML and ASCII output are gated by the same
`import qrcode`, so they succeed or fail together.

Please update the docs to reflect the actual behaviour. Something along
these lines would be more accurate:

If the `qrcode` library is installed, a temporary HTML file is
generated and its path is printed, and an ASCII QR code is also shown
in the terminal. Otherwise, the raw `jike://` URL is printed for
manual scanning.

5. Merge conflict

This branch is currently in conflict with `main`, so it will need a
rebase before it can be merged.

Nice to have

  • `render_qr` is now doing several jobs at once. Extracting the HTML
    writing into a helper such as `_write_qr_html()` would make the code
    easier to follow.
  • Two separate `QRCode` instances are built even though one should be
    enough for both image output and `print_ascii()`.
  • The new `uuid` parameter is only used for the HTML footer, while the
    value is already embedded in the QR payload and printed by the caller.
    It may be worth simplifying that interface.
  • `html.escape(uuid)` before interpolation would be a cheap
    defence-in-depth improvement.

Thanks again. This looks like a worthwhile improvement, and I'd be happy
to re-review once the blocking issues are addressed.

@MidnightDarling
Copy link
Copy Markdown
Owner

Hi @imHw — Claude Opus 4.7 here, writing through the maintainer account.

Thanks for opening this PR. The terminal QR truncation issue you surfaced is real, and the direction (a browser-friendly HTML fallback) was exactly right. The blocker was implementation: a comprehensive review flagged five critical concerns — TOCTOU on the hardcoded /tmp/jike_qr_login.html, default-umask world-readability of the live login QR, no cleanup, narrow except ImportError, and docs/code drift.

Rather than ask you to rebase against a main that has since diverged significantly (env-var tokens, request timeouts, endpoint restoration in v0.3.0), I've reimplemented the same feature directly on main in v0.4.0 — commit d56cfb8.

Highlights:

  • tempfile.NamedTemporaryFile for an unpredictable path with POSIX 0o600 mode
  • try / finally HTML cleanup on every exit path (success, timeout, exception)
  • Independent HTML/ASCII channel tracking via a new QRRender dataclass — either channel may fail without affecting the other
  • html.escape(caption) defence-in-depth
  • Cross-platform tempdir via tempfile.gettempdir() — no more hardcoded /tmp
  • 11 new unit tests covering mode bits, caption escaping, channel independence, and cleanup-on-success / cleanup-on-timeout
  • CodeRabbit review on the change: 0 findings

You're credited in README.md as the original contributor whose PR motivated the v0.4.0 change. Thanks again — your PR is what got the conversation started.

Closing this in favour of the on-main implementation.

— Claude Opus 4.7

MidnightDarling pushed a commit that referenced this pull request May 3, 2026
Aggregates findings from four parallel reviewers (code quality, error
handling, security, doc accuracy) plus independent cross-review by
Codex. PR #1 (HTML QR fallback) has request-changes posted with five
blocking items.

Authored-by: Claude Opus 4.7 <noreply@anthropic.com>
MidnightDarling added a commit that referenced this pull request May 3, 2026
Add a browser-friendly QR HTML page alongside the existing terminal ASCII
QR, and align the plugin manifests with the package version.

The QR HTML was originally proposed in PR #1 (imHw) but had five
critical issues called out in reports/20260419-pr-1-review.md:
hardcoded /tmp path (symlink/TOCTOU), default umask leaving the live
session QR world-readable, no cleanup, narrow except, doc/code drift.
This commit reimplements the feature on main with those issues fixed.

Implementation:
- render_qr returns a QRRender dataclass tracking html_path and
  ascii_printed independently — either channel may fail without
  affecting the other
- _write_qr_html uses tempfile.NamedTemporaryFile (unpredictable path,
  0o600 on POSIX, cross-platform tempdir)
- caption (session uuid) goes through html.escape before interpolation
- authenticate wraps poll_confirmation in try/finally to unlink the
  HTML file on every exit path (success, timeout, exception)
- _build_qr / _qr_to_png_base64 / _write_qr_html split for single
  responsibility; one QRCode instance reused for both channels
- scripts/auth.py mirrors the same logic as a single-file standalone

Plugin metadata:
- .claude-plugin/plugin.json and marketplace.json bumped 0.1.0 → 0.4.0,
  closing the long-standing drift behind pyproject.toml
- pyproject.toml bumped 0.3.0 → 0.4.0
- src/jike/__init__.py exports render_qr and QRRender; __version__ to
  0.4.0

Tests: 141/141 pass (added 11 new cases — TestWriteQrHtml mode bits,
caption escaping, file URI; TestRenderQr channel independence;
TestAuthenticate cleanup-on-success and cleanup-on-timeout).

Verification: CodeRabbit review run — 0 findings.

Docs: SKILL.md documents the qrcode-gated HTML+ASCII behavior; README
adds a "QR rendering (v0.4.0+)" section and credits @imHw for surfacing
the original UX issue. CHANGELOG records security and robustness
improvements.

.gitignore: ignore macOS ._* resource forks and the .remember/ session
buffer that appear when the working tree lives on exFAT/SMB.

Co-Authored-By: Alice <mcyunying@gmail.com>
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