Skip to content

fix(auth/security): harden against missing or null policy entries (#3076)#3077

Open
WhoamiI00 wants to merge 2 commits into
appwrite:mainfrom
WhoamiI00:fix-3076-security-tab-null-policies
Open

fix(auth/security): harden against missing or null policy entries (#3076)#3077
WhoamiI00 wants to merge 2 commits into
appwrite:mainfrom
WhoamiI00:fix-3076-security-tab-null-policies

Conversation

@WhoamiI00
Copy link
Copy Markdown
Contributor

What

Hardens the load function and passwordPolicies component for the /auth/security route against null or missing policy entries returned by project.listPolicies().

Why

After upgrading a self-hosted instance from 1.8.1 to 1.9.0, opening Auth → Security crashes with:

Uncaught TypeError: Cannot read properties of null (reading 'group')
    at Array.filter (<anonymous>)

— reported in #3076.

The current code makes two unsafe assumptions:

  1. src/routes/.../auth/security/+page.ts calls .map((policy) => [policy.$id, policy]) on the policies array, so a single null entry throws before the page can render.
  2. passwordPolicies.svelte accesses historyPolicy.total, dictionaryPolicy.enabled, and personalDataPolicy.enabled directly, but +page.ts already casts the lookup result as the policy type without verifying it is defined. After a migration from 1.8.11.9.0 it is possible for some policies not to be present yet, in which case those reads throw too.

What changed

  • +page.ts.filter((policy) => policy != null && policy.$id != null) before .map.
  • passwordPolicies.svelte — typed the three policy props as … | undefined (to match the runtime reality), used optional chaining + nullish-coalescing defaults in onMount and in the hasChanges derived.

Verification

  • bun run check — 0 errors, no new warnings.
  • bun run lint — 0 errors.
  • bun run test:unit — 235/235 pass.
  • Dev server (bun run dev) loads with no regression.

Caveats

The original stack trace in #3076 (null.group inside Array.filter) does not directly map to either site changed here — it points into minified JS without source maps. None of the policy models in @appwrite.io/console expose a group field, so the exact failing call site is somewhere I could not identify from the source alone (likely a code path I missed, or inside a dependency).

This PR fixes a same-class null-safety bug in the same load path that is reachable from the same user action. It is defensive hardening rather than a confirmed root-cause fix. If maintainers can share an unminified stack from a reproducing environment, I'm happy to follow up with a targeted patch for the precise failing site.

Related to: #3076

The /auth/security page assumes every policy in the listPolicies()
response is non-null and has a $id, and that the policies it indexes
into the result map always exist. After a 1.8.1 -> 1.9.0 upgrade,
listPolicies may return entries that are absent for policies not yet
present in the migrated project, which causes:

  - +page.ts: TypeError when .map((policy) => [policy.$id, policy])
    encounters a null entry.
  - passwordPolicies.svelte: TypeError accessing .total / .enabled
    on an undefined policy in onMount and in the hasChanges derived.

Changes:
  - +page.ts: filter out null entries before building policiesById.
  - passwordPolicies.svelte: type the three password-policy props as
    | undefined to match the (already-possible) runtime shape, and
    use optional chaining + nullish-coalescing fallbacks everywhere
    the policies are read.

Related to appwrite#3076. This addresses the same class of null-safety bug in
the same load path; the exact failing call site reported in appwrite#3076
points into minified JS without source maps, so this is defensive
hardening rather than a confirmed root-cause fix.
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 3, 2026

Greptile Summary

This PR hardens all seven Auth → Security components against null or missing policy entries returned by project.listPolicies() — a regression observed when upgrading self-hosted instances from 1.8.1 → 1.9.0. The previous review's concern about sibling components crashing on top-level $state / $derived initialisers has been fully addressed in this revision.

  • +page.ts: adds a .filter((p) => p != null && p.$id != null) guard before .map, and widens all nine policy return types to T | undefined so TypeScript correctly propagates the optionality to every consumer.
  • All six Svelte components: props are widened to | undefined; every direct property access is replaced with optional chaining and a sensible ?? false / ?? 0 fallback, using either onMount initialisation (for sessionSecurity) or $derived-backed initialisers (for the remaining components).

Confidence Score: 5/5

Safe to merge — all crash paths identified in the previous review round are now guarded, and no new error paths are introduced.

The changes are narrowly scoped: null filters in the load function and optional-chaining wrappers in each component. Every direct property read on a potentially-absent policy is guarded, and the disabled-button logic in each component ensures users cannot accidentally persist a default-fallback value.

updateSessionsLimit.svelte — minor cosmetic edge case when sessions policy is absent.

Important Files Changed

Filename Overview
src/routes/(console)/project-[region]-[project]/auth/security/+page.ts Adds a null/undefined filter before .map and correctly widens all nine policy return types to `
src/routes/(console)/project-[region]-[project]/auth/security/passwordPolicies.svelte Props widened to `
src/routes/(console)/project-[region]-[project]/auth/security/sessionSecurity.svelte Props widened to `
src/routes/(console)/project-[region]-[project]/auth/security/updateMembershipPrivacy.svelte All five top-level $state initialisers and the isSubmitDisabled derived are guarded with ?. and ?? false; submit is disabled until the user changes a value.
src/routes/(console)/project-[region]-[project]/auth/security/updateSessionLength.svelte Introduces a policyDuration derived with ?? 0 fallback; the createTimeUnitPair call and button disabled comparison both use it safely.
src/routes/(console)/project-[region]-[project]/auth/security/updateSessionsLimit.svelte Introduces policyTotal derived; maxSessions initialises to 0 when policy is absent, which is below the InputNumber min={1} — button is disabled so no bad data is sent, but the field shows a below-minimum default.
src/routes/(console)/project-[region]-[project]/auth/security/updateUsersLimit.svelte Top-level $state initialisers now go through a policyTotal derived; both btnDisabled paths handle the undefined-policy case with no crash risk.

Reviews (2): Last reviewed commit: "fix(auth/security): extend null-safety t..." | Re-trigger Greptile

Addresses Greptile review on PR appwrite#3077: the prior commit only hardened
+page.ts and passwordPolicies.svelte, leaving five sibling components
that read policy fields in top-level $state / $derived initializers
which crash before render when a policy is absent.

Changes:
  - +page.ts: widen the eight cast-as policy return types to
    `... | undefined` so downstream components see the real runtime
    shape. The three email-policy return values keep their fallback
    defaults and remain non-nullable.
  - sessionSecurity.svelte, updateUsersLimit.svelte,
    updateSessionLength.svelte, updateSessionsLimit.svelte,
    updateMembershipPrivacy.svelte: type the `policy` prop as
    `... | undefined`, replace direct field reads with optional
    chaining + nullish-coalescing defaults (false for booleans,
    0 for numeric totals/durations).

This makes the entire /auth/security page tolerant of a partially
migrated 1.8.1 -> 1.9.0 backend that returns only a subset of
policies.

Verification: `bun run check` 0 errors, `bun run lint` clean on the
security folder, `bun run test:unit` 235/235 pass.

Related to appwrite#3076.
@WhoamiI00
Copy link
Copy Markdown
Contributor Author

WhoamiI00 commented Jun 3, 2026

Good catch, you're right — the first commit only fixed two files and left the others reading policy fields in top-level $state/$derived that would still throw if any policy was missing.

Pushed c9bfd4d. The five sibling components (updateUsersLimit, updateSessionLength, updateSessionsLimit, updateMembershipPrivacy, sessionSecurity) now take policy: ... | undefined and use ?./?? defaults at the read sites — false for the booleans, 0 for totals and duration. I also widened the eight cast-as types in +page.ts so the prop types actually reflect what can show up at runtime; the three email policies still go through getDefaultEnabledPolicy so they stay non-nullable.

Whole page should survive a partial migration now. Check/lint/tests still clean.

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.

1 participant