🔒 [security fix] Replace insecure Math.random() with crypto.randomUUID() for Participant ID - #10
hashexplaindata wants to merge 1 commit into
Conversation
This commit addresses a security vulnerability where participant IDs were generated using the predictable Math.random() function. Changes: - Replaced insecure PID generation with self.crypto.randomUUID() in code/experiment.js. - Removed all console.log, console.warn, and console.error statements from code/experiment.js to ensure silent client-side execution as per repository directives. - Added an automated Playwright verification script in telemetry_verification/verify_pid.py to validate UUID format and end-to-end experiment flow. The fix ensures cryptographic unpredictability for session identifiers while maintaining the strict behavioral and privacy requirements of the telemetry engine. Co-authored-by: hashexplaindata <221828969+hashexplaindata@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a security vulnerability by upgrading the participant ID generation to use cryptographically secure UUIDs, moving away from the predictable Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully addresses a security vulnerability by replacing the predictable Math.random() with the cryptographically secure self.crypto.randomUUID() for participant ID generation. The removal of console logs aligns with the goal of silent client-side execution. The addition of a Playwright verification script is a great step towards ensuring the fix is effective and preventing regressions.
My review includes a few suggestions to improve the robustness and maintainability of the code. I've pointed out a concern with removing error logging for critical operations in experiment.js. For the new verification script, I've suggested improvements to make it more portable and reliable by avoiding hardcoded paths/executables, brittle sleeps, and overly broad exception handling.
| } 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 | ||
| } |
There was a problem hiding this comment.
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.
| def run_verification(): | ||
| # Start the server | ||
| port = 8081 | ||
| server_process = subprocess.Popen(["python3", "-m", "http.server", str(port)]) |
There was a problem hiding this comment.
This line makes two assumptions that can reduce the script's portability:
- The script is run from the repository root. If run from its own directory (
telemetry_verification/), the server will fail to findcode/index.html. - The Python 3 executable is named
python3and 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
)| pid = page.evaluate("typeof STATE !== 'undefined' ? STATE.pid : null") | ||
| if pid: | ||
| break | ||
| except: |
There was a problem hiding this comment.
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).
| except: | |
| except Exception: |
| print(f"Trial {i+1}") | ||
| page.wait_for_selector(".bento-choice-card") | ||
| page.click(".bento-choice-card") | ||
| time.sleep(0.5) # Transition time |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pull request overview
This PR hardens participant ID generation by replacing the predictable Math.random()-based ID with a cryptographically secure UUID, and adds a Playwright-based verification script intended to exercise a full participant session and confirm PID stability/format.
Changes:
- Replace
STATE.pidgeneration withself.crypto.randomUUID()in the experiment runtime. - Remove
console.*calls fromcode/experiment.jsto keep client-side execution silent. - Add
telemetry_verification/verify_pid.pyto automate an end-to-end run and validate PID persistence + UUIDv4 format.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
code/experiment.js |
Switches participant ID generation to Web Crypto UUIDs and removes console logging during init/sync paths. |
telemetry_verification/verify_pid.py |
Adds a Playwright-driven E2E verification that runs the experiment, captures PID, and validates UUIDv4 + display consistency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
|
||
| def run_verification(): | ||
| # Start the server | ||
| port = 8081 | ||
| server_process = subprocess.Popen(["python3", "-m", "http.server", str(port)]) |
There was a problem hiding this comment.
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.
| 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)]) |
|
|
||
| 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 | ||
|
|
There was a problem hiding this comment.
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.
| 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}") |
| try: | ||
| pid = page.evaluate("typeof STATE !== 'undefined' ? STATE.pid : null") | ||
| if pid: | ||
| break | ||
| except: | ||
| pass | ||
| time.sleep(1) |
There was a problem hiding this comment.
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.
| with sync_playwright() as p: | ||
| browser = p.chromium.launch(headless=True) | ||
| page = browser.new_page() | ||
|
|
There was a problem hiding this comment.
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.
| print("Verification successful, screenshot saved.") | ||
|
|
||
| finally: | ||
| server_process.terminate() |
There was a problem hiding this comment.
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.
| 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 |
| const STATE = { | ||
| pid: Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15), | ||
| pid: self.crypto.randomUUID(), | ||
| condition: CFG.CONDITION, |
There was a problem hiding this comment.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e59810df0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // --- State Machine --- | ||
| const STATE = { | ||
| pid: Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15), | ||
| pid: self.crypto.randomUUID(), |
There was a problem hiding this comment.
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 👍 / 👎.
| def run_verification(): | ||
| # Start the server | ||
| port = 8081 | ||
| server_process = subprocess.Popen(["python3", "-m", "http.server", str(port)]) |
There was a problem hiding this comment.
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 👍 / 👎.
|
Closing: Fixes have already been incorporated directly into the master branch. |
Understood. Acknowledging that this work is now obsolete as the fixes have been incorporated into the master branch, and I am stopping work on this task. |
🎯 What
Fixed a security vulnerability in the participant ID generation logic and ensured silent application execution.
The previous implementation used
Math.random(), which produces predictable values. This could theoretically lead to participant ID collisions in large-scale studies or allow for predictability in session identifiers, compromising the integrity of the anonymous telemetry.🛡️ Solution
STATE.pidgeneration to the Web Crypto API (self.crypto.randomUUID()), providing cryptographically secure UUID v4 identifiers.consolemethod calls fromcode/experiment.jsto align with the repository's directive for silent client-side execution.telemetry_verification/that simulates a full participant session to ensure the fix is effective and doesn't introduce regressions in the experiment flow.PR created automatically by Jules for task 17192357271034769754 started by @hashexplaindata