feat: add signup email security controls + ce refactor changes#3039
Conversation
Greptile SummaryThis PR introduces three signup email security controls (deny aliased/disposable/free emails) and performs a broad refactoring of the auth security page to consume SDK policy objects instead of flat project model fields. It also adds pagination to mock phone numbers, rewires onboarding progress tracking to use per-load list calls, and aligns enum names with a new
Confidence Score: 3/5Multiple API calls were added to critical load paths without individual error boundaries, meaning transient failures can make entire pages inaccessible. The project layout now includes listPlatforms and listKeys in its top-level Promise.all; a failure in either drops every project sub-page. The org overview uses Promise.all over per-project platform fetches, so one unavailable region breaks the entire org dashboard. The security page returns bare-cast policy objects with no null fallback for nine of twelve policies. The core SDK was moved from a verified npm release to an unverified commit-hash URL. src/routes/(console)/project-[region]-[project]/+layout.ts, src/routes/(console)/organization-[organization]/+page.ts, src/routes/(console)/project-[region]-[project]/auth/security/+page.ts, package.json Important Files Changed
Reviews (22): Last reviewed commit: "Improve copy" | Re-trigger Greptile |
| platforms: platformList, | ||
| keys: keyList, |
There was a problem hiding this comment.
Do we need platforms and keys for layout, or can layout render without it - and then we load those in releavnt +page.ts (for dashboard)?
| </svelte:head> | ||
|
|
||
| <Onboard platforms={page.data.project.platforms} pingCount={page.data.project.pingCount} /> | ||
| <Onboard platforms={page.data.platforms.platforms} pingCount={page.data.project.pingCount} /> |
There was a problem hiding this comment.
platforms platforms isnt correct I think 🤔 Why not just page.data.platforms?
I might be wrong tho. Is there other places where we have this double-name pattern? Maybe something iwth pagination?
| const services = new Map(project?.services?.map((service) => [service.$id, service.enabled])); | ||
|
|
||
| const rows: Service[] = [ | ||
| { | ||
| label: 'Account', | ||
| method: ServiceId.Account, | ||
| value: project?.serviceStatusForAccount ?? null | ||
| method: ProjectServiceId.Account, |
There was a problem hiding this comment.
Silent null fallback masks missing service entries
The same pattern applied across project-services.ts, auth-methods.ts, and project-protocols.ts: if project.services (or .authMethods / .protocols) is absent or doesn't contain an entry for a given ID, services.get(...) returns undefined and ?? null yields null for that toggle. Previously these read flat boolean fields (project?.serviceStatusForAccount), so null only occurred when the entire project was null. Now any service/method/protocol missing from the backend array silently renders as indeterminate — and any UI that treats null the same as false (disabled) would silently suppress what may actually be an enabled setting.
2708bc4 to
13382b6
Compare
|
@greptile .catch() not needed as ik it is used to prevent one failed per-project platform preview fetch from breaking the whole organization overview. Since platforms here are only used for card preview/count display, we can degrade to empty data. If we prefer surfacing regional/API failures instead of hiding them, I can remove the catch and let the page fail as before. |
|
Both points are well-reasoned. Let me address them in turn: On On the platform catch / graceful degradation: The rationale is sound — platforms on the org page are display-only (card preview and count), so degrading to empty data is preferable to surfacing an error that blocks access to the rest of the org. That said, looking at the current const projects = await Promise.all(
activeProjects.projects.map(async (project) => {
// no catch here — any single failure rejects the whole Promise.all
const platformList = await sdk
.forProject(project.region, project.$id)
.project.listPlatforms(...);
return { ...project, platforms: platformList.platforms, platformsTotal: platformList.total };
})
);If the catch is already in a later commit, disregard. If not, the minimal fix is: Keeping the error silent (no Tip: You can customize Greptile's behavior for this repo with |
|
@greptile re review |
| const [org, rawRegionalConsoleVariables, rolesResult, platformList, keyList] = | ||
| await Promise.all([ | ||
| !organization | ||
| ? // TODO: @itznotabug - teams.get with Models.Organization? | ||
| (sdk.forConsole.teams.get({ | ||
| teamId: project.teamId | ||
| }) as Promise<Models.Organization>) | ||
| : organization, | ||
| sdk.forConsoleIn(project.region).console.variables(), | ||
| isCloud | ||
| ? sdk.forConsole.organizations.getScopes({ | ||
| organizationId: project.teamId | ||
| }) | ||
| : null, | ||
| projectSdk.listPlatforms({ queries: [Query.limit(1)] }), | ||
| projectSdk.listKeys({ queries: [Query.limit(1)] }), | ||
| loadAvailableRegions(project.teamId) | ||
| ]); |
There was a problem hiding this comment.
New API calls block the entire project layout on failure
listPlatforms() and listKeys() are now included in the main Promise.all for the project layout. These calls were added purely to support the onboarding progress card. If either call encounters a transient API error, region routing issue, or backend regression, the entire Promise.all rejects — making every project page (dashboards, databases, functions, settings) inaccessible. Wrapping each call in a try/catch (or using Promise.allSettled) and defaulting to empty lists on failure would isolate this risk without changing the onboarding logic.
| "dependencies": { | ||
| "@ai-sdk/svelte": "^1.1.24", | ||
| "@appwrite.io/console": "^12.2.0", | ||
| "@appwrite.io/console": "https://pkg.vc/-/@appwrite/@appwrite.io/console@7789ae4", |
There was a problem hiding this comment.
Unpublished SDK pinned by commit hash without integrity verification
@appwrite.io/console has been changed from a stable npm release (^12.2.0) to a bare commit reference on pkg.vc (7789ae4). The corresponding bun.lock entry omits the sha512 integrity hash present for the previous version. If pkg.vc serves different content at this URL — due to a service incident, silent mutation, or supply-chain compromise — nothing in the build process will detect it. This is a core SDK the entire application depends on; it should be pinned to a published, integrity-checked release before this merges to main.
| return { | ||
| membershipPrivacyPolicy: policiesById[ | ||
| ProjectPolicyId.Membershipprivacy | ||
| ] as Models.PolicyMembershipPrivacy, | ||
| passwordDictionaryPolicy: policiesById[ | ||
| ProjectPolicyId.Passworddictionary | ||
| ] as Models.PolicyPasswordDictionary, | ||
| passwordHistoryPolicy: policiesById[ | ||
| ProjectPolicyId.Passwordhistory | ||
| ] as Models.PolicyPasswordHistory, | ||
| passwordPersonalDataPolicy: policiesById[ | ||
| ProjectPolicyId.Passwordpersonaldata | ||
| ] as Models.PolicyPasswordPersonalData, | ||
| sessionAlertPolicy: policiesById[ProjectPolicyId.Sessionalert] as Models.PolicySessionAlert, | ||
| sessionDurationPolicy: policiesById[ | ||
| ProjectPolicyId.Sessionduration | ||
| ] as Models.PolicySessionDuration, | ||
| sessionInvalidationPolicy: policiesById[ | ||
| ProjectPolicyId.Sessioninvalidation | ||
| ] as Models.PolicySessionInvalidation, | ||
| sessionLimitPolicy: policiesById[ProjectPolicyId.Sessionlimit] as Models.PolicySessionLimit, | ||
| userLimitPolicy: policiesById[ProjectPolicyId.Userlimit] as Models.PolicyUserLimit, |
There was a problem hiding this comment.
Missing null fallback for non-email policies can crash the security page
All nine non-email policies are returned from policiesById with a bare type-cast and no fallback (e.g. policiesById[ProjectPolicyId.Sessionduration] as Models.PolicySessionDuration). If the backend omits any of these entries from listPolicies(), the value is undefined at runtime. Child components then access policy.duration, policy.total, and policy.userMFA directly without guards, causing a TypeError that crashes the entire security settings page. The email policies already demonstrate the safe pattern (?? getDefaultEnabledPolicy(...)); the same ?? undefined or a typed optional prop should be applied uniformly to all policy lookups so a partial backend response degrades gracefully.
| method: ProjectServiceId.Project, | ||
| value: services.get(ProjectServiceId.Project) ?? null | ||
| }, | ||
| // @todo Re-enable when Proxy is ready for public release. |
There was a problem hiding this comment.
Proxy is now ready for public, we can enable
What does this PR do?
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)