Skip to content

fix: TOTP/HOTP の otpauth URI で発行者名・アカウント未入力時の fallback を廃止 - #429

Merged
fumtas1k merged 3 commits into
developfrom
fix/totp-hotp-otpauth-uri-required-fields
May 13, 2026
Merged

fix: TOTP/HOTP の otpauth URI で発行者名・アカウント未入力時の fallback を廃止#429
fumtas1k merged 3 commits into
developfrom
fix/totp-hotp-otpauth-uri-required-fields

Conversation

@fumtas1k

Copy link
Copy Markdown
Owner

概要

TOTP/HOTP ツールで発行者名・アカウントが未入力のとき、otpauth URI に MyApp / user@example.com を fallback で埋めてしまう UX バグを修正します。

問題

otpauthUriuseMemo で:

issuer: issuer.trim() || 'MyApp',
account: accountLabel.trim() || 'user@example.com',

placeholder と同じ文字列が実 URI に書き込まれるため、ユーザーが入力忘れに気付かずコピー → 認証アプリで「MyApp / user@example.com」として登録されると、複数アカウントを並べたときに本人のどのサービスか判別不能になります。

修正

  • 両方が入力されるまで URI 生成を抑止 (!issuer.trim() || !accountLabel.trim() で early return)
  • 未入力時は OutputField 下に「発行者名とアカウントを両方入力すると otpauth URI が生成されます。」の案内を表示
  • buildOtpauthUri に fallback を渡さない

テスト

ID 内容 種類
1 初期空 → URI 空 + 案内表示 → 発行者名のみ → URI 空 → 両方入力 → URI 生成 陰性対照
2 secret 入力済み + issuer/account 空のとき URI に MyApp / user@example.com が含まれない 陽性対照 (旧 fallback 実装に当てれば fail)

動作確認

  • node_modules/.bin/astro check — 0 errors
  • npm run test:e2e --grep TOTP — 15 passed
  • Playwright 実機: 空入力時に URI 出力欄が空 + 案内テキスト表示を確認

🤖 Generated with Claude Code

旧実装は `issuer.trim() || 'MyApp'` / `accountLabel.trim() || 'user@example.com'`
で空入力時に fallback を埋めて URI を完成形で表示していたため、ユーザーが
入力忘れに気付かずコピーすると認証アプリで「MyApp / user@example.com」として
登録される事故が起き得た。

- 両方が入力されるまで URI 生成を抑止し空文字列を返す
- 未入力時は OutputField 下に案内テキスト「発行者名とアカウントを両方入力すると
  otpauth URI が生成されます。」を表示
- E2E 修正: 「発行者名・アカウント両方入力時のみ URI 生成」陰性対照に置き換え、
  さらに「fallback で MyApp / user@example.com を埋めない」陽性対照を追加。
  旧実装に当てれば fallback 文字列を検知して fail する設計

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@fumtas1k fumtas1k self-assigned this May 13, 2026
@github-actions

github-actions Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor

🖼️ Visual Regression Test 結果

  • Status: ✅ 全 40 件 pass
  • Workflow run: 25801483559
  • Artifact (diff 画像 / playwright-report): 上記 workflow run の Artifacts セクションから download

diff が 意図的な visual 変更の場合: Update Visual Regression Baseline workflow を本 PR ブランチで workflow_dispatch trigger して baseline を更新。
diff が 意図しない regression の場合: 該当変更を fix。
本 check は required ではないため fail のままでも merge は可能(reviewer 判断)。

@fumtas1k fumtas1k closed this May 13, 2026
@fumtas1k fumtas1k reopened this May 13, 2026
github-actions Bot added a commit that referenced this pull request May 13, 2026
@fumtas1k

Copy link
Copy Markdown
Owner Author

レビュー — 多角的評価

✅ 良い点

修正の正しさ

  • 実害のある UX trap を正しく特定: placeholder の "MyApp" / "user@example.com" がそのまま fallback として URI に埋め込まれていたため、ユーザーが「自分のサービス」として登録したつもりが認証アプリ上で placeholder 文字列に化けるという、実害ベース・気付きにくい・元に戻しにくい三拍子の bug。修正の必要性に異論なし。
  • 修正レイヤが適切: buildOtpauthUri (util) には fallback ロジックが入っていない(totp-hotp.ts:151-184)ため、修正は component 側の useMemo で正しく 'MyApp' / 'user@example.com' を削除。util の純粋関数性が守られている。

コードドキュメント

  • useMemo 内コメント(TotpHotpGenerator.tsx:196-198)が「なぜ fallback を消すか」の事故シナリオまで含めて明文化。CLAUDE.md の「WHY が非自明な場合のみコメント」基準を満たした良いコメント。

テスト

  • 陽性対照が機能的に有効: expect(value).not.toContain('MyApp') は旧実装に当てると issuer.trim() || 'MyApp' で URI が完成形になり 'MyApp' が混入 → fail する構造。silent regression を確実に捕捉する正しい positive control。
  • happy path + partial fill の網羅: 「空 → 片方 → 両方」遷移を 1 ケースで通しており、guard の boundary を検証している。

セキュリティ

  • 新規 attack surface なし。buildOtpauthUri 側で encodeURIComponent 済み、issuer の colon validation は既存ロジックを通過。

🟡 改善提案

1. ヒントメッセージが誤誘導する条件分岐(中優先度)

{!otpauthUri && secretBase32.trim() && !issuerHasColon && (
  <p className="caption text-muted">
    発行者名とアカウントを両方入力すると otpauth URI が生成されます。
  </p>
)}

!otpauthUriissuer/account 未入力以外の理由でも true になる:

URI が空になる理由 現在の表示 ユーザーの誤解
発行者名・アカウント未入力 ✅ メッセージ正しい
無効な Base32 secret(secretBytes === null だが secretBase32.trim() は truthy) ❌ 「発行者名とアカウントを両方...」と出る issuer/account を埋めても URI は出ないため二重に混乱
HOTP mode で counter エラー ❌ 同上 counter を直してもメッセージは消えない

つまり「invalid Base32 を入力した状態で issuer/account も空」のケースで、ユーザーが issuer/account を埋めても URI が生成されず、ヒントメッセージは消えない → 「両方入力したのになぜ?」と詰まる可能性。

修正案(条件を narrowing):

{secretBytes && !issuerHasColon && !(mode === 'hotp' && counterError) && (!issuer.trim() || !accountLabel.trim()) && (
  <p className="caption text-muted">
    発行者名とアカウントを両方入力すると otpauth URI が生成されます。
  </p>
)}

ポイント:

  • secretBytes (= 有効な Base32) を必須条件にする
  • HOTP の counter エラー時も非表示にする
  • !otpauthUri 代わりに「issuer/account が空」を 直接 条件にする方が意味的に明確

または otpauthUri の useMemo を { uri, reason } 型に拡張して reason ベースで個別メッセージを出す path もあるが、現状 3 ケースしかないので前者で十分。

2. SR 通知が無い(低優先度)

ヒント <p> は単なる static text。発行者名/アカウントを編集中の SR / 音声制御利用者は、URI が「未生成」のままであることを能動的にナビゲートしないと気付けない。role="status" または aria-live="polite" を付与すれば、入力をクリアした瞬間に announce される:

<p className="caption text-muted" role="status">
  発行者名とアカウントを両方入力すると otpauth URI が生成されます。
</p>

ただし入力中も毎 keystroke で発火する懸念があるなら aria-live は付けない判断もあり得る。判断は author に委ねる。


🟢 任意(スコープ外候補)

  • issuerHasColon と並列に accountHasColon も検討余地: account 自体に colon が含まれると buildOtpauthUri の label encode (${issuer}:${account}encodeURIComponent) で %3A 化されるため URI 自体は壊れないが、認証アプリ側のパース挙動は実装依存。本 PR スコープ外で扱う問題ではない。

CI 状況

test ✅ / visual-regression ✅ / cleanup ✅ / e2e ⏳ pending(3 + 1pending)

VRT baseline 差分はヒントメッセージ追加に伴う意図的な可視変更で -linux.png 命名から CI Linux runner 生成と判別可能。


結論

Approve 相当(軽い修正リクエストあり)。bug 修正自体は正しいが、指摘 1(ヒントメッセージの誤誘導条件)は同 PR で巻き取り推奨(条件 1 行の narrowing で fix 可能)。指摘 2 は本 PR か follow-up 化かは判断にお任せ。

- 指摘 1: ヒントメッセージ条件を narrowing。旧 `!otpauthUri` 条件だと invalid
  Base32 入力 / HOTP counter エラー時にも「発行者名とアカウントを両方入力すると」
  が出てしまい、user は issuer/account を埋めても URI 生成されず混乱する状態
  だった。`secretBytes && !issuerHasColon && !(mode === 'hotp' && counterError)
  && (issuer 空 || account 空)` で意図ケースのみ表示するよう narrowing
- 指摘 2: ヒント <p> に `role="status"` を付与。SR / 音声制御利用者にも
  表示・非表示の遷移時に announce される
- E2E 陽性対照追加: 無効 Base32 入力時にヒントが表示されないことを assert
  (旧条件に当てると fail する設計)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@fumtas1k

Copy link
Copy Markdown
Owner Author

レビューありがとうございます。指摘 1・2 を本 PR で巻き取りました(commit 0a7f054)。任意項目(account の colon 検証)は本 PR スコープ外として見送ります。

指摘 対応
1. ヒント条件の誤誘導 secretBytes && !issuerHasColon && !(mode === 'hotp' && counterError) && (issuer 空 || account 空) で narrowing。invalid Base32 / counter エラー時には表示しない
2. SR 通知 ✅ ヒント <p>role="status" を追加。SR 利用者にも表示・非表示遷移で announce される

陽性対照 E2E も追加: 無効 Base32 入力時にヒントが表示されないことを assert(旧 !otpauthUri 条件に当てると fail する設計)。

動作確認

  • node_modules/.bin/astro check: 0 errors
  • npm run test:e2e --grep TOTP: 16 passed

@fumtas1k

Copy link
Copy Markdown
Owner Author

再レビュー(commit 0a7f054

両指摘の対応 + 追加陽性対照テストを確認しました。

指摘 反映内容 評価
1. ヒント条件の誤誘導 secretBytes && !issuerHasColon && !(mode === 'hotp' && counterError) && (!issuer.trim() || !accountLabel.trim()) で narrowing(TotpHotpGenerator.tsx:495-498)。invalid Base32 / HOTP counter エラー / issuer colon エラー時は非表示、issuer/account 未入力時のみ表示 ✅ 提案通り。意味的に「issuer/account 未入力」を直接条件にしているので読みやすい
2. SR 通知 <p className="caption text-muted" role="status">TotpHotpGenerator.tsx:499)。表示・非表示遷移時に SR で announce される
追加: narrowing 自体の陽性対照 E2E「無効な Base32 入力時はヒントメッセージが表示されない」を追加。page.getByLabel('Base32 シークレット').fill('INVALID!')expect(...).not.toBeVisible() 指摘によって narrowing が入ったこと自体に対する positive control が成立。旧 !otpauthUri 条件に当てると INVALID!secretBytes === null → URI 空 → ヒント表示 → 本テスト fail。意図したガード壊しを確実に検知する設計

追加観点

CI 状況

test ✅ / e2e ⏳ pending / visual-regression ⏳ pending(1 pass、2 pending)

結論

LGTM。指摘事項は全件解消、narrowing の陽性対照まで追加されており質的にむしろ前進。新規懸念なし、CI(e2e / VRT)green 確認後に merge 可能状態です。

@fumtas1k
fumtas1k merged commit 0ff3429 into develop May 13, 2026
3 checks passed
@fumtas1k
fumtas1k deleted the fix/totp-hotp-otpauth-uri-required-fields branch May 13, 2026 13:22
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.

1 participant