Skip to content

🔒 [security fix] Replace insecure Math.random() with crypto.randomUUID() for Participant ID #10

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.

Sign up for GitHub

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

Closed
hashexplaindata wants to merge 1 commit into from
Closed

🔒 [security fix] Replace insecure Math.random() with crypto.randomUUID() for Participant ID #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions code/experiment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const CFG = Object.freeze({

// --- State Machine ---
const STATE = {
pid: Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15),
pid: self.crypto.randomUUID(),

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Guard UUID generation for unsupported contexts

Replace the unconditional self.crypto.randomUUID() call with a guarded/fallback path, because randomUUID is only available in secure contexts and newer browser builds; when it is missing, this line throws during script evaluation and the experiment never initializes. In HTTP deployments that are not localhost (or on older clients), this causes a full data-collection outage rather than just weaker randomness.

Useful? React with 👍 / 👎.

condition: CFG.CONDITION,
Comment on lines 18 to 20

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

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

self.crypto.randomUUID() will throw in environments where crypto or randomUUID is unavailable (older browsers / non-secure contexts), which would break initialization since it runs at script load. Consider feature-detecting and providing a fallback (e.g., UUID v4 via crypto.getRandomValues) or a clear user-facing error before continuing.

Copilot uses AI. Check for mistakes.
covariate: 0,
currentTrial: 0,
Expand Down Expand Up @@ -349,7 +349,6 @@ function init() {
executeBatchPayload();
});

console.log(`Diagnostic Engine Initialized. PID: ${STATE.pid} | Condition: ${STATE.condition}`);
}

function loadNextTrial() {
Expand Down Expand Up @@ -469,11 +468,9 @@ async function executeBatchPayload() {
await batch.commit();
onSyncSuccess();
} else {
console.warn("Firebase not detected. Payload logged to console:", STATE.results);
setTimeout(onSyncSuccess, 1500); // Simulate sync delay
}
} catch (error) {
console.error("Critical Sync Failure:", error);
DOM.syncStatus.innerHTML = `<span style="color:#ff453a">⚠️ Sync Failed. Error: ${error.code || 'Network'}</span>`;
// Potential fallback: Save to localStorage for later recovery
}
Comment on lines 473 to 476

@gemini-code-assist gemini-code-assist Bot Mar 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

While the goal of silent client-side execution is understood, completely removing error logging for a critical operation like data synchronization can make debugging production issues extremely difficult. The full error object is lost, and only a generic message is shown to the user. Consider using a conditional logging approach that is disabled in production builds, or sending this critical error to a monitoring service. If a full logging/monitoring solution is out of scope, at a minimum, the error details should be stored along with the data for the fallback recovery mechanism mentioned on line 475.

Expand Down
97 changes: 97 additions & 0 deletions telemetry_verification/verify_pid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
from playwright.sync_api import sync_playwright
import time
import subprocess
import os
import re

def run_verification():
# Start the server
port = 8081
server_process = subprocess.Popen(["python3", "-m", "http.server", str(port)])

@gemini-code-assist gemini-code-assist Bot Mar 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This line makes two assumptions that can reduce the script's portability:

  1. The script is run from the repository root. If run from its own directory (telemetry_verification/), the server will fail to find code/index.html.
  2. The Python 3 executable is named python3 and is in the system's PATH.

A more robust approach is to explicitly set the server's working directory and use sys.executable.

import os
import sys
# ...
repo_root = os.path.abspath(os.path.join(os.path.dirname(__file__), '..'))
server_process = subprocess.Popen(
    [sys.executable, "-m", "http.server", str(port)],
    cwd=repo_root
)

Comment on lines +6 to +10

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

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

Hard-coding python3 makes this script non-portable (e.g., Windows, venvs, environments where only python exists). Use sys.executable to launch the server with the same interpreter that runs this script.

Suggested change
def run_verification():
# Start the server
port = 8081
server_process = subprocess.Popen(["python3", "-m", "http.server", str(port)])
import sys
def run_verification():
# Start the server
port = 8081
server_process = subprocess.Popen([sys.executable, "-m", "http.server", str(port)])

Copilot uses AI. Check for mistakes.

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Start the verification server from the project root

The verification script serves whatever the current working directory is, but always navigates to /code/index.html; if the script is run from telemetry_verification/ (a common invocation pattern), the HTTP server cannot resolve that path and the check fails with a 404 before any PID assertions run. Derive and set cwd from __file__ so the script is location-independent.

Useful? React with 👍 / 👎.

time.sleep(5) # Wait for server to start

Comment on lines +6 to +12

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

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

Using a fixed port (8081) can fail if the port is already in use, causing the http.server process to exit immediately and the verification to time out later. Prefer selecting an available ephemeral port (bind a socket to port 0) or making the port configurable and verifying the server started successfully before continuing.

Suggested change
def run_verification():
# Start the server
port = 8081
server_process = subprocess.Popen(["python3", "-m", "http.server", str(port)])
time.sleep(5) # Wait for server to start
import socket
import urllib.request
import urllib.error
def run_verification():
# Start the server
# Select an available ephemeral port to avoid conflicts
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as temp_socket:
temp_socket.bind(("", 0))
port = temp_socket.getsockname()[1]
server_process = subprocess.Popen(["python3", "-m", "http.server", str(port)])
# Wait for server to start by polling instead of a fixed sleep
server_started = False
for _ in range(50): # Up to ~5 seconds (50 * 0.1s)
try:
with urllib.request.urlopen(f"http://localhost:{port}", timeout=0.5):
server_started = True
break
except (urllib.error.URLError, ConnectionRefusedError, TimeoutError):
time.sleep(0.1)
if not server_started:
server_process.terminate()
raise RuntimeError(f"Failed to start HTTP server on port {port}")

Copilot uses AI. Check for mistakes.
try:
with sync_playwright() as p:
browser = p.chromium.launch(headless=True)
page = browser.new_page()

Comment on lines +14 to +17

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

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

The Playwright browser instance is never explicitly closed. If an exception occurs before exiting the sync_playwright() context, this can leave stray processes around and make repeated runs flaky. Close the browser in a finally (or use a context manager for the browser/page) before terminating the server.

Copilot uses AI. Check for mistakes.
# Listen for console messages
page.on("console", lambda msg: print(f"PAGE CONSOLE: {msg.text}"))

# Navigate to the experiment
print(f"Navigating to http://localhost:{port}/code/index.html")
page.goto(f"http://localhost:{port}/code/index.html")

# 1. Capture initial PID
print("Waiting for STATE.pid...")
# Instead of wait_for_function which seems to hang if there are load issues,
# let's try a retry loop in evaluate
pid = None
for _ in range(30):
try:
pid = page.evaluate("typeof STATE !== 'undefined' ? STATE.pid : null")
if pid:
break
except:

@gemini-code-assist gemini-code-assist Bot Mar 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using a bare except: is discouraged as it catches all exceptions, including system-level ones like KeyboardInterrupt, which can hide bugs and make debugging difficult. It's better to catch a more specific exception, like Exception, to avoid swallowing signals. For Playwright operations, catching playwright.sync_api.Error would be even more precise (after importing it).

Suggested change
except:
except Exception:

pass
time.sleep(1)
Comment on lines +31 to +37

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

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

The retry loop swallows all exceptions with a bare except: pass, which can hide real failures (e.g., navigation errors, evaluation syntax errors) and make debugging flaky runs difficult. Catch the specific Playwright error(s) you expect here and surface unexpected exceptions (or at least log them once) so failures are actionable.

Copilot uses AI. Check for mistakes.

print(f"Initial PID: {pid}")
if not pid:
# Fallback: check if script is even there
scripts = page.evaluate("Array.from(document.scripts).map(s => s.src)")
print(f"Loaded scripts: {scripts}")
raise Exception("PID not found in STATE after timeout")

# 2. Click Consent
page.wait_for_selector("#btn-consent")
page.click("#btn-consent")
print("Clicked consent")

# 3. Select Familiarity
page.wait_for_selector(".btn-familiarity")
page.click(".btn-familiarity")
print("Selected familiarity")

# 4. Complete 6 trials
for i in range(6):
print(f"Trial {i+1}")
page.wait_for_selector(".bento-choice-card")
page.click(".bento-choice-card")
time.sleep(0.5) # Transition time

@gemini-code-assist gemini-code-assist Bot Mar 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

This fixed sleep is likely redundant because the next iteration of the loop calls page.wait_for_selector('.bento-choice-card'), which already waits for the next trial to be rendered. Using explicit waits over fixed sleeps makes tests faster and more reliable. Consider removing this line to improve test speed and robustness.


# 5. Justification
page.wait_for_selector("#semantic-justification")
page.fill("#semantic-justification", "This is a security verification test justification.")
print("Filled justification")

# 6. Finalize
page.wait_for_function("!document.getElementById('btn-finalize').disabled")
page.click("#btn-finalize")
print("Clicked finalize")

# 7. Final Screen
page.wait_for_selector("#display-pid", timeout=10000)
displayed_pid = page.inner_text("#display-pid")
print(f"Displayed PID: {displayed_pid}")

if pid != displayed_pid:
raise Exception(f"PID mismatch! Initial: {pid}, Displayed: {displayed_pid}")

# Check UUID format
uuid_regex = r"^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$"
if not re.match(uuid_regex, pid, re.IGNORECASE):
raise Exception(f"PID {pid} is not a valid UUID v4")
print("Verified PID is a valid UUID v4")

# Take a screenshot
screenshot_dir = os.path.join(os.path.dirname(__file__), "screenshots")
os.makedirs(screenshot_dir, exist_ok=True)
page.screenshot(path=os.path.join(screenshot_dir, "verification.png"))
print("Verification successful, screenshot saved.")

finally:
server_process.terminate()

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

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

server_process.terminate() is issued but the process is not waited on or force-killed if it doesn't exit, which can leave the port occupied for subsequent runs. Consider calling wait() with a timeout and falling back to kill() if needed.

Suggested change
server_process.terminate()
try:
# Ensure the server process is properly terminated so the port is freed
if server_process is not None and server_process.poll() is None:
server_process.terminate()
try:
server_process.wait(timeout=5)
except subprocess.TimeoutExpired:
server_process.kill()
# Final wait without timeout; process should now exit promptly
server_process.wait()
except Exception:
# Swallow cleanup errors to avoid masking original exceptions
pass

Copilot uses AI. Check for mistakes.

if __name__ == "__main__":
run_verification()
Loading