fix: capture CAP LS values so WebAuthn 2FA button enables on supported servers#204
Merged
Conversation
…d servers The "Add biometric / security key" button was always grayed out, even on obbyircd which advertises draft/account-2fa=totp,webauthn,oauth. Root cause: the button gated on grepping "webauthn" inside the server.capabilities[] entries, but that array is populated from CAP ACK -- which only echoes cap NAMES. The =value suffix lives in CAP LS, which the handler ignored for everything except unrealircd.org/link-security. - Add Server.capabilityValues: Record<string, string> to carry the CAP LS values. - Parse every name=value pair out of CAP LS in the existing handler and merge it into capabilityValues. - Modal now reads server.capabilityValues["draft/account-2fa"], splits on commas, and checks for "webauthn". If the cap is acked but no value was advertised (some servers omit the factor list), assume the standard set rather than silently disabling.
Contributor
📝 WalkthroughWalkthroughThis PR adds infrastructure for parsing and consuming server capability sub-features. A new ChangesCapability Values for Feature Detection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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 `@src/store/handlers/auth.ts`:
- Around line 499-522: The loop that parses cliCaps currently ignores tokens
without "=", so previously cached capability values are never cleared; update
the parsing in auth.ts (the for loop over cliCaps and the advertised map) to
treat tokens like "draft/account-2fa" as an explicit removal by adding an entry
for that name (e.g., advertised[name] = null) when eq <= 0, and keep the
subsequent store.setState merge logic so the null value overwrites the old value
in server.capabilityValues (ensuring stale values are cleared); reference tokens
parsing, advertised, and the store.setState/server.capabilityValues merge to
locate where to apply this change.
🪄 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: 6ae751b1-c4c9-4e36-8c3a-9695d71e6412
📒 Files selected for processing (3)
src/components/ui/TwoFactorSettingsModal.tsxsrc/store/handlers/auth.tssrc/types/index.ts
Comment on lines
+499
to
+522
| for (const tok of cliCaps.split(/\s+/)) { | ||
| if (!tok) continue; | ||
| const eq = tok.indexOf("="); | ||
| if (eq <= 0) continue; | ||
| const name = tok.slice(0, eq); | ||
| const value = tok.slice(eq + 1); | ||
| if (name === "unrealircd.org/link-security") continue; | ||
| advertised[name] = value; | ||
| } | ||
| if (Object.keys(advertised).length) { | ||
| store.setState((state) => ({ | ||
| servers: state.servers.map((server) => | ||
| server.id === serverId | ||
| ? { | ||
| ...server, | ||
| capabilityValues: { | ||
| ...(server.capabilityValues ?? {}), | ||
| ...advertised, | ||
| }, | ||
| } | ||
| : server, | ||
| ), | ||
| })); | ||
| } |
Contributor
There was a problem hiding this comment.
Clear previously cached cap values when the cap is now value-less.
Line 508 only updates state when name=value tokens exist. If draft/account-2fa was previously cached with a value and later arrives as just draft/account-2fa, stale data can persist and incorrectly affect WebAuthn enablement.
Suggested fix
- const advertised: Record<string, string> = {};
+ const advertised: Record<string, string> = {};
+ const valueLessCaps = new Set<string>();
for (const tok of cliCaps.split(/\s+/)) {
if (!tok) continue;
const eq = tok.indexOf("=");
- if (eq <= 0) continue;
+ if (eq <= 0) {
+ if (tok !== "unrealircd.org/link-security") valueLessCaps.add(tok);
+ continue;
+ }
const name = tok.slice(0, eq);
const value = tok.slice(eq + 1);
if (name === "unrealircd.org/link-security") continue;
advertised[name] = value;
}
- if (Object.keys(advertised).length) {
+ if (Object.keys(advertised).length || valueLessCaps.size > 0) {
store.setState((state) => ({
servers: state.servers.map((server) =>
server.id === serverId
? {
...server,
- capabilityValues: {
- ...(server.capabilityValues ?? {}),
- ...advertised,
- },
+ capabilityValues: (() => {
+ const next = { ...(server.capabilityValues ?? {}) };
+ for (const cap of valueLessCaps) delete next[cap];
+ return { ...next, ...advertised };
+ })(),
}
: server,
),
}));
}📝 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
| for (const tok of cliCaps.split(/\s+/)) { | |
| if (!tok) continue; | |
| const eq = tok.indexOf("="); | |
| if (eq <= 0) continue; | |
| const name = tok.slice(0, eq); | |
| const value = tok.slice(eq + 1); | |
| if (name === "unrealircd.org/link-security") continue; | |
| advertised[name] = value; | |
| } | |
| if (Object.keys(advertised).length) { | |
| store.setState((state) => ({ | |
| servers: state.servers.map((server) => | |
| server.id === serverId | |
| ? { | |
| ...server, | |
| capabilityValues: { | |
| ...(server.capabilityValues ?? {}), | |
| ...advertised, | |
| }, | |
| } | |
| : server, | |
| ), | |
| })); | |
| } | |
| const advertised: Record<string, string> = {}; | |
| const valueLessCaps = new Set<string>(); | |
| for (const tok of cliCaps.split(/\s+/)) { | |
| if (!tok) continue; | |
| const eq = tok.indexOf("="); | |
| if (eq <= 0) { | |
| if (tok !== "unrealircd.org/link-security") valueLessCaps.add(tok); | |
| continue; | |
| } | |
| const name = tok.slice(0, eq); | |
| const value = tok.slice(eq + 1); | |
| if (name === "unrealircd.org/link-security") continue; | |
| advertised[name] = value; | |
| } | |
| if (Object.keys(advertised).length || valueLessCaps.size > 0) { | |
| store.setState((state) => ({ | |
| servers: state.servers.map((server) => | |
| server.id === serverId | |
| ? { | |
| ...server, | |
| capabilityValues: (() => { | |
| const next = { ...(server.capabilityValues ?? {}) }; | |
| for (const cap of valueLessCaps) delete next[cap]; | |
| return { ...next, ...advertised }; | |
| })(), | |
| } | |
| : server, | |
| ), | |
| })); | |
| } |
🤖 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 499 - 522, The loop that parses
cliCaps currently ignores tokens without "=", so previously cached capability
values are never cleared; update the parsing in auth.ts (the for loop over
cliCaps and the advertised map) to treat tokens like "draft/account-2fa" as an
explicit removal by adding an entry for that name (e.g., advertised[name] =
null) when eq <= 0, and keep the subsequent store.setState merge logic so the
null value overwrites the old value in server.capabilityValues (ensuring stale
values are cleared); reference tokens parsing, advertised, and the
store.setState/server.capabilityValues merge to locate where to apply this
change.
matheusfillipe
approved these changes
May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The "Add biometric / security key" button in TwoFactorSettingsModal was always grayed out, even on obbyircd which advertises `draft/account-2fa=totp,webauthn,oauth`. Tooltip read "Server does not advertise WebAuthn support".
Root cause: the button gated on grepping `"webauthn"` inside the `server.capabilities[]` entries, but that array is populated from CAP ACK (store/handlers/auth.ts:557) — which only echoes cap names. The `=value` suffix lives only in CAP LS (`:server CAP * LS :sasl draft/account-2fa=totp,webauthn,oauth ...`), and the LS handler ignored every `=value` except `unrealircd.org/link-security`.
Fix
Test plan
Summary by CodeRabbit