Open
Conversation
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:
📝 WalkthroughWalkthroughAdds local MFA and session config to the embedded IdP, introduces signer-based token signing, extends Dex YAML schema with sessions and MFA providers (TOTP/WebAuthn), injects post-logout URIs and MFA chains into static clients, adds UI templates, and updates module dependencies. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant Dex as "Embedded IdP (Dex)"
participant Storage
participant Signer
User->>Browser: Start login
Browser->>Dex: Submit primary credentials
Dex->>Storage: Verify credentials / fetch user record
Storage-->>Dex: User record (may require MFA)
Dex->>Browser: Serve MFA challenge (TOTP or WebAuthn)
alt TOTP flow
Browser->>Dex: POST TOTP code
Dex->>Storage: Validate TOTP secret
Storage-->>Dex: Validation result
else WebAuthn flow
Browser->>Dex: Request challenge
Dex->>Signer: Prepare/sign challenge
Signer-->>Dex: Challenge payload
Dex->>Browser: Return challenge
Browser->>Dex: POST assertion
Dex->>Storage: Validate assertion/credential
Storage-->>Dex: Validation result
end
Dex->>Signer: Sign ID/access tokens
Signer-->>Dex: Signed tokens
Dex->>Browser: Redirect to application with tokens
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 6
🧹 Nitpick comments (3)
idp/dex/web/templates/totp_verify.html (1)
23-35: Consider using a stricter pattern for TOTP input validation.The current
pattern="[0-9]*"allows empty strings and any number of digits. Since TOTP codes are exactly 6 digits, a stricter pattern would provide better client-side validation feedback.🔧 Suggested pattern improvement
<input type="text" id="totp" name="totp" class="nb-input" inputmode="numeric" - pattern="[0-9]*" + pattern="[0-9]{6}" maxlength="6" autocomplete="one-time-code" placeholder="000000" autofocus required >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idp/dex/web/templates/totp_verify.html` around lines 23 - 35, The TOTP input (id="totp", name="totp", class="nb-input") uses pattern="[0-9]*" which allows empty or variable-length digits; update the pattern to enforce exactly 6 digits (e.g. use a regex like ^[0-9]{6}$) while keeping inputmode="numeric", maxlength="6", and required so browsers provide proper client-side validation for a 6-digit TOTP code.idp/dex/provider.go (1)
105-112: Note: Different default key rotation periods between NewProvider and NewProviderFromYAML.
NewProviderusesKeysRotationPeriod: "6h"whilegetSigner(used byNewProviderFromYAML) defaults to"720h"(30 days). This may be intentional for different use cases (standalone vs embedded mode), but it's worth documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idp/dex/provider.go` around lines 105 - 112, NewProvider sets signer.LocalConfig.KeysRotationPeriod to "6h" while getSigner (used by NewProviderFromYAML) defaults to "720h", causing inconsistent key rotation behavior; update either NewProvider or getSigner to use the same default value or add a clear comment/docstring near NewProvider, NewProviderFromYAML and getSigner explaining the intentional difference and rationale. Locate the LocalConfig usage in NewProvider (localSignerConfig) and the getSigner function, then either unify the KeysRotationPeriod string in both places or add explicit documentation in both NewProvider and getSigner describing why one uses "6h" and the other "720h" so maintainers understand the divergence.management/server/idp/embedded.go (1)
222-228: Clarify variable naming for remember-me default.The variable
rememberMeEnabled := falseis then assigned toRememberMeCheckedByDefault, which can be confusing. Consider naming it to reflect its actual purpose.♻️ Clearer variable naming
- rememberMeEnabled := false + rememberMeCheckedByDefault := false cfg.Sessions = &dex.Sessions{ CookieName: "netbird-session", AbsoluteLifetime: "24h", ValidIfNotUsedFor: "1h", - RememberMeCheckedByDefault: &rememberMeEnabled, + RememberMeCheckedByDefault: &rememberMeCheckedByDefault, SSOSharedWithDefault: "", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/idp/embedded.go` around lines 222 - 228, The local variable rememberMeEnabled used to set cfg.Sessions.RememberMeCheckedByDefault is misleading; rename it to reflect its purpose (e.g., rememberMeCheckedByDefault or rememberMeDefaultChecked) and update its declaration and the assignment to cfg.Sessions.RememberMeCheckedByDefault in embedded.go so the name clearly indicates it's the default checked state for the "remember me" checkbox when constructing cfg.Sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@idp/dex/config.go`:
- Around line 57-61: The MFA field in the struct is missing a yaml tag which
causes inconsistency with Sessions; update the MFA field declaration (MFA
MFAConfig `json:"mfa"`) to include a matching yaml tag (e.g., `yaml:"mfa"`) so
it reads similarly to Sessions and will be populated when loading YAML configs.
- Around line 561-577: In buildSessionsConfig, avoid dereferencing
sessions.RememberMeCheckedByDefault without a nil check and stop silently
discarding parseDuration errors: first verify
sessions.RememberMeCheckedByDefault != nil before using it (e.g., use a safe
default when nil), and capture the errors returned by
parseDuration(sessions.AbsoluteLifetime) and
parseDuration(sessions.ValidIfNotUsedFor) instead of ignoring them—handle them
by returning an error up the call chain or logging and applying sensible default
durations; update buildSessionsConfig and any callers to propagate/handle the
error if you choose to return one.
In `@idp/dex/web/templates/webauthn_verify.html`:
- Around line 33-37: The apiParams() helper and related fetch calls currently
append req/hmac/authenticator to the URL query string; change them to send those
values in the POST request body instead (e.g., build a URLSearchParams or JSON
payload from reqID, hmacVal, authenticator and pass it as fetch body with
appropriate Content-Type) and call the endpoints without those query params;
update all similar places (register/finish, login/begin, login/finish and the
occurrences around lines referenced) and ensure server handlers expect the
values in the POST body rather than the query string.
- Around line 33-37: The apiParams function (and the surrounding initialization
that reads reqID, hmacVal, authenticator via URLSearchParams.get) must fail fast
when any of reqID, hmacVal, or authenticator are null/undefined: add a preflight
check after extracting those values (before any network calls or calling
apiParams) that tests for null/empty and, if present, stops further processing,
displays a deterministic user-facing error message (e.g., "Missing required URL
parameters") and prevents calling apiParams or initiating requests; update any
code paths that assume apiParams returns a valid string to handle the early
exit. Ensure you reference the variables reqID, hmacVal, authenticator and the
apiParams function to locate where to add the check.
- Line 25: The injected template value for .Mode is inserted as a bare
identifier causing a runtime ReferenceError; fix the assignment to the mode
variable by injecting .Mode as a quoted string (i.e., change the const mode = {{
.Mode }} assignment so the template emits a JavaScript string literal for .Mode)
and ensure the template escaping/quoting used by the HTML template engine is
applied so values like "register" or "login" are safely emitted; update the line
that defines mode to use the quoted .Mode injection.
In `@management/server/idp/embedded.go`:
- Around line 231-232: The comment points out a typo and a problematic global
side effect: fix the spelling in the comment ("otherwsise" → "otherwise") and
remove the call to os.Setenv("DEX_SESSIONS_ENABLED", "true") from the
ToYAMLConfig/config conversion helper; instead document the requirement in the
helper's comment (referencing DEX_SESSIONS_ENABLED) and ensure the environment
variable is set explicitly at application startup (e.g., main or server
initialization) so ToYAMLConfig remains pure and safe for repeated calls or
tests.
---
Nitpick comments:
In `@idp/dex/provider.go`:
- Around line 105-112: NewProvider sets signer.LocalConfig.KeysRotationPeriod to
"6h" while getSigner (used by NewProviderFromYAML) defaults to "720h", causing
inconsistent key rotation behavior; update either NewProvider or getSigner to
use the same default value or add a clear comment/docstring near NewProvider,
NewProviderFromYAML and getSigner explaining the intentional difference and
rationale. Locate the LocalConfig usage in NewProvider (localSignerConfig) and
the getSigner function, then either unify the KeysRotationPeriod string in both
places or add explicit documentation in both NewProvider and getSigner
describing why one uses "6h" and the other "720h" so maintainers understand the
divergence.
In `@idp/dex/web/templates/totp_verify.html`:
- Around line 23-35: The TOTP input (id="totp", name="totp", class="nb-input")
uses pattern="[0-9]*" which allows empty or variable-length digits; update the
pattern to enforce exactly 6 digits (e.g. use a regex like ^[0-9]{6}$) while
keeping inputmode="numeric", maxlength="6", and required so browsers provide
proper client-side validation for a 6-digit TOTP code.
In `@management/server/idp/embedded.go`:
- Around line 222-228: The local variable rememberMeEnabled used to set
cfg.Sessions.RememberMeCheckedByDefault is misleading; rename it to reflect its
purpose (e.g., rememberMeCheckedByDefault or rememberMeDefaultChecked) and
update its declaration and the assignment to
cfg.Sessions.RememberMeCheckedByDefault in embedded.go so the name clearly
indicates it's the default checked state for the "remember me" checkbox when
constructing cfg.Sessions.
🪄 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: caaf5b60-4cb8-4b1f-8c1c-32592e9d1939
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
combined/cmd/config.gogo.modidp/dex/config.goidp/dex/provider.goidp/dex/web/templates/home.htmlidp/dex/web/templates/logout.htmlidp/dex/web/templates/totp_verify.htmlidp/dex/web/templates/webauthn_verify.htmlmanagement/server/idp/embedded.gonetbird-server
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@idp/dex/config.go`:
- Around line 516-528: The WebAuthnConfig's UserVerification and
AuthenticatorAttachment fields are defined but unused; either remove them from
the WebAuthnConfig struct and its docs (and any references) or, if you plan to
support upstream changes, wire them into the provider by updating
buildWebAuthnConfig to pass those values into server.NewWebAuthnProvider and
update the NewWebAuthnProvider signature to accept UserVerification and
AuthenticatorAttachment parameters; locate the struct WebAuthnConfig, the fields
UserVerification and AuthenticatorAttachment, the function buildWebAuthnConfig,
and the call to server.NewWebAuthnProvider to make the corresponding removal or
signature/argument changes and update any callers or tests accordingly.
In `@management/server/idp/embedded.go`:
- Around line 204-238: The TOTP issuer string in configureMFA
(totpConfig.Issuer) uses "Netbird" but other places use "NetBird"; update the
Issuer field in the totpConfig inside configureMFA to "NetBird" to match
Frontend.Issuer and any other usages so authenticator apps display a consistent
name (also scan the file for other "Netbird" occurrences and normalize their
casing to "NetBird" if present).
🪄 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: 8969eafb-5cb4-4030-b37a-fb4bf43c28f8
📒 Files selected for processing (3)
idp/dex/config.goidp/dex/web/templates/webauthn_verify.htmlmanagement/server/idp/embedded.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
management/server/idp/embedded.go (2)
205-207:⚠️ Potential issue | 🟡 MinorKeep the TOTP issuer name consistent with the rest of the UI.
Frontend.Issueris"NetBird", but Line 206 still uses"Netbird". That inconsistency is visible in authenticator apps during TOTP enrollment.🔧 Proposed fix
totpConfig := dex.TOTPConfig{ - Issuer: "Netbird", + Issuer: "NetBird", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/idp/embedded.go` around lines 205 - 207, TOTP issuer string is inconsistent with the frontend; in the dex.TOTPConfig initialization (totpConfig variable in embedded.go) change Issuer from "Netbird" to match Frontend.Issuer "NetBird" so authenticator apps show the same name during TOTP enrollment.
231-232:⚠️ Potential issue | 🟡 MinorMove
DEX_SESSIONS_ENABLEDout of the config builder.Setting a process-wide env var here makes
ToYAMLConfig()impure: one MFA-enabled call leaves sessions enabled for later callers/tests, even whenEnableMFAis false. This should be set once during server/Dex startup, not from a YAML conversion helper.#!/bin/bash set -euo pipefail printf 'DEX_SESSIONS_ENABLED references:\n' rg -n --type=go 'DEX_SESSIONS_ENABLED' printf '\nToYAMLConfig call sites:\n' rg -n --type=go '\.ToYAMLConfig\s*\(' printf '\nEmbedded IdP initialization paths:\n' rg -n --type=go 'NewEmbeddedIdPManager\s*\('🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/idp/embedded.go` around lines 231 - 232, The call to os.Setenv("DEX_SESSIONS_ENABLED","true") inside ToYAMLConfig() makes the helper impure; remove that side-effect from ToYAMLConfig() and any other YAML conversion helpers so they only produce config text. Instead set the DEX_SESSIONS_ENABLED env var once during server/Dex startup or during Embedded IdP initialization (e.g. in NewEmbeddedIdPManager or the main server bootstrap path that constructs the embedded IdP), so tests and later callers aren’t affected by prior ToYAMLConfig() calls; update tests that relied on the env being set to explicitly set it in their setup if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/idp/embedded.go`:
- Around line 160-173: PostLogoutRedirectURIs currently only contains c.Issuer
and logoutURL which breaks post-logout redirects for dashboards/CLIs hosted on
different origins; update the client construction for both dashboard and
staticClientCLI to populate PostLogoutRedirectURIs from each client's configured
redirect URIs (use DashboardRedirectURIs for the dashboard client and
cliRedirectURIs for staticClientCLI) and, where necessary, add derived base
origins of those redirect URIs (strip path/query to form origin) so Dex will
accept post_logout_redirect_uri for each configured app origin instead of
hard-coding c.Issuer/logoutURL.
---
Duplicate comments:
In `@management/server/idp/embedded.go`:
- Around line 205-207: TOTP issuer string is inconsistent with the frontend; in
the dex.TOTPConfig initialization (totpConfig variable in embedded.go) change
Issuer from "Netbird" to match Frontend.Issuer "NetBird" so authenticator apps
show the same name during TOTP enrollment.
- Around line 231-232: The call to os.Setenv("DEX_SESSIONS_ENABLED","true")
inside ToYAMLConfig() makes the helper impure; remove that side-effect from
ToYAMLConfig() and any other YAML conversion helpers so they only produce config
text. Instead set the DEX_SESSIONS_ENABLED env var once during server/Dex
startup or during Embedded IdP initialization (e.g. in NewEmbeddedIdPManager or
the main server bootstrap path that constructs the embedded IdP), so tests and
later callers aren’t affected by prior ToYAMLConfig() calls; update tests that
relied on the env being set to explicitly set it in their setup if needed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f3776a2-2c4b-4640-bf07-1d492c0371eb
📒 Files selected for processing (2)
idp/dex/provider.gomanagement/server/idp/embedded.go
🚧 Files skipped from review as they are similar to previous changes (1)
- idp/dex/provider.go
Comment on lines
+160
to
+173
| PostLogoutRedirectURIs: []string{ | ||
| c.Issuer, | ||
| logoutURL, | ||
| }, | ||
| }, | ||
| { | ||
| ID: staticClientCLI, | ||
| Name: "NetBird CLI", | ||
| Public: true, | ||
| RedirectURIs: cliRedirectURIs, | ||
| PostLogoutRedirectURIs: []string{ | ||
| c.Issuer, | ||
| logoutURL, | ||
| }, |
Contributor
There was a problem hiding this comment.
Preserve configured app URLs in the post-logout allowlist.
These PostLogoutRedirectURIs ignore DashboardRedirectURIs/CLIRedirectURIs and only allow the issuer root. In deployments where the dashboard or CLI callback lives on a different origin, Dex will reject post_logout_redirect_uri back to that client and logout will break. Build the post-logout list from each client’s configured redirect URLs (or their derived app base URLs) instead of hard-coding c.Issuer/logoutURL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/idp/embedded.go` around lines 160 - 173,
PostLogoutRedirectURIs currently only contains c.Issuer and logoutURL which
breaks post-logout redirects for dashboards/CLIs hosted on different origins;
update the client construction for both dashboard and staticClientCLI to
populate PostLogoutRedirectURIs from each client's configured redirect URIs (use
DashboardRedirectURIs for the dashboard client and cliRedirectURIs for
staticClientCLI) and, where necessary, add derived base origins of those
redirect URIs (strip path/query to form origin) so Dex will accept
post_logout_redirect_uri for each configured app origin instead of hard-coding
c.Issuer/logoutURL.
Signed-off-by: jnfrati <nicofrati@gmail.com>
Signed-off-by: jnfrati <nicofrati@gmail.com>
Signed-off-by: jnfrati <nicofrati@gmail.com>
Signed-off-by: jnfrati <nicofrati@gmail.com>
Signed-off-by: jnfrati <nicofrati@gmail.com>
Signed-off-by: jnfrati <nicofrati@gmail.com>
jnfrati
force-pushed
the
feat/local-user-totp
branch
from
6d4dcca to
4501f0f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
idp/dex/config.go (1)
571-572:⚠️ Potential issue | 🟡 MinorDuration parsing errors are silently discarded.
Lines 571-572 ignore parsing errors for
AbsoluteLifetimeandValidIfNotUsedFor. Invalid duration strings will result in zero-value durations (0s), potentially causing unexpected session behavior (e.g., sessions expiring immediately).🛠️ Proposed fix with sensible defaults
- absoluteLifetime, _ := parseDuration(sessions.AbsoluteLifetime) - validIfNotUsedFor, _ := parseDuration(sessions.ValidIfNotUsedFor) + absoluteLifetime, err := parseDuration(sessions.AbsoluteLifetime) + if err != nil && sessions.AbsoluteLifetime != "" { + // Log warning and use sensible default + absoluteLifetime = 24 * time.Hour + } + validIfNotUsedFor, err := parseDuration(sessions.ValidIfNotUsedFor) + if err != nil && sessions.ValidIfNotUsedFor != "" { + // Log warning and use sensible default + validIfNotUsedFor = time.Hour + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idp/dex/config.go` around lines 571 - 572, The code silently discards parseDuration errors for sessions.AbsoluteLifetime and sessions.ValidIfNotUsedFor; update the handling in the block that calls parseDuration so you check the returned error for both calls (parseDuration(sessions.AbsoluteLifetime) and parseDuration(sessions.ValidIfNotUsedFor)), and on error either return/wrap the error up to the caller or set a sensible default timeout (e.g., preserve current behavior with a non-zero default or a configured fallback) while logging the parse failure; ensure you update the variables absoluteLifetime and validIfNotUsedFor only after successful parsing or assignment of the fallback so invalid strings cannot become zero-valued durations.management/server/idp/embedded.go (1)
204-207:⚠️ Potential issue | 🟡 MinorInconsistent casing in TOTP issuer name.
Line 206 uses
"Netbird"while line 148 uses"NetBird"forFrontend.Issuer. This inconsistency will appear in users' authenticator apps when they set up TOTP.🔧 Proposed fix
func configureMFA(cfg *dex.YAMLConfig) error { totpConfig := dex.TOTPConfig{ - Issuer: "Netbird", + Issuer: "NetBird", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/idp/embedded.go` around lines 204 - 207, The TOTP issuer string is cased differently ("Netbird") than the frontend issuer ("NetBird"); update configureMFA so TOTPConfig.Issuer uses the same value as the frontend to ensure consistent casing — e.g., set totpConfig.Issuer = cfg.Frontend.Issuer (or reference the same constant/variable used for Frontend.Issuer) in the configureMFA function where dex.TOTPConfig is constructed.
🧹 Nitpick comments (1)
idp/dex/config.go (1)
566-569: Input parameter mutation is unexpected.
buildSessionsConfigmutates the inputsessionsparameter by settingRememberMeCheckedByDefault(line 568). This side effect can cause unexpected behavior if the caller reuses theSessionsobject.♻️ Avoid mutating input
func buildSessionsConfig(sessions *Sessions) *server.SessionConfig { if sessions == nil { return nil } - if sessions.RememberMeCheckedByDefault == nil { - defaultRememberMeCheckedByDefault := false - sessions.RememberMeCheckedByDefault = &defaultRememberMeCheckedByDefault - } + rememberMeDefault := false + if sessions.RememberMeCheckedByDefault != nil { + rememberMeDefault = *sessions.RememberMeCheckedByDefault + } absoluteLifetime, _ := parseDuration(sessions.AbsoluteLifetime) validIfNotUsedFor, _ := parseDuration(sessions.ValidIfNotUsedFor) return &server.SessionConfig{ CookieEncryptionKey: []byte(sessions.CookieEncryptionKey), CookieName: sessions.CookieName, AbsoluteLifetime: absoluteLifetime, ValidIfNotUsedFor: validIfNotUsedFor, - RememberMeCheckedByDefault: *sessions.RememberMeCheckedByDefault, + RememberMeCheckedByDefault: rememberMeDefault, SSOSharedWithDefault: sessions.SSOSharedWithDefault, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idp/dex/config.go` around lines 566 - 569, buildSessionsConfig currently mutates the input Sessions parameter by setting RememberMeCheckedByDefault on the passed object; instead make a local copy and modify that copy before returning (e.g., copy := *sessions; if copy.RememberMeCheckedByDefault == nil { defaultVal := false; copy.RememberMeCheckedByDefault = &defaultVal } ) or construct and return a new Sessions value so the original input is not mutated; update buildSessionsConfig to operate on and return the copied/constructed Sessions instance rather than altering the input parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@idp/dex/web/templates/webauthn_verify.html`:
- Around line 1-12: The webauthn_verify.html template currently only contains a
redirect (globalThis.location.replace("/")) and mirrors home.html, indicating a
leftover debug placeholder; either delete this orphan template or replace its
body with a clear TODO comment and brief spec of the intended WebAuthn UI (e.g.,
page for credential creation and assertion flows, expected endpoints, and any
client-side WebAuthn JS interactions) so future developers know its
purpose—refer to the template name webauthn_verify.html and the existing
redirect/script block (globalThis.location.replace("/")) when locating and
updating the file, and keep header.html/footer.html includes intact.
---
Duplicate comments:
In `@idp/dex/config.go`:
- Around line 571-572: The code silently discards parseDuration errors for
sessions.AbsoluteLifetime and sessions.ValidIfNotUsedFor; update the handling in
the block that calls parseDuration so you check the returned error for both
calls (parseDuration(sessions.AbsoluteLifetime) and
parseDuration(sessions.ValidIfNotUsedFor)), and on error either return/wrap the
error up to the caller or set a sensible default timeout (e.g., preserve current
behavior with a non-zero default or a configured fallback) while logging the
parse failure; ensure you update the variables absoluteLifetime and
validIfNotUsedFor only after successful parsing or assignment of the fallback so
invalid strings cannot become zero-valued durations.
In `@management/server/idp/embedded.go`:
- Around line 204-207: The TOTP issuer string is cased differently ("Netbird")
than the frontend issuer ("NetBird"); update configureMFA so TOTPConfig.Issuer
uses the same value as the frontend to ensure consistent casing — e.g., set
totpConfig.Issuer = cfg.Frontend.Issuer (or reference the same constant/variable
used for Frontend.Issuer) in the configureMFA function where dex.TOTPConfig is
constructed.
---
Nitpick comments:
In `@idp/dex/config.go`:
- Around line 566-569: buildSessionsConfig currently mutates the input Sessions
parameter by setting RememberMeCheckedByDefault on the passed object; instead
make a local copy and modify that copy before returning (e.g., copy :=
*sessions; if copy.RememberMeCheckedByDefault == nil { defaultVal := false;
copy.RememberMeCheckedByDefault = &defaultVal } ) or construct and return a new
Sessions value so the original input is not mutated; update buildSessionsConfig
to operate on and return the copied/constructed Sessions instance rather than
altering the input parameter.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d643c7f-ff40-4e2f-bcc0-c9d78f9811cb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
combined/cmd/config.gogo.modidp/dex/config.goidp/dex/provider.goidp/dex/web/templates/home.htmlidp/dex/web/templates/logout.htmlidp/dex/web/templates/totp_verify.htmlidp/dex/web/templates/webauthn_verify.htmlmanagement/server/idp/embedded.go
✅ Files skipped from review due to trivial changes (1)
- combined/cmd/config.go
🚧 Files skipped from review as they are similar to previous changes (2)
- idp/dex/provider.go
- go.mod
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
idp/dex/config.go (1)
576-593:⚠️ Potential issue | 🟠 Major
expiry.signingKeysbecame a silent no-op.
Expiry.SigningKeysis still part of the public config model in this file, butToServerConfig()no longer copies it into the Dex server config while it still maps the other expiry settings. That makesexpiry.signingKeysa silent no-op for existing deployments and weakens signing-key rotation. Please wire it back when populatingcfg(for example viacfg.RotateKeysAfter).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idp/dex/config.go` around lines 576 - 593, The expiry.signingKeys setting in YAMLConfig is not being propagated in ToServerConfig, making signing key rotation a silent no-op; update YAMLConfig.ToServerConfig to copy the expiry signing-key setting (Expiry.SigningKeys) into the server.Config (e.g., set cfg.RotateKeysAfter or the appropriate rotation field on server.Config) when building cfg so key rotation behavior is preserved; locate the Expiry struct usage in ToServerConfig and assign its SigningKeys/rotation value into cfg before returning.
♻️ Duplicate comments (1)
idp/dex/config.go (1)
562-563:⚠️ Potential issue | 🟠 MajorReject invalid session durations instead of silently zeroing them.
These parse errors are still discarded, so a typo in
absoluteLifetimeorvalidIfNotUsedForquietly becomes0and changes session behavior without failing startup. Please surface an error for non-empty invalid values here or validate them earlier inValidate().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idp/dex/config.go` around lines 562 - 563, The current code silently ignores parseDuration errors for sessions.AbsoluteLifetime and sessions.ValidIfNotUsedFor causing invalid values to become zero; update the code in the function that computes absoluteLifetime/validIfNotUsedFor to check the error returned by parseDuration and return or propagate an error when the input string is non-empty and parseDuration fails (alternatively perform this validation in Validate()). Specifically, when calling parseDuration for sessions.AbsoluteLifetime and sessions.ValidIfNotUsedFor, if err != nil and the original string is not empty, surface that error (with context mentioning the field name) instead of discarding it so startup fails on invalid durations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@idp/dex/config.go`:
- Around line 85-88: The MFAAuthenticator.Config field currently defined as
json.RawMessage causes YAML unmarshalling to leave YAML bytes in the field,
which later breaks json.Unmarshal in buildTotpConfig and buildWebAuthnConfig;
change MFAAuthenticator.Config to map[string]interface{} (matching Connector) so
yaml.Unmarshal produces a native map, then where buildTotpConfig and
buildWebAuthnConfig expect JSON bytes, json.Marshal the map before calling
json.Unmarshal (or implement a custom UnmarshalYAML on MFAAuthenticator to
decode directly into the target structs); update references to
MFAAuthenticator.Config in LoadConfig, buildTotpConfig, and buildWebAuthnConfig
to handle the map-to-JSON conversion.
---
Outside diff comments:
In `@idp/dex/config.go`:
- Around line 576-593: The expiry.signingKeys setting in YAMLConfig is not being
propagated in ToServerConfig, making signing key rotation a silent no-op; update
YAMLConfig.ToServerConfig to copy the expiry signing-key setting
(Expiry.SigningKeys) into the server.Config (e.g., set cfg.RotateKeysAfter or
the appropriate rotation field on server.Config) when building cfg so key
rotation behavior is preserved; locate the Expiry struct usage in ToServerConfig
and assign its SigningKeys/rotation value into cfg before returning.
---
Duplicate comments:
In `@idp/dex/config.go`:
- Around line 562-563: The current code silently ignores parseDuration errors
for sessions.AbsoluteLifetime and sessions.ValidIfNotUsedFor causing invalid
values to become zero; update the code in the function that computes
absoluteLifetime/validIfNotUsedFor to check the error returned by parseDuration
and return or propagate an error when the input string is non-empty and
parseDuration fails (alternatively perform this validation in Validate()).
Specifically, when calling parseDuration for sessions.AbsoluteLifetime and
sessions.ValidIfNotUsedFor, if err != nil and the original string is not empty,
surface that error (with context mentioning the field name) instead of
discarding it so startup fails on invalid durations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db1a14df-eee5-4630-a002-a91e81f43604
📒 Files selected for processing (2)
idp/dex/config.gomanagement/server/idp/embedded.go
🚧 Files skipped from review as they are similar to previous changes (1)
- management/server/idp/embedded.go
6 tasks
2 tasks
|
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.
Describe your changes
Added support to enable MFA (TOTP only) for the embedded IDP.
Now the server can use the config
server.auth.enableLocalMFAwhich will automatically configure TOTP for all local users.Issue ticket number and link
#5088
Stack
Checklist
Dashboard
Related PR for dashboard:
netbirdio/dashboard#615
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#704
Summary by CodeRabbit
New Features
Chores