Skip to content
This repository was archived by the owner on Oct 22, 2025. It is now read-only.

fix(examples-*): fix cors configs#1393

Closed
NathanFlurry wants to merge 1 commit into10-15-chore_core_update_createrivetkit_to_accept_a_client_optionsfrom
10-15-fix_examples-_fix_cors_configs
Closed

fix(examples-*): fix cors configs#1393
NathanFlurry wants to merge 1 commit into10-15-chore_core_update_createrivetkit_to_accept_a_client_optionsfrom
10-15-fix_examples-_fix_cors_configs

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 15, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Oct 15, 2025 9:56am

@claude
Copy link

claude bot commented Oct 15, 2025

Claude finished @NathanFlurry's task —— View job


Code Review: CORS Configuration Updates

Reviewing the CORS configuration changes across 14 example projects.

Todo List:

  • Analyze changed files to understand CORS configuration pattern
  • Review CORS settings for security implications
  • Check consistency across all examples
  • Identify potential bugs or issues
  • Assess adherence to project best practices
  • Provide comprehensive feedback

Review Findings

Positive Changes

  • Security Enhancement: Adding credentials: true enables proper authentication flow with cookies/sessions
  • Consistency: Standardizes CORS configuration across examples
  • Missing Configuration: Adds CORS to examples that previously lacked it

⚠️ Issues Identified

1. Port Inconsistency (Medium Priority)

The origin ports are inconsistent across examples:

  • Port 3000: ai-agent, crdt, database, game, rate, stream, sync, tenant
  • Port 5173: better-auth-external-db, hono-bun, hono-react, raw-fetch-handler, raw-websocket-handler-proxy, react

Root Cause: Examples use different frontend frameworks with different default ports:

  • ai-agent/vite.config.ts:8 explicitly sets port 3000
  • react/vite.config.ts uses Vite's default (5173)

Recommendation: Standardize on one port (suggest 5173 as Vite default) or document the port requirements clearly in each example's README.

2. Security Considerations (Low Priority)

  • Localhost-only origins: Current configuration is appropriate for development examples
  • Credentials enabled: Necessary for authentication examples but should be documented
  • No wildcard origins: Good security practice maintained

3. Redundant CORS Configuration (Low Priority)

In examples/raw-fetch-handler/src/backend/server.ts:17-22 and examples/better-auth-external-db/src/backend/server.ts:19-30, CORS is configured both in registry.start() and in the Hono middleware. This creates redundancy but doesn't cause functional issues.

📋 Recommendations

  1. Fix Port Consistency: Update all examples to use consistent origin ports (recommend 5173)
  2. Update Documentation: Add comments explaining why credentials: true is needed
  3. Consider Environment Variables: For production-ready examples, use environment variables for origins
  4. Verify Frontend Ports: Ensure each example's frontend actually runs on the specified port

🔒 Security Assessment

  • ✅ No security vulnerabilities introduced
  • ✅ Localhost-only origins appropriate for development
  • ✅ No wildcard CORS origins
  • ✅ Credentials properly scoped to specific origins

🧪 Test Coverage

No test files were modified in this PR. Consider adding integration tests that verify CORS headers are correctly set.

📊 Overall Assessment

This is a solid improvement that standardizes CORS configuration across examples. The main issue is port inconsistency, which should be addressed for better developer experience.

Recommendation: ✅ Approve with minor fixes - Address port consistency before merge.


Copy link
Member Author

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.
Learn more


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

@graphite-app
Copy link

graphite-app bot commented Oct 15, 2025

Merge activity

  • Oct 15, 9:51 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Oct 15, 9:52 AM UTC: CI is running for this pull request on a draft pull request (#1394) due to your merge queue CI optimization settings.
  • Oct 15, 9:53 AM UTC: Merged by the Graphite merge queue via draft PR: #1394.

graphite-app bot pushed a commit that referenced this pull request Oct 15, 2025
@graphite-app graphite-app bot closed this Oct 15, 2025
@graphite-app graphite-app bot deleted the 10-15-fix_examples-_fix_cors_configs branch October 15, 2025 09:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant