Skip to content

Remove rp id from settings and derive it from defguard_url#2326

Merged
j-chmielewski merged 17 commits into
devfrom
remove-rp-id
Mar 16, 2026
Merged

Remove rp id from settings and derive it from defguard_url#2326
j-chmielewski merged 17 commits into
devfrom
remove-rp-id

Conversation

@j-chmielewski
Copy link
Copy Markdown
Contributor

@j-chmielewski j-chmielewski commented Mar 13, 2026

Related issue: #2233

  • Remove webauthn_rp_id from Settings and DefguardConfig
  • Derive it from Settings::defguard_url in runtime
  • Strict Settings::defguard_url validation to ensure rp id can be derived from it (no IP addresses)
  • Fallback to host-only cookies (instead of panic) if cookie domain is invalid
  • Add defguard_url to instance settings UI
  • disable_stats_purge -> enable_stats_purge
  • Stats purge settings form tweaks

@j-chmielewski j-chmielewski marked this pull request as ready for review March 13, 2026 11:47
@j-chmielewski j-chmielewski requested a review from Copilot March 13, 2026 11:47
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the persisted WebAuthn RP ID setting and instead derives it at runtime from Settings.defguard_url, with stricter validation to ensure the derivation is always possible. It also renames the stats purge flag to a positive form (enable_stats_purge), updates cookie handling to avoid panics when a cookie domain can’t be derived, and wires defguard_url into the web UI setup/settings flows.

Changes:

  • Remove webauthn_rp_id from DB/config and derive WebAuthn configuration + cookie domain from Settings.defguard_url (with validation rejecting IP hosts).
  • Rename disable_stats_purgeenable_stats_purge across DB, backend, and web UI (including form UX tweaks).
  • Add defguard_url input + validation to initial setup, auto-adoption setup, migration wizard, and instance settings UI.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
web/src/shared/utils/defguardUrl.ts Adds client-side defguard_url validation (reject IP hosts).
web/src/shared/api/types.ts Renames settings field to enable_stats_purge.
web/src/pages/settings/SettingsInstancePage/SettingsInstancePage.tsx Adds defguard_url to instance settings UI; renames stats purge toggle + folds options under toggle.
web/src/pages/SetupPage/initial/steps/SetupGeneralConfigStep.tsx Adds invalid-host validation for defguard_url.
web/src/pages/SetupPage/autoAdoption/steps/AutoAdoptionUrlSettingsStep.tsx Adds invalid-host validation for defguard_url.
web/src/pages/MigrationWizardPage/steps/MigrationWizardGeneralConfigurationStep.tsx Adds invalid-host validation for defguard_url.
web/messages/en/settings.json Adds new instance-settings strings and renames stats purge toggle label.
web/messages/en/migration_wizard.json Adds invalid-host error string for defguard_url.
web/messages/en/initial_wizard.json Adds invalid-host error string for defguard_url.
migrations/20260312072940_[2.0.0]_rp_id_stats_purge.up.sql Renames stats purge column, drops webauthn_rp_id, inverts boolean semantics.
migrations/20260312072940_[2.0.0]_rp_id_stats_purge.down.sql Restores webauthn_rp_id column and reverses stats purge column rename/semantics.
crates/defguard_setup/src/auto_adoption.rs Adjusts error conversion for updated update_current_settings result type.
crates/defguard_core/tests/integration/common.rs Removes test setup of webauthn_rp_id and URL parsing dependency.
crates/defguard_core/src/handlers/openid_flow.rs Uses derived cookie domain when available instead of panicking.
crates/defguard_core/src/handlers/mod.rs Introduces shared cookie_domain() helper with derivation fallback + warning.
crates/defguard_core/src/handlers/auth.rs Uses derived cookie domain; rebuilds WebAuthn config from current settings via appstate.webauthn().
crates/defguard_core/src/error.rs Adds SettingsSaveError / SettingsUrlError conversions into WebError.
crates/defguard_core/src/enterprise/license.rs Propagates settings save failures via LicenseError.
crates/defguard_core/src/enterprise/ldap/error.rs Propagates settings save failures via LdapError.
crates/defguard_core/src/enterprise/handlers/openid_login.rs Uses derived cookie domain when available instead of panicking.
crates/defguard_core/src/appstate.rs Removes stored WebAuthn instance; derives WebAuthn from current settings on demand.
crates/defguard_common/src/db/models/settings.rs Removes persisted webauthn_rp_id, adds URL parsing/validation, derives RP ID + cookie domain, and introduces SettingsSaveError/SettingsUrlError.
crates/defguard_common/src/config.rs Removes CLI/config webauthn_rp_id; derives cookie domain from settings.
crates/defguard/src/main.rs Updates purge task guard to use enable_stats_purge.
.sqlx/query-dab137a626956fe0a0f2fbfc17c45075372f6963ff73760a53f843eaf5ebed4a.json Updates SQLx metadata for settings SELECT (removes rp_id, renames purge column).
.sqlx/query-89698ecaa251e056770bb90827d2d41284e6629883f647462029872c9fad2bfb.json Updates SQLx metadata for settings UPDATE (removes rp_id, renames purge column).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread crates/defguard_core/src/handlers/auth.rs
Comment thread migrations/20260312072940_[2.0.0]_rp_id_stats_purge.up.sql
Comment thread migrations/20260312072940_[2.0.0]_rp_id_stats_purge.down.sql
Comment thread web/src/pages/settings/SettingsInstancePage/SettingsInstancePage.tsx Outdated
Comment thread web/src/pages/SetupPage/initial/steps/SetupGeneralConfigStep.tsx Outdated
@j-chmielewski j-chmielewski merged commit 2b741b7 into dev Mar 16, 2026
3 checks passed
@j-chmielewski j-chmielewski deleted the remove-rp-id branch March 16, 2026 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants