-
Notifications
You must be signed in to change notification settings - Fork 0
fix: Phase 1 Maintenance and Staging Tenancy isolation #1033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2a78e9d
a965679
dd8936f
578e94a
30dc0db
dfaebf1
d284743
a6262ab
07a2c65
074c2ed
79d7db6
cb98094
229c779
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,22 +4,13 @@ import { spawn, spawnSync } from "node:child_process"; | |
| import { resolve, dirname } from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
|
|
||
| const projectRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); | ||
| const isWindows = process.platform === "win32"; | ||
| export const projectRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); | ||
| export const isWindows = process.platform === "win32"; | ||
| const [targetScript, ...forwardArgs] = process.argv.slice(2); | ||
| const offlineProviderRequested = forwardArgs.some( | ||
| (token, index) => token === "--provider-mode" && forwardArgs[index + 1] === "offline", | ||
| ); | ||
|
|
||
| if (!targetScript) { | ||
| console.error("Usage: node scripts/run-eval-safe.mjs <script> [args...]"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| function normalizeCommandLine(value) { | ||
| return value.toLowerCase().replaceAll("/", "\\"); | ||
| } | ||
|
|
||
| function toDateOrDefault(rawValue) { | ||
| if (!rawValue) return null; | ||
| const dateValue = Date.parse(rawValue); | ||
|
|
@@ -44,61 +35,7 @@ function toDateOrDefault(rawValue) { | |
| return null; | ||
| } | ||
|
|
||
| function shouldTerminateCandidate(commandLine) { | ||
| const normalizedCommandLine = normalizeCommandLine(commandLine); | ||
| const hasRepoScripts = normalizedCommandLine.includes("\\scripts\\"); | ||
|
|
||
| if (normalizedCommandLine.includes("\\run-eval-safe.mjs")) return false; | ||
|
|
||
| if (hasRepoScripts && normalizedCommandLine.includes("\\node_modules\\tsx\\dist\\cli.mjs")) { | ||
| if (normalizedCommandLine.includes("\\scripts\\eval-")) return true; | ||
| if (normalizedCommandLine.includes("\\scripts\\eval")) return true; | ||
| return false; | ||
| } | ||
|
|
||
| if (hasRepoScripts && normalizedCommandLine.includes("\\scripts\\run-tsx.mjs")) { | ||
| return normalizedCommandLine.includes("\\scripts\\eval"); | ||
| } | ||
|
|
||
| if ( | ||
| hasRepoScripts && | ||
| normalizedCommandLine.includes("\\node_modules\\tsx\\dist\\preflight.cjs") && | ||
| normalizedCommandLine.includes(" --import ") | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| if (normalizedCommandLine.includes("\\node_modules\\playwright")) { | ||
| return true; | ||
| } | ||
|
|
||
| if ( | ||
| normalizedCommandLine.includes("\\node_modules\\vitest\\vitest.mjs") && | ||
| normalizedCommandLine.includes("\\tests\\") | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| if (normalizedCommandLine.includes("\\node_modules\\next\\dist\\bin\\next")) { | ||
| return true; | ||
| } | ||
|
|
||
| if (normalizedCommandLine.includes("\\node_modules\\next\\dist\\server\\lib\\start-server.js")) { | ||
| return true; | ||
| } | ||
|
|
||
| if (normalizedCommandLine.includes("\\node_modules\\next\\dist\\compiled\\jest-worker\\processchild.js")) { | ||
| return true; | ||
| } | ||
|
|
||
| if (normalizedCommandLine.includes("\\.next\\build\\") || normalizedCommandLine.includes("\\.next\\dev\\build\\")) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| function listRepoNodeProcesses() { | ||
| export function listRepoNodeProcesses() { | ||
| if (!isWindows) return []; | ||
|
|
||
| const command = [ | ||
|
|
@@ -142,16 +79,7 @@ function listRepoNodeProcesses() { | |
| } | ||
| } | ||
|
|
||
| function listCandidateProcesses() { | ||
| if (!isWindows) return []; | ||
|
|
||
| return listRepoNodeProcesses().filter((candidate) => { | ||
| if (!candidate?.commandLine) return false; | ||
| return shouldTerminateCandidate(candidate.commandLine); | ||
| }); | ||
| } | ||
|
|
||
| function getDescendantPids(rootPid, allProcesses = listRepoNodeProcesses()) { | ||
| export function getDescendantPids(rootPid, allProcesses = listRepoNodeProcesses()) { | ||
| const visited = new Set([rootPid]); | ||
| const queue = [rootPid]; | ||
|
|
||
|
|
@@ -168,7 +96,7 @@ function getDescendantPids(rootPid, allProcesses = listRepoNodeProcesses()) { | |
| return Array.from(visited); | ||
| } | ||
|
|
||
| function terminateProcesses(pids, context) { | ||
| export function terminateProcesses(pids, context) { | ||
| if (!isWindows) return 0; | ||
|
|
||
| let killed = 0; | ||
|
|
@@ -190,29 +118,23 @@ function terminateProcesses(pids, context) { | |
| return killed; | ||
| } | ||
|
|
||
| function cleanupResidualEvaluationProcesses() { | ||
| if (!isWindows) return; | ||
|
|
||
| const selfPid = process.pid; | ||
| const candidates = listCandidateProcesses().filter((candidate) => candidate.pid > 0 && candidate.pid !== selfPid); | ||
| if (candidates.length === 0) return; | ||
| const pids = candidates.map((candidate) => candidate.pid); | ||
| terminateProcesses(pids, "cleanupResidualEvaluationProcesses"); | ||
| } | ||
|
|
||
| function terminateEvalProcessTree(pid) { | ||
| if (!isWindows || !pid || pid <= 0) return; | ||
| const processSnapshot = listRepoNodeProcesses(); | ||
| export function terminateOwnedProcessTree(pid, processSnapshot = listRepoNodeProcesses()) { | ||
| if (!isWindows || !pid || pid <= 0) return 0; | ||
| const descendants = getDescendantPids(pid, processSnapshot); | ||
| terminateProcesses(descendants, "terminateEvalProcessTree"); | ||
| return terminateProcesses(descendants, "terminateOwnedProcessTree"); | ||
| } | ||
|
Comment on lines
+121
to
125
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift Prevent stale-PID termination.
🤖 Prompt for AI Agents |
||
|
|
||
| function terminateEvalProcess(pid) { | ||
| if (!pid || pid <= 0) return; | ||
| terminateEvalProcessTree(pid); | ||
| terminateOwnedProcessTree(pid); | ||
| } | ||
|
|
||
| function runEvalScript() { | ||
| if (!targetScript) { | ||
| console.error("Usage: node scripts/run-eval-safe.mjs <script> [args...]"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const targetPath = resolve(projectRoot, targetScript); | ||
| const command = process.execPath; | ||
| const commandArgs = [resolve(projectRoot, "scripts", "run-tsx.mjs"), targetPath, ...forwardArgs]; | ||
|
|
@@ -226,8 +148,6 @@ function runEvalScript() { | |
| ? { | ||
| ...process.env, | ||
| RAG_PROVIDER_MODE: "offline", | ||
| // Empty values prevent @next/env from restoring credentials from an | ||
| // env file before eval-quality actively deletes them in the child. | ||
| OPENAI_API_KEY: "", | ||
| OPENAI_ORG_ID: "", | ||
| OPENAI_PROJECT_ID: "", | ||
|
|
@@ -275,5 +195,7 @@ function runEvalScript() { | |
| }); | ||
| } | ||
|
|
||
| cleanupResidualEvaluationProcesses(); | ||
| runEvalScript(); | ||
| const isMain = process.argv[1] && fileURLToPath(import.meta.url) === resolve(process.argv[1]); | ||
| if (isMain) { | ||
| runEvalScript(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -619,6 +619,17 @@ async function main() { | |
| auth: { persistSession: false, autoRefreshToken: false, detectSessionInUrl: false }, | ||
| global: { fetch: crossTenantStagingFetch }, | ||
| }); | ||
|
|
||
| // Ensure test users have administrator privileges for mutation routes | ||
| await admin.auth.admin.updateUserById(sessionA.userId, { app_metadata: { site_role: "administrator" } }); | ||
| await admin.auth.admin.updateUserById(sessionB.userId, { app_metadata: { site_role: "administrator" } }); | ||
|
Comment on lines
+624
to
+625
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n## File excerpt\n'
sed -n '600,660p' scripts/test-cross-tenant-staging.ts
printf '\n## Related helper\n'
sed -n '1,220p' scripts/set-site-administrator.ts
printf '\n## Supabase auth-admin type usage in repo\n'
rg -n "updateUserById\\(" -S .
printf '\n## Supabase client package version / types if present\n'
node - <<'NODE'
const fs = require('fs');
for (const p of ['package.json','package-lock.json','pnpm-lock.yaml','yarn.lock']) {
if (fs.existsSync(p)) {
console.log(`FOUND ${p}`);
if (p === 'package.json') {
const pkg = JSON.parse(fs.readFileSync(p,'utf8'));
console.log('supabase-js dep:', pkg.dependencies?.['`@supabase/supabase-js`'] || pkg.devDependencies?.['`@supabase/supabase-js`'] || 'not found');
}
break;
}
}
NODERepository: BigSimmo/Database Length of output: 5512 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n## Relevant lines in scripts/test-cross-tenant-staging.ts\n'
nl -ba scripts/test-cross-tenant-staging.ts | sed -n '620,650p'
printf '\n## Relevant lines in scripts/set-site-administrator.ts\n'
nl -ba scripts/set-site-administrator.ts | sed -n '1,220p'Repository: BigSimmo/Database Length of output: 254 🌐 Web query:
💡 Result: The supabase.auth.admin.updateUserById method employs shallow merge semantics when updating the app_metadata object [1]. When you provide a new app_metadata object, Supabase merges it with the existing metadata rather than replacing the entire object [1]. Specifically, the merge behavior functions as follows: - Existing keys not included in your update are preserved [1]. - Keys provided in the update overwrite their existing values [1]. - Keys explicitly set to null (or nil) in your update are removed from the object [1]. This means if you want to set a site_role, you can safely update just that field without clearing other existing data in app_metadata [1]. Example of updating the site_role to administrator: // Server-side (Edge Function or Node.js) const { data, error } = await supabase.auth.admin.updateUserById( 'user-uuid', { app_metadata: { site_role: 'administrator' } }); Important implementation notes: - Admin API Only: This method must be called from a secure, server-side context (e.g., Edge Functions, backend server) using the service_role key [2][3][4]. Never expose the service_role key in client-side code [4]. - Client Sync: Updates made via the admin API do not automatically notify or update the user's active session on the client-side [5]. To reflect these changes in the UI or trigger onAuthStateChange listeners, you must call supabase.auth.refreshSession on the client after the admin update is complete [2][5]. Citations:
🌐 Web query:
💡 Result: When using the supabase-js library to update a user's app_metadata via updateUserById, the operation performs a merge by default, not a full replacement [1][2]. In Supabase, app_metadata is stored as a JSONB column [2]. When you provide a partial object to the app_metadata field in an update call, the new values are merged into the existing JSON object [1][3]. For example, if you call the update with { app_metadata: { plan: 'trial' } }, and the user already has { role: 'admin' }, the resulting app_metadata will contain both fields: { plan: 'trial', role: 'admin' }. To effectively "replace" the existing metadata with a new set of values, you must retrieve the current user's metadata first, construct the desired final object, and then perform the update with the complete object. Note that updateUserById must be called from a secure, server-side environment (such as an Edge Function or backend server) using your project's service_role key, as it provides administrative privileges that should never be exposed in client-side code [1][3]. Citations:
Handle admin mutation errors before refreshing sessions. The two 🤖 Prompt for AI Agents |
||
|
|
||
| // Refresh sessions to get the new role claim in the JWT | ||
| const sessionARefreshed = await signIn(clientA, config.userAEmail, config.userAPassword, "User A"); | ||
| const sessionBRefreshed = await signIn(clientB, config.userBEmail, config.userBPassword, "User B"); | ||
| sessionA.token = sessionARefreshed.token; | ||
| sessionB.token = sessionBRefreshed.token; | ||
|
|
||
| const fixtureA = await createFixture(admin, config, runId, sessionA.userId, "a", (fixture) => | ||
| fixtures.push(fixture), | ||
| ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Correct the contradictory drift-verification claim.
This row says the drift manifest was verified byte-identically, but also records
check:driftas skipped;package.json:176wires that script to the relevant checker. Either remove the verification claim or document the independent comparison and complete hashes.🤖 Prompt for AI Agents