Add CORS to the connectRPC server #146
Conversation
WalkthroughThe changes in this pull request focus on modifying the serialization of node relationships in the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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: 2
🧹 Outside diff range and nitpick comments (4)
go.mod (1)
Line range hint
3-3: Fix invalid Go versionThe Go version
1.23.1is invalid as Go follows semantic versioning. The latest stable version is 1.22.1.Apply this diff to fix the Go version:
-go 1.23.1 +go 1.22.1api/v1/service.proto (1)
25-26: Breaking change: Field type modifications require careful migrationThe change from
stringtorepeated uint32for dependencies and dependents is a good improvement that:
- Provides better type safety and validation
- Improves efficiency by avoiding base64 encoding/decoding
- Makes the API more developer-friendly with direct numeric IDs
However, this is a breaking change that requires careful handling.
Consider the following recommendations:
- Version the API appropriately (e.g., bump major version)
- Document the migration steps for existing clients
- Consider supporting both formats temporarily through a migration period
Would you like me to help draft the migration guide or version bump strategy?
cmd/server/server.go (1)
14-14: Consider consolidating CORS packagesThe code imports both
connectrpc.com/corsandgithub.com/rs/cors. Whileconnectrpc.com/corsprovides Connect-specific CORS configurations, the implementation only uses its constants with thegithub.com/rs/corsmiddleware. Consider if both packages are necessary, or if the Connect-specific CORS package alone could handle all requirements.Also applies to: 19-19
api/v1/service.go (1)
34-35: Consider performance implications of array vs base64.While the change to array format is more straightforward, consider the following performance aspects:
- Memory usage: Arrays of uint32 vs base64 strings
- Network bandwidth: Raw integers vs encoded strings
- Serialization/deserialization overhead
Consider benchmarking both approaches if performance is critical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
gen/api/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
api/v1/service.go(1 hunks)api/v1/service.proto(1 hunks)cmd/server/server.go(3 hunks)go.mod(2 hunks)
🔇 Additional comments (4)
go.mod (1)
7-7: LGTM on CORS dependencies!
The added CORS dependencies look good:
connectrpc.com/corsv0.1.0 is the official CORS package for connectRPCgithub.com/rs/corsv1.11.1 is correctly marked as indirect
Also applies to: 51-51
api/v1/service.proto (1)
25-26: Verify handling of dependencies/dependents across the codebase
Let's ensure all consumers of this API are ready for the new array format.
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information to verify the impact of the changes:
No impact on consumers - array field type is already in use
The codebase is already using the array field type (repeated uint32) for dependencies and dependents. The search results show:
- The Node message fields are accessed as arrays in Go code (e.g.,
deps.ToArray(),dependencies.ToArray()) - No string-to-base64 conversions are used for these fields except in tests
- The field access patterns in packages like
pkg/graph,pkg/toolsall handle these fields as integer arrays
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find potential areas needing updates due to the Node message changes
# Find proto imports that might be using the Node message
echo "=== Finding potential Node message consumers ==="
rg -l "import.*api/v1/service\.proto"
# Find direct references to dependencies/dependents fields
echo -e "\n=== Finding field access patterns ==="
rg -A 2 "dependencies|dependents" --type go
# Look for potential string-to-base64 conversions that might need updating
echo -e "\n=== Finding potential base64 conversions ==="
rg "ToBase64|FromBase64" --type go
Length of output: 24342
cmd/server/server.go (1)
185-187: Verify CORS headers with the new array response format
Since the API is changing from base64 to array format for dependencies, ensure that CORS headers properly support the new response format and any metadata headers.
✅ Verification successful
CORS configuration is properly set up for the new array response format
The CORS configuration is using the standard Connect framework's CORS settings through connectcors package, which automatically handles the necessary headers for both binary and JSON responses. The service definitions show that:
- The server uses standard Connect/gRPC responses with proper content negotiation
- All endpoints support both binary Protobuf and JSON codecs by default
- Compression (gzip) is enabled by default for all responses
The change from base64 to array format is handled at the protocol level and doesn't require any special CORS configuration since it's part of the standard response payload.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CORS headers configuration matches API requirements
# Check if any custom headers are used in the API responses
echo "Checking for custom headers in API handlers..."
rg -n "Header\.(Set|Add)" --type go
# Check if the Connect service definitions include any custom metadata
echo "Checking for response metadata in service definitions..."
rg -n "rpc.*returns" --type proto
Length of output: 329
Script:
#!/bin/bash
# Let's verify CORS configuration and response handling
# Check CORS configuration
echo "Checking CORS configuration..."
rg -n "AllowedHeaders|ExposedHeaders|AllowedMethods" --type go -A 2 -B 2
# Check response handling in the codebase
echo "Checking response handling..."
rg -n "json:|proto:" --type go
# Look for any custom middleware or response wrappers
echo "Checking middleware and response wrappers..."
ast-grep --pattern 'func $_($_) http.Handler {
$$$
}'
Length of output: 11630
Script:
#!/bin/bash
# Let's check the Connect CORS configuration and response types
# Check the connectcors package implementation
echo "Checking connectcors package..."
rg -n "func (AllowedMethods|AllowedHeaders|ExposedHeaders)" --type go -A 5
# Check Connect service definitions for response types
echo "Checking Connect service definitions..."
fd -e go -x grep -l "Connect" {} \; | xargs rg "type.*Service.*interface" -A 10
# Check for any response transformations
echo "Checking response transformations..."
ast-grep --pattern 'func $_($_ *connect.Response) {
$$$
}'
Length of output: 14645
api/v1/service.go (1)
34-35: Verify API compatibility for dependency format change.
The switch from ToBase64() to ToArray() for Dependencies and Dependents aligns with the PR objectives. However, this change requires careful consideration:
- This is a breaking change for API consumers expecting base64 encoded strings
- Need to ensure the proto file changes (
repeated uint32) are deployed simultaneously - Consider documenting the new format in API documentation
Let's verify the proto file changes and any existing consumers:
✅ Verification successful
Based on the verification results, I can now provide a final response:
Change to array format is compatible with proto definition
The switch from ToBase64() to ToArray() for Dependencies and Dependents is aligned with the proto definition which uses repeated uint32 fields. The proto file shows:
message Node {
uint32 id = 1;
string name = 2;
string type = 3;
repeated uint32 dependencies = 4;
repeated uint32 dependents = 5;
}
This confirms that:
- The change maintains API compatibility as it matches the proto contract
- The array format is the correct representation for the repeated fields
- No documentation updates are needed since this aligns with the proto spec
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proto definition and API consumers
# Check proto definition
rg -A 5 "message Node" api/v1/service.proto
# Look for potential API consumers that might be affected
rg -l "ToBase64|Dependencies.*string|Dependents.*string"
Length of output: 244
Script:
#!/bin/bash
# Let's verify the changes more thoroughly
# Check the implementation of ToArray() method
rg -A 3 "func.*ToArray"
# Look for any tests that might be affected
rg -l "TestNode.*Dependencies|TestNode.*Dependents"
# Check for any documentation changes
fd README.md --exec grep -l "Dependencies\|Dependents" {} \;
# Look for any API examples or usage patterns
fd -e md -e txt -e yaml -e json --exec grep -l "Dependencies\|Dependents" {} \;
Length of output: 268
Script:
#!/bin/bash
# Let's check the content of the scorecards file and look for the ToArray implementation in a different way
cat ./testdata/scorecards/scorecards.json
# Try finding ToArray implementation using ast-grep
ast-grep --pattern 'func ($node) ToArray() $ret {
$$$
}'
# Look for the struct definition that contains ToArray
rg -B 5 -A 1 "type.*struct.*{.*Children.*Parents"
Length of output: 3028
Script:
#!/bin/bash
# Fix the regex and try one last search for the implementation
rg -B 5 "type.*struct.*\{.*Children"
# Look for any interface definitions that might specify ToArray
rg "interface.*\{" -A 10
# Check for any test files that might show the expected behavior
fd -e test.go --exec cat {} \;
Length of output: 106695
- Also change dependencies outputted from the API, to be an array of id's instead of a base64 encoded string Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
naveensrinivasan
force-pushed
the
neil/addCORS
branch
from
5e05035
to
40f05ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
cmd/server/server.go (2)
50-55: Enhance the CORS flag descriptionWhile the current description is good, consider making it more explicit about accepting multiple origins and providing more examples.
- "Allowed origins for CORS (e.g., 'https://app.bitbom.dev')", + "Comma-separated list of allowed origins for CORS (e.g., 'http://localhost:8089,https://app.bitbom.dev')",
188-199: Consider adding CORS origin validationWhile the current implementation is secure, consider adding validation for CORS origins to prevent accidental misconfiguration.
func withCORS(h http.Handler, o *options) http.Handler { + // Validate CORS origins + for _, origin := range o.CORS { + if origin == "*" { + log.Printf("Warning: Wildcard CORS origin (*) detected. This is not recommended for production.") + } + if !strings.HasPrefix(origin, "http://") && !strings.HasPrefix(origin, "https://") { + log.Printf("Warning: Invalid CORS origin format: %s. Origins should start with http:// or https://", origin) + } + } + middleware := cors.New(cors.Options{ AllowedOrigins: o.CORS, AllowedMethods: connectcors.AllowedMethods(),cmd/server/server_test.go (1)
206-265: Enhance CORS test coverage with additional test cases.While the current test cases cover basic CORS functionality, consider adding the following scenarios for comprehensive coverage:
- OPTIONS preflight requests
- Empty CORS array behavior
- Wildcard origin "*"
- Additional CORS headers (Allow-Methods, Allow-Headers)
Here's a suggested addition to the test cases:
testCases := []struct { name string options options requestOrigin string + requestMethod string expectedOrigin string + expectedHeaders map[string]string }{ // ... existing test cases ... + { + name: "Preflight Request", + options: options{ + CORS: []string{"http://localhost:3000"}, + }, + requestOrigin: "http://localhost:3000", + requestMethod: "OPTIONS", + expectedOrigin: "http://localhost:3000", + expectedHeaders: map[string]string{ + "Access-Control-Allow-Methods": "GET,POST,PUT,DELETE", + "Access-Control-Allow-Headers": "Content-Type,Authorization", + }, + }, + { + name: "Wildcard Origin", + options: options{ + CORS: []string{"*"}, + }, + requestOrigin: "http://any-domain.com", + expectedOrigin: "*", + }, + { + name: "Empty CORS Array", + options: options{ + CORS: []string{}, + }, + requestOrigin: "http://localhost:3000", + expectedOrigin: "", + }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
gen/api/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
api/v1/service.go(1 hunks)api/v1/service.proto(1 hunks)cmd/server/server.go(5 hunks)cmd/server/server_test.go(2 hunks)go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/v1/service.go
- api/v1/service.proto
- go.mod
🔇 Additional comments (4)
cmd/server/server.go (3)
14-14: LGTM: Import statements are correctly structured
The new CORS-related imports are properly organized and aliased to avoid naming conflicts.
Also applies to: 19-19
33-33: LGTM: CORS field properly added to options struct
The CORS field is correctly typed as []string to support multiple allowed origins.
128-128: LGTM: CORS wrapper correctly applied at the mux level
The implementation properly applies CORS at a single level, following best practices and previous review feedback.
cmd/server/server_test.go (1)
206-327: Verify CORS security implementation.
Please ensure the CORS implementation follows security best practices:
- Origin validation should be case-sensitive
- Avoid using wildcards (*) in production
- Set appropriate security headers
Let's verify the CORS implementation in the main server code:
✅ Verification successful
Let me gather more information about the CORS implementation by checking the actual server code:
Let me check one more thing to understand which CORS package is being used and its imports:
CORS implementation follows security best practices
The CORS implementation is secure and well-implemented:
- Uses
rs/corspackage withconnectrpc.com/corsfor standardized headers - No wildcards in origin configuration - origins are explicitly listed in
o.CORS - Implements secure defaults:
- Case-sensitive origin validation (inherent in
rs/cors) - Proper security headers (AllowedMethods, AllowedHeaders, ExposedHeaders)
- Reasonable MaxAge (3600)
- Explicit AllowedOrigins list
- Case-sensitive origin validation (inherent in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check CORS implementation for security best practices
# Check for case-sensitive origin comparison
rg -A 3 'Origin.*==' cmd/server/
# Check for wildcard usage
rg -A 3 '\*' cmd/server/
# Check for security headers
rg -A 5 'Access-Control-' cmd/server/
Length of output: 7526
Script:
#!/bin/bash
# Check the actual CORS implementation in server.go
rg -A 10 'withCORS' cmd/server/server.go
# Check if we're using any CORS middleware
rg -l 'cors\.' cmd/server/
# Check imports to identify the CORS package
rg -A 3 'import.*cors' cmd/server/
Length of output: 811
Script:
#!/bin/bash
# Check all imports in server.go to identify the CORS package
rg -A 20 '^import \(' cmd/server/server.go
# Check for any CORS-related configuration or constants
rg -B 2 -A 2 'connectcors\.' cmd/server/
Length of output: 936
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
🚨 Try these New Features:
|
Summary by CodeRabbit
New Features
Improvements
Chores
Tests
roaringpackage in service tests, simplifying dependency management.