fix(secrets): restore unsaved-changes guard for settings tab navigation (#4009)

* fix(secrets): restore unsaved-changes guard for settings tab navigation - Add useSettingsDirtyStore (stores/settings/dirty) to track dirty state across the settings sidebar and section components - Wire credentials-manager and integrations-manager to sync dirty state to the store and clean up on unmount; also reset store synchronously in handleDiscardAndNavigate - Update settings-sidebar to check dirty state before tab switches and Back navigation, showing an Unsaved Changes dialog if needed - Remove dead stores/settings/environment directory; move EnvironmentVariable type into lib/environment/api * fix(teams): harden Microsoft content URL validation - Add isMicrosoftContentUrl helper with typed allowlist covering SharePoint, OneDrive, and Teams CDN domains - Replace loose substring checks in Teams webhook handler with parsed-hostname matching to prevent bypass via partial domain names - Deduplicate OneDrive share-link detection into isOneDriveShareLink flag and use searchParams API instead of string splitting * fix(env): remove type re-exports from query file, drop keepPreviousData on static key * fix(teams): remove smba.trafficmanager.net from Microsoft content allowlist The subdomain check for smba.trafficmanager.net was unnecessary — Azure Traffic Manager does not support nested subdomains of existing profiles, but the pattern still raised a valid audit concern. Teams bot-framework attachment URLs from this host fall through to the generic fetchWithDNSPinning branch, which provides the same protection without the ambiguity. * fix(secrets): guard active-tab re-click, restore keepPreviousData on workspace env query * fix(teams): add 1drv.com apex to OneDrive share-link branch 1drv.com (apex) is a short-link domain functionally equivalent to 1drv.ms and requires share-token resolution, not direct fetch. CDN subdomains (files.1drv.com) are unaffected — the exact-match check leaves them on the direct-fetch path.
2026-04-28 03:00:29 -04:00 · 2026-04-06 22:00:25 -07:00
parent 1e00a06e86
commit 779358388d
13 changed files with 217 additions and 75 deletions
--- a/apps/sim/app/api/environment/route.ts
+++ b/apps/sim/app/api/environment/route.ts
@@ -10,7 +10,7 @@ import { decryptSecret, encryptSecret } from '@/lib/core/security/encryption'
 import { generateRequestId } from '@/lib/core/utils/request'
 import { generateId } from '@/lib/core/utils/uuid'
 import { syncPersonalEnvCredentialsForUser } from '@/lib/credentials/environment'
-import type { EnvironmentVariable } from '@/stores/settings/environment'
+import type { EnvironmentVariable } from '@/lib/environment/api'

 const logger = createLogger('EnvironmentAPI')

--- a/apps/sim/app/workspace/[workspaceId]/settings/components/credentials/credentials-manager.tsx
+++ b/apps/sim/app/workspace/[workspaceId]/settings/components/credentials/credentials-manager.tsx
@@ -30,6 +30,7 @@ import {
  type PendingCredentialCreateRequest,
  readPendingCredentialCreateRequest,
 } from '@/lib/credentials/client-state'
+import type { WorkspaceEnvironmentData } from '@/lib/environment/api'
 import { getUserColor } from '@/lib/workspaces/colors'
 import { isValidEnvVarName } from '@/executor/constants'
 import {
@@ -48,9 +49,9 @@ import {
  useSavePersonalEnvironment,
  useUpsertWorkspaceEnvironment,
  useWorkspaceEnvironment,
-  type WorkspaceEnvironmentData,
 } from '@/hooks/queries/environment'
 import { useWorkspacePermissionsQuery } from '@/hooks/queries/workspace'
+import { useSettingsDirtyStore } from '@/stores/settings/dirty/store'

 const logger = createLogger('SecretsManager')

@@ -482,6 +483,15 @@ export function CredentialsManager() {
  hasChangesRef.current = hasChanges
  shouldBlockNavRef.current = hasChanges || isDetailsDirty

+  const setNavGuardDirty = useSettingsDirtyStore((s) => s.setDirty)
+  const resetNavGuard = useSettingsDirtyStore((s) => s.reset)
+
+  useEffect(() => {
+    setNavGuardDirty(hasChanges || isDetailsDirty)
+  }, [hasChanges, isDetailsDirty, setNavGuardDirty])
+
+  useEffect(() => () => resetNavGuard(), [resetNavGuard])
+
  // --- Effects ---
  useEffect(() => {
    if (hasSavedRef.current) return
@@ -981,6 +991,7 @@ export function CredentialsManager() {

  const handleDiscardAndNavigate = useCallback(() => {
    shouldBlockNavRef.current = false
+    resetNavGuard()
    resetToSaved()
    setSelectedCredentialId(null)

@@ -989,7 +1000,7 @@ export function CredentialsManager() {
      pendingNavigationUrlRef.current = null
      router.push(url)
    }
-  }, [router, resetToSaved])
+  }, [router, resetToSaved, resetNavGuard])

  const renderEnvVarRow = useCallback(
    (envVar: UIEnvironmentVariable, originalIndex: number) => {
--- a/apps/sim/app/workspace/[workspaceId]/settings/components/integrations/integrations-manager.tsx
+++ b/apps/sim/app/workspace/[workspaceId]/settings/components/integrations/integrations-manager.tsx
@@ -54,6 +54,7 @@ import {
 } from '@/hooks/queries/oauth/oauth-connections'
 import { useWorkspacePermissionsQuery } from '@/hooks/queries/workspace'
 import { useOAuthReturnRouter } from '@/hooks/use-oauth-return'
+import { useSettingsDirtyStore } from '@/stores/settings/dirty/store'

 const logger = createLogger('IntegrationsManager')

@@ -247,6 +248,15 @@ export function IntegrationsManager() {

  const isDetailsDirty = isDescriptionDirty || isDisplayNameDirty

+  const setNavGuardDirty = useSettingsDirtyStore((s) => s.setDirty)
+  const resetNavGuard = useSettingsDirtyStore((s) => s.reset)
+
+  useEffect(() => {
+    setNavGuardDirty(isDetailsDirty)
+  }, [isDetailsDirty, setNavGuardDirty])
+
+  useEffect(() => () => resetNavGuard(), [resetNavGuard])
+
  const handleSaveDetails = async () => {
    if (!selectedCredential || !isSelectedAdmin || !isDetailsDirty || updateCredential.isPending)
      return
--- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/env-var-dropdown.tsx
+++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/env-var-dropdown.tsx
@@ -10,11 +10,8 @@ import {
 } from '@/components/emcn'
 import { cn } from '@/lib/core/utils/cn'
 import { writePendingCredentialCreateRequest } from '@/lib/credentials/client-state'
-import {
-  usePersonalEnvironment,
-  useWorkspaceEnvironment,
-  type WorkspaceEnvironmentData,
-} from '@/hooks/queries/environment'
+import type { WorkspaceEnvironmentData } from '@/lib/environment/api'
+import { usePersonalEnvironment, useWorkspaceEnvironment } from '@/hooks/queries/environment'
 import { useSettingsNavigation } from '@/hooks/use-settings-navigation'

 /**
--- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-sidebar/settings-sidebar.tsx
+++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-sidebar/settings-sidebar.tsx
@@ -1,9 +1,18 @@
 'use client'

-import { useCallback, useMemo } from 'react'
+import { useCallback, useMemo, useState } from 'react'
 import { useQueryClient } from '@tanstack/react-query'
 import { useParams, usePathname, useRouter } from 'next/navigation'
-import { ChevronDown, Skeleton } from '@/components/emcn'
+import {
+  Button,
+  ChevronDown,
+  Modal,
+  ModalBody,
+  ModalContent,
+  ModalFooter,
+  ModalHeader,
+  Skeleton,
+} from '@/components/emcn'
 import { useSession } from '@/lib/auth/auth-client'
 import { getSubscriptionAccessState } from '@/lib/billing/client'
 import { isHosted } from '@/lib/core/config/feature-flags'
@@ -23,6 +32,7 @@ import { useOrganizations } from '@/hooks/queries/organization'
 import { prefetchSubscriptionData, useSubscriptionData } from '@/hooks/queries/subscription'
 import { usePermissionConfig } from '@/hooks/use-permission-config'
 import { useSettingsNavigation } from '@/hooks/use-settings-navigation'
+import { useSettingsDirtyStore } from '@/stores/settings/dirty/store'

 const SKELETON_SECTIONS = [3, 2, 2] as const

@@ -41,6 +51,13 @@ export function SettingsSidebar({
  const router = useRouter()

  const queryClient = useQueryClient()
+
+  const requestNavigation = useSettingsDirtyStore((s) => s.requestNavigation)
+  const confirmNavigation = useSettingsDirtyStore((s) => s.confirmNavigation)
+  const cancelNavigation = useSettingsDirtyStore((s) => s.cancelNavigation)
+  const isDirty = useSettingsDirtyStore((s) => s.isDirty)
+  const [showDiscardDialog, setShowDiscardDialog] = useState(false)
+
  const { data: session, isPending: sessionLoading } = useSession()
  const { data: organizationsData, isLoading: orgsLoading } = useOrganizations()
  const { data: generalSettings } = useGeneralSettings()
@@ -180,8 +197,27 @@ export function SettingsSidebar({
  const { popSettingsReturnUrl, getSettingsHref } = useSettingsNavigation()

  const handleBack = useCallback(() => {
+    if (isDirty) {
+      setShowDiscardDialog(true)
+      return
+    }
    router.push(popSettingsReturnUrl(`/workspace/${workspaceId}/home`))
-  }, [router, popSettingsReturnUrl, workspaceId])
+  }, [router, popSettingsReturnUrl, workspaceId, isDirty])
+
+  const handleConfirmDiscard = useCallback(() => {
+    const section = confirmNavigation()
+    setShowDiscardDialog(false)
+    if (section) {
+      router.replace(getSettingsHref({ section }), { scroll: false })
+    } else {
+      router.push(popSettingsReturnUrl(`/workspace/${workspaceId}/home`))
+    }
+  }, [confirmNavigation, router, getSettingsHref, popSettingsReturnUrl, workspaceId])
+
+  const handleCancelDiscard = useCallback(() => {
+    cancelNavigation()
+    setShowDiscardDialog(false)
+  }, [cancelNavigation])

  return (
    <>
@@ -286,11 +322,15 @@ export function SettingsSidebar({
                        className={itemClassName}
                        onMouseEnter={() => handlePrefetch(item.id)}
                        onFocus={() => handlePrefetch(item.id)}
-                        onClick={() =>
-                          router.replace(getSettingsHref({ section: item.id as SettingsSection }), {
-                            scroll: false,
-                          })
-                        }
+                        onClick={() => {
+                          const section = item.id as SettingsSection
+                          if (section === activeSection) return
+                          if (!requestNavigation(section)) {
+                            setShowDiscardDialog(true)
+                            return
+                          }
+                          router.replace(getSettingsHref({ section }), { scroll: false })
+                        }}
                      >
                        {content}
                      </button>
@@ -312,6 +352,25 @@ export function SettingsSidebar({
          })
        )}
      </div>
+
+      <Modal open={showDiscardDialog} onOpenChange={(open) => !open && handleCancelDiscard()}>
+        <ModalContent size='sm'>
+          <ModalHeader>Unsaved Changes</ModalHeader>
+          <ModalBody>
+            <p className='text-[var(--text-secondary)]'>
+              You have unsaved changes. Are you sure you want to discard them?
+            </p>
+          </ModalBody>
+          <ModalFooter>
+            <Button variant='default' onClick={handleCancelDiscard}>
+              Keep Editing
+            </Button>
+            <Button variant='destructive' onClick={handleConfirmDiscard}>
+              Discard Changes
+            </Button>
+          </ModalFooter>
+        </ModalContent>
+      </Modal>
    </>
  )
 }
--- a/apps/sim/hooks/queries/environment.ts
+++ b/apps/sim/hooks/queries/environment.ts
@@ -1,13 +1,9 @@
 import { createLogger } from '@sim/logger'
 import { keepPreviousData, useMutation, useQuery, useQueryClient } from '@tanstack/react-query'
-import type { WorkspaceEnvironmentData } from '@/lib/environment/api'
+import type { EnvironmentVariable, WorkspaceEnvironmentData } from '@/lib/environment/api'
 import { fetchPersonalEnvironment, fetchWorkspaceEnvironment } from '@/lib/environment/api'
 import { workspaceCredentialKeys } from '@/hooks/queries/credentials'
 import { API_ENDPOINTS } from '@/stores/constants'
-import type { EnvironmentVariable } from '@/stores/settings/environment'
-
-export type { WorkspaceEnvironmentData } from '@/lib/environment/api'
-export type { EnvironmentVariable } from '@/stores/settings/environment'

 const logger = createLogger('EnvironmentQueries')

@@ -27,8 +23,7 @@ export function usePersonalEnvironment() {
  return useQuery({
    queryKey: environmentKeys.personal(),
    queryFn: ({ signal }) => fetchPersonalEnvironment(signal),
-    staleTime: 60 * 1000, // 1 minute
-    placeholderData: keepPreviousData,
+    staleTime: 60 * 1000,
  })
 }

--- a/apps/sim/lib/core/security/input-validation.ts
+++ b/apps/sim/lib/core/security/input-validation.ts
@@ -741,18 +741,8 @@ export function validateExternalUrl(
    }
  }

-  // Block suspicious ports commonly used for internal services
  const port = parsedUrl.port
-  const blockedPorts = [
-    '22', // SSH
-    '23', // Telnet
-    '25', // SMTP
-    '3306', // MySQL
-    '5432', // PostgreSQL
-    '6379', // Redis
-    '27017', // MongoDB
-    '9200', // Elasticsearch
-  ]
+  const blockedPorts = ['22', '23', '25', '3306', '5432', '6379', '27017', '9200']

  if (port && blockedPorts.includes(port)) {
    return {
@@ -842,7 +832,6 @@ export function validateAirtableId(
    }
  }

-  // Airtable IDs: prefix (3 chars) + 14 alphanumeric characters = 17 chars total
  const airtableIdPattern = new RegExp(`^${expectedPrefix}[a-zA-Z0-9]{14}$`)

  if (!airtableIdPattern.test(value)) {
@@ -893,11 +882,6 @@ export function validateAwsRegion(
    }
  }

-  // AWS region patterns:
-  // - Standard: af|ap|ca|eu|me|sa|us|il followed by direction and number
-  // - GovCloud: us-gov-east-1, us-gov-west-1
-  // - China: cn-north-1, cn-northwest-1
-  // - ISO: us-iso-east-1, us-iso-west-1, us-isob-east-1
  const awsRegionPattern =
    /^(af|ap|ca|cn|eu|il|me|sa|us|us-gov|us-iso|us-isob)-(central|north|northeast|northwest|south|southeast|southwest|east|west)-\d{1,2}$/

@@ -1156,7 +1140,6 @@ export function validatePaginationCursor(
    }
  }

-  // Allow alphanumeric, base64 chars (+, /, =), and URL-safe chars (-, _, ., ~, %)
  const cursorPattern = /^[A-Za-z0-9+/=\-_.~%]+$/
  if (!cursorPattern.test(value)) {
    logger.warn('Pagination cursor contains disallowed characters', {
@@ -1224,3 +1207,43 @@ export function validateOktaDomain(rawDomain: string): string {
  }
  return domain
 }
+
+const MICROSOFT_CONTENT_SUFFIXES = [
+  'sharepoint.com',
+  'sharepoint.us',
+  'sharepoint.de',
+  'sharepoint.cn',
+  'sharepointonline.com',
+  'onedrive.com',
+  'onedrive.live.com',
+  '1drv.ms',
+  '1drv.com',
+  'microsoftpersonalcontent.com',
+] as const
+
+/**
+ * Returns true if the given URL is hosted on a trusted Microsoft SharePoint or
+ * OneDrive domain. Validates the parsed hostname against an allowlist using exact
+ * match or subdomain suffix, preventing incomplete-substring bypasses.
+ *
+ * Covers SharePoint Online (commercial, GCC/GCC High/DoD, Germany, China),
+ * OneDrive business and consumer, OneDrive short-link and CDN domains,
+ * and Microsoft personal content CDN.
+ *
+ * @see https://learn.microsoft.com/en-us/sharepoint/required-urls-and-ports
+ * @see https://learn.microsoft.com/en-us/microsoft-365/enterprise/microsoft-365-u-s-government-gcc-high-endpoints
+ *
+ * @param url - The URL to check
+ * @returns Whether the URL belongs to a trusted Microsoft content host
+ */
+export function isMicrosoftContentUrl(url: string): boolean {
+  let hostname: string
+  try {
+    hostname = new URL(url).hostname.toLowerCase()
+  } catch {
+    return false
+  }
+  return MICROSOFT_CONTENT_SUFFIXES.some(
+    (suffix) => hostname === suffix || hostname.endsWith(`.${suffix}`)
+  )
+}
--- a/apps/sim/lib/environment/api.ts
+++ b/apps/sim/lib/environment/api.ts
@@ -1,5 +1,9 @@
 import { API_ENDPOINTS } from '@/stores/constants'
-import type { EnvironmentVariable } from '@/stores/settings/environment'
+
+export interface EnvironmentVariable {
+  key: string
+  value: string
+}

 export interface WorkspaceEnvironmentData {
  workspace: Record<string, string>
--- a/apps/sim/lib/webhooks/providers/microsoft-teams.ts
+++ b/apps/sim/lib/webhooks/providers/microsoft-teams.ts
@@ -5,6 +5,7 @@ import { createLogger } from '@sim/logger'
 import { eq } from 'drizzle-orm'
 import { type NextRequest, NextResponse } from 'next/server'
 import { safeCompare } from '@/lib/core/security/encryption'
+import { isMicrosoftContentUrl } from '@/lib/core/security/input-validation'
 import {
  type SecureFetchResponse,
  secureFetchWithPinnedIP,
@@ -240,10 +241,24 @@ async function formatTeamsGraphNotification(

                if (!contentUrl) continue

+                let parsedContentUrl: URL
+                try {
+                  parsedContentUrl = new URL(contentUrl)
+                } catch {
+                  continue
+                }
+                const contentHost = parsedContentUrl.hostname.toLowerCase()
+
                let buffer: Buffer | null = null
                let mimeType = 'application/octet-stream'

-                if (contentUrl.includes('sharepoint.com') || contentUrl.includes('onedrive')) {
+                const isOneDriveShareLink =
+                  contentHost === '1drv.ms' ||
+                  contentHost === '1drv.com' ||
+                  contentHost === 'microsoftpersonalcontent.com' ||
+                  contentHost.endsWith('.microsoftpersonalcontent.com')
+
+                if (isMicrosoftContentUrl(contentUrl) && !isOneDriveShareLink) {
                  try {
                    const directRes = await fetchWithDNSPinning(
                      contentUrl,
@@ -285,22 +300,15 @@ async function formatTeamsGraphNotification(
                  } catch {
                    continue
                  }
-                } else if (
-                  contentUrl.includes('1drv.ms') ||
-                  contentUrl.includes('onedrive.live.com') ||
-                  contentUrl.includes('onedrive.com') ||
-                  contentUrl.includes('my.microsoftpersonalcontent.com')
-                ) {
+                } else if (isOneDriveShareLink) {
                  try {
                    let shareToken: string | null = null

-                    if (contentUrl.includes('1drv.ms')) {
-                      const urlParts = contentUrl.split('/').pop()
-                      if (urlParts) shareToken = urlParts
-                    } else if (contentUrl.includes('resid=')) {
-                      const urlParams = new URL(contentUrl).searchParams
-                      const resId = urlParams.get('resid')
-                      if (resId) shareToken = resId
+                    if (contentHost === '1drv.ms') {
+                      const lastSegment = parsedContentUrl.pathname.split('/').pop()
+                      if (lastSegment) shareToken = lastSegment
+                    } else if (parsedContentUrl.searchParams.has('resid')) {
+                      shareToken = parsedContentUrl.searchParams.get('resid')
                    }

                    if (!shareToken) {
--- a/apps/sim/stores/settings/dirty/store.ts
+++ b/apps/sim/stores/settings/dirty/store.ts
@@ -0,0 +1,53 @@
+import { create } from 'zustand'
+import { devtools } from 'zustand/middleware'
+import type { SettingsSection } from '@/app/workspace/[workspaceId]/settings/navigation'
+
+interface SettingsDirtyStore {
+  isDirty: boolean
+  pendingSection: SettingsSection | null
+  setDirty: (dirty: boolean) => void
+  /**
+   * Call before navigating to a new section. Returns `true` if navigation may
+   * proceed immediately; returns `false` if there are unsaved changes — in that
+   * case `pendingSection` is set so a confirmation dialog can be shown.
+   */
+  requestNavigation: (section: SettingsSection) => boolean
+  /** Clears dirty + pending state and returns the section to navigate to. */
+  confirmNavigation: () => SettingsSection | null
+  /** Cancels a pending navigation without clearing dirty state. */
+  cancelNavigation: () => void
+  /** Resets all state — call on component unmount. */
+  reset: () => void
+}
+
+const initialState = {
+  isDirty: false,
+  pendingSection: null as SettingsSection | null,
+}
+
+export const useSettingsDirtyStore = create<SettingsDirtyStore>()(
+  devtools(
+    (set, get) => ({
+      ...initialState,
+
+      setDirty: (dirty) => set({ isDirty: dirty }),
+
+      requestNavigation: (section) => {
+        if (!get().isDirty) return true
+        set({ pendingSection: section })
+        return false
+      },
+
+      confirmNavigation: () => {
+        const { pendingSection } = get()
+        set({ ...initialState })
+        return pendingSection
+      },
+
+      cancelNavigation: () => set({ pendingSection: null }),
+
+      reset: () => set({ ...initialState }),
+    }),
+    { name: 'settings-dirty-store' }
+  )
+)
--- a/apps/sim/stores/settings/environment/index.ts
+++ b/apps/sim/stores/settings/environment/index.ts
@@ -1 +0,0 @@
-export type { CachedWorkspaceEnvData, EnvironmentState, EnvironmentVariable } from './types'
--- a/apps/sim/stores/settings/environment/types.ts
+++ b/apps/sim/stores/settings/environment/types.ts
@@ -1,17 +0,0 @@
-export interface EnvironmentVariable {
-  key: string
-  value: string
-}
-
-export interface CachedWorkspaceEnvData {
-  workspace: Record<string, string>
-  personal: Record<string, string>
-  conflicts: string[]
-  cachedAt: number
-}
-
-export interface EnvironmentState {
-  variables: Record<string, EnvironmentVariable>
-  isLoading: boolean
-  error: string | null
-}
--- a/apps/sim/tools/utils.ts
+++ b/apps/sim/tools/utils.ts
@@ -1,9 +1,9 @@
 import { createLogger } from '@sim/logger'
 import { getMaxExecutionTimeout } from '@/lib/core/execution-limits'
+import type { EnvironmentVariable } from '@/lib/environment/api'
 import { getQueryClient } from '@/app/_shell/providers/get-query-client'
 import type { CustomToolDefinition } from '@/hooks/queries/custom-tools'
 import { environmentKeys } from '@/hooks/queries/environment'
-import type { EnvironmentVariable } from '@/stores/settings/environment'
 import { tools } from '@/tools/registry'
 import type { ToolConfig } from '@/tools/types'
				`@@ -1 +0,0 @@`
				`export type { CachedWorkspaceEnvData, EnvironmentState, EnvironmentVariable } from './types'`