feat: modern auth (SCRAM-SHA-256, 2FA, WebAuthn) + tic-tac-toe#180
Merged
Conversation
Adds three new SASL paths plus an enrolment UI for second-factor credentials, all behind a single feature branch. * `src/lib/sasl/scram.ts` - SCRAM-SHA-256 (RFC 7677) using Web Crypto for PBKDF2/HMAC/SHA-256. Picked automatically when the server advertises it; falls back to PLAIN. * `src/lib/sasl/webauthn.ts` - thin `navigator.credentials` wrapper for the DRAFT-WEBAUTHN-BIO mechanism and `2FA ADD webauthn` enrolment. * `src/store/handlers/auth.ts` - per-server SASL session state machine that dispatches AUTHENTICATE messages by mechanism and routes `AUTHENTICATE 2FA-REQUIRED` to the step-up modal. * `2FA` IRC command + `TWOFA` / `TWOFA_NOTE` events for status, listing, enrolment, removal, enable/disable replies from the server. * `TotpStepUpModal` - prompts for the 6-digit code mid-SASL. * `TwoFactorSettingsModal` - status, credential list, TOTP enrol with QR (qrcode dep), WebAuthn biometric enrol via `navigator.credentials. create`, removal, and password-free disable via TOTP proof. * Wired into `EditServerModal` (button visible when the server advertises `draft/account-2fa`).
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (27)
📝 WalkthroughWalkthroughAdds two independent feature sets: comprehensive two-factor authentication (SCRAM-SHA-256, WebAuthn, TOTP UI, store/IRC hooks, and qrcode dependency) and a Tic‑Tac‑Toe game (protocol, game model, store actions, handlers, UI, and Chat header wiring). ChangesTwo-Factor Authentication (2FA) Flow
Tic‑Tac‑Toe Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Store
participant IRCClient
participant Server
participant WebAuthn
User->>UI: Open TwoFactorSettingsModal / start enrollment
UI->>Store: twofaChallenge(serverId, type)
Store->>IRCClient: sendRaw TWOFA challenge request
IRCClient->>Server: transport request
Server-->>IRCClient: challenge blob (base64 JSON)
IRCClient->>Store: dispatch pendingTwofaChallenge(blob)
Store->>UI: pendingTwofaChallenge updated
UI-->>WebAuthn: webauthnRegister(challenge) (if webauthn)
WebAuthn-->>UI: attestation result
UI->>Store: twofaAdd(serverId, "webauthn", name, payload)
Store->>IRCClient: sendRaw TWOFA add with payload
IRCClient->>Server: send enrollment result
Server-->>IRCClient: TWOFA/TWOFA_NOTE event
IRCClient->>Store: dispatch TWOFA/TWOFA_NOTE -> update twofaStatus/credentials
Store->>UI: update credential list/status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
package.json (1)
41-41: 💤 Low valueMove
@types/qrcodetodevDependencies.Type definition packages are only needed at compile time and should be in
devDependencies, notdependencies. This reduces the production bundle and aligns with the placement of other@types/*packages in this project (e.g.,@types/dompurify,@types/react).📦 Suggested fix
Move line 41 (
"@types/qrcode": "^1.5.6") fromdependenciestodevDependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 41, The package "@types/qrcode" is listed under dependencies but should be a devDependency; remove the "@types/qrcode": "^1.5.6" entry from the dependencies block in package.json and add the same entry to the devDependencies block (matching the project's version style for other `@types` packages like "@types/dompurify" and "@types/react"), then run a package manager install to update lockfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/TotpStepUpModal.tsx`:
- Around line 14-21: The effect inside the TotpStepUpModal component schedules a
requestAnimationFrame to focus inputRef but never cancels it; capture the rAF id
when calling requestAnimationFrame and return a cleanup that calls
cancelAnimationFrame(id) so the scheduled callback is cancelled on unmount or
when pending changes, ensuring no post-unmount side effects from the
inputRef.current?.focus() call and preserving the existing setCode/setError
behavior.
In `@src/components/ui/TwoFactorSettingsModal.tsx`:
- Around line 63-66: twofaListQuery currently resets twofaCredentials[serverId]
instead of requesting and emitting the list, so opening the modal clears the UI;
update the twofaListQuery implementation in the store (the function named
twofaListQuery in src/store/index.ts that touches twofaCredentials[serverId]) to
perform the actual fetch/IPC request and dispatch a "2FA LIST" action (with the
returned credentials payload) rather than just resetting the array, or
alternatively ensure it triggers the existing action that emits "2FA LIST" so
the modal receives and populates the credentials; keep twofaStatusQuery usage in
useEffect as-is.
- Around line 129-135: The WebAuthn attestation payload produced in
webauthnRegister (encoded via bytesToB64Std into payload) is far too large to be
sent in one IRC command via twofaAdd; change the flow to upload or store the
full attestation blob to the server (or an existing upload API) first and then
call twofaAdd(serverId, "webauthn", name, referenceId) with a small reference/ID
instead of the full payload; update the code around webauthnRegister /
bytesToB64Std / twofaAdd to POST the attestation to a safe endpoint, retrieve a
short token/ID, and pass that token to twofaAdd, keeping setEnrollBusy usage the
same.
- Around line 326-364: The UI currently only checks credentials.length when
enabling 2FA, which allows enabling when only WebAuthn exists but the disable
flow (onDisable) always expects a TOTP code; update the enable button to require
a TOTP credential instead: compute a hasTotp flag by checking credentials.some(c
=> c.type === 'totp') and use it to set disabled and the button text (e.g., "Add
a TOTP credential to enable 2FA" vs "Enable 2FA"); also ensure
twofaEnable(serverId) still receives serverId unchanged (no behavior change
needed) so the user cannot enable 2FA unless hasTotp is true.
- Around line 114-141: The effect starts an async webauthnRegister and never
cancels it, so if the modal closes or serverId changes the then/catch still run
and call twofaAdd or update state from a stale closure; fix by adding a local
"cancelled" flag (e.g., let cancelled = false) at the top of the useEffect and
return a cleanup () => { cancelled = true; } and then check if (cancelled)
before calling twofaAdd, setEnrollBusy, or setEnrollError inside the .then and
.catch handlers of webauthnRegister; keep the existing guards (challenge,
enrollBusy) and the same parameters for webauthnRegister, but early-return from
promise handlers when cancelled to avoid submitting or mutating state for a
stale serverId/enrollment.
In `@src/store/handlers/auth.ts`:
- Around line 34-45: The chooseMechanism function currently returns "PLAIN" even
when the server doesn't advertise it; change the branches to only return "PLAIN"
when available.includes("PLAIN") is true (i.e., for the pref === "PLAIN" case
and the final fallback), and otherwise fall through to other supported mech
choices (prefer "SCRAM-SHA-256" if available, otherwise pick a supported
advertised mechanism or return the first available advertised mechanism). Update
chooseMechanism to check availability before returning "PLAIN" and to gracefully
pick a valid advertised mechanism (or the first element of available) when the
preferred one isn't advertised; refer to function chooseMechanism and type
SaslMech.
- Around line 203-212: The AUTHENTICATE payload can exceed a single IRC frame;
after building the JSON reply from webauthnAssert (the current reply variable),
base64-encode it with bytesToB64Std and split that base64 string into safe-sized
chunks (e.g., ~400 chars) and send each chunk in sequence via
ircClient.sendRaw(serverId, `AUTHENTICATE ${chunk}`) inside a loop; ensure the
final chunk is sent as the last AUTHENTICATE and keep session.step update
(session.step = 2) after the final send. Use the existing
TextEncoder/TextDecoder and webauthnAssert flow but replace the single sendRaw
call with a chunking loop that sends multiple AUTHENTICATE frames.
- Around line 377-383: The CAP negotiation is being closed only when
savedServer?.saslPassword exists, which breaks passwordless SASL (e.g.,
DRAFT-WEBAUTHN-BIO) because AUTHENTICATE is sent but CAP END is allowed
immediately; change the condition in the block that checks caps.some(cap =>
cap.startsWith("sasl")) to set preventCapEnd = true whenever the loaded
savedServer indicates saslEnabled is true (savedServer?.saslEnabled) regardless
of saslPassword so the SASL exchange (AUTHENTICATE / CAP_ACKNOWLEDGED) can
complete; update the logic around storage.servers.load(), savedServer, serverId,
and preventCapEnd accordingly.
In `@src/types/index.ts`:
- Around line 62-64: The comment for the saslMechanism field is incorrect: it
mentions "webauthn" but the type union for saslMechanism on the same line only
accepts "DRAFT-WEBAUTHN-BIO"; update the comment to refer to
"DRAFT-WEBAUTHN-BIO" (or alternatively add "webauthn" to the union if that value
is intended) so the documented mechanism name matches the union type for
saslMechanism in src/types/index.ts.
---
Nitpick comments:
In `@package.json`:
- Line 41: The package "@types/qrcode" is listed under dependencies but should
be a devDependency; remove the "@types/qrcode": "^1.5.6" entry from the
dependencies block in package.json and add the same entry to the devDependencies
block (matching the project's version style for other `@types` packages like
"@types/dompurify" and "@types/react"), then run a package manager install to
update lockfile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a773824-a4c1-49c5-b11e-bfbb47a27313
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
package.jsonsrc/App.tsxsrc/components/ui/EditServerModal.tsxsrc/components/ui/TotpStepUpModal.tsxsrc/components/ui/TwoFactorSettingsModal.tsxsrc/lib/irc/IRCClient.tssrc/lib/irc/handlers/auth.tssrc/lib/irc/handlers/index.tssrc/lib/sasl/scram.tssrc/lib/sasl/webauthn.tssrc/store/handlers/auth.tssrc/store/index.tssrc/types/index.tstests/fixtures/uiState.ts
Comment on lines
+14
to
+21
| useEffect(() => { | ||
| if (pending) { | ||
| setCode(""); | ||
| setError(null); | ||
| // Autofocus once the modal mounts. | ||
| requestAnimationFrame(() => inputRef.current?.focus()); | ||
| } | ||
| }, [pending]); |
Contributor
There was a problem hiding this comment.
Add cleanup to cancel the requestAnimationFrame call.
The requestAnimationFrame on line 19 isn't cancelled if the component unmounts before the callback fires. While the optional chaining on inputRef.current?.focus() prevents a crash, the guidelines require canceling nested rAF calls in effect cleanup to prevent post-unmount side effects.
🛡️ Proposed fix
useEffect(() => {
if (pending) {
setCode("");
setError(null);
// Autofocus once the modal mounts.
- requestAnimationFrame(() => inputRef.current?.focus());
+ const rafId = requestAnimationFrame(() => inputRef.current?.focus());
+ return () => cancelAnimationFrame(rafId);
}
}, [pending]);As per coding guidelines: "Cancel nested requestAnimationFrame calls in effect cleanup to avoid post-unmount setState errors".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (pending) { | |
| setCode(""); | |
| setError(null); | |
| // Autofocus once the modal mounts. | |
| requestAnimationFrame(() => inputRef.current?.focus()); | |
| } | |
| }, [pending]); | |
| useEffect(() => { | |
| if (pending) { | |
| setCode(""); | |
| setError(null); | |
| // Autofocus once the modal mounts. | |
| const rafId = requestAnimationFrame(() => inputRef.current?.focus()); | |
| return () => cancelAnimationFrame(rafId); | |
| } | |
| }, [pending]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/TotpStepUpModal.tsx` around lines 14 - 21, The effect
inside the TotpStepUpModal component schedules a requestAnimationFrame to focus
inputRef but never cancels it; capture the rAF id when calling
requestAnimationFrame and return a cleanup that calls cancelAnimationFrame(id)
so the scheduled callback is cancelled on unmount or when pending changes,
ensuring no post-unmount side effects from the inputRef.current?.focus() call
and preserving the existing setCode/setError behavior.
Comment on lines
+114
to
+141
| useEffect(() => { | ||
| if ( | ||
| !challenge || | ||
| challenge.serverId !== serverId || | ||
| challenge.type !== "webauthn" || | ||
| enrollBusy | ||
| ) | ||
| return; | ||
| const decoded = decodeChallenge(challenge.blob); | ||
| if (!decoded || typeof decoded !== "object") { | ||
| setEnrollError("Server returned an invalid WebAuthn challenge."); | ||
| return; | ||
| } | ||
| const name = enrollName.trim() || `Device-${Date.now()}`; | ||
| setEnrollBusy(true); | ||
| webauthnRegister(decoded as Parameters<typeof webauthnRegister>[0]) | ||
| .then((result) => { | ||
| const payload = bytesToB64Std( | ||
| new TextEncoder().encode(JSON.stringify(result)), | ||
| ); | ||
| twofaAdd(serverId, "webauthn", name, payload); | ||
| setEnrollBusy(false); | ||
| }) | ||
| .catch((err) => { | ||
| setEnrollError(err instanceof Error ? err.message : String(err)); | ||
| setEnrollBusy(false); | ||
| }); | ||
| }, [challenge?.blob, challenge?.type, challenge?.serverId, serverId]); |
Contributor
There was a problem hiding this comment.
Cancel the in-flight WebAuthn enrollment effect.
If the modal closes or serverId changes while the biometric prompt is open, this promise chain still calls twofaAdd and updates local state from a stale closure. That can submit an enrollment after the user already dismissed the dialog.
Suggested guard
useEffect(() => {
+ let cancelled = false;
if (
!challenge ||
challenge.serverId !== serverId ||
challenge.type !== "webauthn" ||
enrollBusy
@@
const name = enrollName.trim() || `Device-${Date.now()}`;
setEnrollBusy(true);
webauthnRegister(decoded as Parameters<typeof webauthnRegister>[0])
.then((result) => {
+ if (cancelled) return;
const payload = bytesToB64Std(
new TextEncoder().encode(JSON.stringify(result)),
);
twofaAdd(serverId, "webauthn", name, payload);
- setEnrollBusy(false);
})
.catch((err) => {
+ if (cancelled) return;
setEnrollError(err instanceof Error ? err.message : String(err));
- setEnrollBusy(false);
+ })
+ .finally(() => {
+ if (!cancelled) setEnrollBusy(false);
});
+ return () => {
+ cancelled = true;
+ };
}, [challenge?.blob, challenge?.type, challenge?.serverId, serverId]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if ( | |
| !challenge || | |
| challenge.serverId !== serverId || | |
| challenge.type !== "webauthn" || | |
| enrollBusy | |
| ) | |
| return; | |
| const decoded = decodeChallenge(challenge.blob); | |
| if (!decoded || typeof decoded !== "object") { | |
| setEnrollError("Server returned an invalid WebAuthn challenge."); | |
| return; | |
| } | |
| const name = enrollName.trim() || `Device-${Date.now()}`; | |
| setEnrollBusy(true); | |
| webauthnRegister(decoded as Parameters<typeof webauthnRegister>[0]) | |
| .then((result) => { | |
| const payload = bytesToB64Std( | |
| new TextEncoder().encode(JSON.stringify(result)), | |
| ); | |
| twofaAdd(serverId, "webauthn", name, payload); | |
| setEnrollBusy(false); | |
| }) | |
| .catch((err) => { | |
| setEnrollError(err instanceof Error ? err.message : String(err)); | |
| setEnrollBusy(false); | |
| }); | |
| }, [challenge?.blob, challenge?.type, challenge?.serverId, serverId]); | |
| useEffect(() => { | |
| let cancelled = false; | |
| if ( | |
| !challenge || | |
| challenge.serverId !== serverId || | |
| challenge.type !== "webauthn" || | |
| enrollBusy | |
| ) | |
| return; | |
| const decoded = decodeChallenge(challenge.blob); | |
| if (!decoded || typeof decoded !== "object") { | |
| setEnrollError("Server returned an invalid WebAuthn challenge."); | |
| return; | |
| } | |
| const name = enrollName.trim() || `Device-${Date.now()}`; | |
| setEnrollBusy(true); | |
| webauthnRegister(decoded as Parameters<typeof webauthnRegister>[0]) | |
| .then((result) => { | |
| if (cancelled) return; | |
| const payload = bytesToB64Std( | |
| new TextEncoder().encode(JSON.stringify(result)), | |
| ); | |
| twofaAdd(serverId, "webauthn", name, payload); | |
| }) | |
| .catch((err) => { | |
| if (cancelled) return; | |
| setEnrollError(err instanceof Error ? err.message : String(err)); | |
| }) | |
| .finally(() => { | |
| if (!cancelled) setEnrollBusy(false); | |
| }); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [challenge?.blob, challenge?.type, challenge?.serverId, serverId]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/TwoFactorSettingsModal.tsx` around lines 114 - 141, The
effect starts an async webauthnRegister and never cancels it, so if the modal
closes or serverId changes the then/catch still run and call twofaAdd or update
state from a stale closure; fix by adding a local "cancelled" flag (e.g., let
cancelled = false) at the top of the useEffect and return a cleanup () => {
cancelled = true; } and then check if (cancelled) before calling twofaAdd,
setEnrollBusy, or setEnrollError inside the .then and .catch handlers of
webauthnRegister; keep the existing guards (challenge, enrollBusy) and the same
parameters for webauthnRegister, but early-return from promise handlers when
cancelled to avoid submitting or mutating state for a stale serverId/enrollment.
Comment on lines
+326
to
+364
| {status === "enabled" ? ( | ||
| <div className="p-3 rounded bg-discord-dark-300"> | ||
| <p className="text-sm text-white mb-2"> | ||
| Disable 2FA. Enter a 6-digit code from any of your registered | ||
| authenticator apps to confirm. | ||
| </p> | ||
| <div className="flex gap-2"> | ||
| <input | ||
| type="text" | ||
| inputMode="numeric" | ||
| maxLength={6} | ||
| pattern="\d{6}" | ||
| placeholder="000000" | ||
| value={disableCode} | ||
| onChange={(e) => | ||
| setDisableCode(e.target.value.replace(/\D/g, "")) | ||
| } | ||
| className="flex-1 px-3 py-2 rounded bg-discord-dark-200 text-white tracking-[0.4em] text-center font-mono" | ||
| /> | ||
| <button | ||
| type="button" | ||
| onClick={onDisable} | ||
| className="px-3 py-2 rounded bg-discord-red text-white text-sm font-medium" | ||
| > | ||
| Disable | ||
| </button> | ||
| </div> | ||
| </div> | ||
| ) : ( | ||
| <button | ||
| type="button" | ||
| onClick={() => twofaEnable(serverId)} | ||
| disabled={credentials.length === 0} | ||
| className="w-full px-3 py-2 rounded bg-discord-green text-white text-sm font-medium disabled:opacity-50 disabled:cursor-not-allowed" | ||
| > | ||
| {credentials.length === 0 | ||
| ? "Add a credential to enable 2FA" | ||
| : "Enable 2FA"} | ||
| </button> |
Contributor
There was a problem hiding this comment.
Don't allow enabling 2FA without a TOTP credential.
The disable path always sends 2FA DISABLE totp <code>, so an account enabled with only WebAuthn has no way to turn 2FA back off from this UI. Either add a non-TOTP disable proof or require at least one TOTP credential before enabling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/TwoFactorSettingsModal.tsx` around lines 326 - 364, The UI
currently only checks credentials.length when enabling 2FA, which allows
enabling when only WebAuthn exists but the disable flow (onDisable) always
expects a TOTP code; update the enable button to require a TOTP credential
instead: compute a hasTotp flag by checking credentials.some(c => c.type ===
'totp') and use it to set disabled and the button text (e.g., "Add a TOTP
credential to enable 2FA" vs "Enable 2FA"); also ensure twofaEnable(serverId)
still receives serverId unchanged (no behavior change needed) so the user cannot
enable 2FA unless hasTotp is true.
Comment on lines
+34
to
+45
| function chooseMechanism( | ||
| available: string[], | ||
| pref: "auto" | "PLAIN" | "SCRAM-SHA-256" | "DRAFT-WEBAUTHN-BIO" | undefined, | ||
| ): SaslMech { | ||
| if (pref === "DRAFT-WEBAUTHN-BIO" && available.includes("DRAFT-WEBAUTHN-BIO")) | ||
| return "DRAFT-WEBAUTHN-BIO"; | ||
| if (pref === "PLAIN") return "PLAIN"; | ||
| if (pref === "SCRAM-SHA-256" && available.includes("SCRAM-SHA-256")) | ||
| return "SCRAM-SHA-256"; | ||
| // auto: prefer SCRAM-SHA-256 over PLAIN. | ||
| if (available.includes("SCRAM-SHA-256")) return "SCRAM-SHA-256"; | ||
| return "PLAIN"; |
Contributor
There was a problem hiding this comment.
Don't select PLAIN unless the server advertises it.
When pref === "PLAIN" — or when auto falls through — this returns PLAIN even if available excludes it. That turns a user preference into an unsupported AUTHENTICATE PLAIN and guarantees SASL failure on mechanism-restricted servers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/handlers/auth.ts` around lines 34 - 45, The chooseMechanism
function currently returns "PLAIN" even when the server doesn't advertise it;
change the branches to only return "PLAIN" when available.includes("PLAIN") is
true (i.e., for the pref === "PLAIN" case and the final fallback), and otherwise
fall through to other supported mech choices (prefer "SCRAM-SHA-256" if
available, otherwise pick a supported advertised mechanism or return the first
available advertised mechanism). Update chooseMechanism to check availability
before returning "PLAIN" and to gracefully pick a valid advertised mechanism (or
the first element of available) when the preferred one isn't advertised; refer
to function chooseMechanism and type SaslMech.
Comment on lines
+203
to
+212
| const challengeJson = JSON.parse( | ||
| new TextDecoder().decode(b64StdDecode(param)), | ||
| ); | ||
| const assertion = await webauthnAssert(challengeJson); | ||
| const reply = JSON.stringify(assertion); | ||
| session.step = 2; | ||
| ircClient.sendRaw( | ||
| serverId, | ||
| `AUTHENTICATE ${bytesToB64Std(new TextEncoder().encode(reply))}`, | ||
| ); |
Contributor
There was a problem hiding this comment.
Chunk the WebAuthn AUTHENTICATE reply.
WebAuthn assertions are far larger than a single IRC AUTHENTICATE payload. Sending the base64 JSON in one frame will overflow or truncate on the wire, so passwordless login won't work against compliant servers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/handlers/auth.ts` around lines 203 - 212, The AUTHENTICATE payload
can exceed a single IRC frame; after building the JSON reply from webauthnAssert
(the current reply variable), base64-encode it with bytesToB64Std and split that
base64 string into safe-sized chunks (e.g., ~400 chars) and send each chunk in
sequence via ircClient.sendRaw(serverId, `AUTHENTICATE ${chunk}`) inside a loop;
ensure the final chunk is sent as the last AUTHENTICATE and keep session.step
update (session.step = 2) after the final send. Use the existing
TextEncoder/TextDecoder and webauthnAssert flow but replace the single sendRaw
call with a chunking loop that sends multiple AUTHENTICATE frames.
Implements the same TAGMSG protocol used by ItsOnlyBinary/kiwiirc-plugin-tictactoe so KiwiIRC and ObsidianIRC users can play each other. * `src/lib/games/tictactoe.ts` -- pure game logic: board, turn bookkeeping, win/draw detection, marker assignment. * `src/lib/games/tictactoeProtocol.ts` -- IRC message-tag escape and unescape per IRCv3 message-tags. * `src/store/handlers/tictactoeActions.ts` -- store-side actions (invite/accept/decline/move/terminate) that mutate state and emit TAGMSGs over `+kiwiirc.com/ttt`. * `src/store/handlers/tictactoe.ts` -- inbound TAGMSG dispatch. Auto-opens the modal on incoming invites, replays moves into the local board, detects out-of-sync turns, handles forfeit / decline. * `src/components/ui/TicTacToeModal.tsx` -- minimal 3x3 board UI with invite accept/decline, in-game forfeit, and game-over close. * "Play Tic-Tac-Toe" entry in the chat-header overflow menu when a private chat is selected.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/layout/ChatHeader.tsx`:
- Around line 319-330: The DM header never renders the overflow trigger so the
"Play Tic-Tac-Toe" overflow item cannot be reached; add the same overflow button
used in the channel header to the private chat action-button row so it can open
HeaderOverflowMenu. Specifically, inside the private chat section where
selectedPrivateChat is used (the DM action buttons block just before its closing
</div>), add a button that calls setIsOverflowMenuOpen(true) (matching the
existing handler used when selectedChannel is truthy) and uses the FaEllipsisV
icon so isOverflowMenuOpen can control rendering of HeaderOverflowMenu and
surface the "Play Tic-Tac-Toe" menu item.
In `@src/components/ui/TicTacToeModal.tsx`:
- Around line 86-113: TicTacToeModal's interactive <td> lacks an ARIA role;
update the cell rendering (the <td> where playable, move, open.serverId,
open.opponent, r, c are used) to include role="button" only when playable (e.g.
role={playable ? "button" : undefined}) so screen readers announce it as an
actionable control; optionally add aria-disabled or aria-pressed if you need to
convey disabled or toggle state, but at minimum add the conditional role to the
<td>.
In `@src/store/handlers/tictactoe.ts`:
- Around line 95-98: The IRC tag value is being incorrectly escaped with `\:`
which turns `:` into `;` and breaks JSON on the receiver; update the call that
builds the tag in the `ircClient.sendRaw` invocation (the one using `TTT_TAG`,
`serverId` and `opponent`) to wrap the JSON payload with
`escapeTagValue(JSON.stringify(...))` instead of manual backslash escapes,
import `escapeTagValue` from `tictactoeProtocol` alongside `unescapeTagValue` if
not already imported, and apply the same change for any other tag-value sends in
this file (mirror how `sendTtt` in `tictactoeActions.ts` constructs tags).
In `@src/store/handlers/tictactoeActions.ts`:
- Around line 88-101: The invite (and similarly accept) handler mutates the live
Zustand state by modifying the object returned from getGame; instead create a
fresh game object instead of assigning fields on g. Use getGame(get(), serverId,
opponent) to read, then build a new object via object spread (e.g., const
newGame = { ...existingOrMakeGame, invitePending: true, inviteIncoming: false,
terminated: false, statusMessage: `Invite sent to ${opponent}` }) and pass that
to setGame(set, serverId, opponent, newGame); update the set((state) => ({
tictactoe: { ...state.tictactoe, open: { serverId, opponent } } })) call
unchanged. Apply the same pattern in accept (replace in-place assignments with a
single spread-created object) and ensure no direct mutations of objects returned
by getGame, localNick, makeGame, setGame are performed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9bdba62-cb46-4ff0-b3e8-e88aba0da08d
📒 Files selected for processing (9)
src/App.tsxsrc/components/layout/ChatHeader.tsxsrc/components/ui/TicTacToeModal.tsxsrc/lib/games/tictactoe.tssrc/lib/games/tictactoeProtocol.tssrc/store/handlers/index.tssrc/store/handlers/tictactoe.tssrc/store/handlers/tictactoeActions.tssrc/store/index.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/games/tictactoeProtocol.ts
- src/App.tsx
Comment on lines
+319
to
+330
| { | ||
| label: "Play Tic-Tac-Toe", | ||
| icon: <span aria-hidden="true">🎮</span>, | ||
| onClick: () => { | ||
| if (selectedServerId && selectedPrivateChat) { | ||
| useStore | ||
| .getState() | ||
| .tictactoeInvite(selectedServerId, selectedPrivateChat.username); | ||
| } | ||
| }, | ||
| show: !!selectedPrivateChat, | ||
| }, |
Contributor
There was a problem hiding this comment.
"Play Tic-Tac-Toe" is unreachable — missing overflow trigger in the DM header.
The overflow <button> that calls setIsOverflowMenuOpen (line 625) lives inside the {selectedChannel && …} block. When a private chat is selected, that block never renders, so isOverflowMenuOpen is never set to true and the HeaderOverflowMenu never opens. The new item effectively has no entry point.
The overflow trigger needs to be added to the private chat header section (around line 860, just before the closing </div> of the DM action buttons):
🐛 Proposed fix — add overflow trigger to the DM action‑button row
+ <button
+ ref={overflowButtonRef}
+ className="md:hidden p-2 hover:text-discord-text-normal"
+ onClick={() => setIsOverflowMenuOpen(!isOverflowMenuOpen)}
+ aria-label="More actions"
+ aria-expanded={isOverflowMenuOpen}
+ title="More"
+ >
+ <FaEllipsisV />
+ </button>
</div>
)}
</div>
)}FaEllipsisV is already imported (line 10).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/layout/ChatHeader.tsx` around lines 319 - 330, The DM header
never renders the overflow trigger so the "Play Tic-Tac-Toe" overflow item
cannot be reached; add the same overflow button used in the channel header to
the private chat action-button row so it can open HeaderOverflowMenu.
Specifically, inside the private chat section where selectedPrivateChat is used
(the DM action buttons block just before its closing </div>), add a button that
calls setIsOverflowMenuOpen(true) (matching the existing handler used when
selectedChannel is truthy) and uses the FaEllipsisV icon so isOverflowMenuOpen
can control rendering of HeaderOverflowMenu and surface the "Play Tic-Tac-Toe"
menu item.
Comment on lines
+86
to
+113
| <td | ||
| // biome-ignore lint/suspicious/noArrayIndexKey: 3x3 fixed grid | ||
| key={c} | ||
| onClick={ | ||
| playable | ||
| ? () => move(open.serverId, open.opponent, r, c) | ||
| : undefined | ||
| } | ||
| onKeyDown={ | ||
| playable | ||
| ? (e) => { | ||
| if (e.key === "Enter" || e.key === " ") | ||
| move(open.serverId, open.opponent, r, c); | ||
| } | ||
| : undefined | ||
| } | ||
| tabIndex={playable ? 0 : -1} | ||
| className={`w-20 h-20 text-4xl font-bold text-center border-4 border-discord-dark-100 select-none ${ | ||
| playable | ||
| ? "cursor-pointer hover:bg-discord-dark-300" | ||
| : "" | ||
| } ${ | ||
| win ? "bg-discord-green text-white" : "text-white" | ||
| }`} | ||
| > | ||
| {cell} | ||
| </td> | ||
| ); |
Contributor
There was a problem hiding this comment.
Interactive cells need role="button" for screen-reader accessibility.
Playable cells have onClick, onKeyDown, and tabIndex={0} but no ARIA role. Screen readers will announce them as "cell" rather than an interactive control, preventing users from understanding they can activate them.
♿ Proposed fix
<td
// biome-ignore lint/suspicious/noArrayIndexKey: 3x3 fixed grid
key={c}
+ role={playable ? "button" : undefined}
onClick={🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/TicTacToeModal.tsx` around lines 86 - 113, TicTacToeModal's
interactive <td> lacks an ARIA role; update the cell rendering (the <td> where
playable, move, open.serverId, open.opponent, r, c are used) to include
role="button" only when playable (e.g. role={playable ? "button" : undefined})
so screen readers announce it as an actionable control; optionally add
aria-disabled or aria-pressed if you need to convey disabled or toggle state,
but at minimum add the conditional role to the <td>.
Comment on lines
+95
to
+98
| ircClient.sendRaw( | ||
| serverId, | ||
| `@${TTT_TAG}={"cmd"\\:"invite_received"} TAGMSG ${opponent}`, | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find tictactoeProtocol file and inspect unescapeTagValue
fd -t f "tictactoeProtocol" --exec cat {}Repository: ObsidianIRC/ObsidianIRC
Length of output: 875
🏁 Script executed:
#!/bin/bash
# Check the handler file around lines 95-98 and 165-169
cat -n src/store/handlers/tictactoe.ts | sed -n '90,100p; 160,175p'Repository: ObsidianIRC/ObsidianIRC
Length of output: 1069
🏁 Script executed:
#!/bin/bash
# Check the imports in the handler file
head -20 src/store/handlers/tictactoe.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 685
🏁 Script executed:
#!/bin/bash
# Check the sendTtt function in tictactoeActions
rg -A 10 "export.*sendTtt|function sendTtt" src/store/actions/tictactoeActions.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Find the tictactoeActions file
fd -t f "tictactoeActions"Repository: ObsidianIRC/ObsidianIRC
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Search for sendTtt function across the codebase
rg -l "sendTtt" --type ts --type tsxRepository: ObsidianIRC/ObsidianIRC
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Check what's exported from tictactoeProtocol to confirm escapeTagValue availability
rg "export.*function|export.*const" src/lib/games/tictactoeProtocol.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 173
🏁 Script executed:
#!/bin/bash
# Check tictactoeActions.ts for sendTtt or similar functions that handle message sending
cat -n src/store/handlers/tictactoeActions.ts | head -100Repository: ObsidianIRC/ObsidianIRC
Length of output: 3415
🏁 Script executed:
#!/bin/bash
# Search for any JSON.stringify + escapeTagValue usage in the codebase
rg "JSON.stringify.*escapeTagValue|escapeTagValue.*JSON.stringify"Repository: ObsidianIRC/ObsidianIRC
Length of output: 49
Broken IRC tag-value escaping causes JSON.parse failures on the receiving end.
In a JavaScript template literal \\: evaluates to the two-character sequence \: in the actual string. Per the IRCv3 message-tags spec, \: is the escape sequence for ; — not :. So the receiver's unescapeTagValue turns:
{"cmd"\:"invite_received"} → {"cmd";"invite_received"} // invalid JSON
{"cmd"\:"error"\,"message"\:"out of sync"} → {"cmd";"error","message";"out of sync"} // invalid JSON
Both calls will silently fail at JSON.parse on the receiver, dropping the invite_received status update and the out-of-sync error signal entirely.
: has no special meaning in IRC tag values and must not be escaped. The sendTtt helper in tictactoeActions.ts does this correctly via escapeTagValue(JSON.stringify(...)). Use the same approach here — import escapeTagValue from tictactoeProtocol (it's already imported as unescapeTagValue from the same module):
🐛 Proposed fix
-import { unescapeTagValue } from "../../lib/games/tictactoeProtocol";
+import { escapeTagValue, unescapeTagValue } from "../../lib/games/tictactoeProtocol";
// ...
- ircClient.sendRaw(
- serverId,
- `@${TTT_TAG}={"cmd"\\:"invite_received"} TAGMSG ${opponent}`,
- );
+ ircClient.sendRaw(
+ serverId,
+ `@${TTT_TAG}=${escapeTagValue(JSON.stringify({ cmd: "invite_received" }))} TAGMSG ${opponent}`,
+ );
// ...
- ircClient.sendRaw(
- serverId,
- `@${TTT_TAG}={"cmd"\\:"error"\\,"message"\\:"out of sync"} TAGMSG ${opponent}`,
- );
+ ircClient.sendRaw(
+ serverId,
+ `@${TTT_TAG}=${escapeTagValue(JSON.stringify({ cmd: "error", message: "out of sync" }))} TAGMSG ${opponent}`,
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/handlers/tictactoe.ts` around lines 95 - 98, The IRC tag value is
being incorrectly escaped with `\:` which turns `:` into `;` and breaks JSON on
the receiver; update the call that builds the tag in the `ircClient.sendRaw`
invocation (the one using `TTT_TAG`, `serverId` and `opponent`) to wrap the JSON
payload with `escapeTagValue(JSON.stringify(...))` instead of manual backslash
escapes, import `escapeTagValue` from `tictactoeProtocol` alongside
`unescapeTagValue` if not already imported, and apply the same change for any
other tag-value sends in this file (mirror how `sendTtt` in
`tictactoeActions.ts` constructs tags).
Comment on lines
+88
to
+101
| const me = localNick(serverId); | ||
| if (!me) return; | ||
| const existing = getGame(get(), serverId, opponent); | ||
| const g = existing ?? makeGame(serverId, me, opponent); | ||
| g.invitePending = true; | ||
| g.inviteIncoming = false; | ||
| g.terminated = false; | ||
| g.statusMessage = `Invite sent to ${opponent}`; | ||
| setGame(set, serverId, opponent, { ...g }); | ||
| sendTtt(serverId, opponent, { cmd: "invite" }); | ||
| set((state) => ({ | ||
| tictactoe: { ...state.tictactoe, open: { serverId, opponent } }, | ||
| })); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Direct mutation of live store state — coding guideline violation.
getGame(get(), ...) returns the actual object stored in Zustand state. Assigning g.invitePending = true (and the other field mutations in invite and accept) mutates the live state tree before setGame is called. Immer is not wired in, so this is a direct mutation.
Replace the field-by-field assignment pattern with object spread to create a fresh object:
♻️ Proposed fix for invite and accept
export function invite(set, get, serverId, opponent) {
const me = localNick(serverId);
if (!me) return;
const existing = getGame(get(), serverId, opponent);
- const g = existing ?? makeGame(serverId, me, opponent);
- g.invitePending = true;
- g.inviteIncoming = false;
- g.terminated = false;
- g.statusMessage = `Invite sent to ${opponent}`;
- setGame(set, serverId, opponent, { ...g });
+ const g: GameSnapshot = {
+ ...(existing ?? makeGame(serverId, me, opponent)),
+ invitePending: true,
+ inviteIncoming: false,
+ terminated: false,
+ statusMessage: `Invite sent to ${opponent}`,
+ };
+ setGame(set, serverId, opponent, g);
// ...
}
export function accept(set, get, serverId, opponent) {
const me = localNick(serverId);
if (!me) return;
- const g =
- getGame(get(), serverId, opponent) ?? makeGame(serverId, me, opponent);
- g.startPlayer = startPlayer;
- g.active = true;
- // ... (remaining field mutations)
- setGame(set, serverId, opponent, { ...g });
+ const base = getGame(get(), serverId, opponent) ?? makeGame(serverId, me, opponent);
+ const startPlayer = pickStartPlayer(me, opponent);
+ const g: GameSnapshot = {
+ ...base,
+ startPlayer,
+ active: true,
+ gameOver: false,
+ winner: null,
+ winLine: null,
+ turn: 1,
+ board: [["", "", ""], ["", "", ""], ["", "", ""]],
+ inviteIncoming: false,
+ invitePending: false,
+ terminated: false,
+ statusMessage: "",
+ };
+ g.statusMessage = isLocalTurn(g) ? "Your turn!" : `Waiting for ${opponent}…`;
+ setGame(set, serverId, opponent, g);
// ...
}As per coding guidelines, "Store setState callbacks must return Partial<AppState> and cannot use direct mutation; Immer middleware is not wired into create()."
Also applies to: 111-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/handlers/tictactoeActions.ts` around lines 88 - 101, The invite
(and similarly accept) handler mutates the live Zustand state by modifying the
object returned from getGame; instead create a fresh game object instead of
assigning fields on g. Use getGame(get(), serverId, opponent) to read, then
build a new object via object spread (e.g., const newGame = {
...existingOrMakeGame, invitePending: true, inviteIncoming: false, terminated:
false, statusMessage: `Invite sent to ${opponent}` }) and pass that to
setGame(set, serverId, opponent, newGame); update the set((state) => ({
tictactoe: { ...state.tictactoe, open: { serverId, opponent } } })) call
unchanged. Apply the same pattern in accept (replace in-place assignments with a
single spread-created object) and ensure no direct mutations of objects returned
by getGame, localNick, makeGame, setGame are performed.
Contributor
Author
|
how to make coderabbit fuck off |
ValwareIRC
changed the title
The overflow menu is only rendered in the channel header (and only on mobile), so the previous "Play Tic-Tac-Toe" entry was unreachable in private chats. Add a dedicated 🎮 button next to the existing Media / Search controls in the PM action cluster instead.
The remove button used to fire an immediate `2FA REMOVE`. That had two problems: 1. No undo path -- a stray click silently dropped a TOTP credential. 2. With 2FA enforcement on, the server's REMOVE_LAST_CREDENTIAL guard would reject the request, but we'd still surface the failure as a plain error toast with no recovery hint. Now the button just stages a confirm prompt inline. When 2FA is enabled and the credential being removed is the only one left, the confirm prompt is replaced with a "can't remove your last credential" notice that points the user at adding another credential or disabling 2FA first. Otherwise the user gets a normal "remove '<name>'?" confirm.
…abilities
Without this CAP REQ, the server's CAP LS advertisement of draft/account-2fa
never lands on server.capabilities (the merger only writes from CAP ACK), so
EditServerModal's gate
server?.capabilities?.some(c => c.startsWith("draft/account-2fa"))
stays false and the 'Manage two-factor authentication' button never renders.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/TwoFactorSettingsModal.tsx`:
- Around line 97-110: The onAddTotp and onAddWebAuthn handlers (and the other
enrollment handlers around lines 308-330) currently call
twofaChallenge(serverId, "...") without checking the single-slot store
pendingTwofaChallenge, allowing concurrent enrollments to overwrite the slot;
modify these handlers to first check the pendingTwofaChallenge store (import/use
the same selector or store instance) and if a challenge is already present,
short-circuit (return) and setEnrollError to a clear message like "Another
enrollment is in progress" (or disable the UI path), otherwise proceed to clear
errors and call twofaChallenge; apply the same guard to all enrollment entry
points (onAddTotp, onAddWebAuthn and the other handlers referenced) so only one
pendingTwofaChallenge can be created at a time.
- Around line 79-94: The code silently clears qrSvg when decodeChallenge or
QRCode.toString fails; add explicit error state and surface the message instead
of failing silently: create a state like qrError (e.g., useState<string | null>)
in the component, set qrError to a descriptive message when uri is missing
(after decodeChallenge(challenge.blob) returns no uri) or when
QRCode.toString(uri...) rejects (catch the error and set qrError to
error.message or a friendly message), and ensure you still clear qrSvg when
errors occur; update the modal UI to render qrError when present so users see
the failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49fd6e8c-8987-4c2e-a51c-c8a0eee22446
📒 Files selected for processing (3)
src/components/layout/ChatHeader.tsxsrc/components/ui/TwoFactorSettingsModal.tsxsrc/lib/irc/IRCClient.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/irc/IRCClient.ts
Comment on lines
+79
to
+94
| const decoded = decodeChallenge(challenge.blob); | ||
| const uri = | ||
| decoded && | ||
| typeof decoded === "object" && | ||
| decoded !== null && | ||
| "uri" in decoded && | ||
| typeof (decoded as { uri: unknown }).uri === "string" | ||
| ? (decoded as { uri: string }).uri | ||
| : null; | ||
| if (!uri) { | ||
| setQrSvg(null); | ||
| return; | ||
| } | ||
| QRCode.toString(uri, { type: "svg", margin: 2, width: 220 }) | ||
| .then(setQrSvg) | ||
| .catch(() => setQrSvg(null)); |
Contributor
There was a problem hiding this comment.
Surface TOTP challenge/render failures instead of failing silently.
If the server sends a malformed TOTP challenge, or QR generation fails, this just clears qrSvg and leaves the modal looking idle. Showing an explicit error here makes the failure diagnosable for the user.
Proposed fix
if (!uri) {
setQrSvg(null);
+ setEnrollError("Server returned an invalid TOTP challenge.");
return;
}
+ setEnrollError(null);
QRCode.toString(uri, { type: "svg", margin: 2, width: 220 })
.then(setQrSvg)
- .catch(() => setQrSvg(null));
+ .catch(() => {
+ setQrSvg(null);
+ setEnrollError("Failed to render the QR code for this TOTP challenge.");
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const decoded = decodeChallenge(challenge.blob); | |
| const uri = | |
| decoded && | |
| typeof decoded === "object" && | |
| decoded !== null && | |
| "uri" in decoded && | |
| typeof (decoded as { uri: unknown }).uri === "string" | |
| ? (decoded as { uri: string }).uri | |
| : null; | |
| if (!uri) { | |
| setQrSvg(null); | |
| return; | |
| } | |
| QRCode.toString(uri, { type: "svg", margin: 2, width: 220 }) | |
| .then(setQrSvg) | |
| .catch(() => setQrSvg(null)); | |
| const decoded = decodeChallenge(challenge.blob); | |
| const uri = | |
| decoded && | |
| typeof decoded === "object" && | |
| decoded !== null && | |
| "uri" in decoded && | |
| typeof (decoded as { uri: unknown }).uri === "string" | |
| ? (decoded as { uri: string }).uri | |
| : null; | |
| if (!uri) { | |
| setQrSvg(null); | |
| setEnrollError("Server returned an invalid TOTP challenge."); | |
| return; | |
| } | |
| setEnrollError(null); | |
| QRCode.toString(uri, { type: "svg", margin: 2, width: 220 }) | |
| .then(setQrSvg) | |
| .catch(() => { | |
| setQrSvg(null); | |
| setEnrollError("Failed to render the QR code for this TOTP challenge."); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/TwoFactorSettingsModal.tsx` around lines 79 - 94, The code
silently clears qrSvg when decodeChallenge or QRCode.toString fails; add
explicit error state and surface the message instead of failing silently: create
a state like qrError (e.g., useState<string | null>) in the component, set
qrError to a descriptive message when uri is missing (after
decodeChallenge(challenge.blob) returns no uri) or when QRCode.toString(uri...)
rejects (catch the error and set qrError to error.message or a friendly
message), and ensure you still clear qrSvg when errors occur; update the modal
UI to render qrError when present so users see the failure.
Comment on lines
+97
to
+110
| const onAddTotp = () => { | ||
| setEnrollError(null); | ||
| setEnrollName(""); | ||
| setEnrollCode(""); | ||
| twofaChallenge(serverId, "totp"); | ||
| }; | ||
|
|
||
| const onAddWebAuthn = async () => { | ||
| if (!isWebAuthnAvailable()) { | ||
| setEnrollError("WebAuthn is not available in this environment."); | ||
| return; | ||
| } | ||
| setEnrollError(null); | ||
| twofaChallenge(serverId, "webauthn"); |
Contributor
There was a problem hiding this comment.
Block new enrollment requests while one is already active.
pendingTwofaChallenge is a single store slot (src/store/index.ts:540-546). Right now the user can start another TOTP/WebAuthn enrollment while the current one is still active, which can overwrite that slot and race the two flows against each other.
Proposed fix
+ const hasActiveEnrollment =
+ enrollBusy || challenge?.serverId === serverId;
+
const onAddTotp = () => {
+ if (hasActiveEnrollment) return;
setEnrollError(null);
setEnrollName("");
setEnrollCode("");
twofaChallenge(serverId, "totp");
};
const onAddWebAuthn = async () => {
+ if (hasActiveEnrollment) return;
if (!isWebAuthnAvailable()) {
setEnrollError("WebAuthn is not available in this environment.");
return;
@@
<button
type="button"
onClick={onAddTotp}
- className="px-3 py-2 rounded bg-discord-blue text-white text-sm font-medium hover:bg-discord-blue-hover"
+ disabled={hasActiveEnrollment}
+ className="px-3 py-2 rounded bg-discord-blue text-white text-sm font-medium hover:bg-discord-blue-hover disabled:opacity-50 disabled:cursor-not-allowed"
>
Add authenticator app
</button>
<button
type="button"
onClick={onAddWebAuthn}
- disabled={!supportsWebAuthn || !isWebAuthnAvailable()}
+ disabled={
+ hasActiveEnrollment ||
+ !supportsWebAuthn ||
+ !isWebAuthnAvailable()
+ }
className="px-3 py-2 rounded bg-discord-dark-100 text-white text-sm font-medium disabled:opacity-50 disabled:cursor-not-allowed"Also applies to: 308-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/TwoFactorSettingsModal.tsx` around lines 97 - 110, The
onAddTotp and onAddWebAuthn handlers (and the other enrollment handlers around
lines 308-330) currently call twofaChallenge(serverId, "...") without checking
the single-slot store pendingTwofaChallenge, allowing concurrent enrollments to
overwrite the slot; modify these handlers to first check the
pendingTwofaChallenge store (import/use the same selector or store instance) and
if a challenge is already present, short-circuit (return) and setEnrollError to
a clear message like "Another enrollment is in progress" (or disable the UI
path), otherwise proceed to clear errors and call twofaChallenge; apply the same
guard to all enrollment entry points (onAddTotp, onAddWebAuthn and the other
handlers referenced) so only one pendingTwofaChallenge can be created at a time.
…dex above LoadingOverlay The button in EditServerModal was buried behind the 'Login to an account' checkbox, so users wouldn't find it. Surface it in the always-reachable Account category of UserSettings instead, gated on the currently selected server's draft/account-2fa capability. LoadingOverlay sits at z-[100001]; the 2FA / TOTP step-up modals were at z-50 and got hidden behind it whenever the server reconnected. Bump both modals to z-[100002].
Per IRCv3 SASL, the server completes the exchange itself after server-final by emitting 900/903 (or AUTHENTICATE 2FA-REQUIRED for step-up). UnrealIRCd's saslserv reads our extra 'AUTHENTICATE +' as an empty/abort payload and fires 904 SASL authentication failed, which dropped the SASL session before the user could supply their TOTP code.
Contributor
@coderabbitai ignore |
matheusfillipe
previously approved these changes
May 4, 2026
| }, | ||
| { | ||
| label: "Play Tic-Tac-Toe", | ||
| icon: <span aria-hidden="true">🎮</span>, |
Contributor
There was a problem hiding this comment.
PLS avoid cheap emojis..
| @@ -0,0 +1,191 @@ | |||
| // SCRAM-SHA-256 (RFC 7677) client. Uses Web Crypto: works in browsers, | |||
Contributor
There was a problem hiding this comment.
I am pretty sure some of these things deserve some test... There are 0 tests in this pr
Comment on lines
+2643
to
+2662
| tictactoeInvite: (serverId, opponent) => | ||
| tictactoeActions.invite(set, get, serverId, opponent), | ||
| tictactoeAccept: (serverId, opponent) => | ||
| tictactoeActions.accept(set, get, serverId, opponent), | ||
| tictactoeDecline: (serverId, opponent) => | ||
| tictactoeActions.decline(set, get, serverId, opponent), | ||
| tictactoeMove: (serverId, opponent, row, col) => | ||
| tictactoeActions.move(set, get, serverId, opponent, row, col), | ||
| tictactoeTerminate: (serverId, opponent) => | ||
| tictactoeActions.terminate(set, get, serverId, opponent), | ||
| tictactoeOpenModal: (serverId, opponent) => { | ||
| set((state) => ({ | ||
| tictactoe: { ...state.tictactoe, open: { serverId, opponent } }, | ||
| })); | ||
| }, | ||
| tictactoeCloseModal: () => { | ||
| set((state) => ({ | ||
| tictactoe: { ...state.tictactoe, open: null }, | ||
| })); | ||
| }, |
Contributor
There was a problem hiding this comment.
I thought i had better refactored this index.ts :P
Makes really no sense this all still needs to come in here.
Lets ObsidianIRC connect to obbyircd via OAuth2 instead of (or in
addition to) PLAIN. Each server entry now carries its own provider
config, so admins can point one server at Logto, another at Auth0,
Keycloak, Okta, or any custom OIDC issuer.
Flow
1. In Edit Server -> "OAuth2 / OIDC sign-in", admin picks a preset
(Logto, Auth0, Keycloak, Custom), pastes the issuer URL + client
ID, optionally tweaks scopes/redirect URI, and clicks "Sign in".
2. We discover the issuer's metadata, build an authorize URL with
PKCE (S256), and open the IdP in a popup.
3. The popup redirects to /oauth/callback (a new same-origin
SPA route) which postMessages the auth code back to the opener
and closes itself.
4. The opener exchanges code+verifier at the token endpoint and
stores the resulting access/id/refresh tokens on the server.
5. On every connect, if the server has an access token, the SASL
step picks IRCV3BEARER, sends "[authzid]\0jwt\0<token>" in 400-
char AUTHENTICATE chunks, and the server validates the JWT
against its configured oauth-provider/JWKS.
Notes
* Tokens live in localStorage with the rest of the server config;
expired tokens just yield a 904 and the user re-authenticates.
* IRCClient gained an oauthBearerEnabled flag on connect() so CAP
requests SASL and we don't race CAP END against AUTHENTICATE.
* saslFrames.ts also exports buildOauthBearerPayload for the
RFC 7628 mech, even though the wired-up flow uses IRCV3BEARER.
Tests cover OIDC discovery, PKCE verifier/challenge (incl. the
RFC 7636 vector), authorize-URL construction, and both SASL frame
builders + chunker behavior. Full suite passes (722 tests).
When the client is built with VITE_HIDE_SERVER_LIST=true (single-server deploy where users can't add their own servers), admins can now bake the OAuth provider settings into the build via VITE_DEFAULT_OAUTH_* env vars. Users see a "Sign in with <ProviderLabel>" button instead of an editable issuer/client_id form -- they don't need to know what Logto, Auth0 or Keycloak even is. VITE_DEFAULT_OAUTH_PROVIDER_LABEL e.g. "Logto" VITE_DEFAULT_OAUTH_ISSUER e.g. https://my-tenant.logto.app/oidc VITE_DEFAULT_OAUTH_CLIENT_ID VITE_DEFAULT_OAUTH_SCOPES optional, defaults "openid" VITE_DEFAULT_OAUTH_REDIRECT_URI optional, defaults <origin>/oauth/callback Both the welcome AddServerModal and EditServerModal pull getBuiltinOAuthConfig() and pass it as `locked` to OAuthSection, which in locked mode renders only the Sign-in / Sign-out actions plus a token status line. The provider fields are no longer editable. In multi-server builds (VITE_HIDE_SERVER_LIST unset), behavior is unchanged: the editable per-server OAuth panel still drives. Tests cover getBuiltinOAuthConfig() across all four present/missing-field combinations. Dockerfile + BUILD.md updated.
The Edit Server modal had grown to seven sections stacked in one
column (server info, account login toggles, server password, SASL,
OAuth, operator, register-account dead code). Migrate it to the same
left-sidebar tabbed shell as ChannelSettingsModal:
General - network name, host, port, nickname
Authentication - SASL PLAIN account/password + server password
OAuth2 - the OAuthSection (locked or editable)
IRC Operator - oper credentials + on-connect toggle + Oper Up
Desktop renders sidebar + main content in a single 80vh dialog;
mobile mirrors ChannelSettingsModal's two-step categories ->
content navigation with back chevron, identical safe-area
insets, and an always-visible Cancel/Update footer.
Validation errors now jump back to the General tab (mobile: into
the content view) so the user sees what's missing instead of
silently failing on submit.
Drops the dead "Register for an account" section from this
modal: the registerAccount/email/password state was collected but
never sent on submit -- registration only happens at server-add
time via AddServerModal -> store.connect().
Behavior preserved:
* SASL PLAIN account/password (was: state initialized empty,
btoa'd into config on submit). Now also exposes "(stored --
leave blank to keep)" placeholder when a SASL/oper/server
password already exists, fixing the prior surprise where the
field appeared blank but was actually preserved on save.
* Operator credentials, on-connect toggle, "Forget Credentials"
+ "Oper Up" buttons.
* OAuth: locked-mode (HIDE_SERVER_LIST + builtin config) still
surfaces only Sign in / Sign out; editable mode still gets the
full preset/issuer/clientId/scopes form.
Implementation also picks up modal-behavior parity with
ChannelSettingsModal: createPortal rendering, useModalBehavior for
Esc + click-outside, animated fade-in on mobile.
No tests changed -- this is a UI-shell refactor over the same
state shape; existing 726-test suite continues to pass.
Adds the client side of the obbyircd opaque-token feature. Users can
now sign in with any provider obbyircd is configured for, including
GitHub-style OAuth2 (opaque tokens, server hits userinfo endpoint)
and Google-style OIDC (id_token JWT, server validates locally).
Two new fields on ServerOAuthConfig:
tokenKind: "jwt" (default) or "opaque"
serverProvider: name of the matching oauth-provider {} on the IRC
server, sent as IRCV3BEARER authzid (or OAUTHBEARER
`provider=` k/v) so obbyircd picks the right
userinfo URL
For non-OIDC providers (GitHub) we add explicit endpoint overrides
since there's no /.well-known/openid-configuration to discover:
authorizeEndpoint, tokenEndpoint
Built-in presets gain Google (jwt, OIDC) and GitHub (opaque, manual
endpoints) so the dropdown auto-fills issuer/scopes/endpoints/
tokenKind/serverProvider in one click.
JWT path now prefers id_token over access_token (matters for Google,
whose access_token is opaque even when an id_token is also issued).
Lock-mode build vars (single-server deploy) gain matching siblings:
VITE_DEFAULT_OAUTH_TOKEN_KIND
VITE_DEFAULT_OAUTH_SERVER_PROVIDER
VITE_DEFAULT_OAUTH_AUTHORIZE_URL
VITE_DEFAULT_OAUTH_TOKEN_URL
so admins can ship a "Sign in with GitHub" or "Sign in with Google"
build with no per-user configuration.
BUILD.md + Dockerfile updated; SASL frame builder gets a new test
covering the opaque format. 727 tests pass.
For each preset the corresponding obbyircd oauth-provider {} block
is documented inline (Google: jwks-file, GitHub: userinfo-url).
The SPA bundle uses base="./" so its asset paths are relative; loading the React OAuthCallback at /oauth/callback would resolve assets to /oauth/assets/... and 404. Replace the in-SPA route with a self-contained public/oauth/callback/index.html that just postMessages the auth code to its opener and closes -- no SPA bundle dependency, works under any base config and any deep-link routing setup.
Resolves conflict in src/lib/irc/IRCClient.ts: keeps both the TWOFA/ TWOFA_NOTE event types from this branch and the CMDSLIST event from main.
…tor replay
Wires the client to the new server-side step-up policy:
- When SASL primary is PLAIN/SCRAM and an OAuth bearer is also
configured, on AUTHENTICATE 2FA-REQUIRED the client now
automatically replies with AUTHENTICATE IRCV3BEARER + the chunked
bearer payload, satisfying 2FA in one round trip with no UI prompt.
- When SASL primary is IRCV3BEARER (the bearer is already spent as
primary), the auto-path is skipped on 2FA-REQUIRED so the
TotpStepUpModal pops -- replaying the same bearer would just be
rejected by the server's same-factor guard.
- Drops the made-up "AUTHENTICATE 2FA-OAUTH" mech name. Step-up uses
the real IRCV3BEARER mech with identical wire format; the server
routes to the step-up handler based on TwoFAStepup state.
- Two new store actions: twofaToken(serverId, chunk) and
linkOauthCredential(serverId, provider, bearer, name) which run
/2FA CHALLENGE oauth + chunked /2FA TOKEN + /2FA ADD oauth.
twofaAdd's data param became optional (oauth ADD takes no inline
data -- token was already streamed).
- TwoFactorSettingsModal grows a "Link an OAuth identity" card that
runs the popup OAuth flow against the per-server (or
deployer-baked, in lock-mode builds) provider config and binds the
resulting (provider, subject) credential to the account via the
actions above.
* feat: draft/account-recovery (RECOVER + SETPASS) and SASL EXTERNAL
Implements the client side of the draft/account-recovery WIP spec we
authored at extensions/account-recovery.md, plus opt-in SASL
EXTERNAL for cert-based login.
* IRCClient.recoverRequest / recoverConfirm / setpass send the new
commands. SETPASS uses the IRC trailing-parameter form so passwords
may contain spaces (passphrases). No base64.
* New typed events RECOVER_NOTE / RECOVER_FAIL / SETPASS_NOTE /
SETPASS_FAIL projected from the generic NOTE/FAIL stream so
components don't have to filter by command on every render.
* PasswordRecoveryModal: three-stage flow (account -> code -> new
password). Surfaces the spec's account-existence-ambiguity to the
user instead of silently swallowing typos.
* ChangePasswordModal: one-shot SETPASS for in-session rotation; only
shown while the connection is authenticated.
* EditServerModal exposes both modals when the server advertises
draft/account-recovery.
* SASL EXTERNAL added to the SaslMech union and the AUTHENTICATE
state machine; an EXTERNAL-configured server replies '+' to '+' and
the TLS cert provides identity. EXTERNAL is only chosen when the
user explicitly picks it -- never under "auto", since it requires a
device-side client cert.
* feat: draft/persistence settings panel
When the server advertises draft/persistence, the user's account
settings tab now gets a tri-state preference:
- Stay in channels (ON) -- ghost survives disconnect
- Leave on disconnect (OFF) -- clean exit
- Use server default (DEFAULT) -- inherit operator setting
Plumbing:
- IRCClient.persistenceGet / persistenceSet send PERSISTENCE GET
and PERSISTENCE SET ON|OFF|DEFAULT.
- handlePersistence parses the server's PERSISTENCE STATUS reply
and emits a typed PERSISTENCE_STATUS event; the FAIL handler
projects PERSISTENCE_FAIL alongside the generic FAIL.
- registerAuthHandlers caches both preference + effective state on
the Server record, and fires an automatic PERSISTENCE GET 1.5s
after CAP ACK so the panel has fresh state when the user opens
settings (the spec gates the command on IsLoggedIn, hence the
small post-SASL delay).
- PersistenceSettingsPanel renders a radio-card list with helper
text and an "Currently ON/OFF" badge so the user can reconcile
their preference with what the server is actually doing (e.g.
preference DEFAULT but the operator default flipped).
- The panel is mounted in UserSettings -> Account, gated on the
server's CAP set, so non-supporting networks see nothing change.
* feat: draft/account-recovery (RECOVER + SETPASS) and SASL EXTERNAL
Implements the client side of the draft/account-recovery WIP spec we
authored at extensions/account-recovery.md, plus opt-in SASL
EXTERNAL for cert-based login.
* IRCClient.recoverRequest / recoverConfirm / setpass send the new
commands. SETPASS uses the IRC trailing-parameter form so passwords
may contain spaces (passphrases). No base64.
* New typed events RECOVER_NOTE / RECOVER_FAIL / SETPASS_NOTE /
SETPASS_FAIL projected from the generic NOTE/FAIL stream so
components don't have to filter by command on every render.
* PasswordRecoveryModal: three-stage flow (account -> code -> new
password). Surfaces the spec's account-existence-ambiguity to the
user instead of silently swallowing typos.
* ChangePasswordModal: one-shot SETPASS for in-session rotation; only
shown while the connection is authenticated.
* EditServerModal exposes both modals when the server advertises
draft/account-recovery.
* SASL EXTERNAL added to the SaslMech union and the AUTHENTICATE
state machine; an EXTERNAL-configured server replies '+' to '+' and
the TLS cert provides identity. EXTERNAL is only chosen when the
user explicitly picks it -- never under "auto", since it requires a
device-side client cert.
* feat: draft/persistence settings panel
When the server advertises draft/persistence, the user's account
settings tab now gets a tri-state preference:
- Stay in channels (ON) -- ghost survives disconnect
- Leave on disconnect (OFF) -- clean exit
- Use server default (DEFAULT) -- inherit operator setting
Plumbing:
- IRCClient.persistenceGet / persistenceSet send PERSISTENCE GET
and PERSISTENCE SET ON|OFF|DEFAULT.
- handlePersistence parses the server's PERSISTENCE STATUS reply
and emits a typed PERSISTENCE_STATUS event; the FAIL handler
projects PERSISTENCE_FAIL alongside the generic FAIL.
- registerAuthHandlers caches both preference + effective state on
the Server record, and fires an automatic PERSISTENCE GET 1.5s
after CAP ACK so the panel has fresh state when the user opens
settings (the spec gates the command on IsLoggedIn, hence the
small post-SASL delay).
- PersistenceSettingsPanel renders a radio-card list with helper
text and an "Currently ON/OFF" badge so the user can reconcile
their preference with what the server is actually doing (e.g.
preference DEFAULT but the operator default flipped).
- The panel is mounted in UserSettings -> Account, gated on the
server's CAP set, so non-supporting networks see nothing change.
* feat: draft/read-marker (MARKREAD)
Cross-session read state: when one of your devices reads up to a
particular message, the other devices clear their unread / mention
state for that buffer too.
- draft/read-marker added to ourCaps so the cap is auto-requested
when the server advertises it.
- IRCClient.markreadGet / markreadSet send the GET and SET wire
forms. SET uses the "timestamp=YYYY-MM-DDThh:mm:ss.sssZ" format
the spec requires.
- handleMarkread parses ":server MARKREAD <target> {timestamp=ts|*}"
into a typed MARKREAD event with timestamp: string | null.
- registerReadMarkerHandlers caches the marker on the matching
Channel (case-insensitive name match) or PrivateChat (username
match). Adds readMarker / readMarkerFetched to the type defs.
- selectChannel: when the user opens a channel, send MARKREAD
SET with the latest message timestamp in that channel's history.
- selectPrivateChat: PMs aren't auto-pushed by the server, so we
issue a one-shot MARKREAD GET on first open to pick up any
marker another session set earlier. Subsequent re-selections
just push the latest timestamp via SET.
- MARKREAD_FAIL projection on the FAIL stream so future UI can
surface server-side rejections without filtering generic FAIL.
# Conflicts: # src/types/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bundles three modern-auth additions and an in-client tic-tac-toe implementation that interoperates with the existing KiwiIRC plugin.
Auth
src/lib/sasl/scram.ts— SCRAM-SHA-256 (RFC 7677) using Web Crypto for PBKDF2 / HMAC / SHA-256. Picked automatically when the server advertises it; falls back to PLAIN.src/lib/sasl/webauthn.ts— thinnavigator.credentialswrapper for theDRAFT-WEBAUTHN-BIOmechanism and2FA ADD webauthnenrolment.src/store/handlers/auth.ts— per-server SASL session state machine that dispatches AUTHENTICATE messages by mechanism and routesAUTHENTICATE 2FA-REQUIREDto the step-up modal.2FAIRC command +TWOFA/TWOFA_NOTEevents for status, listing, enrolment, removal, and enable/disable replies.TotpStepUpModal— prompts for the 6-digit code mid-SASL.TwoFactorSettingsModal— status, credential list, TOTP enrol with QR (qrcodedep), WebAuthn biometric enrol vianavigator.credentials.create(), removal, and password-free disable via a TOTP proof.EditServerModal— the button is visible only when the server advertisesdraft/account-2fa.Tic-tac-toe (KiwiIRC compatible)
Wire-compatible with ItsOnlyBinary/kiwiirc-plugin-tictactoe so KiwiIRC and ObsidianIRC users can play each other. Same TAGMSG protocol on
+kiwiirc.com/ttt.src/lib/games/tictactoe.ts— pure game logic: board, turn bookkeeping, win/draw detection, marker assignment.src/lib/games/tictactoeProtocol.ts— IRCv3 message-tag escape / unescape.src/store/handlers/tictactoeActions.ts— invite / accept / decline / move / terminate, mutating store state and emitting TAGMSGs.src/store/handlers/tictactoe.ts— inbound TAGMSG dispatch. Auto-opens the modal on incoming invites, replays moves into the local board, detects out-of-sync turns, handles forfeit / decline.src/components/ui/TicTacToeModal.tsx— minimal 3×3 board UI with invite accept/decline, in-game forfeit, and game-over close.Test plan
sasl=PLAIN,SCRAM-SHA-256,TOTP,DRAFT-WEBAUTHN-BIO— login transparently uses SCRAM-SHA-256.Summary by CodeRabbit
Release Notes
New Features
Chores