feat(auth): SASL IRCV3BEARER + per-server OAuth2/OIDC sign-in#196
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`).
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.
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.
…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.
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).
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request adds PKCE-based OAuth2/OIDC popup login with provider presets and built-in defaults, editable and locked OAuth configuration UI, a static OAuth callback page, store connect passthrough and persistent oauth storage, IRCv3 bearer SASL negotiation and IRCClient SASL controls, SCRAM/WebAuthn helpers and 2FA management UIs/step-up, a tic‑tac‑toe game over TAGMSG (model, protocol, handlers, UI), tests, and build-time configuration/documentation updates. ChangesOAuth2/OIDC Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/store/index.ts (1)
1023-1042:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the saved OAuth block when rewriting the server entry.
A successful connect rewrites the saved server config, but this object never copies
savedServer?.oauth. That means the first successful OAuth login silently erases the provider config and tokens from localStorage, so the next reconnect loses bearer auth entirely.Suggested fix
updatedServers.push({ id: server.id, name: server.name, host: urlHost, // Always save as full URL port, nickname, saslEnabled: !!saslPassword, password, channels: channelsToJoin, saslAccountName, saslPassword, + oauth: savedServer?.oauth, // Preserve existing oper credentials and warning preferences operUsername: savedServer?.operUsername, operPassword: savedServer?.operPassword, operOnConnect: savedServer?.operOnConnect,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/index.ts` around lines 1023 - 1042, The server rewrite in the updatedServers.push block currently omits savedServer?.oauth so OAuth provider config and tokens get dropped on first successful connect; update the object built in updatedServers.push to include an oauth field copied from savedServer?.oauth (e.g., oauth: savedServer?.oauth) so the provider config and tokens are preserved when rewriting the server entry (keep the existing pattern used for operUsername/operPassword/addedAt to maintain compatibility).
🧹 Nitpick comments (1)
src/lib/oauth.ts (1)
156-167: 💤 Low valueReusing
generateCodeVerifier()to produce the OAuthstateis functional but semantically misleading.The PKCE verifier and the CSRF
stateparameter are different things — co-locating them under one helper makes the code read as if the verifier is being sent in the URL. A tiny rename (e.g.generateRandomToken()used by both, or a dedicatedgenerateState()wrapper) would make intent obvious. Optional polish.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/oauth.ts` around lines 156 - 167, The code currently reuses generateCodeVerifier() to produce the OAuth state which is semantically misleading; change the helper to a clearly named function (e.g., generateRandomToken() or generateState()) and use it for the state variable while keeping generateCodeVerifier() for PKCE only—update the call site where state is set (the block that computes codeVerifier, deriveCodeChallenge, state, scope, redirectUri and calls buildAuthorizeUrl) to call the new generateRandomToken()/generateState() for state, leaving codeVerifier and deriveCodeChallenge unchanged so intent is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ui/OAuthSection.tsx`:
- Around line 55-72: The emit function currently uses `patch.X ?? stateX` which
treats an explicit patch field set to undefined as "not provided", causing stale
tokens to be re-used (e.g., on handleSignOut). Change the merge logic in emit
(the function named emit in OAuthSection.tsx) to detect whether each field is
present in patch (e.g., `if ('accessToken' in patch) use patch.accessToken else
use accessToken`) or use hasOwnProperty checks for
providerLabel/issuer/clientId/scopes/redirectUri/accessToken/idToken/refreshToken/tokenExpiresAt
so an explicit undefined in patch overwrites the previous state; update all
token-related lines (and the other fields around them) to follow this "field
present" semantics so sign-out and re-sign-in behave correctly.
In `@src/lib/oauth.ts`:
- Around line 182-224: The Promise that waits for the OAuth popup (the code
promise created around onMessage/cleanup/closeTimer) lacks an overall timeout,
treats transient popup.closed as immediate cancellation, and declares closeTimer
after cleanup which risks TDZ; fix by declaring closeTimer before cleanup,
implement an overall timeout (e.g., 2min) that rejects with a clear error and is
cleared on any resolve/reject, and harden popup-closed detection by requiring
sustained closed state (e.g., two consecutive ticks or a short grace period)
before rejecting; ensure cleanup clears both the closeTimer interval and the
timeout and that onMessage, popup.close(), resolve(data.code) and all reject
paths invoke cleanup so timers are always cleared.
- Around line 51-66: The discoverOidc function currently fetches the issuer URL
without validating its scheme, which can leak requests to non-HTTPS endpoints;
update discoverOidc to validate the issuer string before performing discovery:
parse the issuer, ensure the URL protocol is https: (permit http: only when
hostname is localhost or 127.0.0.1 for a dev escape hatch), and throw a clear
error if the scheme is not allowed; keep the rest of the logic (using
discoveryCache, building the /.well-known/openid-configuration URL, and
validating authorization_endpoint/token_endpoint) unchanged.
In `@src/store/handlers/auth.ts`:
- Around line 72-79: The chunkSaslPayload function currently adds a trailing "+"
when b64.length % SASL_CHUNK === 0, which wrongly triggers for an empty payload;
update the boundary check in chunkSaslPayload to only push the "+" sentinel when
b64.length > 0 && b64.length % SASL_CHUNK === 0 so that a zero-length b64 (from
buildIrcv3BearerPayload/token) does not produce an extraneous AUTHENTICATE +;
keep the rest of the chunking logic unchanged and reference SASL_CHUNK and the
b64 variable within chunkSaslPayload when making this change.
In `@src/store/index.ts`:
- Around line 988-991: The oauthBearerEnabled check currently treats any
non-empty accessToken as valid; update the logic in the block that computes
oauthBearerEnabled (referencing existingSavedServer,
existingSavedServer.oauth.accessToken, and
existingSavedServer.oauth.tokenExpiresAt) to only return true when accessToken
exists AND tokenExpiresAt is in the future (e.g. tokenExpiresAt > Date.now()).
Ensure this change prevents selecting IRCV3BEARER/forceSaslOauth when the saved
token is expired so the UI falls back to prompting sign-in or other auth flows.
In `@src/types/index.ts`:
- Around line 75-85: The ServerOAuthConfig interface currently includes
sensitive fields (accessToken, idToken, refreshToken, tokenExpiresAt) which
causes ServerConfig persistence to store credentials; remove these token fields
from ServerOAuthConfig and persist only non-secret metadata (enabled,
providerLabel, issuer, clientId, scopes, redirectUri) and instead store runtime
tokens in a separate non-persisted structure (e.g., a RuntimeOAuthSession or
PlatformSecureStore) or keep them in memory/session management; update any code
referencing ServerOAuthConfig token properties to read/write from the new
runtime token store (search for ServerOAuthConfig and ServerConfig usages to
locate places to change).
In `@tests/lib/oauth.test.ts`:
- Around line 69-76: The test suite replaces globalThis.fetch with fetchMock but
never restores the original implementation, causing later tests to see the mock;
capture the original fetch (e.g., const originalFetch = globalThis.fetch) before
overriding in the beforeEach (or beforeAll), assign globalThis.fetch = fetchMock
in beforeEach, and then restore globalThis.fetch = originalFetch in afterEach
(and continue to reset fetchMock there) so the original global fetch is returned
after each test; update references around fetchMock, beforeEach, and afterEach
accordingly.
---
Outside diff comments:
In `@src/store/index.ts`:
- Around line 1023-1042: The server rewrite in the updatedServers.push block
currently omits savedServer?.oauth so OAuth provider config and tokens get
dropped on first successful connect; update the object built in
updatedServers.push to include an oauth field copied from savedServer?.oauth
(e.g., oauth: savedServer?.oauth) so the provider config and tokens are
preserved when rewriting the server entry (keep the existing pattern used for
operUsername/operPassword/addedAt to maintain compatibility).
---
Nitpick comments:
In `@src/lib/oauth.ts`:
- Around line 156-167: The code currently reuses generateCodeVerifier() to
produce the OAuth state which is semantically misleading; change the helper to a
clearly named function (e.g., generateRandomToken() or generateState()) and use
it for the state variable while keeping generateCodeVerifier() for PKCE
only—update the call site where state is set (the block that computes
codeVerifier, deriveCodeChallenge, state, scope, redirectUri and calls
buildAuthorizeUrl) to call the new generateRandomToken()/generateState() for
state, leaving codeVerifier and deriveCodeChallenge unchanged so intent is
explicit.
🪄 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: 9542d3e1-ef8a-4a56-8cc2-7cf5847e3b38
📒 Files selected for processing (12)
src/App.tsxsrc/components/OAuthCallback.tsxsrc/components/ui/EditServerModal.tsxsrc/components/ui/OAuthSection.tsxsrc/lib/irc/IRCClient.tssrc/lib/oauth.tssrc/lib/saslFrames.tssrc/store/handlers/auth.tssrc/store/index.tssrc/types/index.tstests/lib/oauth.test.tstests/lib/saslFrames.test.ts
Comment on lines
+51
to
+66
| export async function discoverOidc(issuer: string): Promise<OidcMetadata> { | ||
| const trimmed = issuer.replace(/\/+$/, ""); | ||
| const cached = discoveryCache.get(trimmed); | ||
| if (cached) return cached; | ||
| const url = `${trimmed}/.well-known/openid-configuration`; | ||
| const res = await fetch(url, { credentials: "omit" }); | ||
| if (!res.ok) { | ||
| throw new Error(`OIDC discovery failed (${res.status}) at ${url}`); | ||
| } | ||
| const meta = (await res.json()) as OidcMetadata; | ||
| if (!meta.authorization_endpoint || !meta.token_endpoint) { | ||
| throw new Error("OIDC metadata missing endpoints"); | ||
| } | ||
| discoveryCache.set(trimmed, meta); | ||
| return meta; | ||
| } |
Contributor
There was a problem hiding this comment.
Validate the issuer scheme before issuing discovery requests.
discoverOidc accepts whatever string the admin pasted and immediately fetches ${issuer}/.well-known/openid-configuration. There is no scheme check, so http://internal.box/oidc (or even an IP literal) will be probed and could leak the user's IP / a JWT-bearing redirect into a hostile network. Reject anything that is not https:// (allowing http://localhost for dev if you want a developer escape hatch).
🔧 Suggested guard
export async function discoverOidc(issuer: string): Promise<OidcMetadata> {
- const trimmed = issuer.replace(/\/+$/, "");
+ const trimmed = issuer.replace(/\/+$/, "");
+ let parsed: URL;
+ try {
+ parsed = new URL(trimmed);
+ } catch {
+ throw new Error(`Invalid issuer URL: ${issuer}`);
+ }
+ const isLocalhost =
+ parsed.hostname === "localhost" || parsed.hostname === "127.0.0.1";
+ if (parsed.protocol !== "https:" && !isLocalhost) {
+ throw new Error("Issuer must use https://");
+ }
const cached = discoveryCache.get(trimmed);📝 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
| export async function discoverOidc(issuer: string): Promise<OidcMetadata> { | |
| const trimmed = issuer.replace(/\/+$/, ""); | |
| const cached = discoveryCache.get(trimmed); | |
| if (cached) return cached; | |
| const url = `${trimmed}/.well-known/openid-configuration`; | |
| const res = await fetch(url, { credentials: "omit" }); | |
| if (!res.ok) { | |
| throw new Error(`OIDC discovery failed (${res.status}) at ${url}`); | |
| } | |
| const meta = (await res.json()) as OidcMetadata; | |
| if (!meta.authorization_endpoint || !meta.token_endpoint) { | |
| throw new Error("OIDC metadata missing endpoints"); | |
| } | |
| discoveryCache.set(trimmed, meta); | |
| return meta; | |
| } | |
| export async function discoverOidc(issuer: string): Promise<OidcMetadata> { | |
| const trimmed = issuer.replace(/\/+$/, ""); | |
| let parsed: URL; | |
| try { | |
| parsed = new URL(trimmed); | |
| } catch { | |
| throw new Error(`Invalid issuer URL: ${issuer}`); | |
| } | |
| const isLocalhost = | |
| parsed.hostname === "localhost" || parsed.hostname === "127.0.0.1"; | |
| if (parsed.protocol !== "https:" && !isLocalhost) { | |
| throw new Error("Issuer must use https://"); | |
| } | |
| const cached = discoveryCache.get(trimmed); | |
| if (cached) return cached; | |
| const url = `${trimmed}/.well-known/openid-configuration`; | |
| const res = await fetch(url, { credentials: "omit" }); | |
| if (!res.ok) { | |
| throw new Error(`OIDC discovery failed (${res.status}) at ${url}`); | |
| } | |
| const meta = (await res.json()) as OidcMetadata; | |
| if (!meta.authorization_endpoint || !meta.token_endpoint) { | |
| throw new Error("OIDC metadata missing endpoints"); | |
| } | |
| discoveryCache.set(trimmed, meta); | |
| return meta; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/oauth.ts` around lines 51 - 66, The discoverOidc function currently
fetches the issuer URL without validating its scheme, which can leak requests to
non-HTTPS endpoints; update discoverOidc to validate the issuer string before
performing discovery: parse the issuer, ensure the URL protocol is https:
(permit http: only when hostname is localhost or 127.0.0.1 for a dev escape
hatch), and throw a clear error if the scheme is not allowed; keep the rest of
the logic (using discoveryCache, building the /.well-known/openid-configuration
URL, and validating authorization_endpoint/token_endpoint) unchanged.
Comment on lines
+182
to
+224
| const code = await new Promise<string>((resolve, reject) => { | ||
| let closed = false; | ||
| const cleanup = () => { | ||
| window.removeEventListener("message", onMessage); | ||
| clearInterval(closeTimer); | ||
| }; | ||
| const onMessage = (event: MessageEvent) => { | ||
| // Same-origin only; the callback page is served by the SPA. | ||
| if (event.origin !== window.location.origin) return; | ||
| const data = event.data as OAuthCallbackMessage | undefined; | ||
| if (!data || data.type !== "obsidianirc:oauth-callback") return; | ||
| if (data.state !== state) { | ||
| cleanup(); | ||
| reject(new Error("OAuth state mismatch (possible CSRF)")); | ||
| return; | ||
| } | ||
| cleanup(); | ||
| try { | ||
| popup.close(); | ||
| } catch {} | ||
| if (data.error) { | ||
| reject( | ||
| new Error( | ||
| data.errorDescription | ||
| ? `${data.error}: ${data.errorDescription}` | ||
| : data.error, | ||
| ), | ||
| ); | ||
| } else if (data.code) { | ||
| resolve(data.code); | ||
| } else { | ||
| reject(new Error("OAuth callback returned no code")); | ||
| } | ||
| }; | ||
| window.addEventListener("message", onMessage); | ||
| const closeTimer = setInterval(() => { | ||
| if (popup.closed && !closed) { | ||
| closed = true; | ||
| cleanup(); | ||
| reject(new Error("OAuth popup was closed before completing")); | ||
| } | ||
| }, 500); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add an overall timeout and harden popup-closed detection.
Two reliability gaps in the popup wait:
- There is no upper bound on how long the Promise stays pending. If the user closes the tab focus, the IdP loses session, or postMessage never fires (e.g., the callback page hits a runtime error),
signingInstaystrueforever and the only escape is reloading the SPA. popup.closedpolling can transiently reporttrueduring the IdP→/oauth/callbackcross-origin navigation in some browsers (notably Safari/iOS). That flips the listener into the "closed before completing" reject branch even though the user did complete sign-in. Consider a short grace period after detecting close, or only treating a sustainedclosed === true(e.g., 2 consecutive ticks) as cancellation.
Also note that closeTimer is referenced inside cleanup (Line 186) before its const declaration (Line 217). It works at runtime because cleanup only runs after the listener fires, but it is fragile and will TDZ-throw if anyone refactors cleanup to be invoked synchronously. Easy to declare the timer above cleanup to remove the foot-gun.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/oauth.ts` around lines 182 - 224, The Promise that waits for the
OAuth popup (the code promise created around onMessage/cleanup/closeTimer) lacks
an overall timeout, treats transient popup.closed as immediate cancellation, and
declares closeTimer after cleanup which risks TDZ; fix by declaring closeTimer
before cleanup, implement an overall timeout (e.g., 2min) that rejects with a
clear error and is cleared on any resolve/reject, and harden popup-closed
detection by requiring sustained closed state (e.g., two consecutive ticks or a
short grace period) before rejecting; ensure cleanup clears both the closeTimer
interval and the timeout and that onMessage, popup.close(), resolve(data.code)
and all reject paths invoke cleanup so timers are always cleared.
Comment on lines
+72
to
+79
| const oauth = getActiveOauth(serv); | ||
| if (oauth?.accessToken) { | ||
| const b64 = buildIrcv3BearerPayload({ token: oauth.accessToken }); | ||
| for (const chunk of chunkSaslPayload(b64)) { | ||
| ircClient.sendRaw(serverId, `AUTHENTICATE ${chunk}`); | ||
| } | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the chunker and its tests for the boundary case.
fd -t f 'saslFrames' src
echo '----- impl -----'
fd -t f 'saslFrames.ts' src --exec cat {}
echo '----- tests -----'
fd -t f 'saslFrames' tests --exec cat {}Repository: ObsidianIRC/ObsidianIRC
Length of output: 4607
Fix the boundary check to only emit the + sentinel for non-zero multiples of 400 bytes.
The implementation correctly emits the trailing AUTHENTICATE + when the payload length is an exact multiple of 400. However, the condition if (b64.length % SASL_CHUNK === 0) is true even when b64.length is 0, which would incorrectly yield ["+"] and send a bare AUTHENTICATE + without auth data. Per the IRCv3 SASL spec's requirement for a "non-zero multiple," the check should be:
if (b64.length > 0 && b64.length % SASL_CHUNK === 0) chunks.push("+");While unlikely in practice due to the if (oauth?.accessToken) guard in the calling code, this fixes the spec violation and prevents the edge case from producing invalid protocol sequences.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/handlers/auth.ts` around lines 72 - 79, The chunkSaslPayload
function currently adds a trailing "+" when b64.length % SASL_CHUNK === 0, which
wrongly triggers for an empty payload; update the boundary check in
chunkSaslPayload to only push the "+" sentinel when b64.length > 0 && b64.length
% SASL_CHUNK === 0 so that a zero-length b64 (from
buildIrcv3BearerPayload/token) does not produce an extraneous AUTHENTICATE +;
keep the rest of the chunking logic unchanged and reference SASL_CHUNK and the
b64 variable within chunkSaslPayload when making this change.
Comment on lines
+988
to
+991
| const oauthBearerEnabled = !!( | ||
| existingSavedServer?.oauth?.enabled && | ||
| existingSavedServer.oauth.accessToken | ||
| ); |
Contributor
There was a problem hiding this comment.
Ignore expired bearer tokens when deciding to force SASL OAuth.
This currently enables IRCV3BEARER for any non-empty token, including tokens that are already past tokenExpiresAt. That turns a stale saved session into a guaranteed reconnect/auth failure instead of prompting the user to sign in again or falling back to other auth.
Suggested fix
- const oauthBearerEnabled = !!(
- existingSavedServer?.oauth?.enabled &&
- existingSavedServer.oauth.accessToken
- );
+ const oauth = existingSavedServer?.oauth;
+ const oauthBearerEnabled = !!(
+ oauth?.enabled &&
+ oauth.accessToken &&
+ (!oauth.tokenExpiresAt || oauth.tokenExpiresAt > Date.now())
+ );📝 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 oauthBearerEnabled = !!( | |
| existingSavedServer?.oauth?.enabled && | |
| existingSavedServer.oauth.accessToken | |
| ); | |
| const oauth = existingSavedServer?.oauth; | |
| const oauthBearerEnabled = !!( | |
| oauth?.enabled && | |
| oauth.accessToken && | |
| (!oauth.tokenExpiresAt || oauth.tokenExpiresAt > Date.now()) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/index.ts` around lines 988 - 991, The oauthBearerEnabled check
currently treats any non-empty accessToken as valid; update the logic in the
block that computes oauthBearerEnabled (referencing existingSavedServer,
existingSavedServer.oauth.accessToken, and
existingSavedServer.oauth.tokenExpiresAt) to only return true when accessToken
exists AND tokenExpiresAt is in the future (e.g. tokenExpiresAt > Date.now()).
Ensure this change prevents selecting IRCV3BEARER/forceSaslOauth when the saved
token is expired so the UI falls back to prompting sign-in or other auth flows.
Comment on lines
+69
to
+76
| const fetchMock = vi.fn(); | ||
| beforeEach(() => { | ||
| fetchMock.mockReset(); | ||
| globalThis.fetch = fetchMock as unknown as typeof fetch; | ||
| }); | ||
| afterEach(() => { | ||
| fetchMock.mockReset(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Restore the original fetch after each test.
This suite replaces globalThis.fetch but only resets the mock state. Later tests will still see the mocked function, which makes cross-suite failures order-dependent.
Suggested fix
describe("discoverOidc", () => {
const fetchMock = vi.fn();
+ const originalFetch = globalThis.fetch;
beforeEach(() => {
fetchMock.mockReset();
globalThis.fetch = fetchMock as unknown as typeof fetch;
});
afterEach(() => {
fetchMock.mockReset();
+ globalThis.fetch = originalFetch;
});📝 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 fetchMock = vi.fn(); | |
| beforeEach(() => { | |
| fetchMock.mockReset(); | |
| globalThis.fetch = fetchMock as unknown as typeof fetch; | |
| }); | |
| afterEach(() => { | |
| fetchMock.mockReset(); | |
| }); | |
| const fetchMock = vi.fn(); | |
| const originalFetch = globalThis.fetch; | |
| beforeEach(() => { | |
| fetchMock.mockReset(); | |
| globalThis.fetch = fetchMock as unknown as typeof fetch; | |
| }); | |
| afterEach(() => { | |
| fetchMock.mockReset(); | |
| globalThis.fetch = originalFetch; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/lib/oauth.test.ts` around lines 69 - 76, The test suite replaces
globalThis.fetch with fetchMock but never restores the original implementation,
causing later tests to see the mock; capture the original fetch (e.g., const
originalFetch = globalThis.fetch) before overriding in the beforeEach (or
beforeAll), assign globalThis.fetch = fetchMock in beforeEach, and then restore
globalThis.fetch = originalFetch in afterEach (and continue to reset fetchMock
there) so the original global fetch is returned after each test; update
references around fetchMock, beforeEach, and afterEach accordingly.
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BUILD.md`:
- Around line 62-65: Update the Docker build example in BUILD.md to document the
missing OAuth build args by adding VITE_DEFAULT_OAUTH_SCOPES and
VITE_DEFAULT_OAUTH_REDIRECT_URI alongside the existing
VITE_DEFAULT_OAUTH_PROVIDER_LABEL, VITE_DEFAULT_OAUTH_ISSUER and
VITE_DEFAULT_OAUTH_CLIENT_ID entries; show example values for
VITE_DEFAULT_OAUTH_SCOPES (e.g. "openid profile email") and
VITE_DEFAULT_OAUTH_REDIRECT_URI (e.g. "https://your-app/callback") and note that
these can be overridden at build time so admins requiring non-default scopes or
an explicit callback URL can pass them.
In `@src/components/ui/EditServerModal.tsx`:
- Around line 140-151: The current assignment to updatedConfig always sets
password/saslPassword/saslEnabled to undefined/false when inputs are empty,
wiping stored credentials; change logic so updatedConfig only includes password,
saslPassword (and saslEnabled) when the user explicitly provided values or
toggled auth, otherwise omit those keys so the existing ServerConfig is
preserved. Concretely, in the code that builds updatedConfig, only add password
when password.trim() !== ''; only add saslPassword (base64-encode) and set
saslEnabled when saslPassword.trim() !== '' or when the user explicitly toggled
the SASL enable control; otherwise do not set saslPassword/saslEnabled on
updatedConfig (leave them untouched). Ensure you modify the object construction
around updatedConfig/ServerConfig to conditionally include those properties
rather than assigning undefined/false unconditionally.
In `@src/store/index.ts`:
- Around line 991-1001: The pre-SASL save only updates an existing saved server,
so first-time connects with oauthConfig miss persistence; modify the block
handling oauthConfig/mergedOauth (the code using mergedOauth, oauthConfig,
existingSavedServer, loadSavedServers, saveServersToLocalStorage) to also create
and save a provisional server record when existingSavedServer is undefined
(populate at least id and oauth) before opening the socket so the auth handler
can read the token; alternatively ensure the oauthConfig is handed directly into
the auth path instead of relying on localStorage reads.
🪄 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: e95bdbb3-4f43-4eec-80d4-7cc83b7ad78a
📒 Files selected for processing (10)
BUILD.mdDockerfilesrc/components/ui/AddServerModal.tsxsrc/components/ui/EditServerModal.tsxsrc/components/ui/OAuthSection.tsxsrc/lib/oauth.tssrc/store/index.tssrc/vite-env.d.tstests/lib/oauth.test.tsvite.config.ts
✅ Files skipped from review due to trivial changes (1)
- src/vite-env.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/oauth.ts
Comment on lines
140
to
151
| const updatedConfig: Partial<ServerConfig> = { | ||
| name: finalServerName, | ||
| host: serverHost.trim(), | ||
| port: Number.parseInt(serverPort, 10), | ||
| nickname: nickname.trim(), | ||
| password: password.trim() || undefined, | ||
| saslAccountName: finalSaslAccountName || undefined, | ||
| saslPassword: saslPassword.trim() || undefined, | ||
| // The existing storage format keeps SASL passwords base64-encoded. | ||
| saslPassword: saslPassword.trim() | ||
| ? btoa(saslPassword.trim()) | ||
| : undefined, | ||
| saslEnabled: !!saslPassword.trim(), |
Contributor
There was a problem hiding this comment.
Blank auth fields currently delete stored credentials.
The UI says “leave blank to keep”, but password/saslPassword are written as undefined when the inputs are empty and saslEnabled flips to false. Saving any unrelated edit will silently wipe the stored PASS/SASL credentials instead of preserving them.
Suggested fix
+ const nextServerPassword = password.trim() || serverConfig?.password;
+ const nextSaslPassword = saslPassword.trim()
+ ? btoa(saslPassword.trim())
+ : serverConfig?.saslPassword;
+
const updatedConfig: Partial<ServerConfig> = {
name: finalServerName,
host: serverHost.trim(),
port: Number.parseInt(serverPort, 10),
nickname: nickname.trim(),
- password: password.trim() || undefined,
+ password: nextServerPassword,
saslAccountName: finalSaslAccountName || undefined,
- saslPassword: saslPassword.trim()
- ? btoa(saslPassword.trim())
- : undefined,
- saslEnabled: !!saslPassword.trim(),
+ saslPassword: nextSaslPassword,
+ saslEnabled: !!nextSaslPassword,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/EditServerModal.tsx` around lines 140 - 151, The current
assignment to updatedConfig always sets password/saslPassword/saslEnabled to
undefined/false when inputs are empty, wiping stored credentials; change logic
so updatedConfig only includes password, saslPassword (and saslEnabled) when the
user explicitly provided values or toggled auth, otherwise omit those keys so
the existing ServerConfig is preserved. Concretely, in the code that builds
updatedConfig, only add password when password.trim() !== ''; only add
saslPassword (base64-encode) and set saslEnabled when saslPassword.trim() !== ''
or when the user explicitly toggled the SASL enable control; otherwise do not
set saslPassword/saslEnabled on updatedConfig (leave them untouched). Ensure you
modify the object construction around updatedConfig/ServerConfig to
conditionally include those properties rather than assigning undefined/false
unconditionally.
Comment on lines
+991
to
+1001
| // Auth-handler reads OAuth out of localStorage when SASL fires, so | ||
| // persist any caller-supplied oauth config BEFORE opening the | ||
| // socket -- otherwise the first SASL turn would race past it. | ||
| const mergedOauth = oauthConfig ?? existingSavedServer?.oauth; | ||
| if (oauthConfig && existingSavedServer?.id) { | ||
| const pre = loadSavedServers(); | ||
| const idx = pre.findIndex((s) => s.id === existingSavedServer.id); | ||
| if (idx !== -1) { | ||
| pre[idx] = { ...pre[idx], oauth: oauthConfig }; | ||
| saveServersToLocalStorage(pre); | ||
| } |
Contributor
There was a problem hiding this comment.
Fresh OAuth connects still miss the pre-SASL token persistence.
This early-save block only runs when the server already exists in localStorage. On the first connect for a new server, existingSavedServer is undefined, so the auth handler has nothing to read when SASL starts and IRCV3BEARER can fail before the server is ever saved. Persist a provisional record first, or pass the OAuth config directly into the auth path instead of reading it back from storage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/index.ts` around lines 991 - 1001, The pre-SASL save only updates
an existing saved server, so first-time connects with oauthConfig miss
persistence; modify the block handling oauthConfig/mergedOauth (the code using
mergedOauth, oauthConfig, existingSavedServer, loadSavedServers,
saveServersToLocalStorage) to also create and save a provisional server record
when existingSavedServer is undefined (populate at least id and oauth) before
opening the socket so the auth handler can read the token; alternatively ensure
the oauthConfig is handed directly into the auth path instead of relying on
localStorage reads.
ValwareIRC
added a commit
to obbyworld/ObbyIRCd
that referenced
this pull request
May 9, 2026
Lets the IRC server accept OAuth2/OIDC bearer tokens (JWTs) as a SASL mechanism, so users can log in via Logto, Auth0, Keycloak, Okta, or any IdP that publishes a JWKS and signs RS256/384/512 access or ID tokens. The companion ObsidianIRC client lives in PR obbyworld/obby#196. config account-registration { oauth-provider "logto" { issuer 'https://my-tenant.logto.app/oidc'; audience 'https://api.example.com'; jwks-file "/home/valware/obby/conf/logto-jwks.json"; subject-claim "sub"; # or "preferred_username" }; }; JWKS is loaded from the file at startup / on /REHASH; admin keeps it fresh out-of-band (curl in a cron is fine for now). Multiple oauth-provider blocks may coexist -- the issuer claim picks which one validates a given token. token validation (oauth_validate_jwt) * splits header.payload.sig, base64url-decodes each * looks up provider by `iss` * checks `aud` (string or array), `exp`, `nbf` with 60s leeway * picks the JWKS key by `kid` (or first key when token has no kid) * EVP_DigestVerify against the RSA public key for RS256/384/512 * returns the configured subject claim ("sub" or "preferred_username") linking + login OAuth identities are stored as rows in the existing account_2fa_credentials table with type='oauth' and secret='<provider>\x1F<subject>'. Reuses the existing 2FA UX surface: /2FA CHALLENGE oauth <provider> -> opens token-input session /2FA TOKEN :<chunk> -> repeated, since JWTs > 512B /2FA ADD oauth <name> -> validates buffered token, stores credential Login then uses either SASL mechanism: AUTHENTICATE IRCV3BEARER # [authzid]\0<type>\0<token> AUTHENTICATE OAUTHBEARER # RFC 7628 GS2 framing Both run through oauth_login_by_token, which validates the JWT, looks up the (provider,subject) credential, and either logs the user in or hands off to twofa_maybe_start_stepup() if 2FA is enrolled. SASL chunked AUTHENTICATE traffic is reassembled via a per-client oauth_sasl_md ModData buffer. other * accreg_configrun() drops the old provider list before re-parsing, so /REHASH doesn't accumulate duplicates. * old account_oauth_links table from an earlier prototype is DROP TABLE IF EXISTS'd on every open_database(). * advertised in saslmechs() and CAP LS sasl=... * end-to-end tested with synthetic RSA keypair + JWKS + JWT against both IRCV3BEARER and OAUTHBEARER mechanisms. Known gaps (not in this commit): * Opaque tokens (GitHub-style) -- needs userinfo HTTP call. * EC keys (ES256/384/512) -- only RSA implemented. * Auto-fetch JWKS from issuer URL -- file-based for now.
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).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BUILD.md`:
- Around line 71-77: The fenced config blocks are unlabeled and triggering
markdownlint; update both fences that contain the oauth-provider "google"
snippet (the block with issuer, audience, jwks-file, subject-claim) and the
other block at lines ~95-99 by adding a language tag (e.g., conf or text) after
the opening ``` so the fences become ```conf (or ```text) to satisfy
markdownlint without altering the content.
In `@src/components/ui/OAuthSection.tsx`:
- Around line 104-117: The emitted object from onChange (used by emit) is
missing authorizeEndpoint and tokenEndpoint so manual/auth token endpoints set
by onPresetChange aren't persisted; update the object passed in onChange (and
the similar emit call around the token fields at lines ~142-148) to include
authorizeEndpoint: patch.authorizeEndpoint ?? authorizeEndpoint ?? undefined and
tokenEndpoint: patch.tokenEndpoint ?? tokenEndpoint ?? undefined so those
endpoints are sent upstream and persisted.
- Around line 135-156: When applying a new preset (inside the preset-handling
block that reads np), ensure provider-specific endpoint state is cleared when
the preset does not supply them: if np.authorizeEndpoint is undefined call
setAuthorizeEndpoint with an empty string (and emit an update to clear
authorizeEndpoint) and likewise if np.tokenEndpoint is undefined call
setTokenEndpoint with an empty string (and emit to clear tokenEndpoint); this
prevents stale GitHub endpoints from persisting and being used by handleSignIn
when switching to Google/Auth0/custom.
In `@src/lib/oauth.ts`:
- Around line 162-169: The code currently uses cfg.authorizeEndpoint and
cfg.tokenEndpoint directly to build meta (bypassing discoverOidc), which can
cause leaking auth code/PKCE to untrusted origins; before using these manual
endpoints (references: cfg.authorizeEndpoint, cfg.tokenEndpoint, and the meta
construction logic that falls back to discoverOidc), validate that both URLs
belong to a trusted origin: perform origin whitelist checks and, if necessary, a
safe HEAD/GET probe only to trusted origins to verify reachability, or
reject/require discovery if the origin is not trusted; apply the same validation
to other manual-endpoint branches (the other manual paths around the same cfg
fields at the later sections) so no network requests are made to untrusted
origins.
- Around line 123-133: The GitHub preset token exchange in src/lib/oauth.ts is
unsafe and brittle: the client-only POST to args.meta.token_endpoint cannot
include the required client_secret and also lacks an Accept header so res.json()
may fail; update the code to either move the token exchange to a backend
endpoint that securely supplies client_secret (and call that backend from the
client), or remove the GitHub preset from the client flow; if keeping a
client-side exchange for non-GitHub providers, add "Accept: application/json" to
the fetch headers and guard the response parsing by inspecting
res.headers.get("content-type") before calling res.json() and map to
OAuthTokenResponse only when JSON is returned (otherwise parse form-encoded
text), ensuring errors include the raw response text when res.ok is false.
🪄 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: 4f72f741-5d40-475f-b5a4-82996dc105f1
📒 Files selected for processing (10)
BUILD.mdDockerfilesrc/components/ui/OAuthSection.tsxsrc/lib/oauth.tssrc/store/handlers/auth.tssrc/store/index.tssrc/types/index.tssrc/vite-env.d.tstests/lib/saslFrames.test.tsvite.config.ts
✅ Files skipped from review due to trivial changes (1)
- tests/lib/saslFrames.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- Dockerfile
- src/store/handlers/auth.ts
- src/types/index.ts
- src/store/index.ts
Comment on lines
+71
to
+77
| ``` | ||
| oauth-provider "google" { | ||
| issuer 'https://accounts.google.com'; | ||
| audience '<your_client_id>.apps.googleusercontent.com'; | ||
| jwks-file "/etc/obbyircd/google-jwks.json"; # curl from https://www.googleapis.com/oauth2/v3/certs | ||
| subject-claim "sub"; | ||
| }; |
Contributor
There was a problem hiding this comment.
Add a language tag to these fenced config blocks.
markdownlint is still flagging both unlabeled fences. Adding a simple language like conf or text will clear the warning without changing the content.
Also applies to: 95-99
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@BUILD.md` around lines 71 - 77, The fenced config blocks are unlabeled and
triggering markdownlint; update both fences that contain the oauth-provider
"google" snippet (the block with issuer, audience, jwks-file, subject-claim) and
the other block at lines ~95-99 by adding a language tag (e.g., conf or text)
after the opening ``` so the fences become ```conf (or ```text) to satisfy
markdownlint without altering the content.
Comment on lines
+104
to
+117
| onChange({ | ||
| enabled: patch.enabled ?? enabled, | ||
| providerLabel: patch.providerLabel ?? providerLabel, | ||
| issuer: patch.issuer ?? issuer, | ||
| clientId: patch.clientId ?? clientId, | ||
| scopes: patch.scopes ?? scopes, | ||
| redirectUri: patch.redirectUri ?? redirectUri ?? undefined, | ||
| accessToken: patch.accessToken ?? accessToken ?? undefined, | ||
| idToken: patch.idToken ?? idToken ?? undefined, | ||
| refreshToken: patch.refreshToken ?? refreshToken ?? undefined, | ||
| tokenExpiresAt: patch.tokenExpiresAt ?? tokenExpiresAt, | ||
| tokenKind: patch.tokenKind ?? tokenKind, | ||
| serverProvider: patch.serverProvider ?? serverProvider ?? undefined, | ||
| }); |
Contributor
There was a problem hiding this comment.
Persist manual auth/token endpoints in emit.
onPresetChange updates authorizeEndpoint and tokenEndpoint locally, but emit never includes them in the object sent upstream. That means GitHub/custom endpoint config only works for the current modal session; once the parent persists/reloads the server, the next sign-in falls back to discovery.
Suggested fix
onChange({
enabled: patch.enabled ?? enabled,
providerLabel: patch.providerLabel ?? providerLabel,
issuer: patch.issuer ?? issuer,
clientId: patch.clientId ?? clientId,
scopes: patch.scopes ?? scopes,
redirectUri: patch.redirectUri ?? redirectUri ?? undefined,
+ authorizeEndpoint:
+ patch.authorizeEndpoint ?? authorizeEndpoint ?? undefined,
+ tokenEndpoint: patch.tokenEndpoint ?? tokenEndpoint ?? undefined,
accessToken: patch.accessToken ?? accessToken ?? undefined,
idToken: patch.idToken ?? idToken ?? undefined,
refreshToken: patch.refreshToken ?? refreshToken ?? undefined,
tokenExpiresAt: patch.tokenExpiresAt ?? tokenExpiresAt,
tokenKind: patch.tokenKind ?? tokenKind,Also applies to: 142-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/OAuthSection.tsx` around lines 104 - 117, The emitted
object from onChange (used by emit) is missing authorizeEndpoint and
tokenEndpoint so manual/auth token endpoints set by onPresetChange aren't
persisted; update the object passed in onChange (and the similar emit call
around the token fields at lines ~142-148) to include authorizeEndpoint:
patch.authorizeEndpoint ?? authorizeEndpoint ?? undefined and tokenEndpoint:
patch.tokenEndpoint ?? tokenEndpoint ?? undefined so those endpoints are sent
upstream and persisted.
Comment on lines
+135
to
+156
| // Apply token kind + manual endpoints baked into the preset (GitHub | ||
| // is opaque + non-OIDC; Google is jwt + OIDC; etc.). For "custom" | ||
| // we don't override anything. | ||
| if (np.tokenKind && np.id !== "custom") { | ||
| setTokenKind(np.tokenKind); | ||
| emit({ tokenKind: np.tokenKind }); | ||
| } | ||
| if (np.authorizeEndpoint !== undefined) { | ||
| setAuthorizeEndpoint(np.authorizeEndpoint); | ||
| emit({ authorizeEndpoint: np.authorizeEndpoint }); | ||
| } | ||
| if (np.tokenEndpoint !== undefined) { | ||
| setTokenEndpoint(np.tokenEndpoint); | ||
| emit({ tokenEndpoint: np.tokenEndpoint }); | ||
| } | ||
| // Default the server-provider hint to the preset id for opaque flows | ||
| // so the user sees a sensible default ("github") matching what | ||
| // obbyircd's oauth-provider {} block would typically be named. | ||
| if (np.tokenKind === "opaque" && !serverProvider.trim()) { | ||
| setServerProvider(np.id); | ||
| emit({ serverProvider: np.id }); | ||
| } |
Contributor
There was a problem hiding this comment.
Clear provider-specific endpoint state when switching presets.
If the user selects GitHub and then switches to Google/Auth0/custom, the old authorizeEndpoint / tokenEndpoint values stay in state because only defined preset values overwrite them. handleSignIn then sees both endpoints, skips discovery, and reuses the stale GitHub endpoints for the new provider.
Suggested fix
- if (np.authorizeEndpoint !== undefined) {
- setAuthorizeEndpoint(np.authorizeEndpoint);
- emit({ authorizeEndpoint: np.authorizeEndpoint });
- }
- if (np.tokenEndpoint !== undefined) {
- setTokenEndpoint(np.tokenEndpoint);
- emit({ tokenEndpoint: np.tokenEndpoint });
- }
+ const nextAuthorizeEndpoint = np.authorizeEndpoint ?? "";
+ const nextTokenEndpoint = np.tokenEndpoint ?? "";
+ setAuthorizeEndpoint(nextAuthorizeEndpoint);
+ setTokenEndpoint(nextTokenEndpoint);
+ emit({
+ authorizeEndpoint: nextAuthorizeEndpoint || undefined,
+ tokenEndpoint: nextTokenEndpoint || undefined,
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/OAuthSection.tsx` around lines 135 - 156, When applying a
new preset (inside the preset-handling block that reads np), ensure
provider-specific endpoint state is cleared when the preset does not supply
them: if np.authorizeEndpoint is undefined call setAuthorizeEndpoint with an
empty string (and emit an update to clear authorizeEndpoint) and likewise if
np.tokenEndpoint is undefined call setTokenEndpoint with an empty string (and
emit to clear tokenEndpoint); this prevents stale GitHub endpoints from
persisting and being used by handleSignIn when switching to Google/Auth0/custom.
Comment on lines
+123
to
+133
| const res = await fetch(args.meta.token_endpoint, { | ||
| method: "POST", | ||
| headers: { "content-type": "application/x-www-form-urlencoded" }, | ||
| body: body.toString(), | ||
| credentials: "omit", | ||
| }); | ||
| if (!res.ok) { | ||
| const txt = await res.text(); | ||
| throw new Error(`Token exchange failed (${res.status}): ${txt}`); | ||
| } | ||
| return (await res.json()) as OAuthTokenResponse; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the oauth.ts file
fd -t f "oauth.ts" | head -20Repository: ObsidianIRC/ObsidianIRC
Length of output: 84
🏁 Script executed:
# Get file size to determine how to read it
wc -l src/lib/oauth.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 88
🏁 Script executed:
# Read the full oauth.ts file
cat -n src/lib/oauth.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 14329
🏁 Script executed:
# Search for GitHub preset references in the codebase
rg -i "github.*preset|preset.*github" -A 3 -B 3Repository: ObsidianIRC/ObsidianIRC
Length of output: 606
🌐 Web query:
GitHub OAuth token endpoint response format client_secret requirement
💡 Result:
The GitHub OAuth token endpoint (POST https://github.com/login/oauth/access_token) requires the client_secret parameter for the standard authorization code flow. It is listed as "Required" alongside client_id and code in the official documentation [1][2]. Omitting it results in an "incorrect_client_credentials" error [1][3]. The response format depends on the Accept header: - Default (form-urlencoded): access_token=...,&scope=...,&token_type=bearer [1][2] - application/json: {"access_token":"...", "scope":"...", "token_type":"bearer"} [1][2] - application/xml: <token_type>bearer</token_type>...<access_token>...</access_token> [1][2] Note: client_secret is not required for the device flow [1].
Citations:
- 1: https://docs.github.com/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps
- 2: https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps
- 3: https://docs2.lfe.io/v3/oauth/
GitHub preset cannot complete token exchange in this client-only implementation.
GitHub's OAuth token endpoint requires client_secret as a mandatory parameter, but this SPA-only code has no way to safely include it (it would be exposed to users). Additionally, the code sends no Accept: application/json header, so GitHub returns form-encoded data by default, yet line 133 unconditionally calls res.json(), which will fail. Either move GitHub behind a backend-assisted exchange that can securely hold the client secret, or remove this preset from the current client-only flow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/oauth.ts` around lines 123 - 133, The GitHub preset token exchange in
src/lib/oauth.ts is unsafe and brittle: the client-only POST to
args.meta.token_endpoint cannot include the required client_secret and also
lacks an Accept header so res.json() may fail; update the code to either move
the token exchange to a backend endpoint that securely supplies client_secret
(and call that backend from the client), or remove the GitHub preset from the
client flow; if keeping a client-side exchange for non-GitHub providers, add
"Accept: application/json" to the fetch headers and guard the response parsing
by inspecting res.headers.get("content-type") before calling res.json() and map
to OAuthTokenResponse only when JSON is returned (otherwise parse form-encoded
text), ensuring errors include the raw response text when res.ok is false.
Comment on lines
+162
to
+169
| const meta: OidcMetadata = | ||
| cfg.authorizeEndpoint && cfg.tokenEndpoint | ||
| ? { | ||
| issuer: cfg.issuer, | ||
| authorization_endpoint: cfg.authorizeEndpoint, | ||
| token_endpoint: cfg.tokenEndpoint, | ||
| } | ||
| : await discoverOidc(cfg.issuer); |
Contributor
There was a problem hiding this comment.
Validate manual authorize/token endpoints before using them.
The new non-OIDC path bypasses discovery, but it still accepts arbitrary authorizeEndpoint / tokenEndpoint values and then opens or posts to them from the browser. A bad server config can leak the auth code and PKCE verifier to the wrong origin.
As per coding guidelines, Only make HTTP requests (HEAD or GET) for URLs from trusted origins; check trust levels before any network request to prevent IP leakage.
Also applies to: 185-189, 240-246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/oauth.ts` around lines 162 - 169, The code currently uses
cfg.authorizeEndpoint and cfg.tokenEndpoint directly to build meta (bypassing
discoverOidc), which can cause leaking auth code/PKCE to untrusted origins;
before using these manual endpoints (references: cfg.authorizeEndpoint,
cfg.tokenEndpoint, and the meta construction logic that falls back to
discoverOidc), validate that both URLs belong to a trusted origin: perform
origin whitelist checks and, if necessary, a safe HEAD/GET probe only to trusted
origins to verify reachability, or reject/require discovery if the origin is not
trusted; apply the same validation to other manual-endpoint branches (the other
manual paths around the same cfg fields at the later sections) so no network
requests are made to untrusted origins.
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/oauth/callback/index.html`:
- Around line 45-47: The code always sets msg.textContent to "Sign-in complete"
regardless of the posted payload; update the logic after
window.opener.postMessage(payload, window.location.origin) to check
payload.error (or payload.errorDescription) and set msg.textContent
conditionally: on success keep "Sign-in complete. You can close this window."
and on error set a clear failure message (e.g., "Sign-in failed: <short error>")
before calling the existing setTimeout/ window.close sequence; ensure you still
post the full payload via window.opener.postMessage and avoid changing the
postMessage target/origin logic.
🪄 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: 3aab27df-68e4-4b24-a2a7-729ab30f36a1
📒 Files selected for processing (1)
public/oauth/callback/index.html
Comment on lines
+45
to
+47
| window.opener.postMessage(payload, window.location.origin); | ||
| msg.textContent = "Sign-in complete. You can close this window."; | ||
| setTimeout(function () { try { window.close(); } catch (_) {} }, 250); |
Contributor
There was a problem hiding this comment.
Avoid showing a success message when OAuth returns an error.
At Line 46, the UI always says “Sign-in complete” after posting to the opener, even when payload.error is present. That can mislead users during failed auth flows.
Suggested fix
- window.opener.postMessage(payload, window.location.origin);
- msg.textContent = "Sign-in complete. You can close this window.";
+ window.opener.postMessage(payload, window.location.origin);
+ msg.textContent = payload.error
+ ? "Sign-in failed. You can close this window."
+ : "Sign-in complete. You can close this window.";
setTimeout(function () { try { window.close(); } catch (_) {} }, 250);📝 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
| window.opener.postMessage(payload, window.location.origin); | |
| msg.textContent = "Sign-in complete. You can close this window."; | |
| setTimeout(function () { try { window.close(); } catch (_) {} }, 250); | |
| window.opener.postMessage(payload, window.location.origin); | |
| msg.textContent = payload.error | |
| ? "Sign-in failed. You can close this window." | |
| : "Sign-in complete. You can close this window."; | |
| setTimeout(function () { try { window.close(); } catch (_) {} }, 250); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/oauth/callback/index.html` around lines 45 - 47, The code always sets
msg.textContent to "Sign-in complete" regardless of the posted payload; update
the logic after window.opener.postMessage(payload, window.location.origin) to
check payload.error (or payload.errorDescription) and set msg.textContent
conditionally: on success keep "Sign-in complete. You can close this window."
and on error set a clear failure message (e.g., "Sign-in failed: <short error>")
before calling the existing setTimeout/ window.close sequence; ensure you still
post the full payload via window.opener.postMessage and avoid changing the
postMessage target/origin logic.
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.
ValwareIRC
added a commit
to obbyworld/ObbyIRCd
that referenced
this pull request
May 10, 2026
Two related fixes for the OAuth-as-2FA experience.
1. Smart step-up planner
Old behavior: any time a primary SASL exchange completed and the
account had twofa_enabled=1 with at least one credential, the server
demanded TOTP step-up. That locked users out who had only an OAuth
credential bound (the only "second factor" they had IS the OAuth
identity that already verified primary).
New behavior: twofa_maybe_start_stepup(client, acc, primary_factor)
takes the credential type that just verified primary -- "password",
"oauth", or "external" -- and consults
twofa_count_credentials_excluding(acc->id, primary_factor) to decide.
* primary "oauth" + only oauth-typed creds -> skip step-up. The
bearer the IdP signed already proved more than a TOTP code would,
and the only step-up factor available IS the same OAuth identity;
demanding it twice locks the user out of their own account.
* primary "oauth" + has totp/webauthn -> demand step-up via that.
User explicitly enabled 2FA so they want belt-and-suspenders.
* primary "password" or "external" -> demand step-up via any
non-matching factor (totp, webauthn, oauth, ...).
Caller sites updated: SCRAM completion, PLAIN completion, SASL
EXTERNAL fingerprint match, and the IRCV3BEARER/OAUTHBEARER login
path all pass the right primary_factor now.
2. AUTHENTICATE 2FA-OAUTH mechanism
New SASL mech for the step-up phase, opened from the AUTHENTICATE
2FA-REQUIRED state. Wire format mirrors IRCV3BEARER:
C: AUTHENTICATE 2FA-OAUTH
S: AUTHENTICATE +
C: AUTHENTICATE <chunked b64 of [authzid]\0<type>\0<token>>
S: 903 / 904
The bearer is validated like the primary IRCV3BEARER path -- jwt
locally against JWKS, opaque async via userinfo-url -- but only
accepts the result if the resulting (provider, subject) credential
row is bound to the account that primary auth proved. So an
attacker who steals a github token can't use it as 2FA on a
victim's password-only account: the github identity must already
be linked to that specific account.
The async path is a third pending-op (OAUTH_OP_STEPUP) that
re-resolves the client via hash_find_id() when the userinfo
response lands, validates the credential binding, then calls
user_account_login() and 903s.
Companion client work in PR obbyworld/obby#196 prefers
password as primary when both PLAIN/SCRAM creds and an OAuth bearer
are configured, then auto-replies to 2FA-REQUIRED with 2FA-OAUTH +
the bearer -- so the user signs in once and the second factor is
proven silently.
Tested locally with the existing JWT mock (RSA jwks) for both the
primary IRCV3BEARER + 2FA-OAUTH paths; opaque path runs through
the same callback already e2e-tested with the userinfo HTTPS mock.
…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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/lib/sasl/scram.ts (1)
33-41: ⚡ Quick winModulo bias in
randomNoncereduces nonce entropy.
b % alpha.lengthis non-uniform because 256 is not a multiple of 62: byte values 248–255 map back to indices 0–7, giving those 8 alphabet positions ~25% higher selection probability than the other 54. The practical risk for a 18-char SCRAM nonce is small, but using rejection sampling is a one-line change and removes a textbook crypto code smell.♻️ Suggested rejection-sampling fix
function randomNonce(bytes = 18): string { - const buf = new Uint8Array(bytes); - crypto.getRandomValues(buf); const alpha = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"; + // Largest multiple of alpha.length that fits in a byte; reject anything above it. + const limit = 256 - (256 % alpha.length); // 248 for 62 chars let s = ""; - for (const b of buf) s += alpha[b % alpha.length]; + const buf = new Uint8Array(1); + while (s.length < bytes) { + crypto.getRandomValues(buf); + if (buf[0] < limit) s += alpha[buf[0] % alpha.length]; + } return s; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/sasl/scram.ts` around lines 33 - 41, The randomNonce function uses b % alpha.length which introduces modulo bias; replace it with rejection sampling inside randomNonce (same function name) by computing maxMultiple = Math.floor(256 / alpha.length) * alpha.length and only accepting bytes < maxMultiple, re-sampling rejected bytes (use crypto.getRandomValues to refill as needed) so each selected index from the alpha string is uniformly distributed; preserve the bytes parameter and return behavior but build the nonce by mapping only accepted bytes to alpha[b % alpha.length].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ui/TicTacToeModal.tsx`:
- Around line 86-109: The playable grid cells are currently focusable <td>
elements which are not accessible; wrap or replace the interactive cell content
with a real <button> for playable cells so assistive tech exposes it correctly:
in the rendering logic around the <td> that uses playable, move(open.serverId,
open.opponent, r, c), tabIndex, onClick/onKeyDown and the interactive classes
onto a <button> inside the <td> (keep non-playable cells as static <td>
content), remove the custom keyboard handler because native button handles
Enter/Space, add an explicit aria-label (e.g., include row/column or current
mark and coordinates) and for non-playable cells set aria-disabled or omit
button; preserve the win styling (win ? "bg-discord-green text-white" :
"text-white") by applying it to the button or the td as appropriate so visuals
remain unchanged.
In `@src/components/ui/TotpStepUpModal.tsx`:
- Around line 14-21: The useEffect that runs when pending changes schedules a
requestAnimationFrame to focus inputRef but doesn't cancel it; update the effect
in TotpStepUpModal to capture the raf id (from requestAnimationFrame) into a
local variable and return a cleanup function that calls
cancelAnimationFrame(rafId) to avoid the callback firing after unmount; keep the
existing logic that resets setCode("") and setError(null) and only run the RAF
when pending is true, and reference useEffect, requestAnimationFrame,
cancelAnimationFrame, inputRef, setCode, setError and pending when making the
change.
In `@src/components/ui/TwoFactorSettingsModal.tsx`:
- Around line 228-237: The disable flow calls twofaDisable(serverId, "totp",
...) unconditionally while the enable button only checks credentials.length,
which can allow enabling without any TOTP credential; update the logic to first
detect a TOTP credential from the credentials array (e.g., find an entry where
credential.type === "totp" or similar) and use that credential's identifier/type
when calling twofaDisable in onDisable; likewise, update the enable path (the
enable button handler referenced around lines 521-529) to require an actual TOTP
credential before allowing enablement and set a clear enrollError if none exists
so users cannot enable 2FA when only WebAuthn/OAuth creds are present.
- Around line 141-148: Before calling beginOauthLogin with provider.issuer,
validate that the issuer is a trusted origin: in TwoFactorSettingsModal (where
beginOauthLogin is invoked with provider.issuer/provider.clientId/etc.) check
provider.issuer against your allowlist or trustedDomains/same-origin policy and
only proceed if it matches; if it is not trusted, abort and show an error. If
you must probe the endpoint first, perform only a safe, non-authenticated
HEAD/GET to a trusted origin whitelist (not the raw provider.issuer) or
otherwise sanitize/normalize the issuer to a known-good hostname before calling
beginOauthLogin; do not make any network requests to arbitrary provider.issuer
values. Ensure validation logic is colocated with the beginOauthLogin call so
untrusted issuers are rejected before any discovery/token requests are
initiated.
In `@src/lib/sasl/scram.ts`:
- Around line 134-148: After parsing server attributes with
parseAttrs(serverFirst) you must reject any mandatory SCRAM extension 'm' per
RFC 5802 §5: check attrs.m (or the parsed attribute keyed 'm') immediately after
const attrs = parseAttrs(serverFirst) and throw a descriptive Error (e.g.
"SCRAM: unsupported mandatory extension 'm'") if it is present/non-empty; also
add the same check in the scramFinal path where parseAttrs is used so any
server-sent mandatory extension causes authentication to fail rather than
proceeding silently.
In `@src/store/handlers/tictactoe.ts`:
- Around line 53-57: Comparisons against IRC nick/target values are currently
case-sensitive (me, channelName, sender), causing valid TAGMSGs to be dropped;
normalize both sides using a consistent casing before filtering. After obtaining
me = ircClient.getNick(serverId) and mtags, compute normalized values (e.g.,
meLower = me.toLowerCase(), channelLower = channelName.toLowerCase(),
senderLower = sender?.toLowerCase()) and then replace the checks with
case-insensitive comparisons (channelLower !== meLower and senderLower ===
meLower) so invites/moves aren’t dropped due to mixed-case nicknames.
---
Nitpick comments:
In `@src/lib/sasl/scram.ts`:
- Around line 33-41: The randomNonce function uses b % alpha.length which
introduces modulo bias; replace it with rejection sampling inside randomNonce
(same function name) by computing maxMultiple = Math.floor(256 / alpha.length) *
alpha.length and only accepting bytes < maxMultiple, re-sampling rejected bytes
(use crypto.getRandomValues to refill as needed) so each selected index from the
alpha string is uniformly distributed; preserve the bytes parameter and return
behavior but build the nonce by mapping only accepted bytes to alpha[b %
alpha.length].
🪄 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: d1f1fc31-be34-4e8d-b677-81d1e4f66a3f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
package.jsonsrc/App.tsxsrc/components/layout/ChatHeader.tsxsrc/components/ui/TicTacToeModal.tsxsrc/components/ui/TotpStepUpModal.tsxsrc/components/ui/TwoFactorSettingsModal.tsxsrc/components/ui/UserSettings.tsxsrc/lib/games/tictactoe.tssrc/lib/games/tictactoeProtocol.tssrc/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/handlers/index.tssrc/store/handlers/tictactoe.tssrc/store/handlers/tictactoeActions.tssrc/store/index.tssrc/types/index.tstests/fixtures/uiState.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/types/index.ts
- src/lib/irc/IRCClient.ts
- src/store/index.ts
- src/store/handlers/auth.ts
Comment on lines
+86
to
+109
| <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" | ||
| }`} |
Contributor
There was a problem hiding this comment.
Use real buttons for playable cells.
These cells are interactive, but a focusable <td> without a role or accessible name is not exposed reliably to assistive tech. Screen-reader users will have a hard time understanding or operating the board.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/TicTacToeModal.tsx` around lines 86 - 109, The playable
grid cells are currently focusable <td> elements which are not accessible; wrap
or replace the interactive cell content with a real <button> for playable cells
so assistive tech exposes it correctly: in the rendering logic around the <td>
that uses playable, move(open.serverId, open.opponent, r, c), tabIndex,
onClick/onKeyDown and the interactive classes onto a <button> inside the <td>
(keep non-playable cells as static <td> content), remove the custom keyboard
handler because native button handles Enter/Space, add an explicit aria-label
(e.g., include row/column or current mark and coordinates) and for non-playable
cells set aria-disabled or omit button; preserve the win styling (win ?
"bg-discord-green text-white" : "text-white") by applying it to the button or
the td as appropriate so visuals remain unchanged.
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 for requestAnimationFrame to prevent potential errors.
The effect schedules a requestAnimationFrame callback without canceling it in the cleanup function. If the component unmounts before the callback fires, the scheduled frame will still execute. As per coding guidelines, nested requestAnimationFrame calls must be canceled in effect cleanup to avoid post-unmount errors.
🔧 Proposed fix
useEffect(() => {
if (pending) {
setCode("");
setError(null);
- // Autofocus once the modal mounts.
- requestAnimationFrame(() => inputRef.current?.focus());
+ const handle = requestAnimationFrame(() => inputRef.current?.focus());
+ return () => cancelAnimationFrame(handle);
}
}, [pending]);📝 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); | |
| const handle = requestAnimationFrame(() => inputRef.current?.focus()); | |
| return () => cancelAnimationFrame(handle); | |
| } | |
| }, [pending]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/TotpStepUpModal.tsx` around lines 14 - 21, The useEffect
that runs when pending changes schedules a requestAnimationFrame to focus
inputRef but doesn't cancel it; update the effect in TotpStepUpModal to capture
the raf id (from requestAnimationFrame) into a local variable and return a
cleanup function that calls cancelAnimationFrame(rafId) to avoid the callback
firing after unmount; keep the existing logic that resets setCode("") and
setError(null) and only run the RAF when pending is true, and reference
useEffect, requestAnimationFrame, cancelAnimationFrame, inputRef, setCode,
setError and pending when making the change.
Comment on lines
+141
to
+148
| const result = await beginOauthLogin({ | ||
| issuer: provider.issuer, | ||
| clientId: provider.clientId, | ||
| scopes: provider.scopes || undefined, | ||
| redirectUri: provider.redirectUri || undefined, | ||
| authorizeEndpoint: provider.authorizeEndpoint || undefined, | ||
| tokenEndpoint: provider.tokenEndpoint || undefined, | ||
| }); |
Contributor
There was a problem hiding this comment.
Check issuer trust before starting OAuth discovery.
This starts the OAuth/OIDC flow for whatever issuer is configured on the server entry, including custom providers. Since the flow does client-side discovery/token calls, a malicious or mistyped issuer can force requests to an arbitrary origin and leak the user's IP before they've approved that host.
As per coding guidelines, "Only make HTTP requests (HEAD or GET) for URLs from trusted origins; check trust levels before any network request to prevent IP leakage".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/TwoFactorSettingsModal.tsx` around lines 141 - 148, Before
calling beginOauthLogin with provider.issuer, validate that the issuer is a
trusted origin: in TwoFactorSettingsModal (where beginOauthLogin is invoked with
provider.issuer/provider.clientId/etc.) check provider.issuer against your
allowlist or trustedDomains/same-origin policy and only proceed if it matches;
if it is not trusted, abort and show an error. If you must probe the endpoint
first, perform only a safe, non-authenticated HEAD/GET to a trusted origin
whitelist (not the raw provider.issuer) or otherwise sanitize/normalize the
issuer to a known-good hostname before calling beginOauthLogin; do not make any
network requests to arbitrary provider.issuer values. Ensure validation logic is
colocated with the beginOauthLogin call so untrusted issuers are rejected before
any discovery/token requests are initiated.
Comment on lines
+228
to
+237
| const onDisable = () => { | ||
| setEnrollError(null); | ||
| if (!/^\d{6}$/.test(disableCode.trim())) { | ||
| setEnrollError( | ||
| "Enter a 6-digit code from any of your authenticator apps to disable 2FA.", | ||
| ); | ||
| return; | ||
| } | ||
| twofaDisable(serverId, "totp", disableCode.trim()); | ||
| setDisableCode(""); |
Contributor
There was a problem hiding this comment.
Don't enable 2FA unless the account has a TOTP credential.
The disable path is hardcoded to twofaDisable(serverId, "totp", ...), but the enable button only checks credentials.length > 0. That lets users enable 2FA with only WebAuthn/OAuth credentials and then gives them no way to disable it from this UI.
Also applies to: 521-529
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/TwoFactorSettingsModal.tsx` around lines 228 - 237, The
disable flow calls twofaDisable(serverId, "totp", ...) unconditionally while the
enable button only checks credentials.length, which can allow enabling without
any TOTP credential; update the logic to first detect a TOTP credential from the
credentials array (e.g., find an entry where credential.type === "totp" or
similar) and use that credential's identifier/type when calling twofaDisable in
onDisable; likewise, update the enable path (the enable button handler
referenced around lines 521-529) to require an actual TOTP credential before
allowing enablement and set a clear enrollError if none exists so users cannot
enable 2FA when only WebAuthn/OAuth creds are present.
Comment on lines
+134
to
+148
| const attrs = parseAttrs(serverFirst); | ||
| const r = attrs.r; | ||
| const s = attrs.s; | ||
| const i = attrs.i; | ||
| if (!r || !s || !i) throw new Error("SCRAM: malformed server-first"); | ||
| if (!r.startsWith(state.clientNonce)) | ||
| throw new Error("SCRAM: server nonce does not extend client nonce"); | ||
|
|
||
| const salt = b64ToBytes(s); | ||
| const iterations = Number.parseInt(i, 10); | ||
| if (!Number.isFinite(iterations) || iterations < 1) | ||
| throw new Error("SCRAM: bad iteration count"); | ||
|
|
||
| state.serverFirst = serverFirst; | ||
| state.combinedNonce = r; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
RFC 5802 SCRAM client mandatory extension m= attribute behavior
💡 Result:
RFC 5802 “SCRAM” uses the “m=” attribute to signal mandatory (future) extensions. If either peer (client or server) receives an “m=” attribute that it does not understand/implement, authentication must fail. More precisely: 1) What “m=” means - RFC 5802 reserves the “m=” attribute for signaling mandatory extensions and states that “Mandatory extensions are encoded as values of the 'm' attribute.” [1][2] 2) Required behavior when “m=” is present but unsupported - RFC 5802 says: “Mandatory extensions sent by one peer but not understood by the other MUST cause authentication failure.” [1][2] - It further notes that the server “SHOULD send the 'extensions-not-supported' server-error-value.” [1][2] - In addition, RFC 5802 explicitly says that in this version, “its presence in a client or a server message MUST cause authentication failure when the attribute is parsed by the other end.” [1][2] 3) Contrast with optional extensions - RFC 5802 distinguishes optional extensions: “Unknown optional extensions MUST be ignored upon receipt.” [1][2] Answering the exact question (“client mandatory extension m= attribute behavior”): - If the SCRAM client receives an “m=” attribute (mandatory extension) that it does not support, it must fail authentication when parsing it. [1][2] - If the server receives an “m=” attribute from a client and it does not support it, authentication must fail (with the server typically sending “extensions-not-supported”). [1][2]
Citations:
🏁 Script executed:
# Find the scram.ts file and check its content
fd -t f "scram.ts"Repository: ObsidianIRC/ObsidianIRC
Length of output: 89
🏁 Script executed:
# Once we find the file, let's examine the parseAttrs function and the code around lines 134-148
cat -n src/lib/sasl/scram.ts | head -200Repository: ObsidianIRC/ObsidianIRC
Length of output: 6850
🏁 Script executed:
# Also search for any existing m= handling in the file
rg -n "\.m\s*==|attrs\.m|m=" src/lib/sasl/scram.tsRepository: ObsidianIRC/ObsidianIRC
Length of output: 49
Add check for SCRAM mandatory extensions (m= attribute) per RFC 5802 §5.
RFC 5802 requires authentication to fail if the server sends a mandatory extension (m= attribute) that the client does not support. Currently, parseAttrs extracts all attributes without validation, and scramFinal proceeds without checking for m=, creating a compliance violation and risking silent incompatibility with future SCRAM extensions or malicious servers.
Add the check immediately after parsing attributes:
Suggested fix
const attrs = parseAttrs(serverFirst);
+ if (attrs.m !== undefined)
+ throw new Error("SCRAM: unsupported mandatory extension");
const r = attrs.r;
const s = attrs.s;
const i = attrs.i;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/sasl/scram.ts` around lines 134 - 148, After parsing server
attributes with parseAttrs(serverFirst) you must reject any mandatory SCRAM
extension 'm' per RFC 5802 §5: check attrs.m (or the parsed attribute keyed 'm')
immediately after const attrs = parseAttrs(serverFirst) and throw a descriptive
Error (e.g. "SCRAM: unsupported mandatory extension 'm'") if it is
present/non-empty; also add the same check in the scramFinal path where
parseAttrs is used so any server-sent mandatory extension causes authentication
to fail rather than proceeding silently.
Comment on lines
+53
to
+57
| const me = ircClient.getNick(serverId); | ||
| if (!me || !mtags) return; | ||
| // Only direct messages from another nick to us. | ||
| if (channelName !== me) return; | ||
| if (!sender || sender === me) return; |
Contributor
There was a problem hiding this comment.
Normalize nick/target comparisons before filtering TAGMSGs.
channelName !== me and sender === me are case-sensitive, but IRC nicknames are not. A mixed-case target like Nick vs nick will make the handler drop valid invites and moves.
Suggested fix
ircClient.on("TAGMSG", ({ serverId, mtags, sender, channelName }) => {
const me = ircClient.getNick(serverId);
if (!me || !mtags) return;
+ const meKey = key(me);
// Only direct messages from another nick to us.
- if (channelName !== me) return;
- if (!sender || sender === me) return;
+ if (key(channelName) !== meKey) return;
+ if (!sender || key(sender) === meKey) return;📝 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 me = ircClient.getNick(serverId); | |
| if (!me || !mtags) return; | |
| // Only direct messages from another nick to us. | |
| if (channelName !== me) return; | |
| if (!sender || sender === me) return; | |
| const me = ircClient.getNick(serverId); | |
| if (!me || !mtags) return; | |
| const meKey = key(me); | |
| // Only direct messages from another nick to us. | |
| if (key(channelName) !== meKey) return; | |
| if (!sender || key(sender) === meKey) return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/handlers/tictactoe.ts` around lines 53 - 57, Comparisons against
IRC nick/target values are currently case-sensitive (me, channelName, sender),
causing valid TAGMSGs to be dropped; normalize both sides using a consistent
casing before filtering. After obtaining me = ircClient.getNick(serverId) and
mtags, compute normalized values (e.g., meLower = me.toLowerCase(), channelLower
= channelName.toLowerCase(), senderLower = sender?.toLowerCase()) and then
replace the checks with case-insensitive comparisons (channelLower !== meLower
and senderLower === meLower) so invites/moves aren’t dropped due to mixed-case
nicknames.
# Conflicts: # src/components/ui/EditServerModal.tsx # src/components/ui/UserSettings.tsx # src/lib/irc/IRCClient.ts # src/lib/irc/handlers/auth.ts # src/store/handlers/auth.ts # src/store/handlers/index.ts # src/types/index.ts
ValwareIRC
added a commit
to obbyworld/ObbyIRCd
that referenced
this pull request
May 18, 2026
Lets the IRC server accept OAuth2/OIDC bearer tokens (JWTs) as a SASL mechanism, so users can log in via Logto, Auth0, Keycloak, Okta, or any IdP that publishes a JWKS and signs RS256/384/512 access or ID tokens. The companion ObsidianIRC client lives in PR obbyworld/obby#196. config account-registration { oauth-provider "logto" { issuer 'https://my-tenant.logto.app/oidc'; audience 'https://api.example.com'; jwks-file "/home/valware/obby/conf/logto-jwks.json"; subject-claim "sub"; # or "preferred_username" }; }; JWKS is loaded from the file at startup / on /REHASH; admin keeps it fresh out-of-band (curl in a cron is fine for now). Multiple oauth-provider blocks may coexist -- the issuer claim picks which one validates a given token. token validation (oauth_validate_jwt) * splits header.payload.sig, base64url-decodes each * looks up provider by `iss` * checks `aud` (string or array), `exp`, `nbf` with 60s leeway * picks the JWKS key by `kid` (or first key when token has no kid) * EVP_DigestVerify against the RSA public key for RS256/384/512 * returns the configured subject claim ("sub" or "preferred_username") linking + login OAuth identities are stored as rows in the existing account_2fa_credentials table with type='oauth' and secret='<provider>\x1F<subject>'. Reuses the existing 2FA UX surface: /2FA CHALLENGE oauth <provider> -> opens token-input session /2FA TOKEN :<chunk> -> repeated, since JWTs > 512B /2FA ADD oauth <name> -> validates buffered token, stores credential Login then uses either SASL mechanism: AUTHENTICATE IRCV3BEARER # [authzid]\0<type>\0<token> AUTHENTICATE OAUTHBEARER # RFC 7628 GS2 framing Both run through oauth_login_by_token, which validates the JWT, looks up the (provider,subject) credential, and either logs the user in or hands off to twofa_maybe_start_stepup() if 2FA is enrolled. SASL chunked AUTHENTICATE traffic is reassembled via a per-client oauth_sasl_md ModData buffer. other * accreg_configrun() drops the old provider list before re-parsing, so /REHASH doesn't accumulate duplicates. * old account_oauth_links table from an earlier prototype is DROP TABLE IF EXISTS'd on every open_database(). * advertised in saslmechs() and CAP LS sasl=... * end-to-end tested with synthetic RSA keypair + JWKS + JWT against both IRCV3BEARER and OAUTHBEARER mechanisms. Known gaps (not in this commit): * Opaque tokens (GitHub-style) -- needs userinfo HTTP call. * EC keys (ES256/384/512) -- only RSA implemented. * Auto-fetch JWKS from issuer URL -- file-based for now.
ValwareIRC
added a commit
to obbyworld/ObbyIRCd
that referenced
this pull request
May 18, 2026
Two related fixes for the OAuth-as-2FA experience.
1. Smart step-up planner
Old behavior: any time a primary SASL exchange completed and the
account had twofa_enabled=1 with at least one credential, the server
demanded TOTP step-up. That locked users out who had only an OAuth
credential bound (the only "second factor" they had IS the OAuth
identity that already verified primary).
New behavior: twofa_maybe_start_stepup(client, acc, primary_factor)
takes the credential type that just verified primary -- "password",
"oauth", or "external" -- and consults
twofa_count_credentials_excluding(acc->id, primary_factor) to decide.
* primary "oauth" + only oauth-typed creds -> skip step-up. The
bearer the IdP signed already proved more than a TOTP code would,
and the only step-up factor available IS the same OAuth identity;
demanding it twice locks the user out of their own account.
* primary "oauth" + has totp/webauthn -> demand step-up via that.
User explicitly enabled 2FA so they want belt-and-suspenders.
* primary "password" or "external" -> demand step-up via any
non-matching factor (totp, webauthn, oauth, ...).
Caller sites updated: SCRAM completion, PLAIN completion, SASL
EXTERNAL fingerprint match, and the IRCV3BEARER/OAUTHBEARER login
path all pass the right primary_factor now.
2. AUTHENTICATE 2FA-OAUTH mechanism
New SASL mech for the step-up phase, opened from the AUTHENTICATE
2FA-REQUIRED state. Wire format mirrors IRCV3BEARER:
C: AUTHENTICATE 2FA-OAUTH
S: AUTHENTICATE +
C: AUTHENTICATE <chunked b64 of [authzid]\0<type>\0<token>>
S: 903 / 904
The bearer is validated like the primary IRCV3BEARER path -- jwt
locally against JWKS, opaque async via userinfo-url -- but only
accepts the result if the resulting (provider, subject) credential
row is bound to the account that primary auth proved. So an
attacker who steals a github token can't use it as 2FA on a
victim's password-only account: the github identity must already
be linked to that specific account.
The async path is a third pending-op (OAUTH_OP_STEPUP) that
re-resolves the client via hash_find_id() when the userinfo
response lands, validates the credential binding, then calls
user_account_login() and 903s.
Companion client work in PR obbyworld/obby#196 prefers
password as primary when both PLAIN/SCRAM creds and an OAuth bearer
are configured, then auto-replies to 2FA-REQUIRED with 2FA-OAUTH +
the bearer -- so the user signs in once and the second factor is
proven silently.
Tested locally with the existing JWT mock (RSA jwks) for both the
primary IRCV3BEARER + 2FA-OAUTH paths; opaque path runs through
the same callback already e2e-tested with the userinfo HTTPS mock.
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
1 participant
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.
Summary
Lets ObsidianIRC connect to an IRC server via OAuth2 instead of (or in addition to) SASL PLAIN. Each server entry now carries its own provider config, so admins can point one server at Logto, another at Auth0/Keycloak/Okta/any OIDC issuer that publishes a
.well-known/openid-configurationand issues JWT tokens.The companion server-side support lives in obbyircd (
oauth-provider {}config + SASLIRCV3BEARER/OAUTHBEARERvalidators).How it works
/oauth/callback(a new same-origin SPA route) whichpostMessages the auth code back to the opener and closes itself.code + verifierat the token endpoint and stores the resulting access/id/refresh tokens on the server config.IRCV3BEARER, sends[authzid]\0jwt\0<token>in 400-charAUTHENTICATEchunks per the IRC SASL spec, and the server validates the JWT against its configured JWKS.Files
src/lib/oauth.ts– OIDC discovery, PKCE generation, authorize-URL builder, popup runner, token exchange, provider presets.src/lib/saslFrames.ts–buildIrcv3BearerPayload/buildOauthBearerPayload/chunkSaslPayload.src/components/OAuthCallback.tsx–/oauth/callbackpage thatpostMessages the auth code to the opener.src/components/ui/OAuthSection.tsx– Edit-Server panel with provider preset, issuer/clientId/scopes fields, Sign-in/Sign-out buttons.src/store/handlers/auth.ts– sendsAUTHENTICATE IRCV3BEARERand the chunked payload when OAuth is active.src/lib/irc/IRCClient.ts– newoauthBearerEnabledconnect arg +setSaslEnabledhelper so CAP negotiation waits for SASL completion.src/types/index.ts–ServerOAuthConfiginterface.Notes & limitations
localStoragewith the rest of the server config; an expired token simply yields a 904 from the server and the user re-authenticates.IRCV3BEARER(JWT).OAUTHBEARER(RFC 7628) frame builder is exported for future use but unused in the connect flow — it requires a server that can introspect opaque tokens (e.g. GitHub).Test plan
npm run formatcleannpm run fix:unsafecleannpm run test— 722 passing (15 new acrosstests/lib/oauth.test.tsandtests/lib/saslFrames.test.ts, including the RFC 7636 PKCE vector)npm run build— clean<origin>/oauth/callback, paste the tenant URL into Edit Server → Issuer URL, sign in via popup, confirmAUTHENTICATE IRCV3BEARERtraffic +903 SASL successfulagainst an obbyircd that has the matchingoauth-provider {}block904then re-auth flowSummary by CodeRabbit
New Features
Documentation
Tests