-
Notifications
You must be signed in to change notification settings - Fork 134
feat(examples): add user-generated-actors example #3560
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
feat(examples): add user-generated-actors example #3560
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
More templates
@rivetkit/cloudflare-workers
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
2d49631 to
9278cb3
Compare
9278cb3 to
a48277b
Compare
a48277b to
59757d7
Compare
b2e0ffb to
849b371
Compare
849b371 to
bc1efb9
Compare
59757d7 to
cc69285
Compare
cc69285 to
093ba9b
Compare
bc1efb9 to
870352b
Compare
093ba9b to
91028de
Compare
870352b to
96e8ddb
Compare
91028de to
eb81abd
Compare
PR Review: User-Generated Rivet Actors Freestyle DeployerThis PR replaces the freestyle example with a more comprehensive user-generated actors deployment example. Overall, the implementation is solid and demonstrates a good use case, but there are several security, reliability, and code quality concerns that should be addressed. Critical Issues1. Security: Arbitrary Code Execution (utils.ts:49, 52)The application writes user-provided code directly to files and executes it without validation or sandboxing. Users can inject malicious code that executes during the build process. Recommendations:
2. Secret Exposure in Logs (index.ts:48-53, deploy-with-rivet-cloud.ts:18)API tokens and credentials are logged directly to console. Recommendations:
3. Command Injection Risk (utils.ts:45)Using shell command with string interpolation: execSync with template literals. Recommendations:
Major Issues4. Error Handling: Incomplete Cleanup (utils.ts:55, 162)Temporary directories are created but never cleaned up on error. 5. Memory Issues: Large Buffers (utils.ts:87)50MB buffer might not be sufficient for large builds. 6. Race Conditions: Namespace Naming (utils.ts:201-203)Namespace generation uses timestamp + random string, but no collision detection. 7. API Design: Missing Validation (index.ts:16-67)No request validation before processing. 8. Reliability: No Deployment Status TrackingDeployments can take up to 10 minutes, but there's no way to track progress or resume. Code Quality Issues
Testing Concerns
Minor Issues
Positive Aspects
Recommendations SummaryBefore Merging (CRITICAL):
Post-Merge Improvements:
VerdictThis PR introduces valuable functionality but has critical security concerns that must be addressed before merging. The arbitrary code execution without sandboxing and credential exposure in logs are blockers. Once security issues are resolved, this will be a great addition to the examples directory. Recommendation: Request changes for security issues, then approve after fixes. Review by Claude Code |
Pull Request Review: User and AI Generated Rivet Actors ExampleOverviewThis PR renames and significantly refactors the Code Quality AssessmentStrengths ✅
Issues Found 🚨1. Security: Credentials Exposure Risk (HIGH)Location: The deployment endpoint accepts sensitive credentials via HTTP POST without authentication or rate limiting. This could allow unauthorized users to deploy code using someone else's credentials if exposed publicly. Recommendation: Add prominent warnings that this is for local development only. 2. Security: Arbitrary Code Execution (CRITICAL)Location: The Recommendations: Add prominent security warnings in the README about not exposing this publicly and document that this example is for development/learning only. 3. Error Handling: Missing Try-Catch in Async OperationsLocation: The 4. Potential Resource Leak: Temporary Directory CleanupLocation: The 5. CLAUDE.md Violation: Inconsistent Logging PatternAccording to CLAUDE.md, log messages should be lowercase unless mentioning specific code symbols. However:
6. Hardcoded ValuesMultiple locations use hardcoded default datacenter "us-west-1". Extract to a constant for consistency. 7. Race Condition RiskLocation: Using TestingIssues:
Recommendations:
Security Concerns Summary🔴 CRITICAL: Arbitrary code execution without validation Note: Since this is an example application meant for local development, these security concerns are acceptable IF properly documented with warnings. Positive Notes
VerdictThis PR makes meaningful improvements to the example and transforms it into a valuable educational resource. The code quality is generally good. The security concerns are acceptable for a local development example, but must be clearly documented with warnings. Recommendation: Approve with minor changes:
Review generated by Claude Code |
eb81abd to
21892b5
Compare
21892b5 to
c8b2675
Compare
Merge activity
|
PR Review: AI and User-Generated Actors Freestyle ExampleThis PR adds a comprehensive example demonstrating how to deploy user or AI-generated Rivet Actor code using a sandboxed namespace and Freestyle. Overall, this is a well-structured example with good separation of concerns. 🟢 Strengths
🔴 Critical Issues1. Security: Temp Directory Cleanup Missing (src/backend/utils.ts:40-48)Issue: Temporary directories are created but never cleaned up. This will leak disk space over time. Fix: Add cleanup in a try/finally block or use a library like tmp that auto-cleans and ensure cleanup is called after deployment completes/fails. 2. Security: Input Validation Missing (src/backend/index.ts:13-14)Issue: No validation of user-provided code or configuration. Accepts arbitrary TypeScript/JavaScript code and unvalidated URLs. Risks: Code injection, resource exhaustion, SSRF attacks Fix: Add input validation using zod or similar library with size limits and URL validation. 3. Security: Sensitive Data Exposure (deploy-with-rivet-cloud.ts:103-107)Issue: Tokens are returned in the API response which could be exposed if logged or stored client-side. Recommendation: Either document this clearly as an example-only pattern or don't return tokens in the response. 🟡 Performance & Reliability Issues4. Resource Management: No Timeout on pnpm install (src/backend/utils.ts:58)Issue: No timeout on dependency installation. A malicious or corrupted package could hang indefinitely. Fix: Add timeout parameter to execa call. 5. Error Recovery: Partial State on Failure (deploy-with-rivet-cloud.ts:28-62)Issue: If deployment fails after creating namespace/tokens, these resources are left orphaned. Fix: Wrap in try/catch and cleanup namespace on failure. 6. Concurrency: No Rate Limiting (src/backend/index.ts)Issue: No rate limiting on /api/deploy endpoint. Users could trigger multiple expensive deployments simultaneously. Fix: Add rate limiting middleware. 🔵 Code Quality Issues7. Code Style: Inconsistent String Quotes (src/backend/index.ts:48-49)Uses single quotes while codebase standard appears to be double quotes. 8. Missing CORS Configuration (src/backend/index.ts)No CORS middleware. API calls will fail if frontend served from different origin during development. 9. Hardcoded Values (src/backend/utils.ts:119, 133)Hardcoded datacenter and timeout values should be configurable. 10. Code Duplication (deploy-with-rivet-cloud.ts & deploy-with-rivet-self-hosted.ts)Similar code patterns between deployment functions. Consider extracting common logic. 🟣 Testing Issues11. All Tests Skipped (tests/deploy.test.ts:40, 79)Both integration tests are skipped. Add unit tests for generateNamespaceName format validation, input validation, and error handling paths. 📋 Documentation12. Missing Security Warnings (README.md)README should warn that this example executes arbitrary user-provided code and should not be exposed to untrusted users without additional sandboxing. Also note it creates billable resources. 🎯 Summary
Must Fix Before Merge:
Should Fix:
Nice to Have:
This is a solid foundation for an example, but needs the critical security and resource management issues addressed. The architecture is sound and the code is generally well-organized. |

No description provided.