fix: secret leak when recovering #965
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several modifications across different files in the typegate and test directories. The changes include adding a new Changes
Sequence DiagramsequenceDiagram
participant Runtime as SubstantialRuntime
participant Agent as Agent
participant Workflow as Workflow
Runtime->>Agent: Initialize with config
Agent->>Agent: #selectReplayRequestsFor
Agent-->>Runtime: Filtered replay requests
Workflow->>Runtime: Start workflow
Runtime->>Workflow: Execute sayHello
Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
tests/sync/sync.py (1)
Line range hint
19-26: Security Risk: Public endpoint exposing secretsThe
helloendpoint is configured withpublicpolicy while using secrets ("ULTRA_SECRET"). This combination could potentially lead to secret leakage.Consider:
- Implementing appropriate access controls instead of using
publicpolicy- Reviewing if the secret is actually needed for this endpoint
- If the secret is required, implement proper secret rotation mechanisms
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.ghjk/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
src/typegate/src/runtimes/substantial.ts(1 hunks)src/typegate/src/runtimes/substantial/agent.ts(2 hunks)src/typegate/src/typegraph/mod.ts(2 hunks)src/typegate/src/utils.ts(1 hunks)tests/sync/scripts/workflow.ts(1 hunks)tests/sync/sync.py(2 hunks)tests/sync/sync_force_remove_test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
🔇 Additional comments (5)
tests/sync/sync.py (1)
12-16: Verify Redis connection securityThe new Redis backend is using "SUB_REDIS" secret. Ensure:
- The Redis connection uses TLS
- The connection string is properly sanitized
tests/sync/scripts/workflow.ts (1)
10-12: Security consideration: Input sanitizationThe
sayHellofunction directly interpolates user input into the output string. While this may be safe in the current context, ensure that:
- The output is properly escaped if used in HTML context
- Input validation is added if there are length or character restrictions
src/typegate/src/runtimes/substantial/agent.ts (2)
143-143: LGTM! Good refactoring.The extraction of filtering logic into a dedicated method improves code readability and maintainability.
167-184:⚠️ Potential issueEnhance error handling for bad runIds.
The current implementation logs bad runIds but continues execution without proper error handling. This could potentially lead to security issues if malformed runIds are not properly sanitized.
Consider these improvements:
#selectReplayRequestsFor(workflowName: string, runsInScope: Array<NextRun>) { const runsToDo = []; for (const run of runsInScope) { try { if (getTaskNameFromId(run.run_id) == workflowName) { runsToDo.push(run); } } catch(err) { - this.logger.warn(`Bad runId ${run.run_id}`); - this.logger.error(err); - - // TODO: - // Force remove? + this.logger.error(`Invalid runId format detected: ${run.run_id}`, err); + // Remove the invalid run from the backend to prevent potential security issues + await Meta.substantial.storeCloseSchedule({ + backend: this.backend, + queue: this.queue, + run_id: run.run_id, + schedule: run.schedule_date, + }); } } return runsToDo; }Let's verify the format of runIds in the codebase:
src/typegate/src/typegraph/mod.ts (1)
8-8: LGTM! Good security improvement.Using
deepCloneon runtime data prevents unintended mutations of the original data structure, which could lead to secret leaks. This change aligns well with the PR's objective of fixing secret leaks during recovery.Let's verify the usage of
deepClonein the codebase:Also applies to: 232-232
✅ Verification successful
Verified:
deepCloneis consistently used for sensitive data protectionThe usage of
deepCloneis properly implemented across the codebase for protecting sensitive data, including runtime data, operation results, and event arguments. This defensive programming approach effectively prevents unintended mutations of the original data structures.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of deepClone for sensitive data. # Test: Search for other instances where deepClone should be used ast-grep --pattern '$obj = $data' # Test: Search for potential data mutation patterns ast-grep --pattern '$obj.$prop = $data'Length of output: 77
Script:
#!/bin/bash # Find all imports of deepClone echo "=== deepClone imports ===" rg "import.*deepClone" -l # Find actual usage of deepClone echo -e "\n=== deepClone usage ===" rg "deepClone\(" -A 2 # Find potential direct assignments without cloning echo -e "\n=== Direct assignments ===" rg "const\s+\w+\s*=\s*(?!deepClone).*data" --type tsLength of output: 1294
Script:
#!/bin/bash # Find potential sensitive data assignments echo "=== Sensitive data assignments ===" rg "=.*data|=.*args|=.*kwargs|=.*runtime|=.*result" --type ts -A 1 # Check the utils implementation echo -e "\n=== deepClone implementation ===" rg -A 5 "export function deepClone"Length of output: 66099
Signed-off-by: michael-0acf4 <[email protected]>
Migration notes
None
Summary by CodeRabbit
Release Notes
New Features
sayHellofunctionImprovements
Testing