feat(server): add visualizer internal api [VIZ-1517, VIZ-1604] #1574
Conversation
WalkthroughThis update introduces a new internal gRPC API for project management within the server. It adds a Protocol Buffers schema defining the API, implements the server-side logic, and integrates gRPC server startup and authentication into the application. Supporting methods and configuration structures are added across repository, usecase, and adapter layers to enable project listing, retrieval, creation, updating visibility, and deletion via gRPC. End-to-end tests for the new API are provided, including visibility and access control scenarios. The Makefile and Go module are updated to support code generation and dependency management for gRPC. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant gRPCServer
participant Interceptor
participant Usecase
participant Repository
Client->>gRPCServer: CreateProject / GetProject / GetProjectList / UpdateVisibility / DeleteProject
gRPCServer->>Interceptor: Logging, Auth, Attach Operator/Usecase
Interceptor->>Usecase: Call corresponding usecase method
Usecase->>Repository: Fetch or modify project data
Repository-->>Usecase: Return project(s) or result
Usecase-->>gRPCServer: Return domain model
gRPCServer-->>Client: Return protobuf response
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
✅ Deploy Preview for reearth-web canceled.
|
hexaforce
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🔭 Outside diff range comments (1)
server/pkg/project/project.go (1)
92-217: 💡 Verification agent🧩 Analysis chain
Consider adding integration with PublishmentStatus.
The new visibility field and the existing PublishmentStatus field seem to have overlapping concerns. Consider how they interact and whether one should influence the other.
🏁 Script executed:
#!/bin/bash # Check usage of PublishmentStatus in relation to visibility echo "Checking relationship between PublishmentStatus and visibility..." rg "PublishmentStatus" --type go echo -e "\nChecking how visibility is used in the codebase..." rg "visibility" --type goLength of output: 27943
Align visibility and PublishmentStatus or clarify their separation
Currently,
visibility(string) andpublishmentStatus(enum) are managed entirely independently, which can lead to inconsistent states (for example,visibility == "public"whilepublishmentStatus == PublishmentStatusPrivate). You should either synchronize them or make their distinct roles explicit:• Define a
Visibilityenum instead of a raw string to mirrorPublishmentStatusand prevent invalid values.
• InUpdatePublishmentStatus, overridevisibility(e.g. set both to “private” when status is private).
• Or inUpdateVisibility, enforce that certain statuses cannot yield visible results (e.g. reject setting visibility to “public” if status is private).
• If they serve different domains (UI vs. publishing), add documentation comments and consider renaming one to reduce confusion (e.g.uiVisibility).File to update:
- server/pkg/project/project.go (methods
UpdateVisibilityandUpdatePublishmentStatus)
🧹 Nitpick comments (16)
server/.env.example (1)
31-33: Configuration added for the new internal API.These environment variables are properly structured to control the internal gRPC API:
REEARTH_INTERNALAPI_ACTIVE=trueenables/disables the APIREEARTH_INTERNALAPI_PORT=50051sets the gRPC server portREEARTH_INTERNALAPI_TOKEN=tokenconfigures authenticationFor security in production, consider using a more complex default token value in the example.
server/pkg/project/project.go (1)
57-57: New visibility field added to Project struct.The string field
visibilityhas been added to represent project visibility state. Consider using an enum or constants for visibility values to ensure type safety.- visibility string + visibility ProjectVisibility +// ProjectVisibility represents project visibility levels +type ProjectVisibility string + +const ( + // ProjectVisibilityPublic represents public visibility + ProjectVisibilityPublic ProjectVisibility = "PUBLIC" + // ProjectVisibilityPrivate represents private visibility + ProjectVisibilityPrivate ProjectVisibility = "PRIVATE" +)server/internal/usecase/interactor/project.go (1)
94-101: UseIDin method name for consistencyThroughout the codebase, identifiers follow Go’s common initial‑ism rule (
ID,URL, etc.). IntroducingFindActiveById(lower‑case d) breaks that convention and also diverges from existing names such asFindByID.Keeping the API surface consistent avoids cognitive overhead and tooling glitches (e.g.,
go vet’sidchecker).-func (i *Project) FindActiveById(ctx context.Context, pid id.ProjectID, operator *usecase.Operator) (*project.Project, error) { +func (i *Project) FindActiveByID(ctx context.Context, pid id.ProjectID, operator *usecase.Operator) (*project.Project, error) {server/internal/app/server.go (2)
78-107:Runignores context cancellation and blocks on signal only
runServeralready passes actxbutRunnever listens to it; this prevents graceful shutdown in programmatic contexts (tests, parent processes).Consider:
go func() { <-ctx.Done() _ = w.Shutdown(context.Background()) }()and keep the signal handling separate; this also lets tests cancel via
context.WithCancel.
113-115:ServeGRPCmay dereference a nil server
ServeGRPCassumesw.internalServeris non‑nil. Calling it when the internal API is disabled will panic.if w.internalServer == nil { return errors.New("internal gRPC server is disabled") } return w.internalServer.Serve(l)server/internal/adapter/internalapi/internalapimodel/project.go (1)
55-63: Use aswitchinstead of chainediffor enum mappingThe current chain is correct but less idiomatic/extensible. A
switchimproves readability and makes the compiler warn on unhandled cases (with adefault:).-func ToVisualizer(v visualizer.Visualizer) pb.Visualizer { - if v == visualizer.VisualizerCesium { - return pb.Visualizer_VISUALIZER_CESIUM - } - if v == visualizer.VisualizerCesiumBeta { - return pb.Visualizer_VISUALIZER_CESIUM_BETA - } - return pb.Visualizer_VISUALIZER_UNSPECIFIED -} +func ToVisualizer(v visualizer.Visualizer) pb.Visualizer { + switch v { + case visualizer.VisualizerCesium: + return pb.Visualizer_VISUALIZER_CESIUM + case visualizer.VisualizerCesiumBeta: + return pb.Visualizer_VISUALIZER_CESIUM_BETA + default: + return pb.Visualizer_VISUALIZER_UNSPECIFIED + } +}server/internal/app/grpc.go (1)
155-170: Minor: avoid double map lookup intokenFromGrpcMetadataUse the already‑fetched value:
- token = strings.TrimPrefix(md["authorization"][0], "Bearer ") + token = strings.TrimPrefix(token, "Bearer ")server/internal/adapter/gql/generated.go (6)
3887-3893: Nil‑check hides missing complexity handlersThe nil‑guard pattern used here silently skips complexity calculation when
Visibilityis leftnil(see previous comment).
Consider logging or failing fast during application start‑up if any complexity handler is missing so that omissions are caught in CI rather than at runtime.
27550-27579: Resolver incorrectly marked as non‑resolver
IsResolver: falsetells gqlgen that_Project_visibilityis a simple field read, but the method is hand‑written and wrapped withResolverMiddleware.
SetIsResolver: trueto avoid confusing tracing / introspection tools, or remove the custom function and let gqlgen auto‑marshal the struct field.- IsResolver: false, + IsResolver: true,
27820-27825: Switch blocks growing – consider code‑generation onlyMultiple identical switch arms were added for
visibility.
Manual edits togenerated.gowill be lost next time the generator runs. Prefer updating the schema and letting gqlgen regenerate, or move custom logic toresolver/files.Also applies to: 28059-28064, 28159-28164, 34466-34471
44341-44346: Missing business‑rule validation in input unmarshal
unmarshalInputCreateProjectInputblindly assignsvisibilitywithout checking against allowed constants.
Inject validation here or in the service layer to prevent bad data entering the domain.Also applies to: 44383-44393
51694-51701: Atomic invalid‑count may hidevisibilitynull‑returnsThe helper increments
Invalidsonly when the resolver returnsgraphql.Null.
Ifobj.Visibilityis an empty string ("") it passes the non‑null check but still violates the semantic requirement of “must be one of valid visibilities”. Rely on enum or explicit validation as noted earlier.
1-1: Commit includes generated file
generated.gois produced bygqlgen. Committing large generated diffs inflates PR noise and invites merge conflicts.
Consider:
- Add
server/internal/adapter/gql/generated.goto.gitignoreand commit only the schema + resolver code, or- Keep the file, but add a comment in CONTRIBUTING explaining why the repo vendors generated sources.
Choose one strategy and apply consistently across the project.
server/e2e/gql_project_test.go (3)
124-142:createProject*helpers ignore errors from GraphQLThe helper immediately dereferences
.Raw().(string)which panics on GraphQL errors and hides the actual response body.Add an
.NotEmpty()/ status assertion first to surface failures clearly:-res := Request(e, u.String(), requestBody) -return res.Path("$.data.createProject.project.id").Raw().(string) +res := Request(e, u.String(), requestBody) +res.Path("$.errors").Null() // fail fast if errors present +return res.Path("$.data.createProject.project.id").NotEmpty().Raw().(string)
363-373: Avoid hidden global dependency inupdateStarProject
updateStarProjectrelies on the package‑leveluID; passing it explicitly improves test isolation and readability.-func updateStarProject(e *httpexpect.Expect, projectID string) { - response := updateProject(e, uID, map[string]any{ +func updateStarProject(e *httpexpect.Expect, userID accountdomain.UserID, projectID string) { + response := updateProject(e, userID, map[string]any{Call sites then provide
uID.
548-562: Hard‑coding default expectations couples test to implementation detailAsserting default
coreSupport=false&visibility=privateis OK, but if defaults evolve these tests will fail noisily. Consider setting explicit inputs in the mutation instead of relying on defaults, or confine expectations via dedicated default‑value tests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.goserver/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (39)
server/.env.example(1 hunks)server/Makefile(1 hunks)server/e2e/common.go(4 hunks)server/e2e/gql_asset_test.go(1 hunks)server/e2e/gql_custom_property_test.go(3 hunks)server/e2e/gql_featureCollection_test.go(1 hunks)server/e2e/gql_nlslayer_test.go(5 hunks)server/e2e/gql_project_test.go(18 hunks)server/e2e/gql_property_schema_test.go(1 hunks)server/e2e/gql_scene_test.go(2 hunks)server/e2e/gql_storytelling_test.go(0 hunks)server/e2e/gql_style_test.go(1 hunks)server/e2e/gql_validate_geojson_test.go(3 hunks)server/e2e/proto_project_test.go(1 hunks)server/e2e/seeder.go(2 hunks)server/go.mod(1 hunks)server/gql/project.graphql(3 hunks)server/internal/adapter/gql/generated.go(15 hunks)server/internal/adapter/gql/gqlmodel/convert_project.go(1 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(3 hunks)server/internal/adapter/gql/loader_project.go(1 hunks)server/internal/adapter/gql/resolver_mutation_project.go(2 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/internalapi/internalapimodel/project.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/app/app.go(1 hunks)server/internal/app/config/config.go(1 hunks)server/internal/app/grpc.go(1 hunks)server/internal/app/server.go(3 hunks)server/internal/infrastructure/memory/project.go(1 hunks)server/internal/infrastructure/mongo/mongodoc/project.go(3 hunks)server/internal/infrastructure/mongo/project.go(1 hunks)server/internal/usecase/interactor/common.go(1 hunks)server/internal/usecase/interactor/project.go(4 hunks)server/internal/usecase/interfaces/project.go(3 hunks)server/internal/usecase/repo/project.go(1 hunks)server/pkg/project/builder.go(1 hunks)server/pkg/project/project.go(3 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
💤 Files with no reviewable changes (1)
- server/e2e/gql_storytelling_test.go
🧰 Additional context used
🧠 Learnings (1)
server/e2e/gql_project_test.go (1)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:48-80
Timestamp: 2024-09-13T01:19:11.267Z
Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.
🧬 Code Graph Analysis (3)
server/internal/usecase/interactor/common.go (2)
server/internal/usecase/repo/container.go (1)
Container(21-40)server/internal/usecase/interfaces/common.go (1)
Container(24-37)
server/e2e/proto_project_test.go (3)
server/e2e/common.go (1)
GRPCServer(186-188)server/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.go (1)
ReEarthVisualizerClient(31-37)server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.go (16)
CreateProjectRequest(452-462)CreateProjectRequest(475-475)CreateProjectRequest(490-492)Visualizer(25-25)Visualizer(57-59)Visualizer(61-63)Visualizer(70-72)GetProjectRequest(408-413)GetProjectRequest(426-426)GetProjectRequest(441-443)Project(127-156)Project(169-169)Project(184-186)GetProjectListRequest(364-369)GetProjectListRequest(382-382)GetProjectListRequest(397-399)
server/internal/app/grpc.go (7)
server/internal/app/server.go (2)
ServerConfig(41-48)NewServer(50-76)server/internal/app/config/config.go (1)
Config(24-77)server/internal/adapter/internalapi/server.go (1)
NewServer(19-21)server/pkg/id/id.go (1)
UserIDFrom(104-104)server/internal/adapter/context.go (3)
AttachUser(41-43)AttachOperator(45-47)AcOperator(97-104)server/internal/usecase/repo/container.go (2)
WorkspaceFilterFromOperator(82-87)SceneFilterFromOperator(133-138)server/internal/usecase/interactor/common.go (2)
NewContainer(31-60)ContainerConfig(24-29)
🪛 ast-grep (0.31.1)
server/e2e/common.go
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
🪛 Buf (1.47.2)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (54)
server/go.mod (1)
52-53: Dependencies correctly moved to direct imports.The gRPC and protobuf dependencies have been properly moved from indirect to direct dependencies, which reflects their direct usage in the new internal gRPC API implementation.
server/Makefile (1)
114-118: Well-structured protobuf code generation target.The new
grpctarget correctly configures protoc to generate both Go code and gRPC service code from the Protocol Buffers schema. The output directory and options are appropriate for the project structure.server/internal/usecase/interactor/common.go (1)
31-38: Function signature reformatting improves readability.The function signature has been reformatted to place each parameter on its own line, which improves readability without changing functionality. This is a good stylistic change.
server/internal/app/app.go (1)
113-126: Middleware registration formatting improves readability.The middleware registration has been reformatted to place each argument on its own line, which improves readability without changing functionality. This is consistent with the formatting changes in other files.
server/pkg/project/builder.go (1)
86-89:✅ Verification successful
Good implementation of project visibility feature.
The new
Visibilitymethod follows the established builder pattern and correctly sets the visibility field. This is a necessary addition for supporting the visibility feature in the internal API.
🏁 Script executed:
#!/bin/bash # Verify that the visibility field is used consistently across the codebase # Look for visibility field in project struct definition echo "Checking for visibility field in Project struct:" rg -A 1 "type\s+Project\s+struct" --glob "server/pkg/project/*.go" # Look for visibility related methods echo -e "\nChecking for visibility getter method:" rg "func\s+\(\w+\s+\*?Project\)\s+Visibility\(\)" --glob "server/pkg/project/*.go" # Check if visibility is used in the internal API proto definition echo -e "\nChecking visibility in protobuf schema:" if [ -f "./schemas/internalapi/v1/schema.proto" ]; then grep -A 10 "message Project" ./schemas/internalapi/v1/schema.proto | grep -i visibility else echo "Schema file not found" fiLength of output: 736
🏁 Script executed:
#!/bin/bash # Search for all "visibility" occurrences in Go project files echo "Occurrences of 'visibility' in server/pkg/project files:" rg -n "visibility" --glob "server/pkg/project/*.go" # Show full Project struct definition context to verify field presence echo -e "\nFull Project struct context:" rg -A 20 "type Project struct" --glob "server/pkg/project/project.go" # Search for visibility in any protobuf schema files echo -e "\nOccurrences of 'visibility' in .proto schemas:" rg -n "visibility" --glob "schemas/**/*.proto" || echo "No visibility occurrences in proto schemas"Length of output: 2368
Approve builder’s Visibility method
All occurrences of thevisibilityfield have been confirmed:
visibility stringis present inserver/pkg/project/project.go- Getter (
func (p *Project) Visibility() string) and updater (UpdateVisibility) exist- Builder method correctly sets
p.visibilityNo
.protoschema files found—no further schema updates are required.server/internal/adapter/gql/resolver_mutation_project.go (2)
37-37: LGTM! Proper implementation of visibility field in CreateProject.The visibility field is correctly added to the CreateProjectParam struct, enabling the GraphQL mutation to set project visibility.
83-83: LGTM! Proper implementation of visibility field in UpdateProject.The visibility field is correctly added to the UpdateProjectParam struct, enabling the GraphQL mutation to update project visibility.
server/internal/adapter/gql/gqlmodel/convert_project.go (1)
70-70: LGTM! Visibility field added to Project GraphQL type.The visibility field is correctly added to the Project struct in the ToProject conversion function, ensuring that project visibility is exposed to GraphQL clients.
server/e2e/gql_style_test.go (1)
150-156: Test refactored to use standardized project creation.The project creation method has been updated to use a more detailed parameter map instead of a simple string, aligning with standardized test approaches across the codebase.
server/e2e/seeder.go (2)
37-44: Added variables for multi-user testing.New variables for a second user and workspace have been added to support multi-user testing scenarios.
70-78: Added second user for visibility testing.A second user is created and saved to the repository, enabling tests that validate project visibility across different users.
server/internal/adapter/gql/resolver_query.go (1)
186-188: New resolver method for visibility filtering looks good.The
VisibilityProjectsresolver properly implements the query operation to filter projects by visibility within a workspace. It correctly delegates to the loader'sVisibilityByWorkspacemethod.server/e2e/gql_custom_property_test.go (3)
13-19: Project creation updated with standardized parameters.The project creation has been enhanced to include additional parameters like description, team ID, visualizer type, and core support flag. This is a good improvement for test consistency.
110-116: Project creation updated with standardized parameters.The project creation in this test has been similarly updated with the standardized set of parameters, maintaining consistency across test files.
237-243: Project creation updated with standardized parameters.The project creation in the third test has been standardized in the same way, ensuring consistent test project configuration across all test cases.
server/pkg/project/project.go (1)
92-94: Visibility getter method added.The getter method follows the consistent pattern used throughout the codebase.
server/e2e/gql_property_schema_test.go (1)
12-18: Good update to align with standardized project creation pattern.The project creation has been updated to follow the improved convention used across other tests, providing more complete project data and explicit user context. This increases test consistency and better reflects real-world usage.
server/internal/app/config/config.go (1)
76-83: Configuration structure for the new internal gRPC API looks good.The InternalApiConfig struct provides the necessary configuration options (activation flag, port, and authentication token) for the new gRPC server. This implementation aligns well with the PR objectives.
server/gql/project.graphql (3)
28-28: Appropriate addition of required visibility field to Project type.The non-nullable visibility field ensures all projects have this attribute, which aligns with the PR requirement that projects should have visibility settings.
60-60: Good implementation of optional visibility in CreateProjectInput.This allows clients to specify visibility when creating projects while maintaining backward compatibility by making it optional.
84-84: Good implementation of optional visibility in UpdateProjectInput.This allows clients to update project visibility while maintaining backward compatibility by making it optional.
server/e2e/gql_validate_geojson_test.go (3)
11-17: Good update to align with standardized project creation pattern.The project creation has been updated to follow the improved convention used across other tests, providing more complete project data and explicit user context.
232-238: Good update to align with standardized project creation pattern.Consistent with the changes in the other test functions, this provides more complete project data and explicit user context.
254-260: Good update to align with standardized project creation pattern.The project creation has been updated to follow the improved convention used across other tests, providing more complete project data and explicit user context.
server/e2e/gql_asset_test.go (2)
129-135: Project creation updated to use structured input.The project creation now includes more detailed attributes - adding description, teamId, visualizer, and coreSupport flags - which aligns with the broader standardization of project creation across the codebase.
139-145: Project creation consistently structured.This follows the same pattern as projectA creation, maintaining consistency in how projects are created throughout tests.
server/e2e/gql_nlslayer_test.go (2)
388-394: Consistent project creation approach applied.The test now creates projects with full metadata including description, teamId, visualizer settings, and the coreSupport flag, which improves test consistency and reflects the actual project creation flow.
781-787: Project creation standardized across multiple test functions.Multiple test functions now use the same structured approach to project creation, ensuring consistency across the test suite and better reflecting how projects are created in production code.
Also applies to: 859-865, 917-923, 991-997
server/e2e/gql_scene_test.go (2)
66-72: Updated project creation with detailed attributes.Test now creates projects with complete metadata, aligning with the standardized approach across the codebase.
86-87: Refactored to use shared project creation mutation.Replaced inline GraphQL query with a reusable CreateProjectMutation constant, improving maintainability and consistency across tests.
server/internal/usecase/repo/project.go (1)
27-28:❓ Verification inconclusive
Interface extended to support project visibility and active status filtering.
These two new repository methods support key requirements of the internal gRPC API:
FindActiveByIdwill fetch only non-deleted projects by IDFindVisibilityByWorkspaceenables filtering projects by visibility within a workspaceThese align with the PR objectives for the new gRPC server that handles project visibility and status filtering.
🏁 Script executed:
#!/bin/bash # Check for implementations of these new interface methods in repository implementations echo "Checking for implementation of FindActiveById and FindVisibilityByWorkspace" rg -A 5 "func \(\w+\) FindActiveById" --type go rg -A 5 "func \(\w+\) FindVisibilityByWorkspace" --type goLength of output: 271
Repository implementations missing new interface methods
I searched for
FindActiveByIdandFindVisibilityByWorkspacebut didn’t find any concrete implementations in the repository layer.• Interface declared in
server/internal/usecase/repo/project.go(lines 27–28)
• No matchingfunc (*… ) FindActiveByIdorFindVisibilityByWorkspacein infra/db or other repo packagesPlease add implementations of these methods in your repository packages (e.g., in the Postgres or other DB adapters) so the new gRPC API can fetch active projects and filter by workspace visibility.
server/internal/adapter/gql/loader_project.go (1)
126-146: LGTM! Well-implemented visibility loader method.The
VisibilityByWorkspacemethod follows the same pattern as existing methods likeFindStarredByWorkspaceandFindDeletedByWorkspace. It properly handles ID conversion, error management, and transformation of domain models to GraphQL models.server/e2e/gql_featureCollection_test.go (1)
179-185: Good enhancement to test setup.The project creation has been updated to include more detailed parameters including visibility-related attributes. This change aligns with the standardized approach used across other tests and provides better test coverage for the new project visibility feature.
server/e2e/common.go (3)
45-55: LGTM! Well-structured configuration for internal API testing.The new configuration properly defines the settings needed for internal API testing including port and authentication token.
130-155: Good conditional server setup logic.The modified server startup logic correctly handles both standard HTTP and gRPC server modes based on configuration.
Note: The static analysis flagged the use of
:0as potentially binding to all interfaces, but this is acceptable in a test context where using a random port is a common practice to avoid port conflicts.🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
186-188: LGTM! Helpful test utility method.The new
GRPCServerhelper method makes it easy to start the server in gRPC mode for testing the new internal API functionality.server/internal/adapter/gql/gqlmodel/models_gen.go (3)
200-200: LGTM! Proper visibility field addition to CreateProjectInput.This field allows specifying project visibility when creating a new project.
709-709: LGTM! Project visibility field correctly added.The Project model now includes a non-optional visibility field, consistent with the domain model changes.
1269-1269: LGTM! Visibility field properly added to UpdateProjectInput.This field allows modifying a project's visibility when updating it.
server/internal/infrastructure/mongo/mongodoc/project.go (3)
36-38: LGTM: Visibility field added to ProjectDocument struct.The visibility field has been properly added to the ProjectDocument struct, consistent with other fields in the structure.
79-81: LGTM: Visibility field included in NewProject constructor.The Visibility field is correctly initialized in the document constructor using the project.Visibility() method.
131-131: LGTM: Visibility field properly passed to model construction.The Visibility field is correctly passed from the document to the project builder, completing the round-trip conversion.
server/internal/usecase/interfaces/project.go (3)
27-27: LGTM: Visibility field added to CreateProjectParam struct.The Visibility field has been correctly added to the CreateProjectParam struct as a pointer to string, consistent with other optional fields.
51-51: LGTM: Visibility field added to UpdateProjectParam struct.The Visibility field has been correctly added to the UpdateProjectParam struct as a pointer to string, consistent with other optional fields.
75-76: LGTM: New interface methods for active project retrieval and visibility filtering.Two new methods have been added to the Project interface:
- FindActiveById - retrieves non-deleted projects by ID
- FindVisibilityByWorkspace - retrieves projects filtered by visibility and workspace
These methods support the PR's goal of implementing project visibility in the internal API.
server/internal/infrastructure/memory/project.go (1)
130-137: LGTM: FindActiveById implementation correctly filters non-deleted projects.The implementation properly iterates through the in-memory data store and returns projects that match the requested ID and are not marked as deleted.
server/internal/infrastructure/mongo/project.go (2)
152-166: LGTM: FindActiveById implementation correctly filters active projects.The implementation properly queries for non-deleted projects by ID and handles errors appropriately, including the case where no document is found.
168-183: LGTM: FindVisibilityByWorkspace correctly implements visibility filtering.The implementation properly filters projects based on visibility and workspace accessibility:
- If the workspace is not readable, only public non-deleted projects are returned
- If the workspace is readable, all non-deleted projects in the workspace are returned
This logic properly supports the visibility requirements in the PR objectives.
server/e2e/proto_project_test.go (2)
25-27: Potential port collision & zombie servers between testsEach test spins up its own server on the same hard‑coded port (
50051) and never callsShutdown, so the second invocation will very likely fail with “address already in use” on slower CI runners.Options:
- Pick an arbitrary free port with
net.Listen("tcp", "127.0.0.1:0")and pass it to both the server and the client.- Ensure
GRPCServerreturns a stopper and call it witht.Cleanup(...).Without this, the E2E suite will be flaky.
75-77: Empty string treated as ‘null’ visibility – verify spec alignmentThe assertions expect an empty string (
"") whenvisibilityis unset, but earlier logic convertsnilto"private". Ensure the domain model and test expectations are in sync; otherwise backend & tests diverge.If
"private"is the canonical default, update these tests accordingly.Also applies to: 140-142
server/internal/adapter/gql/generated.go (3)
556-561: Generated complexity stub left uninitialised
Visibilityhas been added toComplexityRoot.Project, but the generator leaves the function pointernil.
If you intend to run query‑cost analysis, remember to wire a real implementation inresolver.go; otherwise complexity forvisibilitywill default to0, potentially under‑estimating query cost.
6804-6808: Input types accept free‑form strings – validation required
CreateProjectInput.visibilityandUpdateProjectInput.visibilityare optional strings.
If you keep them as strings, add explicit validation in the mutation resolver / use‑case layer to reject unsupported values; otherwise internal logic may interpret unknown strings as “private”.Also applies to: 6828-6832
46313-46317: UpdateProjectInput: keep field naming consistentThe struct already has
deletedandisDeletedas separate booleans; addingvisibilitycontinues this pattern of legacy names.
Confirm you genuinely need bothdeletedandisDeleted; otherwise deprecate one to avoid API confusion.Also applies to: 46463-46469
server/schemas/internalapi/v1/schema.proto (1)
70-77: Validate optional scalar usage
optional bool/stringin proto3 emits synthetichas_accessors, but some languages (e.g. JS) still cannot distinguish unset vs. zero‑value. Ensure all target languages rely on those semantics, otherwise use wrapper types (google.protobuf.BoolValue, etc.).
server/pkg/project/project.go
Outdated
| func (p *Project) UpdateVisibility(visibility string) { | ||
| p.visibility = visibility | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Visibility setter method added.
The setter method lacks validation for valid visibility values. Consider adding validation to ensure only valid visibility values are set.
-func (p *Project) UpdateVisibility(visibility string) {
- p.visibility = visibility
+func (p *Project) UpdateVisibility(visibility string) {
+ // If null or empty, default to private
+ if visibility == "" {
+ p.visibility = "PRIVATE"
+ return
+ }
+
+ // Normalize to uppercase and validate
+ vis := strings.ToUpper(visibility)
+ if vis == "PUBLIC" || vis == "PRIVATE" {
+ p.visibility = vis
+ } else {
+ // Default to private for invalid values
+ p.visibility = "PRIVATE"
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *Project) UpdateVisibility(visibility string) { | |
| p.visibility = visibility | |
| } | |
| func (p *Project) UpdateVisibility(visibility string) { | |
| // If empty, default to PRIVATE | |
| if visibility == "" { | |
| p.visibility = "PRIVATE" | |
| return | |
| } | |
| // Normalize to uppercase and validate | |
| vis := strings.ToUpper(visibility) | |
| if vis == "PUBLIC" || vis == "PRIVATE" { | |
| p.visibility = vis | |
| } else { | |
| // Default to PRIVATE for invalid values | |
| p.visibility = "PRIVATE" | |
| } | |
| } |
| if input.Visibility != nil { | ||
| prj = prj.Visibility(*input.Visibility) | ||
| } else { | ||
| prj = prj.Visibility("private") | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate “magic strings”; centralise & validate default
You fall back to "private" when input.Visibility is nil, but the same hard‑coded literals are scattered elsewhere (update, tests, repo queries). This invites typos and makes future additions (e.g., "team") risky.
Create a project.Visibility value‑object (or at least constants) and use those everywhere, performing validation in one place.
Example:
const (
VisibilityPrivate = "private"
VisibilityPublic = "public"
)
func ParseVisibility(v string) (string, error) {
switch v {
case VisibilityPrivate, VisibilityPublic:
return v, nil
default:
return "", rerror.ErrInvalidFormat
}
}Then:
-if input.Visibility != nil {
- prj = prj.Visibility(*input.Visibility)
-} else {
- prj = prj.Visibility("private")
-}
+vis := VisibilityPrivate
+if input.Visibility != nil {
+ if v, err := ParseVisibility(*input.Visibility); err != nil {
+ return nil, err
+ } else {
+ vis = v
+ }
+}
+prj = prj.Visibility(vis)| if p.Visibility != nil { | ||
| prj.UpdateVisibility(*p.Visibility) | ||
| } |
There was a problem hiding this comment.
Visibility value is accepted unchecked – add validation
prj.UpdateVisibility(*p.Visibility) blindly trusts user input.
If any arbitrary string slips through, subsequent queries relying on "public"/"private" comparisons will misbehave and tests will silently pass because the invalid value is opaque.
Guard against this by validating the new value against an allow‑list (or by converting Visibility into a dedicated enum‑like type).
+validVis := map[string]struct{}{"public": {}, "private": {}}
+if _, ok := validVis[*p.Visibility]; !ok {
+ return nil, rerror.ErrInvalidFormat // or a domain‑specific error
+}
prj.UpdateVisibility(*p.Visibility)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if p.Visibility != nil { | |
| prj.UpdateVisibility(*p.Visibility) | |
| } | |
| if p.Visibility != nil { | |
| validVis := map[string]struct{}{"public": {}, "private": {}} | |
| if _, ok := validVis[*p.Visibility]; !ok { | |
| return nil, rerror.ErrInvalidFormat | |
| } | |
| prj.UpdateVisibility(*p.Visibility) | |
| } |
server/e2e/proto_project_test.go
Outdated
| conn, err := grpc.NewClient("localhost:50051", grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| if err != nil { | ||
| t.Fatalf("failed to connect: %v", err) |
There was a problem hiding this comment.
Compilation fails – grpc.NewClient does not exist
google.golang.org/grpc exposes grpc.Dial[Context], not NewClient.
The file will not compile.
-conn, err := grpc.NewClient("localhost:50051", grpc.WithTransportCredentials(insecure.NewCredentials()))
+conn, err := grpc.DialContext(ctx, "localhost:50051",
+ grpc.WithTransportCredentials(insecure.NewCredentials()),
+ grpc.WithBlock())Also wrap the dial in a short timeout (ctx) so tests don’t hang on connection errors.
Committable suggestion skipped: line range outside the PR's diff.
| option go_package = "proto/v1"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide a fully‑qualified go_package option
option go_package = "proto/v1"; yields import paths such as import "proto/v1", which collide easily and violate Go module conventions.
Specify the complete module path, e.g.
-option go_package = "proto/v1";
+option go_package = "github.com/reearth/reearth/server/internal/adapter/internalapi/proto/v1;internalapiv1";This prevents ambiguous imports when the proto is consumed outside the repo.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| option go_package = "proto/v1"; | |
| -option go_package = "proto/v1"; | |
| +option go_package = "github.com/reearth/reearth/server/internal/adapter/internalapi/proto/v1;internalapiv1"; |
| string basic_auth_username = 4; | ||
| string basic_auth_password = 5; | ||
| google.protobuf.Timestamp created_at = 6; |
There was a problem hiding this comment.
Avoid returning raw Basic‑Auth credentials
basic_auth_username and especially basic_auth_password are included in every Project response. Even for an internal API this creates unnecessary credential exposure risk and makes accidental leakage (logs, client dumps, etc.) more likely.
Consider:
- string basic_auth_username = 4;
- string basic_auth_password = 5;
+ string basic_auth_username = 4;
+ // Do NOT expose the password. If it must be handled, create a dedicated Write‑only request field.Alternatively, move them to a separate request/response that is protected by stricter ACLs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| string basic_auth_username = 4; | |
| string basic_auth_password = 5; | |
| google.protobuf.Timestamp created_at = 6; | |
| string basic_auth_username = 4; | |
| // Do NOT expose the password. If it must be handled, create a dedicated Write-only request field. | |
| google.protobuf.Timestamp created_at = 6; |
| string tracking_id = 22; | ||
| bool starred = 23; | ||
| bool is_deleted = 24; | ||
| string visibility = 25; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model visibility as an enum, not free‑form string
Allowed values appear to be public, limited, private. Defining an enum prevents typo bugs and simplifies client code:
-string visibility = 25;
+Visibility visibility = 25;
+
+enum Visibility {
+ VISIBILITY_UNSPECIFIED = 0;
+ VISIBILITY_PUBLIC = 1;
+ VISIBILITY_LIMITED = 2;
+ VISIBILITY_PRIVATE = 3;
+}Update request/response types accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| string visibility = 25; | |
| - string visibility = 25; | |
| + Visibility visibility = 25; | |
| + enum Visibility { | |
| + VISIBILITY_UNSPECIFIED = 0; | |
| + VISIBILITY_PUBLIC = 1; | |
| + VISIBILITY_LIMITED = 2; | |
| + VISIBILITY_PRIVATE = 3; | |
| + } |
server/e2e/gql_project_test.go
Outdated
| const ProjectFragment = ` | ||
| fragment ProjectFragment on Project { | ||
| id | ||
| isArchived | ||
| isBasicAuthActive | ||
| basicAuthUsername | ||
| basicAuthPassword | ||
| createdAt | ||
| updatedAt | ||
| publishedAt | ||
| name | ||
| description | ||
| alias | ||
| publicTitle | ||
| publicDescription | ||
| publicImage | ||
| publicNoIndex | ||
| imageUrl | ||
| teamId | ||
| visualizer | ||
| publishmentStatus | ||
| coreSupport | ||
| enableGa | ||
| trackingId | ||
| starred | ||
| isDeleted | ||
| visibility | ||
| __typename | ||
| }` |
There was a problem hiding this comment.
GraphQL fragment leaks basic‑auth credentials
basicAuthUsername/basicAuthPassword are fetched in every test query. This mirrors the security concern in the proto file and means:
- The GraphQL public schema exposes passwords.
- Tests may dump them into logs/CI artefacts.
Unless absolutely required, drop those fields from the fragment.
-basicAuthUsername
-basicAuthPassword📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ProjectFragment = ` | |
| fragment ProjectFragment on Project { | |
| id | |
| isArchived | |
| isBasicAuthActive | |
| basicAuthUsername | |
| basicAuthPassword | |
| createdAt | |
| updatedAt | |
| publishedAt | |
| name | |
| description | |
| alias | |
| publicTitle | |
| publicDescription | |
| publicImage | |
| publicNoIndex | |
| imageUrl | |
| teamId | |
| visualizer | |
| publishmentStatus | |
| coreSupport | |
| enableGa | |
| trackingId | |
| starred | |
| isDeleted | |
| visibility | |
| __typename | |
| }` | |
| const ProjectFragment = ` | |
| fragment ProjectFragment on Project { | |
| id | |
| isArchived | |
| isBasicAuthActive | |
| createdAt | |
| updatedAt | |
| publishedAt | |
| name | |
| description | |
| alias | |
| publicTitle | |
| publicDescription | |
| publicImage | |
| publicNoIndex | |
| imageUrl | |
| teamId | |
| visualizer | |
| publishmentStatus | |
| coreSupport | |
| enableGa | |
| trackingId | |
| starred | |
| isDeleted | |
| visibility | |
| __typename | |
| }` |
server/e2e/gql_project_test.go
Outdated
| requestBody := GraphQLRequest{ | ||
| OperationName: "GetProjects", | ||
| Query: GetProjectsQuery, | ||
| Variables: map[string]any{ | ||
| "input": map[string]any{ | ||
| "projectId": project2ID, | ||
| "description": "test updaet", | ||
| "pagination": map[string]any{ | ||
| "last": 5, | ||
| }, | ||
| "teamId": wID.String(), | ||
| "sort": map[string]string{ | ||
| "field": "NAME", | ||
| "direction": "ASC", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // default private | ||
| edges := Request(e, uID.String(), requestBody). | ||
| Path("$.data.projects.edges").Array() | ||
| edges.Value(1).Path("$.node.visibility").IsEqual("private") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Index access without length check can flake
edges.Value(1) assumes ≥ 2 results; test will panic if dataset changes.
Assert length first or locate the wanted node by ID instead:
-edges.Value(1).Path("$.node.visibility").IsEqual("private")
+edges.Length().Ge(1)
+edges.First().Path("$.node.visibility").IsEqual("private") // or search by id📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| requestBody := GraphQLRequest{ | |
| OperationName: "GetProjects", | |
| Query: GetProjectsQuery, | |
| Variables: map[string]any{ | |
| "input": map[string]any{ | |
| "projectId": project2ID, | |
| "description": "test updaet", | |
| "pagination": map[string]any{ | |
| "last": 5, | |
| }, | |
| "teamId": wID.String(), | |
| "sort": map[string]string{ | |
| "field": "NAME", | |
| "direction": "ASC", | |
| }, | |
| }, | |
| } | |
| // default private | |
| edges := Request(e, uID.String(), requestBody). | |
| Path("$.data.projects.edges").Array() | |
| edges.Value(1).Path("$.node.visibility").IsEqual("private") | |
| // default private | |
| edges := Request(e, uID.String(), requestBody). | |
| Path("$.data.projects.edges").Array() | |
| edges.Length().Ge(1) | |
| edges.First().Path("$.node.visibility").IsEqual("private") // or search by id |
hexaforce
changed the title
hexaforce
force-pushed
the
feat/reearth-visualizer-proto2
branch
from
f8748ff to
9113596
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
server/e2e/proto_project_test.go (1)
165-168:⚠️ Potential issueFix compilation error in grpc client setup
The
grpc.NewClientfunction doesn't exist in the gRPC package. This code needs to be fixed to usegrpc.DialContextinstead.- conn, err := grpc.NewClient("localhost:50051", grpc.WithTransportCredentials(insecure.NewCredentials())) + conn, err := grpc.DialContext(ctx, "localhost:50051", + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithBlock())server/schemas/internalapi/v1/schema.proto (4)
3-3:⚠️ Potential issueFix package-directory mismatch
Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root.
Either:
- Move the file to
server/schemas/reearth/visualizer/v1/schema.proto, or- Change the package to match the current directory, e.g.,
package server.schemas.internalapi.v1;This issue will cause failures in
buf lintand potentially block CI pipelines.🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
7-7: 🛠️ Refactor suggestionProvide a fully-qualified go_package option
Using an abbreviated
go_packagepath can lead to import collisions and violates Go module conventions.-option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/internal/adapter/internalapi/schemas/internalapi/v1;internalapiv1";
28-30:⚠️ Potential issueAvoid exposing sensitive credentials in responses
Including
basic_auth_usernameand especiallybasic_auth_passwordin everyProjectresponse creates unnecessary security risks.Consider one of these approaches:
- Remove the password field entirely from responses
- Create a separate, more secured request/response for credential management
- Only include credentials in specific create/update operations, not in general responses
string basic_auth_username = 4; - string basic_auth_password = 5; + // Password should not be exposed in responses
49-49: 🛠️ Refactor suggestionModel visibility as an enum instead of a string
Using a string for visibility allows arbitrary values and makes client code more complex. An enum would provide type safety and prevent typo bugs.
- string visibility = 25; + Visibility visibility = 25; // Add at end of file: +enum Visibility { + VISIBILITY_UNSPECIFIED = 0; + VISIBILITY_PUBLIC = 1; + VISIBILITY_PRIVATE = 2; +}Then update related request fields accordingly.
🧹 Nitpick comments (3)
server/e2e/proto_project_test.go (3)
67-78: Comment needs clarificationThe comments on lines 67-69 are incorrect. They mention "public list size 3" but the actual test is validating private projects.
- // -------- public list size 3 + // -------- project list size 3 // 0: creante seeder => null // 1: creante default => private // 2: creante private => private
131-143: Fix typo in commentThere are multiple typos in the comments where "create" is spelled as "creante".
- // 0: creante seeder => null - // 1: creante public => public - // 2: creante private => private + // 0: created by seeder => null + // 1: created as public => public + // 2: created as private => private
156-173: Consider adding test cleanupThe tests create multiple projects but don't clean them up afterward, which might affect subsequent test runs. Consider adding cleanup logic to delete the created projects at the end of each test.
You could add a slice to track created project IDs and delete them in a deferred function:
func TestInternalAPI_private(t *testing.T) { GRPCServer(t, baseSeeder) var createdProjectIDs []string // Track created projects for cleanup defer func() { runTestWithUser(t, uID.String(), func(client pb.ReEarthVisualizerClient, ctx context.Context) { for _, id := range createdProjectIDs { _, err := client.DeleteProject(ctx, &pb.DeleteProjectRequest{ProjectId: id}) if err != nil { t.Logf("Failed to clean up project %s: %v", id, err) } } }) }() runTestWithUser(t, uID.String(), func(client pb.ReEarthVisualizerClient, ctx context.Context) { // When a project is created, add its ID to the cleanup list res, err := client.CreateProject(ctx, &pb.CreateProjectRequest{ // ... existing code ... }) assert.Nil(t, err) createdProjectIDs = append(createdProjectIDs, res.Project.Id) // ... rest of test ... }) // ... rest of test ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.goserver/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
server/.env.example(1 hunks)server/Makefile(1 hunks)server/e2e/common.go(4 hunks)server/e2e/proto_project_test.go(1 hunks)server/e2e/seeder.go(2 hunks)server/go.mod(1 hunks)server/internal/adapter/gql/loader_project.go(1 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/internalapi/internalapimodel/project.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/app/app.go(1 hunks)server/internal/app/config/config.go(1 hunks)server/internal/app/grpc.go(1 hunks)server/internal/app/server.go(3 hunks)server/internal/infrastructure/memory/project.go(1 hunks)server/internal/infrastructure/mongo/project.go(1 hunks)server/internal/usecase/interactor/common.go(1 hunks)server/internal/usecase/interactor/project.go(1 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/internal/usecase/repo/project.go(1 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- server/go.mod
- server/internal/adapter/gql/resolver_query.go
🚧 Files skipped from review as they are similar to previous changes (16)
- server/internal/app/app.go
- server/internal/usecase/interfaces/project.go
- server/Makefile
- server/internal/app/config/config.go
- server/internal/usecase/interactor/common.go
- server/.env.example
- server/e2e/seeder.go
- server/internal/usecase/interactor/project.go
- server/internal/usecase/repo/project.go
- server/internal/infrastructure/memory/project.go
- server/internal/adapter/internalapi/internalapimodel/project.go
- server/internal/app/server.go
- server/internal/adapter/internalapi/server.go
- server/internal/app/grpc.go
- server/internal/infrastructure/mongo/project.go
- server/internal/adapter/gql/loader_project.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/e2e/proto_project_test.go (3)
server/e2e/common.go (1)
GRPCServer(186-188)server/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.go (1)
ReEarthVisualizerClient(31-37)server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.go (16)
CreateProjectRequest(452-462)CreateProjectRequest(475-475)CreateProjectRequest(490-492)Visualizer(25-25)Visualizer(57-59)Visualizer(61-63)Visualizer(70-72)GetProjectRequest(408-413)GetProjectRequest(426-426)GetProjectRequest(441-443)Project(127-156)Project(169-169)Project(184-186)GetProjectListRequest(364-369)GetProjectListRequest(382-382)GetProjectListRequest(397-399)
🪛 Buf (1.47.2)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
🪛 ast-grep (0.31.1)
server/e2e/common.go
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy-server / deploy_test
🔇 Additional comments (4)
server/e2e/common.go (3)
45-55: LGTM: Well-structured configuration for internal API testsThe added configuration for internal API testing is clean and properly organized, including all necessary parameters (active state, port, and token).
129-155: LGTM: Good conditional server startup implementationThe server startup logic now elegantly handles both gRPC and standard HTTP serving modes based on the configuration. The error handling is thorough in both cases.
Regarding the static analysis warning about binding to all interfaces with
:0: this is acceptable in the test context as it's a standard practice to get a random available port for testing. The server is only used locally during tests and not exposed in production.🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
186-188: LGTM: Useful helper function for gRPC testingThe
GRPCServerfunction provides a clean abstraction for starting the gRPC server with the proper configuration for testing purposes.server/e2e/proto_project_test.go (1)
19-23: LGTM: Good error handling patternThe
SafeClosehelper function provides clean error handling when closing resources, preventing error leaks while logging any issues that occur.
| optional string name = 3; | ||
| optional string description = 4; | ||
| optional bool core_support = 5; | ||
| optional string visibility = 6; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Match visibility field type with Project message
For consistency with the recommendation to change the Project.visibility field to an enum, this field should also be changed.
- optional string visibility = 6;
+ optional Visibility visibility = 6;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| optional string visibility = 6; | |
| optional Visibility visibility = 6; |
hexaforce
force-pushed
the
feat/reearth-visualizer-proto2
branch
from
c37d7f5 to
84d252d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/e2e/common.go (1)
45-55: Consider using dynamic port allocation for test configurationThe
internalApiConfigdefines a hardcoded port "8080" for the internal API. This could lead to test failures if this port is already in use on the test system, particularly in CI environments where multiple tests might run in parallel.Consider using a dynamic port (or at least a less common port) to avoid potential conflicts:
internalApiConfig = &config.Config{ Origins: []string{"https://example.com"}, AuthSrv: config.AuthSrvConfig{ Disabled: true, }, InternalApi: config.InternalApiConfig{ Active: true, - Port: "8080", + Port: "0", // Use dynamic port allocation Token: "test", }, }Then update the implementation to determine the actual port after starting the server.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/.env.example(1 hunks)server/e2e/common.go(4 hunks)server/e2e/proto_project_test.go(1 hunks)server/internal/app/config/config.go(1 hunks)server/internal/app/server.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- server/e2e/proto_project_test.go
- server/internal/app/config/config.go
- server/.env.example
- server/internal/app/server.go
🧰 Additional context used
🪛 ast-grep (0.31.1)
server/e2e/common.go
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy-server / deploy_test
🔇 Additional comments (3)
server/e2e/common.go (3)
133-155: Clean implementation of conditional server startupThe implementation correctly handles different startup modes based on configuration. This approach elegantly supports both the existing GraphQL server and the new gRPC internal API.
🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
144-144: Listening on all interfaces is appropriate for test contextThe static analysis tool flagged this line for using ":0" which listens on all interfaces. This is actually a standard pattern in test code to let the OS assign a free port and is acceptable in this test context.
🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
186-188: Good addition of the GRPCServer helper functionThe new
GRPCServerfunction follows the existing pattern of server initialization functions, making it easy to set up a server with the internal API configuration for testing.
hexaforce
changed the title
hexaforce
force-pushed
the
feat/reearth-visualizer-proto2
branch
from
e92d40b to
178bcbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
server/internal/infrastructure/memory/project.go (1)
140-169:⚠️ Potential issueBug: FindVisibilityByWorkspace will include duplicates and mix visibility levels.
The current implementation has a logical issue that will lead to duplicate results and incorrect visibility filtering:
- If the workspace is not in the readable filter (lines 144-155), it adds public projects to the result and sorts them
- Regardless of the previous condition, it then adds ALL non-deleted projects from the workspace (lines 157-161)
- This means public projects will appear twice in the results, and the visibility filter will not be properly applied
Refactor the method to apply the visibility filter correctly:
func (r *Project) FindVisibilityByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, authenticated bool) ([]*project.Project, error) { r.lock.Lock() defer r.lock.Unlock() var result []*project.Project if r.f.Readable == nil || !r.f.Readable.Has(id) { for _, p := range r.data { if p.Workspace() == id && !p.IsDeleted() && p.Visibility() == "public" { result = append(result, p) } } - - sort.Slice(result, func(i, j int) bool { - return result[i].UpdatedAt().After(result[j].UpdatedAt()) - }) - - } - - for _, p := range r.data { - if p.Workspace() == id && !p.IsDeleted() { - result = append(result, p) - } - } + } else if authenticated { + for _, p := range r.data { + if p.Workspace() == id && !p.IsDeleted() { + result = append(result, p) + } + } + } else { + for _, p := range r.data { + if p.Workspace() == id && !p.IsDeleted() && p.Visibility() == "public" { + result = append(result, p) + } + } + } sort.Slice(result, func(i, j int) bool { return result[i].UpdatedAt().After(result[j].UpdatedAt()) }) return result, nil }Note: The
authenticatedparameter should be used to determine whether to return all projects or only public ones when the user has read access to the workspace.server/schemas/internalapi/v1/schema.proto (5)
3-3:⚠️ Potential issueFix package-directory mismatch reported by
buf
bufenforces that the proto package hierarchy mirrors the on-disk directory layout (reearth.visualizer.v1).
Placing the file underserver/schemas/internalapi/v1breaks that rule and will fail lint / code-gen.Either:
- Move the file to
server/schemas/reearth/visualizer/v1/schema.proto, or- Change the
packageoption to match the current path (e.g.internalapi.v1).Not fixing this will block CI in repos using
buf generate/buf lint.🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
7-7: 🛠️ Refactor suggestionProvide a fully-qualified
go_packageoption
option go_package = "proto/v1";yields import paths such asimport "proto/v1", which collide easily and violate Go module conventions.Specify the complete module path:
-option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/internal/adapter/internalapi/proto/v1;internalapiv1";This prevents ambiguous imports when the proto is consumed outside the repo.
28-29:⚠️ Potential issueAvoid returning raw Basic-Auth credentials
basic_auth_usernameand especiallybasic_auth_passwordare included in everyProjectresponse. Even for an internal API this creates unnecessary credential exposure risk and makes accidental leakage (logs, client dumps, etc.) more likely.Consider:
string basic_auth_username = 4; -string basic_auth_password = 5; +// Do NOT expose the password. If it must be handled, create a dedicated Write-only request field.Alternatively, move these credentials to a separate request/response that is protected by stricter ACLs.
49-49: 🛠️ Refactor suggestionModel
visibilityas an enum, not free-form stringAllowed values appear to be
public,limited,private. Defining an enum prevents typo bugs and simplifies client code.-string visibility = 25; +Visibility visibility = 25; + +enum Visibility { + VISIBILITY_UNSPECIFIED = 0; + VISIBILITY_PUBLIC = 1; + VISIBILITY_LIMITED = 2; + VISIBILITY_PRIVATE = 3; +}Update request/response types accordingly.
79-79: 🛠️ Refactor suggestionMatch visibility field type with Project message
For consistency with the recommendation to change the Project.visibility field to an enum, this field should also be changed.
- optional string visibility = 6; + optional Visibility visibility = 6;
🧹 Nitpick comments (1)
server/schemas/internalapi/v1/schema.proto (1)
21-21: Document the format of user-id metadataThe implementation note mentions that user-id should be passed in metadata, but doesn't specify the expected format. This can lead to integration issues.
-// UserId should be passed in metadata with key "user-id" +// UserId should be passed in metadata with key "user-id" as a string (UUID format)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
server/e2e/proto_project_test.go(1 hunks)server/internal/adapter/gql/loader_project.go(1 hunks)server/internal/adapter/gql/resolver_mutation_storytelling.go(3 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/app/server.go(3 hunks)server/internal/infrastructure/memory/project.go(1 hunks)server/internal/infrastructure/mongo/project.go(1 hunks)server/internal/usecase/interactor/project.go(1 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/internal/usecase/repo/project.go(1 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- server/internal/adapter/gql/resolver_query.go
- server/internal/adapter/gql/resolver_mutation_storytelling.go
- server/internal/adapter/gql/loader_project.go
- server/internal/usecase/repo/project.go
- server/internal/usecase/interfaces/project.go
- server/internal/infrastructure/mongo/project.go
- server/internal/adapter/internalapi/server.go
- server/internal/usecase/interactor/project.go
- server/e2e/proto_project_test.go
- server/internal/app/server.go
🧰 Additional context used
🪛 Buf (1.47.2)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy-server / deploy_test
| func (r *Project) FindActiveById(ctx context.Context, id id.ProjectID) (*project.Project, error) { | ||
| for _, p := range r.data { | ||
| if p.ID() == id && !p.IsDeleted() { | ||
| return p, nil | ||
| } | ||
| } | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Missing lock acquisition in FindActiveById
This method accesses the shared data map without acquiring a lock, which could lead to race conditions if the map is being modified concurrently by other methods. All other methods in this file follow the pattern of acquiring a lock before accessing the data.
Apply this diff to fix the race condition:
func (r *Project) FindActiveById(ctx context.Context, id id.ProjectID) (*project.Project, error) {
+ r.lock.Lock()
+ defer r.lock.Unlock()
+
for _, p := range r.data {
if p.ID() == id && !p.IsDeleted() {
return p, nil
}
}
return nil, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *Project) FindActiveById(ctx context.Context, id id.ProjectID) (*project.Project, error) { | |
| for _, p := range r.data { | |
| if p.ID() == id && !p.IsDeleted() { | |
| return p, nil | |
| } | |
| } | |
| return nil, nil | |
| } | |
| func (r *Project) FindActiveById(ctx context.Context, id id.ProjectID) (*project.Project, error) { | |
| r.lock.Lock() | |
| defer r.lock.Unlock() | |
| for _, p := range r.data { | |
| if p.ID() == id && !p.IsDeleted() { | |
| return p, nil | |
| } | |
| } | |
| return nil, nil | |
| } |
🤖 Prompt for AI Agents (early access)
In server/internal/infrastructure/memory/project.go around lines 131 to 138, the
FindActiveById method accesses the shared data map without acquiring a lock,
risking race conditions. To fix this, acquire the read lock at the start of the
method and defer its release before iterating over r.data. This ensures safe
concurrent access consistent with other methods in the file.
Missing permission check in FindActiveById
The method doesn't check if the caller has permission to read the project using the repository's workspace filter. This could allow unauthorized access to projects.
Apply this diff to add the permission check:
func (r *Project) FindActiveById(ctx context.Context, id id.ProjectID) (*project.Project, error) {
r.lock.Lock()
defer r.lock.Unlock()
for _, p := range r.data {
- if p.ID() == id && !p.IsDeleted() {
+ if p.ID() == id && !p.IsDeleted() && r.f.CanRead(p.Workspace()) {
return p, nil
}
}
return nil, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *Project) FindActiveById(ctx context.Context, id id.ProjectID) (*project.Project, error) { | |
| for _, p := range r.data { | |
| if p.ID() == id && !p.IsDeleted() { | |
| return p, nil | |
| } | |
| } | |
| return nil, nil | |
| } | |
| func (r *Project) FindActiveById(ctx context.Context, id id.ProjectID) (*project.Project, error) { | |
| r.lock.Lock() | |
| defer r.lock.Unlock() | |
| for _, p := range r.data { | |
| if p.ID() == id && !p.IsDeleted() && r.f.CanRead(p.Workspace()) { | |
| return p, nil | |
| } | |
| } | |
| return nil, nil | |
| } |
🤖 Prompt for AI Agents (early access)
In server/internal/infrastructure/memory/project.go around lines 131 to 138, the
FindActiveById method lacks a permission check to verify if the caller has
access to the project based on the repository's workspace filter. To fix this,
add a condition inside the loop to ensure the project matches the workspace
filter before returning it. This will prevent unauthorized access by only
returning projects the caller is permitted to read.
178bcbf to
e5f304c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
server/e2e/proto_project_test.go (1)
179-180:⚠️ Potential issueFix gRPC client connection creation.
The
grpc.NewClientfunction doesn't exist in the gRPC library. This should be replaced withgrpc.DialContext.- conn, err := grpc.NewClient("localhost:8080", grpc.WithTransportCredentials(insecure.NewCredentials())) + conn, err := grpc.DialContext(ctx, "localhost:8080", + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithBlock())server/schemas/internalapi/v1/schema.proto (4)
3-3:⚠️ Potential issuePackage directory mismatch.
The proto package is defined as
reearth.visualizer.v1but the file is located inserver/schemas/internalapi/v1. This mismatch violates Buf's PACKAGE_DIRECTORY_MATCH rule.Either:
- Move the file to
server/schemas/reearth/visualizer/v1/schema.proto, or- Change the package to match the current directory structure (e.g.,
server.schemas.internalapi.v1)🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
7-7: 🛠️ Refactor suggestionUse fully-qualified go_package option.
The current go_package option is not fully qualified, which can lead to import collisions.
-option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/internal/adapter/internalapi/schemas/internalapi/v1;internalapiv1";
28-29:⚠️ Potential issueAvoid exposing Basic Auth credentials in responses.
Including basic auth credentials (especially passwords) in every Project response creates security risks, even for internal APIs.
Consider removing the password field from responses or creating separate request/response types for credential management that have stricter access controls.
49-49: 🛠️ Refactor suggestionModel visibility as an enum instead of a string.
Using a string for visibility status allows for typos and inconsistencies. An enum would provide type safety and clearer documentation.
- string visibility = 25; + Visibility visibility = 25; + +enum Visibility { + VISIBILITY_UNSPECIFIED = 0; + VISIBILITY_PUBLIC = 1; + VISIBILITY_PRIVATE = 2; +}The same change should be applied to the CreateProjectRequest:
- optional string visibility = 6; + optional Visibility visibility = 6;Also applies to: 79-79
🧹 Nitpick comments (1)
server/e2e/proto_project_test.go (1)
73-76: Fix typos in test comments.There are typos in the comments explaining the test cases.
- // 0: creante seeder => null - // 1: creante default => private - // 2: creante private => private + // 0: created seeder => null + // 1: created default => private + // 2: created private => privateSimilar typos exist in the public test case comments.
Also applies to: 143-146
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (13)
server/e2e/proto_project_test.go(1 hunks)server/internal/adapter/gql/loader_project.go(1 hunks)server/internal/adapter/gql/resolver_mutation_storytelling.go(3 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/app/server.go(3 hunks)server/internal/infrastructure/memory/project.go(1 hunks)server/internal/infrastructure/mongo/project.go(1 hunks)server/internal/usecase/interactor/project.go(1 hunks)server/internal/usecase/interactor/storytelling.go(5 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/internal/usecase/repo/project.go(1 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- server/internal/usecase/repo/project.go
- server/internal/adapter/gql/resolver_query.go
- server/internal/usecase/interfaces/project.go
- server/internal/adapter/gql/resolver_mutation_storytelling.go
- server/internal/adapter/gql/loader_project.go
- server/internal/app/server.go
- server/internal/infrastructure/mongo/project.go
- server/internal/usecase/interactor/project.go
- server/internal/adapter/internalapi/server.go
- server/internal/infrastructure/memory/project.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/e2e/proto_project_test.go (6)
server/e2e/common.go (1)
GRPCServer(186-188)server/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.go (1)
ReEarthVisualizerClient(31-37)server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.go (19)
CreateProjectRequest(460-470)CreateProjectRequest(483-483)CreateProjectRequest(498-500)Visualizer(25-25)Visualizer(57-59)Visualizer(61-63)Visualizer(70-72)GetProjectRequest(416-421)GetProjectRequest(434-434)GetProjectRequest(449-451)Project(127-156)Project(169-169)Project(184-186)GetProjectListRequest(364-370)GetProjectListRequest(383-383)GetProjectListRequest(398-400)CreateProjectResponse(677-682)CreateProjectResponse(695-695)CreateProjectResponse(710-712)server/internal/usecase/repo/project.go (1)
Project(19-35)server/internal/usecase/repo/container.go (1)
Container(21-40)server/pkg/id/id.go (1)
ProjectIDFrom(100-100)
🪛 Buf (1.47.2)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy-server / deploy_test
🔇 Additional comments (12)
server/internal/usecase/interactor/storytelling.go (5)
91-92: Added scene-based filtering to the story creation process.The introduction of the Filter function with SceneID parameter enables proper context-based filtering when creating stories, ensuring operations are scoped to the specific scene.
111-112: Proper filtering implementation for story saving.The filtered repository is now used for saving stories, ensuring consistent filtering across operations.
500-502: Added scene-based filtering to page operations.The implementation ensures that all page-related operations are properly scoped to the correct scene context, maintaining data integrity across concurrent operations.
508-512: Filtered property creation for pages.Using the scene filter when creating page properties ensures proper scope and permission handling consistent with the story context.
561-563: Consistent filtering for story saving after page operations.The filtered repository ensures operations maintain scene-scoped context when saving stories after page modifications.
server/e2e/proto_project_test.go (5)
27-29: Helpful test execution instructions.The comment provides clear instructions for running the tests, which helps other developers understand how to execute these tests.
21-25: Well-implemented SafeClose helper.The SafeClose function properly handles potential errors when closing connections, logging warnings instead of propagating errors that can't be handled by the caller.
189-200: Comprehensive project validation helper.The
checProjectfunction thoroughly verifies that all related entities (project, scene, storytelling) are properly created, ensuring the gRPC endpoint for project creation works as expected.
33-96: Well-structured tests for visibility scenarios.The test cases effectively verify both private and public visibility behaviors, confirming that:
- Default projects are created with private visibility
- Explicit visibility settings are properly persisted
- Authenticated users see all projects regardless of visibility
- Unauthenticated users only see public projects
This thorough testing ensures the visibility feature works as expected.
Also applies to: 103-166
170-187: Good test helper with proper authentication metadata.The
runTestWithUserfunction correctly sets up the authentication context with token and user ID, allowing tests to run with proper authentication context.server/schemas/internalapi/v1/schema.proto (2)
17-21: Good documentation of authentication mechanism.The comments clearly document how authentication should be implemented using gRPC interceptors, which is helpful for developers implementing clients for this API.
9-15: Clear and concise service definition.The service definition is well-organized with logical method names and appropriate request/response message pairs for each operation.
| fmt.Println("=================== Error i.addNewProperty(") | ||
| return nil, err |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug println statements before production deployment.
These debug print statements were likely added during development for troubleshooting but should be removed in production code.
- fmt.Println("=================== Error i.addNewProperty(")
+ // Error during property creation
- fmt.Println("=================== Error i.storytellingRepo.Filtered(")
+ // Error during story saving
- fmt.Println("------------------- Error addNewProperty1(")
+ // Error during unfiltered property saving
- fmt.Println("------------------- Error addNewProperty2(")
+ // Error during filtered property savingAlso applies to: 112-113, 1219-1220, 1224-1225
🤖 Prompt for AI Agents
In server/internal/usecase/interactor/storytelling.go at lines 94-95, 112-113,
1219-1220, and 1224-1225, remove all debug println statements such as
fmt.Println used for troubleshooting. These debug prints should be eliminated
before production deployment to keep the code clean and avoid unnecessary
console output.
38837e0 to
6b6f9ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
server/schemas/internalapi/v1/schema.proto (4)
7-7: Use a fully-qualifiedgo_package
The currentoption go_package = "proto/v1";is ambiguous and collides with other imports.
Please follow Go module conventions by specifying the full module path, e.g.:-option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/schemas/internalapi/v1;internalapiv1";This avoids import conflicts when consuming the generated code.
28-29: Avoid exposing raw credentials in responses
Returningbasic_auth_usernameand especiallybasic_auth_passwordin theProjectmessage risks accidental credential leakage (logs, client dumps).
Consider:- string basic_auth_password = 5; + // Write-only: clients should not receive the password. + // string basic_auth_password = 5;Or move credentials to a separate, strictly protected RPC.
49-49: Modelvisibilityas an enum
Free-form string fields are error-prone. Define aVisibilityenum:- string visibility = 25; + Visibility visibility = 25; + + enum Visibility { + VISIBILITY_UNSPECIFIED = 0; + VISIBILITY_PUBLIC = 1; + VISIBILITY_LIMITED = 2; + VISIBILITY_PRIVATE = 3; + }This ensures clients use valid values only.
73-80: AlignCreateProjectRequest.visibilitywithProject.visibility
IfProject.visibilitybecomes an enum (as suggested), this field should also useVisibilityrather thanstring:- optional string visibility = 6; + optional Visibility visibility = 6;
🧹 Nitpick comments (2)
server/schemas/internalapi/v1/schema.proto (2)
17-22: Standardize authentication notes
Consider converting these implementation notes into protobuf comments on the service or method options (e.g., using//above each RPC oroption (google.api.http) = { … }) so they appear in generated docs.
65-69: Reevaluate filtering by explicitauthenticatedflag
Passingauthenticatedin payload duplicates interceptor/metadata logic.
You may infer authentication purely via gRPC metadata (i.e., presence of valid M2M token) instead of a boolean flag in the request.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (13)
server/e2e/proto_project_test.go(1 hunks)server/internal/adapter/gql/loader_project.go(1 hunks)server/internal/adapter/gql/resolver_mutation_storytelling.go(3 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/app/server.go(3 hunks)server/internal/infrastructure/memory/project.go(1 hunks)server/internal/infrastructure/mongo/project.go(1 hunks)server/internal/usecase/interactor/project.go(1 hunks)server/internal/usecase/interactor/storytelling.go(4 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/internal/usecase/repo/project.go(1 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- server/internal/adapter/gql/resolver_query.go
- server/internal/usecase/interfaces/project.go
- server/internal/adapter/gql/loader_project.go
- server/internal/usecase/repo/project.go
- server/internal/app/server.go
- server/internal/infrastructure/mongo/project.go
- server/internal/usecase/interactor/storytelling.go
- server/internal/usecase/interactor/project.go
- server/internal/adapter/gql/resolver_mutation_storytelling.go
- server/internal/infrastructure/memory/project.go
- server/internal/adapter/internalapi/server.go
- server/e2e/proto_project_test.go
🧰 Additional context used
🪛 Buf (1.47.2)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-server / ci-server-test
🔇 Additional comments (3)
server/schemas/internalapi/v1/schema.proto (3)
9-15: gRPC service definition looks good
The four RPCs (GetProjectList,GetProject,CreateProject,DeleteProject) are defined correctly with clear request/response types.
52-63: Enums for Visualizer & PublishmentStatus are well-defined
Using enums here prevents magic strings for visualization type and publishment state. Good practice.
82-91: Delete & Response messages look correct
DeleteProjectRequest,GetProjectResponse,GetProjectListResponse,CreateProjectResponse, andDeleteProjectResponsedefinitions follow conventions and map neatly to operations.
| syntax = "proto3"; | ||
|
|
||
| package reearth.visualizer.v1; | ||
|
|
||
| import "google/protobuf/timestamp.proto"; | ||
|
|
||
| option go_package = "proto/v1"; | ||
|
|
There was a problem hiding this comment.
Enforce package-directory consistency and validate Buf linting
Buf requires the proto package option to mirror the file path. Currently:
package reearth.visualizer.v1;- File is under
server/schemas/internalapi/v1/schema.proto
This will fail buf lint with PACKAGE_DIRECTORY_MATCH.
Either:
- Move the file to
server/schemas/reearth/visualizer/v1/schema.proto, - Or change the
packageto match the on-disk path (e.g.server.schemas.internalapi.v1).
🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
🤖 Prompt for AI Agents
In server/schemas/internalapi/v1/schema.proto at lines 1 to 8, the proto package
declaration 'reearth.visualizer.v1' does not match the file path, causing Buf
lint errors. To fix this, either move the schema.proto file to the directory
path server/schemas/reearth/visualizer/v1/ to match the package, or update the
package declaration to 'server.schemas.internalapi.v1' to reflect the current
file location.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
server/schemas/internalapi/v1/schema.proto (2)
51-52:⚠️ Potential issueAvoid exposing raw Basic-Auth credentials in responses
Including
basic_auth_password(and evenbasic_auth_username) in everyProjectresponse poses a security risk. At minimum, remove the password field from the response or move it to a dedicated write-only RPC/field. For example:message Project { - string basic_auth_username = 23; - string basic_auth_password = 24; + string basic_auth_username = 23; + // Write-only; never exposed in Project responses: + // optional string basic_auth_password = 24; }
3-8:⚠️ Potential issueEnsure Buf package-directory consistency and fully-qualified Go import path
The
package reearth.visualizer.v1;does not match the file path (server/schemas/internalapi/v1), causing Buf lint errors (PACKAGE_DIRECTORY_MATCH), and thego_packageis too generic.
Either:
- Move this file under
server/schemas/reearth/visualizer/v1/schema.prototo align withreearth.visualizer.v1.- Or update the package to reflect its directory (
server.schemas.internalapi.v1) and set a fully qualifiedgo_package, for example:-package reearth.visualizer.v1; +package server.schemas.internalapi.v1; -option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/schemas/internalapi/v1;internalapiv1";🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
🧹 Nitpick comments (1)
server/schemas/internalapi/v1/schema.proto (1)
7-7: Consider adding language-specificoptionsettingsFor broader multi-language support, you might include additional protobuf options alongside
go_package, such as:option java_package = "io.reearth.visualizer.v1"; option java_multiple_files = true; option csharp_namespace = "Reearth.Visualizer.V1";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (1)
server/schemas/internalapi/v1/schema.proto(1 hunks)
🧰 Additional context used
🪛 Buf (1.47.2)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-server / ci-server-test
hexaforce
force-pushed
the
feat/reearth-visualizer-proto2
branch
from
6d82c1e to
9e7a519
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
server/internal/app/grpc.go (2)
65-86: Return proper gRPC status codes & align metadata key (authorization)
Same concern as in the previous review: returning a bareerrors.New("unauthorized")maps tocodes.Unknown, which clients cannot distinguish from internal errors. Usestatus.Error(codes.Unauthenticated, …)orcodes.PermissionDenied.Additionally, the spec (see schema.proto lines 18-20) says the bearer token comes in the
authorizationheader, buttokenFromGrpcMetadatalooks form2m-token. This mismatch makes every request fail once the interceptor is turned on.- md, ok := metadata.FromIncomingContext(ctx) + md, ok := metadata.FromIncomingContext(ctx) if !ok { - log.Errorf("unaryAuthInterceptor: no metadata found") - return nil, errors.New("unauthorized") + log.Warn("unaryAuthInterceptor: no metadata found") + return nil, status.Error(codes.Unauthenticated, "missing metadata") } @@ - if token != cfg.Config.InternalApi.Token { - log.Errorf("unaryAuthInterceptor: invalid token") - return nil, errors.New("unauthorized") + if token != cfg.Config.InternalApi.Token { + log.Warn("unaryAuthInterceptor: invalid token") + return nil, status.Error(codes.PermissionDenied, "invalid token") }Update
tokenFromGrpcMetadatato use"authorization"(see comment below).🧰 Tools
🪛 golangci-lint (1.64.8)
65-65: func
unaryAuthInterceptoris unused(unused)
🪛 GitHub Check: ci-server / ci-server-lint
[failure] 65-65:
funcunaryAuthInterceptoris unused (unused)
90-99:user-idshould be optional to support anonymous listing
The PR description explicitly allows unauthenticated users to callGetProjectList, yet this interceptor rejects every request that lacks auser-id. Treat it as optional: attach an operator only when present.- if len(md["user-id"]) < 1 { - log.Errorf("unaryAttachOperatorInterceptor: no user id found") - return nil, status.Error(codes.Unauthenticated, "user id missing") - } + if len(md["user-id"]) < 1 { + // proceed anonymously + return handler(ctx, req) + }server/schemas/internalapi/v1/schema.proto (1)
7-7: 🛠️ Refactor suggestionSet a fully-qualified
go_packageto avoid import collisions-option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/internal/adapter/internalapi/proto/v1;internalapiv1";This produces stable, module-aware import paths.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
server/e2e/proto_project_test.go(1 hunks)server/e2e/seeder.go(3 hunks)server/internal/adapter/gql/loader_project.go(1 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/app/grpc.go(1 hunks)server/internal/infrastructure/memory/project.go(1 hunks)server/internal/infrastructure/mongo/project.go(1 hunks)server/internal/usecase/interactor/project.go(1 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/internal/usecase/repo/project.go(1 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- server/e2e/seeder.go
- server/internal/adapter/gql/resolver_query.go
- server/internal/adapter/gql/loader_project.go
- server/internal/usecase/interfaces/project.go
- server/internal/usecase/repo/project.go
- server/internal/usecase/interactor/project.go
- server/internal/infrastructure/memory/project.go
- server/internal/infrastructure/mongo/project.go
- server/internal/adapter/internalapi/server.go
- server/e2e/proto_project_test.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
server/internal/app/grpc.go
65-65: func unaryAuthInterceptor is unused
(unused)
156-156: func tokenFromGrpcMetadata is unused
(unused)
🪛 GitHub Check: ci-server / ci-server-lint
server/internal/app/grpc.go
[failure] 156-156:
func tokenFromGrpcMetadata is unused (unused)
[failure] 65-65:
func unaryAuthInterceptor is unused (unused)
🪛 Buf (1.47.2)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-server / ci-server-test
- GitHub Check: test
| ui := grpc.ChainUnaryInterceptor( | ||
| unaryLogInterceptor(cfg), | ||
| // unaryAuthInterceptor(cfg), // TODO: When using M2M token | ||
| unaryAttachOperatorInterceptor(cfg), | ||
| unaryAttachUsecaseInterceptor(cfg), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
unaryAuthInterceptor is commented out → linter failures & unauthenticated access
unaryAuthInterceptor and tokenFromGrpcMetadata are now reported as unused by golangci-lint, and every call reaches business logic without any M2M token validation.
Unless anonymous access is intentionally allowed for all RPCs, please either:
- Re-enable the interceptor when
cfg.Config.InternalApi.Activeis true, or - Remove the dead code (and add a
//nolint:unusedonly if you plan to re-introduce it very shortly).
Example re-enable (guarded by config):
ui := grpc.ChainUnaryInterceptor(
unaryLogInterceptor(cfg),
- // unaryAuthInterceptor(cfg), // TODO: When using M2M token
+ unaryAuthInterceptor(cfg),
unaryAttachOperatorInterceptor(cfg),
unaryAttachUsecaseInterceptor(cfg),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ui := grpc.ChainUnaryInterceptor( | |
| unaryLogInterceptor(cfg), | |
| // unaryAuthInterceptor(cfg), // TODO: When using M2M token | |
| unaryAttachOperatorInterceptor(cfg), | |
| unaryAttachUsecaseInterceptor(cfg), | |
| ) | |
| ui := grpc.ChainUnaryInterceptor( | |
| unaryLogInterceptor(cfg), | |
| unaryAuthInterceptor(cfg), | |
| unaryAttachOperatorInterceptor(cfg), | |
| unaryAttachUsecaseInterceptor(cfg), | |
| ) |
🤖 Prompt for AI Agents
In server/internal/app/grpc.go around lines 28 to 33, the unaryAuthInterceptor
is commented out causing linter errors and allowing unauthenticated access to
all RPCs. To fix this, re-enable unaryAuthInterceptor conditionally based on
cfg.Config.InternalApi.Active by adding it to the ChainUnaryInterceptor only
when this config flag is true. This will ensure M2M token validation is enforced
when required and remove the unused code linter warnings.
server/internal/app/grpc.go
Outdated
| func tokenFromGrpcMetadata(md metadata.MD) string { | ||
| // The keys within metadata.MD are normalized to lowercase. | ||
| // See: https://godoc.org/google.golang.org/grpc/metadata#New | ||
| if len(md["m2m-token"]) < 1 { | ||
| return "" | ||
| } | ||
| token := md["m2m-token"][0] | ||
| if !strings.HasPrefix(token, "Bearer ") { | ||
| return "" | ||
| } | ||
| token = strings.TrimPrefix(md["m2m-token"][0], "Bearer ") | ||
| if token == "" { | ||
| return "" | ||
| } | ||
| return token |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Metadata key & parsing bug in tokenFromGrpcMetadata
- The interceptor spec (schema comment) uses
authorization, notm2m-token. - The value is re-read from
md["m2m-token"][0]after trimming, which is redundant. - Consider case-insensitive handling (
md.Get("authorization")).
Suggested fix:
-func tokenFromGrpcMetadata(md metadata.MD) string {
- if len(md["m2m-token"]) < 1 {
+func tokenFromGrpcMetadata(md metadata.MD) string {
+ vals := md.Get("authorization") // returns []string
+ if len(vals) < 1 {
return ""
}
- token := md["m2m-token"][0]
+ token := vals[0]
if !strings.HasPrefix(token, "Bearer ") {
return ""
}
- token = strings.TrimPrefix(md["m2m-token"][0], "Bearer ")
+ token = strings.TrimPrefix(token, "Bearer ")
return strings.TrimSpace(token)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func tokenFromGrpcMetadata(md metadata.MD) string { | |
| // The keys within metadata.MD are normalized to lowercase. | |
| // See: https://godoc.org/google.golang.org/grpc/metadata#New | |
| if len(md["m2m-token"]) < 1 { | |
| return "" | |
| } | |
| token := md["m2m-token"][0] | |
| if !strings.HasPrefix(token, "Bearer ") { | |
| return "" | |
| } | |
| token = strings.TrimPrefix(md["m2m-token"][0], "Bearer ") | |
| if token == "" { | |
| return "" | |
| } | |
| return token | |
| func tokenFromGrpcMetadata(md metadata.MD) string { | |
| // The keys within metadata.MD are normalized to lowercase. | |
| // See: https://godoc.org/google.golang.org/grpc/metadata#New | |
| vals := md.Get("authorization") // returns []string | |
| if len(vals) < 1 { | |
| return "" | |
| } | |
| token := vals[0] | |
| if !strings.HasPrefix(token, "Bearer ") { | |
| return "" | |
| } | |
| token = strings.TrimPrefix(token, "Bearer ") | |
| return strings.TrimSpace(token) | |
| } |
🧰 Tools
🪛 golangci-lint (1.64.8)
156-156: func tokenFromGrpcMetadata is unused
(unused)
🪛 GitHub Check: ci-server / ci-server-lint
[failure] 156-156:
func tokenFromGrpcMetadata is unused (unused)
🤖 Prompt for AI Agents
In server/internal/app/grpc.go between lines 156 and 170, the function
tokenFromGrpcMetadata incorrectly uses the metadata key "m2m-token" instead of
"authorization" as per the interceptor spec. Also, it redundantly rereads the
token value after trimming the "Bearer " prefix and does not handle
case-insensitive key lookup. To fix this, replace all occurrences of "m2m-token"
with "authorization", use md.Get("authorization") for case-insensitive
retrieval, and avoid rereading the token after trimming by trimming the already
retrieved token variable.
| bool core_support = 11; | ||
| bool starred = 12; | ||
| bool is_deleted = 13; | ||
| string visibility = 14; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
visibility should be an enum, not free-form string
Using string visibility invites typos and inconsistent clients. Define a Visibility enum once and reference it here and in CreateProjectRequest.
- string visibility = 14;
+ Visibility visibility = 14;
...
- optional string visibility = 6;
+ optional Visibility visibility = 6;
+
+enum Visibility {
+ VISIBILITY_UNSPECIFIED = 0;
+ VISIBILITY_PUBLIC = 1;
+ VISIBILITY_LIMITED = 2;
+ VISIBILITY_PRIVATE = 3;
+}Also applies to: 81-82
🤖 Prompt for AI Agents
In server/schemas/internalapi/v1/schema.proto at line 40 and also lines 81-82,
the field `visibility` is currently defined as a string, which risks typos and
inconsistent usage. Define a new enum type called `Visibility` with the allowed
visibility values, then change the `visibility` field type from string to this
`Visibility` enum in both places. This ensures consistent and validated usage of
visibility values across the schema.
| @@ -0,0 +1,93 @@ | |||
| syntax = "proto3"; | |||
|
|
|||
| package reearth.visualizer.v1; | |||
There was a problem hiding this comment.
Buf PACKAGE_DIRECTORY_MATCH violation
package reearth.visualizer.v1; requires the file to live under reearth/visualizer/v1/.
Either move the file accordingly or change the package path to mirror the current directory (e.g. server.schemas.internalapi.v1).
🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
🤖 Prompt for AI Agents
In server/schemas/internalapi/v1/schema.proto at line 3, the package declaration
`reearth.visualizer.v1` does not match the file's directory path, causing a Buf
PACKAGE_DIRECTORY_MATCH violation. To fix this, either move the schema.proto
file to the directory path `reearth/visualizer/v1/` to match the package or
update the package declaration to `server.schemas.internalapi.v1` to reflect the
current directory structure.
| bool is_basic_auth_active = 22; | ||
| string basic_auth_username = 23; | ||
| string basic_auth_password = 24; | ||
| bool enable_ga = 25; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid returning raw Basic-Auth passwords
basic_auth_password in every Project response dramatically increases leakage risk (logs, dumps, etc.). Make it write-only (request-only) or serve it via a dedicated secured endpoint.
At minimum, remove it from Project and only include a boolean flag indicating whether basic auth is enabled.
🤖 Prompt for AI Agents
In server/schemas/internalapi/v1/schema.proto around lines 50 to 53, the field
basic_auth_password exposes raw Basic-Auth passwords in Project responses,
increasing security risks. Remove the basic_auth_password field from the Project
response message entirely and replace it with only a boolean flag indicating if
basic auth is enabled. If password input is needed, make it write-only or serve
it through a separate secured endpoint to avoid exposing sensitive data.
b91e55e to
dd81658
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
server/internal/app/grpc.go (4)
65-86: 🛠️ Refactor suggestionReturn proper gRPC status codes for authentication failures
Generic
errors.New("unauthorized")calls are converted tocodes.Unknownon the wire, preventing clients from distinguishing between auth failures and server bugs.Use appropriate status codes with meaningful error messages:
- log.Errorf("unaryAuthInterceptor: no metadata found") - return nil, errors.New("unauthorized") + log.Warn("unaryAuthInterceptor: no metadata found") + return nil, status.Error(codes.Unauthenticated, "missing metadata") ... - log.Errorf("unaryAuthInterceptor: invalid token") - return nil, errors.New("unauthorized") + log.Warn("unaryAuthInterceptor: invalid token") + return nil, status.Error(codes.Unauthenticated, "invalid token")Also add missing import:
"google.golang.org/grpc/status"🧰 Tools
🪛 golangci-lint (1.64.8)
65-65: func
unaryAuthInterceptoris unused(unused)
🪛 GitHub Check: ci-server / ci-server-lint
[failure] 65-65:
funcunaryAuthInterceptoris unused (unused)
90-104: 🛠️ Refactor suggestionMake
user-idheader optional to support anonymous accessThe PR description indicates that unauthenticated users should be able to access public projects via
GetProjectList, but this interceptor rejects all requests without a user ID.Make the user-id header optional and proceed anonymously when missing:
- if len(md["user-id"]) < 1 { - log.Errorf("unaryAttachOperatorInterceptor: no user id found") - return nil, errors.New("unauthorized") + if len(md["user-id"]) < 1 { + // proceed anonymously + return handler(ctx, req)Also, update error handling to use appropriate status codes:
- return nil, errors.New("unauthorized") + return nil, status.Error(codes.Unauthenticated, "user id missing")
156-170: 🛠️ Refactor suggestionFix metadata key & parsing in
tokenFromGrpcMetadataThe function has several issues:
- Uses
m2m-tokenbut the schema comment (line 20) specifiesauthorization- Unnecessarily re-reads the token after trimming
- Doesn't handle case-insensitive header names
func tokenFromGrpcMetadata(md metadata.MD) string { - if len(md["m2m-token"]) < 1 { + vals := md.Get("authorization") // returns []string, case-insensitive + if len(vals) < 1 { return "" } - token := md["m2m-token"][0] + token := vals[0] if !strings.HasPrefix(token, "Bearer ") { return "" } - token = strings.TrimPrefix(md["m2m-token"][0], "Bearer ") + token = strings.TrimPrefix(token, "Bearer ") return strings.TrimSpace(token) }🧰 Tools
🪛 golangci-lint (1.64.8)
156-156: func
tokenFromGrpcMetadatais unused(unused)
🪛 GitHub Check: ci-server / ci-server-lint
[failure] 156-156:
functokenFromGrpcMetadatais unused (unused)
28-33: 🛠️ Refactor suggestionEnable authentication interceptor or fix unused code warnings
The
unaryAuthInterceptoris commented out with a TODO, resulting in:
- Missing M2M token validation (all RPCs accessible without authentication)
- Unused function warnings from the linter
Since this is a new internal API, you should either:
ui := grpc.ChainUnaryInterceptor( unaryLogInterceptor(cfg), - // unaryAuthInterceptor(cfg), // TODO: When using M2M token + unaryAuthInterceptor(cfg), unaryAttachOperatorInterceptor(cfg), unaryAttachUsecaseInterceptor(cfg), )Or if you intentionally want to defer token validation, add linter directives to prevent warnings:
+//nolint:unused func unaryAuthInterceptor(cfg *ServerConfig) grpc.UnaryServerInterceptor { // ... } +//nolint:unused func tokenFromGrpcMetadata(md metadata.MD) string { // ... }server/e2e/proto_project_test.go (1)
187-187:⚠️ Potential issueFix compilation error:
grpc.NewClientdoes not existThe function
grpc.NewClientdoesn't exist in the gRPC package - this code won't compile.Use
grpc.DialContextinstead:-conn, err := grpc.NewClient("localhost:8080", grpc.WithTransportCredentials(insecure.NewCredentials())) +conn, err := grpc.DialContext(ctx, "localhost:8080", + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithBlock())Also, consider adding timeout handling to prevent tests from hanging indefinitely if the server is unavailable.
server/schemas/internalapi/v1/schema.proto (4)
7-7: 🛠️ Refactor suggestionProvide a fully-qualified Go package path
The current
go_packageoption can cause import collisions since it's not fully qualified.-option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/internal/adapter/internalapi/schemas/internalapi/v1;internalapiv1";This ensures the generated Go code has an unambiguous import path, especially when referenced from other repositories.
40-40: 🛠️ Refactor suggestionModel
visibilityas an enum, not a free-form stringUsing string for visibility invites typos and inconsistency. Valid values appear to be "public", "private", etc.
Define a
Visibilityenum and use it consistently:- string visibility = 14; + Visibility visibility = 14; // And later in the file: +enum Visibility { + VISIBILITY_UNSPECIFIED = 0; + VISIBILITY_PUBLIC = 1; + VISIBILITY_LIMITED = 2; + VISIBILITY_PRIVATE = 3; +} // Also update in CreateProjectRequest: - optional string visibility = 6; + optional Visibility visibility = 6;This provides type safety, consistent client behavior, and better documentation.
50-52: 🛠️ Refactor suggestionAvoid storing and returning raw Basic Auth credentials
Including
basic_auth_passwordin everyProjectresponse increases credential exposure risk.Either:
- Remove the password from responses entirely, only including a flag indicating if basic auth is enabled
- Move credential retrieval to a separate dedicated endpoint with stricter access controls
- bool is_basic_auth_active = 22; - string basic_auth_username = 23; - string basic_auth_password = 24; + bool is_basic_auth_active = 22; + string basic_auth_username = 23; + // Password intentionally omitted from responsesIf needed, create a separate dedicated RPC for credential management.
3-4:⚠️ Potential issueFix package name to match directory structure
The package
reearth.visualizer.v1doesn't match the file location, causing Buf lint errors.Either:
- Move this file to
reearth/visualizer/v1/schema.prototo match the package name, or- Change the package name to match the current directory:
-package reearth.visualizer.v1; +package server.schemas.internalapi.v1;This is flagged by Buf as a
PACKAGE_DIRECTORY_MATCHerror.🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
🧹 Nitpick comments (2)
server/e2e/proto_project_test.go (2)
107-115: DRY: Extract project creation params into helper functionYou're duplicating the request parameters for creating projects across multiple test cases, which could lead to maintenance issues.
Consider extracting the common parameters into a helper function:
+func createProjectRequest(teamID, visibility string) *pb.CreateProjectRequest { + return &pb.CreateProjectRequest{ + TeamId: teamID, + Visualizer: pb.Visualizer_VISUALIZER_CESIUM, + Name: lo.ToPtr("Test Project1"), + Description: lo.ToPtr("Test Description1"), + CoreSupport: lo.ToPtr(true), + Visibility: lo.ToPtr(visibility), + } +} // Then in your test: CreateProjectInternal( t, ctx, r, client, "private", - &pb.CreateProjectRequest{ - TeamId: wID.String(), - Visualizer: pb.Visualizer_VISUALIZER_CESIUM, - Name: lo.ToPtr("Test Project1"), - Description: lo.ToPtr("Test Description1"), - CoreSupport: lo.ToPtr(true), - Visibility: lo.ToPtr("private"), - }) + createProjectRequest(wID.String(), "private"))
148-149: Add type-specific assertion for permission errorsThe error checking here relies on string comparison which is brittle and might break if error messages change.
Use proper gRPC status code checking instead:
-assert.Equal(t, "rpc error: code = Unknown desc = operation denied", err.Error()) +st, ok := status.FromError(err) +assert.True(t, ok, "Expected gRPC status error") +assert.Equal(t, codes.PermissionDenied, st.Code(), "Expected permission denied error code") +assert.Contains(t, st.Message(), "operation denied")This requires adding the following imports:
"google.golang.org/grpc/status" "google.golang.org/grpc/codes"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
server/e2e/proto_project_test.go(1 hunks)server/e2e/seeder.go(3 hunks)server/internal/adapter/gql/loader_project.go(1 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/app/grpc.go(1 hunks)server/internal/infrastructure/memory/project.go(1 hunks)server/internal/infrastructure/mongo/project.go(1 hunks)server/internal/usecase/interactor/project.go(1 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/internal/usecase/repo/project.go(1 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- server/internal/usecase/repo/project.go
- server/internal/adapter/gql/resolver_query.go
- server/e2e/seeder.go
- server/internal/adapter/gql/loader_project.go
- server/internal/infrastructure/mongo/project.go
- server/internal/usecase/interfaces/project.go
- server/internal/usecase/interactor/project.go
- server/internal/infrastructure/memory/project.go
- server/internal/adapter/internalapi/server.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
server/internal/app/grpc.go
65-65: func unaryAuthInterceptor is unused
(unused)
156-156: func tokenFromGrpcMetadata is unused
(unused)
🪛 GitHub Check: ci-server / ci-server-lint
server/internal/app/grpc.go
[failure] 156-156:
func tokenFromGrpcMetadata is unused (unused)
[failure] 65-65:
func unaryAuthInterceptor is unused (unused)
🪛 Buf (1.47.2)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-server / ci-server-test
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
server/internal/adapter/internalapi/client.go (1)
23-24: Consider making HTTPS and port configurable.The audience URL and port are hard-coded to use HTTPS and port 443, which limits flexibility for testing or different deployment environments.
-audience := "https://" + endpoint -host := endpoint + ":443" +// Allow overriding the scheme and port via parameters or use sensible defaults +audience := "https://" + endpoint +host := endpoint + ":443"Consider updating the function signature to accept these as parameters with defaults:
func InvokeApi( ctx context.Context, userID string, endpoint string, scheme string, // Default to "https" port string, // Default to "443" handleFunc func(client pb.ReEarthVisualizerClient, ctx context.Context), ) { if scheme == "" { scheme = "https" } if port == "" { port = "443" } audience := scheme + "://" + endpoint host := endpoint + ":" + port // ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
server/internal/adapter/internalapi/client.go(1 hunks)server/internal/usecase/interactor/project.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/usecase/interactor/project.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/internal/adapter/internalapi/client.go (1)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.go (2)
ReEarthVisualizerClient(31-37)NewReEarthVisualizerClient(43-45)
🪛 ast-grep (0.31.1)
server/internal/adapter/internalapi/client.go
[warning] 34-34: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-server / ci-server-test
- GitHub Check: test
| if err != nil { | ||
| log.Fatalf("failed to get ID token source: %v", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fatal error handling prevents proper error propagation.
Using log.Fatalf terminates the program, preventing the caller from handling the error. Consider returning errors instead.
-ts, err := idtoken.NewTokenSource(ctx, audience)
-if err != nil {
- log.Fatalf("failed to get ID token source: %v", err)
-}
+ts, err := idtoken.NewTokenSource(ctx, audience)
+if err != nil {
+ log.Printf("failed to get ID token source: %v", err)
+ return
+}Additionally, consider modifying the function signature to return errors:
func InvokeApi(
ctx context.Context,
userID string,
endpoint string,
handleFunc func(client pb.ReEarthVisualizerClient, ctx context.Context) error,
) error {
// ...
if err := handleFunc(client, ctx); err != nil {
return err
}
return nil
}🤖 Prompt for AI Agents
In server/internal/adapter/internalapi/client.go around lines 31 to 33, replace
the use of log.Fatalf with returning the error to allow proper error
propagation. Modify the function signature to return an error, and update the
error handling to return the error instead of terminating the program. Ensure
all callers handle the returned error appropriately.
| if err != nil { | ||
| log.Fatalf("failed to create gRPC client: %v", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fatal error handling prevents proper error propagation.
Similar to the previous comment, using log.Fatalf terminates the program instead of returning the error to the caller.
-if err != nil {
- log.Fatalf("failed to create gRPC client: %v", err)
-}
+if err != nil {
+ log.Printf("failed to create gRPC client: %v", err)
+ return
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/internal/adapter/internalapi/client.go around lines 42 to 44, replace
the use of log.Fatalf which terminates the program on error with proper error
propagation by returning the error to the caller. Modify the function to return
the error instead of calling log.Fatalf, allowing the caller to handle the error
appropriately.
| conn, err := grpc.NewClient(host, | ||
| grpc.WithTransportCredentials(tlsCreds), | ||
| grpc.WithPerRPCCredentials(callCreds), | ||
| ) |
There was a problem hiding this comment.
Critical error: grpc.NewClient is not a valid gRPC function.
The grpc.NewClient function doesn't exist in the gRPC library. You should use grpc.Dial instead to create a connection.
-conn, err := grpc.NewClient(host,
+conn, err := grpc.Dial(host,
grpc.WithTransportCredentials(tlsCreds),
grpc.WithPerRPCCredentials(callCreds),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| conn, err := grpc.NewClient(host, | |
| grpc.WithTransportCredentials(tlsCreds), | |
| grpc.WithPerRPCCredentials(callCreds), | |
| ) | |
| conn, err := grpc.Dial(host, | |
| grpc.WithTransportCredentials(tlsCreds), | |
| grpc.WithPerRPCCredentials(callCreds), | |
| ) |
🤖 Prompt for AI Agents
In server/internal/adapter/internalapi/client.go around lines 38 to 41, replace
the invalid grpc.NewClient call with grpc.Dial to create the gRPC connection.
Use grpc.Dial with the host and appropriate options such as
grpc.WithTransportCredentials and grpc.WithPerRPCCredentials to establish the
client connection correctly.
| log.Fatalf("failed to get ID token source: %v", err) | ||
| } | ||
|
|
||
| tlsCreds := credentials.NewTLS(&tls.Config{}) |
There was a problem hiding this comment.
Security vulnerability: TLS configuration is missing minimum version specification.
The TLS configuration should specify a minimum version to prevent downgrade attacks. Without it, older insecure protocols could be negotiated.
-tlsCreds := credentials.NewTLS(&tls.Config{})
+tlsCreds := credentials.NewTLS(&tls.Config{
+ MinVersion: tls.VersionTLS13,
+})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tlsCreds := credentials.NewTLS(&tls.Config{}) | |
| tlsCreds := credentials.NewTLS(&tls.Config{ | |
| MinVersion: tls.VersionTLS13, | |
| }) |
🤖 Prompt for AI Agents
In server/internal/adapter/internalapi/client.go at line 35, the TLS
configuration lacks a minimum version specification, which can allow insecure
protocol downgrades. Update the tls.Config struct to include the MinVersion
field set to tls.VersionTLS12 or higher to enforce a minimum TLS version and
enhance security.
a4d489f to
7f1dab8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
server/internal/adapter/internalapi/client.go (3)
17-34:⚠️ Potential issueFix error handling in InvokeApi and return errors consistently.
The function uses
log.Fatalfwhich terminates the program, but then attempts to return the error. This makes the return statement unreachable.func InvokeApi( ctx context.Context, userID string, endpoint string, handleFunc func(client pb.ReEarthVisualizerClient, ctx context.Context) error, ) error { audience := "https://" + endpoint host := endpoint + ":443" ctx = metadata.NewOutgoingContext(ctx, metadata.New(map[string]string{ "user-id": userID, })) ts, err := idtoken.NewTokenSource(ctx, audience) if err != nil { - log.Fatalf("failed to get ID token source: %v", err) - return err + log.Printf("failed to get ID token source: %v", err) + return fmt.Errorf("failed to get ID token source: %w", err) }
36-36: 🛠️ Refactor suggestionEnhance TLS security by specifying minimum version.
The TLS configuration should specify a minimum version to prevent downgrade attacks.
-tlsCreds := credentials.NewTLS(&tls.Config{}) +tlsCreds := credentials.NewTLS(&tls.Config{ + MinVersion: tls.VersionTLS13, +})
39-46:⚠️ Potential issueFix the gRPC client creation method.
grpc.NewClientdoesn't exist in the gRPC library; usegrpc.Dialinstead. Also, the same fatal error handling issue exists here.-conn, err := grpc.NewClient(host, +conn, err := grpc.Dial(host, grpc.WithTransportCredentials(tlsCreds), grpc.WithPerRPCCredentials(callCreds), ) if err != nil { - log.Fatalf("failed to create gRPC client: %v", err) - return err + log.Printf("failed to create gRPC client: %v", err) + return fmt.Errorf("failed to create gRPC client: %w", err) }server/internal/app/grpc.go (4)
23-38: 🛠️ Refactor suggestionRe-enable authentication interceptor for secured endpoints.
The authentication interceptor is commented out with a TODO note. This leaves all gRPC endpoints unauthenticated, which is a security risk.
ui := grpc.ChainUnaryInterceptor( unaryLogInterceptor(cfg), - // unaryAuthInterceptor(cfg), // TODO: When using M2M token + unaryAuthInterceptor(cfg), unaryAttachOperatorInterceptor(cfg), unaryAttachUsecaseInterceptor(cfg), )Alternatively, if some endpoints should allow anonymous access, update the interceptor to check the method name and selectively enforce authentication.
65-86: 🛠️ Refactor suggestionUse proper gRPC status codes for authentication errors.
The commented-out authentication interceptor uses plain errors that will be marshalled to
codes.Unknown. Use proper gRPC status codes instead.// func unaryAuthInterceptor(cfg *ServerConfig) grpc.UnaryServerInterceptor { // return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) { // md, ok := metadata.FromIncomingContext(ctx) // if !ok { // log.Errorf("unaryAuthInterceptor: no metadata found") -// return nil, errors.New("unauthorized") +// return nil, status.Error(codes.Unauthenticated, "missing metadata") // } // // token := tokenFromGrpcMetadata(md) // if token == "" { // log.Errorf("unaryAuthInterceptor: no token found") -// return nil, errors.New("unauthorized") +// return nil, status.Error(codes.Unauthenticated, "missing token") // } // // if token != cfg.Config.InternalApi.Token { // log.Errorf("unaryAuthInterceptor: invalid token") -// return nil, errors.New("unauthorized") +// return nil, status.Error(codes.Unauthenticated, "invalid token") // } // // return handler(ctx, req) // } // }
88-104: 🛠️ Refactor suggestionSupport anonymous access in operator interceptor.
The PR description states that unauthenticated users should receive only public projects for
GetProjectList, but this interceptor rejects all requests without a user ID.func unaryAttachOperatorInterceptor(cfg *ServerConfig) grpc.UnaryServerInterceptor { return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) { md, ok := metadata.FromIncomingContext(ctx) if !ok { log.Errorf("unaryAttachOperatorInterceptor: no metadata found") - return nil, errors.New("unauthorized") + return nil, status.Error(codes.Unauthenticated, "missing metadata") } if len(md["user-id"]) < 1 { - log.Errorf("unaryAttachOperatorInterceptor: no user id found") - return nil, errors.New("unauthorized") + // Allow anonymous access for certain methods (GetProjectList, GetProject) + if info.FullMethod == "/reearth.visualizer.v1.ReEarthVisualizer/GetProjectList" || + info.FullMethod == "/reearth.visualizer.v1.ReEarthVisualizer/GetProject" { + log.Debugw("unaryAttachOperatorInterceptor: anonymous access", "method", info.FullMethod) + return handler(ctx, req) + } + log.Errorw("unaryAttachOperatorInterceptor: no user id found for authenticated method", "method", info.FullMethod) + return nil, status.Error(codes.Unauthenticated, "user id required for this operation") }
156-171: 🛠️ Refactor suggestionFix token extraction from metadata.
The commented-out
tokenFromGrpcMetadatafunction extracts the token from the wrong header key and has redundant code.// func tokenFromGrpcMetadata(md metadata.MD) string { // // The keys within metadata.MD are normalized to lowercase. // // See: https://godoc.org/google.golang.org/grpc/metadata#New -// if len(md["m2m-token"]) < 1 { +// vals := md.Get("authorization") +// if len(vals) < 1 { // return "" // } -// token := md["m2m-token"][0] +// token := vals[0] // if !strings.HasPrefix(token, "Bearer ") { // return "" // } -// token = strings.TrimPrefix(md["m2m-token"][0], "Bearer ") +// token = strings.TrimPrefix(token, "Bearer ") // if token == "" { // return "" // } // return token // }server/schemas/internalapi/v1/schema.proto (4)
3-7: 🛠️ Refactor suggestionFix package directory mismatch.
The package name
reearth.visualizer.v1doesn't match the file's directory path, causing Buf linting errors.Either move the file to match the package name:
server/schemas/reearth/visualizer/v1/schema.protoOr change the package name to match the directory:
-package reearth.visualizer.v1; +package server.schemas.internalapi.v1;Also update the go_package option to use a fully-qualified path:
-option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/internal/adapter/internalapi/schemas/internalapi/v1;internalapiv1";🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
40-40: 🛠️ Refactor suggestionDefine visibility as an enum instead of string.
Using string for visibility values introduces the risk of typos and inconsistent usage across the codebase.
- string visibility = 14; + Visibility visibility = 14;And add the enum definition:
enum Visibility { VISIBILITY_UNSPECIFIED = 0; VISIBILITY_PUBLIC = 1; VISIBILITY_LIMITED = 2; VISIBILITY_PRIVATE = 3; }Remember to update all related fields (line 81 in CreateProjectRequest).
50-53: 🛠️ Refactor suggestionAvoid exposing Basic Auth credentials in responses.
Exposing sensitive information like passwords in API responses increases security risks, even for internal APIs.
bool is_basic_auth_active = 22; string basic_auth_username = 23; - string basic_auth_password = 24; + // Password is not returned in responses for security reasonsConsider adding a separate request message for updating credentials, without including the password in regular responses.
81-81: 🛠️ Refactor suggestionUpdate visibility field type in CreateProjectRequest.
For consistency, the visibility field in CreateProjectRequest should also use the Visibility enum.
- optional string visibility = 6; + optional Visibility visibility = 6;
🧹 Nitpick comments (2)
server/e2e/common.go (2)
45-55: Update internalApiConfig with proper token and port values.The internal API configuration has
Activeset to true but the port and token values are commented out. For e2e testing, these should have proper default values to ensure consistent test execution.InternalApi: config.InternalApiConfig{ Active: true, - // Port: "8080", - // Token: "test", + Port: "8080", + Token: "test", },
133-155: Refactor conditional code blocks for better readability.The conditional code for starting the server has duplicate error handling logic. Consider refactoring to reduce duplication.
var l net.Listener var err error + port := ":0" // Default to random port + if cfg.InternalApi.Active { + port = ":" + cfg.Port + } + + l, err = net.Listen("tcp", port) + if err != nil { + t.Fatalf("server failed to listen: %v", err) + } + if cfg.InternalApi.Active { - l, err = net.Listen("tcp", ":"+cfg.Port) - if err != nil { - t.Fatalf("server failed to listen: %v", err) - } go func() { if err := srv.ServeGRPC(l); err != http.ErrServerClosed { ch <- err } close(ch) }() } else { - l, err = net.Listen("tcp", ":0") - if err != nil { - t.Fatalf("server failed to listen: %v", err) - } go func() { if err := srv.Serve(l); err != http.ErrServerClosed { ch <- err } close(ch) }() }🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
server/.env.example(1 hunks)server/e2e/common.go(4 hunks)server/e2e/proto_project_test.go(1 hunks)server/e2e/seeder.go(3 hunks)server/internal/adapter/gql/loader_project.go(1 hunks)server/internal/adapter/gql/resolver_mutation_storytelling.go(2 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/internalapi/client.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/app/config/config.go(1 hunks)server/internal/app/grpc.go(1 hunks)server/internal/app/server.go(3 hunks)server/internal/infrastructure/memory/project.go(1 hunks)server/internal/infrastructure/mongo/project.go(1 hunks)server/internal/usecase/interactor/project.go(2 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/internal/usecase/repo/project.go(1 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- server/internal/adapter/gql/resolver_query.go
- server/.env.example
- server/internal/adapter/gql/resolver_mutation_storytelling.go
- server/internal/usecase/interfaces/project.go
- server/e2e/seeder.go
- server/internal/app/config/config.go
- server/internal/usecase/repo/project.go
- server/internal/infrastructure/memory/project.go
- server/internal/adapter/gql/loader_project.go
- server/internal/usecase/interactor/project.go
- server/internal/infrastructure/mongo/project.go
- server/internal/app/server.go
- server/internal/adapter/internalapi/server.go
- server/e2e/proto_project_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/internal/adapter/internalapi/client.go (3)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.go (2)
ReEarthVisualizerClient(31-37)NewReEarthVisualizerClient(43-45)server/internal/app/config/config.go (1)
Config(24-77)server/e2e/proto_project_test.go (1)
SafeClose(219-223)
🪛 Buf (1.47.2)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
🪛 ast-grep (0.31.1)
server/e2e/common.go
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
server/internal/adapter/internalapi/client.go
[warning] 35-35: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 golangci-lint (1.64.8)
server/internal/adapter/internalapi/client.go
50-50: Error return value is not checked
(errcheck)
🪛 GitHub Check: ci-server / ci-server-lint
server/internal/adapter/internalapi/client.go
[failure] 50-50:
Error return value is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-server / ci-server-test
- GitHub Check: test
🔇 Additional comments (4)
server/e2e/common.go (1)
186-188: LGTM!The
GRPCServerhelper function is well-implemented, providing a convenient way to start the server with internal API enabled for testing.server/internal/adapter/internalapi/client.go (1)
55-59: LGTM!The
SafeClosehelper function is well-implemented, providing a safe way to close resources and handle errors.server/internal/app/grpc.go (1)
40-63: LGTM!The logging interceptor is well-implemented, providing clear logs of RPC method calls, durations, and errors.
server/schemas/internalapi/v1/schema.proto (1)
9-15: LGTM!The service definition clearly describes the available operations with well-named methods and appropriate request/response types.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
server/internal/adapter/internalapi/client.go (4)
30-34:⚠️ Potential issueFix unreachable code after log.Fatalf.
Using
log.Fatalfterminates the program, making thereturn errstatement unreachable. This prevents proper error propagation.ts, err := idtoken.NewTokenSource(ctx, audience) if err != nil { - log.Fatalf("failed to get ID token source: %v", err) - return err + log.Printf("failed to get ID token source: %v", err) + return err }
36-36:⚠️ Potential issueSecurity vulnerability: TLS configuration is missing minimum version specification.
The TLS configuration should specify a minimum version to prevent downgrade attacks. Without it, older insecure protocols could be negotiated.
-tlsCreds := credentials.NewTLS(&tls.Config{}) +tlsCreds := credentials.NewTLS(&tls.Config{ + MinVersion: tls.VersionTLS13, +})
39-42:⚠️ Potential issueCritical error: grpc.NewClient is not a valid gRPC function.
The
grpc.NewClientfunction doesn't exist in the gRPC library. You should usegrpc.Dialinstead to create a connection.-conn, err := grpc.NewClient(host, +conn, err := grpc.Dial(host, grpc.WithTransportCredentials(tlsCreds), grpc.WithPerRPCCredentials(callCreds), )
43-46:⚠️ Potential issueFix unreachable code after log.Fatalf.
Using
log.Fatalfterminates the program, making thereturn errstatement unreachable. This prevents proper error propagation.if err != nil { - log.Fatalf("failed to create gRPC client: %v", err) - return err + log.Printf("failed to create gRPC client: %v", err) + return err }
🧹 Nitpick comments (1)
server/internal/adapter/internalapi/client.go (1)
54-58: Consider moving SafeClose to a common utility package.This function is duplicated in
server/e2e/proto_project_test.go. Consider moving it to a common utility package to avoid duplication.// Move this function to a common utility package, for example: // server/pkg/util/closer.go func SafeClose(c io.Closer) { if err := c.Close(); err != nil { log.Printf("warning: failed to close: %v", err) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
server/internal/adapter/internalapi/client.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/internal/adapter/internalapi/client.go (3)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.go (2)
ReEarthVisualizerClient(31-37)NewReEarthVisualizerClient(43-45)server/internal/app/config/config.go (1)
Config(24-77)server/e2e/proto_project_test.go (1)
SafeClose(219-223)
🪛 ast-grep (0.31.1)
server/internal/adapter/internalapi/client.go
[warning] 35-35: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-server / ci-server-test
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
server/schemas/internalapi/v1/schema.proto (4)
1-3: Buf package-directory mismatch
The declared packagereearth.visualizer.v1does not align with the file pathserver/schemas/internalapi/v1, causing a BufPACKAGE_DIRECTORY_MATCHerror.
Choose one: move the file underreearth/visualizer/v1/to match the package, or update thepackagedeclaration toserver.schemas.internalapi.v1.🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
7-7: Fully-qualified go_package option required
Usingoption go_package = "proto/v1";can lead to ambiguous imports. Specify the full module path and an explicit alias, for example:-option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/schemas/internalapi/v1;internalapi_v1";
68-69: Modelvisibilityas an enum rather than a string
Free-formstring visibilityinvites typos and inconsistent usage. Define aVisibilityenum (e.g.,VISIBILITY_PUBLIC,VISIBILITY_LIMITED,VISIBILITY_PRIVATE) and change this field to use that enum.
151-151: Use enum forCreateProjectRequest.visibility
Similarly, inCreateProjectRequest, replaceoptional string visibility = 6;with theVisibilityenum for consistency and compile-time validation.
🧹 Nitpick comments (1)
server/schemas/internalapi/v1/schema.proto (1)
71-94: Typographical: correct repeated “publishmen” typo in comments
Multiple comments read// publishmen value; this is a typo. It should be// publishment valueor a more descriptive comment (e.g.,// Publishment status).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
server/schemas/internalapi/docs/schema.mdis excluded by!**/*.md
📒 Files selected for processing (2)
server/Makefile(1 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/Makefile
🧰 Additional context used
🪛 Buf (1.47.2)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-server / ci-server-test
- GitHub Check: test
| string basic_auth_username = 23; | ||
| // publishmen value | ||
| string basic_auth_password = 24; |
There was a problem hiding this comment.
Avoid exposing raw basic_auth_password in responses
Returning basic_auth_password in the Project message exposes sensitive credentials and increases leakage risk. Remove this field from the response, or make it write-only in a separate request-only message.
🤖 Prompt for AI Agents
In server/schemas/internalapi/v1/schema.proto around lines 88 to 90, the field
basic_auth_password exposes sensitive credentials in the Project message. Remove
the basic_auth_password field from this response message to avoid leaking
sensitive data, or alternatively, move it to a separate message used only for
write operations and not included in responses.
| } | ||
|
|
||
| // Request messages | ||
| message GetProjectListRequest { string team_id = 1; } |
There was a problem hiding this comment.
| message GetProjectListRequest { string team_id = 1; } | |
| message GetProjectListRequest { string team_id = 1; bool authenticated = 2; } |
There was a problem hiding this comment.
server/.env.example
Outdated
| REEARTH_INTERNALAPI_ACTIVE=false | ||
| # REEARTH_INTERNALAPI_PORT=8080 | ||
| # REEARTH_INTERNALAPI_TOKEN=token |
There was a problem hiding this comment.
| REEARTH_INTERNALAPI_ACTIVE=false | |
| # REEARTH_INTERNALAPI_PORT=8080 | |
| # REEARTH_INTERNALAPI_TOKEN=token | |
| REEARTH_VISUALIZER_INTERNALAPI_ACTIVE=false | |
| # REEARTH_VISUALIZER_INTERNALAPI_PORT=8080 | |
| # REEARTH_VISUALIZER_INTERNALAPI_TOKEN=token |
| return &server{} | ||
| } | ||
|
|
||
| func (s server) GetProjectList(ctx context.Context, req *pb.GetProjectListRequest) (*pb.GetProjectListResponse, error) { |
There was a problem hiding this comment.
| func (s server) GetProjectList(ctx context.Context, req *pb.GetProjectListRequest) (*pb.GetProjectListResponse, error) { | |
| func (s server) GetProjectList(ctx context.Context, req *pb.GetProjectListRequest) (*pb.GetProjectListResponse, error) { |
We'd like to add the provision to get list project when the user authenticated or not. i.e
authenticated -> All projects
not-authenticated -> Public Projects
There was a problem hiding this comment.
Please address the above on the schema as well.
There was a problem hiding this comment.
Owner(workspace) can only change the visibility of the project.
GetProjectList (not authenticated) -> Return all the public projects that the user is member
There was a problem hiding this comment.
(Authenticated) User Alias -> User -> Personal Workspace -> All the Projects
(Not Authenticated) User Alias -> User -> Personal Workspace -> All Public Projects
| // !!!!!!!! connectivity check !!!!!!!! | ||
| // internalapi.InvokeApi( | ||
| // ctx, | ||
| // operator.AcOperator.User.String(), | ||
| // "reearth-visualizer-internal-api-xxxxxx-an.a.run.app", | ||
| // func(client pb.ReEarthVisualizerClient, ctx context.Context) { | ||
| // res, err := client.GetProject(ctx, &pb.GetProjectRequest{ | ||
| // ProjectId: prj.ID().String(), | ||
| // }) | ||
| // if err != nil { | ||
| // log.Printf("!!! gRPC GetProject Error : %v", err) | ||
| // } else { | ||
| // log.Printf("OK!! gRPC GetProject : %v", res) | ||
| // } | ||
| // }) | ||
| // !!!!!!!! connectivity check !!!!!!!! |
| message GetProjectListRequest { | ||
| // Team ID (Workspace ID) | ||
| string team_id = 1; | ||
| } |
There was a problem hiding this comment.
| message GetProjectListRequest { | |
| // Team ID (Workspace ID) | |
| string team_id = 1; | |
| } | |
| message GetProjectListRequest { | |
| // Team ID (Workspace ID) | |
| string workspace_id = 1; | |
| bool authenticated = 2; | |
| } |
hexaforce
force-pushed
the
feat/reearth-visualizer-proto2
branch
from
b95230c to
38e0883
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
server/internal/adapter/internalapi/client.go (4)
32-34:⚠️ Potential issueFix the error handling:
log.Fatalfprevents proper error propagationUsing
log.Fatalfterminates the program, preventing the caller from handling the error. The code after it will never execute.ts, err := idtoken.NewTokenSource(ctx, audience) if err != nil { - log.Fatalf("failed to get ID token source: %v", err) - return err + return fmt.Errorf("failed to get ID token source: %w", err) }Don't forget to add
"fmt"to your imports.
36-36:⚠️ Potential issueSecurity vulnerability: TLS configuration is missing minimum version specification
The TLS configuration should specify a minimum version to prevent downgrade attacks. Without it, older insecure protocols could be negotiated.
-tlsCreds := credentials.NewTLS(&tls.Config{}) +tlsCreds := credentials.NewTLS(&tls.Config{ + MinVersion: tls.VersionTLS13, +})
39-42:⚠️ Potential issueCritical error:
grpc.NewClientis not a valid gRPC functionThe
grpc.NewClientfunction doesn't exist in the gRPC library. You should usegrpc.Dialinstead to create a connection.-conn, err := grpc.NewClient(host, +conn, err := grpc.Dial(host, grpc.WithTransportCredentials(tlsCreds), grpc.WithPerRPCCredentials(callCreds), )
44-46:⚠️ Potential issueFix the error handling:
log.Fatalfprevents proper error propagationSimilar to the previous issue, using
log.Fatalfterminates the program, preventing the caller from handling the error.if err != nil { - log.Fatalf("failed to create gRPC client: %v", err) - return err + return fmt.Errorf("failed to create gRPC client: %w", err) }server/schemas/internalapi/v1/schema.proto (5)
3-3:⚠️ Potential issueBuf
PACKAGE_DIRECTORY_MATCHviolation
package reearth.visualizer.v1;requires the file to live underreearth/visualizer/v1/.
Either move the file accordingly or change the package path to mirror the current directory (e.g.server.schemas.internalapi.v1).-package reearth.visualizer.v1; +package server.schemas.internalapi.v1;🧰 Tools
🪛 Buf (1.54.0)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
7-7: 🛠️ Refactor suggestionProvide a fully‑qualified
go_packageoption
option go_package = "proto/v1";yields import paths such asimport "proto/v1", which collide easily and violate Go module conventions.
Specify the complete module path instead:-option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/internal/adapter/internalapi/schemas/internalapi/v1;internalapiv1";This prevents ambiguous imports when the proto is consumed outside the repo.
69-69: 🛠️ Refactor suggestionModel
visibilityas an enum, not free-form stringUsing
string visibilityinvites typos and inconsistent clients. Define aVisibilityenum once and reference it here and inCreateProjectRequest:- string visibility = 14; + Visibility visibility = 14;And add an enum definition:
enum Visibility { VISIBILITY_UNSPECIFIED = 0; VISIBILITY_PUBLIC = 1; VISIBILITY_LIMITED = 2; VISIBILITY_PRIVATE = 3; }Update all related fields (e.g., line 151) to use this enum as well.
88-90:⚠️ Potential issueAvoid exposing raw
basic_auth_passwordin responsesReturning
basic_auth_passwordin theProjectmessage exposes sensitive credentials and increases leakage risk. Remove this field from the response, or make it write-only in a separate request-only message.Consider replacing it with a boolean indicating if basic auth is configured:
- // publishmen value - string basic_auth_username = 23; - // publishmen value - string basic_auth_password = 24; + // publishmen value + string basic_auth_username = 23; + // No password here - avoid exposing credentials in responsesAnd create a separate message for setting passwords when needed.
120-123: 🛠️ Refactor suggestionUpdate GetProjectListRequest to include
authenticatedflagBased on the PR objectives, the GetProjectList response depends on user authentication. Add an
authenticatedfield to the request:message GetProjectListRequest { // Team ID (Workspace ID) string team_id = 1; + bool authenticated = 2; }This will allow the server to correctly filter projects based on authentication status.
🧹 Nitpick comments (3)
server/schemas/internalapi/v1/schema.proto (1)
71-95: Fix typo in comments: "publishmen" should be "publishment"All comments from lines 71-94 have a typo - "publishmen" should be "publishment".
- // publishmen value + // publishment valueThis is a minor issue but improves code readability and documentation quality.
server/e2e/common.go (2)
45-55: Config has commented-out fields that may be neededThe
internalApiConfigvariable has commented-outPortandTokenfields. Consider either:
- Uncomment and provide default values for these fields
- Document why they're commented out
- Remove the comments entirely if they're not needed
This will make the configuration intent clearer for future developers.
130-155: Consider refactoring duplicated error handlingThere's duplicated error handling and goroutine code between the two branches. Consider refactoring to reduce duplication.
Here's a possible refactoring:
var l net.Listener var err error var serveFunc func(net.Listener) error if cfg.InternalApi.Active { l, err = net.Listen("tcp", "127.0.0.1:8080") serveFunc = srv.ServeGRPC } else { l, err = net.Listen("tcp", "127.0.0.1:0") serveFunc = srv.Serve } if err != nil { t.Fatalf("server failed to listen: %v", err) } go func() { if err := serveFunc(l); err != http.ErrServerClosed { ch <- err } close(ch) }()🧰 Tools
🪛 ast-grep (0.38.1)
[warning] 133-133: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":8080")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.goserver/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.gois excluded by!**/*.pb.goserver/schemas/internalapi/docs/schema.mdis excluded by!**/*.md
📒 Files selected for processing (24)
server/.env.example(1 hunks)server/Makefile(1 hunks)server/e2e/common.go(4 hunks)server/e2e/proto_project_test.go(1 hunks)server/e2e/seeder.go(3 hunks)server/go.mod(1 hunks)server/internal/adapter/gql/loader_project.go(1 hunks)server/internal/adapter/gql/resolver_mutation_storytelling.go(2 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/internalapi/client.go(1 hunks)server/internal/adapter/internalapi/internalapimodel/project.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/app/app.go(1 hunks)server/internal/app/config/config.go(1 hunks)server/internal/app/grpc.go(1 hunks)server/internal/app/server.go(3 hunks)server/internal/infrastructure/memory/project.go(1 hunks)server/internal/infrastructure/mongo/project.go(1 hunks)server/internal/usecase/interactor/common.go(1 hunks)server/internal/usecase/interactor/project.go(2 hunks)server/internal/usecase/interactor/storytelling.go(4 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/internal/usecase/repo/project.go(1 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- server/internal/adapter/gql/resolver_query.go
- server/internal/adapter/gql/resolver_mutation_storytelling.go
- server/go.mod
- server/internal/app/app.go
- server/internal/usecase/interactor/common.go
- server/e2e/seeder.go
- server/internal/app/config/config.go
- server/internal/usecase/interfaces/project.go
- server/.env.example
- server/internal/adapter/gql/loader_project.go
- server/internal/usecase/interactor/project.go
- server/Makefile
- server/internal/usecase/repo/project.go
- server/internal/infrastructure/mongo/project.go
- server/internal/adapter/internalapi/internalapimodel/project.go
- server/internal/usecase/interactor/storytelling.go
- server/internal/adapter/internalapi/server.go
- server/internal/infrastructure/memory/project.go
- server/internal/app/server.go
- server/internal/app/grpc.go
- server/e2e/proto_project_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/internal/adapter/internalapi/client.go (2)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.go (2)
ReEarthVisualizerClient(31-37)NewReEarthVisualizerClient(43-45)server/e2e/proto_project_test.go (1)
SafeClose(222-226)
🪛 ast-grep (0.38.1)
server/e2e/common.go
[warning] 133-133: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":8080")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
server/internal/adapter/internalapi/client.go
[warning] 35-35: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 Buf (1.54.0)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
- GitHub Check: ci-server / ci-server-test
🔇 Additional comments (2)
server/internal/adapter/internalapi/client.go (1)
54-58: LGTM: SafeClose function is well implementedThe
SafeClosefunction correctly handles closing anio.Closerand logs a warning if closing fails. This is a good utility function that helps prevent resource leaks.server/e2e/common.go (1)
186-188: LGTM! Well-designed helper functionThe
GRPCServerfunction is a clean abstraction that will simplify writing gRPC-based tests.
server/e2e/common.go
Outdated
| if cfg.InternalApi.Active { | ||
| l, err = net.Listen("tcp", ":8080") | ||
| if err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use config port instead of hardcoded value
The port is hardcoded as 8080 rather than using the Port field from the InternalApi config.
Apply this diff to use the configured port:
- l, err = net.Listen("tcp", ":8080")
+ port := "8080"
+ if cfg.InternalApi.Port != "" {
+ port = cfg.InternalApi.Port
+ }
+ l, err = net.Listen("tcp", "127.0.0.1:"+port)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if cfg.InternalApi.Active { | |
| l, err = net.Listen("tcp", ":8080") | |
| if err != nil { | |
| if cfg.InternalApi.Active { | |
| port := "8080" | |
| if cfg.InternalApi.Port != "" { | |
| port = cfg.InternalApi.Port | |
| } | |
| l, err = net.Listen("tcp", "127.0.0.1:"+port) | |
| if err != nil { |
🧰 Tools
🪛 ast-grep (0.38.1)
[warning] 133-133: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":8080")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
🤖 Prompt for AI Agents
In server/e2e/common.go around lines 133 to 135, the code uses a hardcoded port
8080 for net.Listen instead of the configured port. Replace the hardcoded
":8080" string with a dynamic string that uses the port value from
cfg.InternalApi.Port, ensuring the port is correctly formatted as ":<port>".
server/e2e/common.go
Outdated
| var l net.Listener | ||
| var err error | ||
|
|
||
| if cfg.InternalApi.Active { | ||
| l, err = net.Listen("tcp", ":8080") | ||
| if err != nil { | ||
| t.Fatalf("server failed to listen: %v", err) | ||
| } | ||
| close(ch) | ||
| }() | ||
| go func() { | ||
| if err := srv.ServeGRPC(l); err != http.ErrServerClosed { | ||
| ch <- err | ||
| } | ||
| close(ch) | ||
| }() | ||
| } else { | ||
| l, err = net.Listen("tcp", ":0") | ||
| if err != nil { | ||
| t.Fatalf("server failed to listen: %v", err) | ||
| } | ||
| go func() { | ||
| if err := srv.Serve(l); err != http.ErrServerClosed { | ||
| ch <- err | ||
| } | ||
| close(ch) | ||
| }() | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix server binding to all network interfaces
The server is binding to all network interfaces (:8080 and :0), which could expose it publicly. For testing environments, it's safer to bind only to the loopback interface.
Apply this diff to fix the security issue:
- l, err = net.Listen("tcp", ":8080")
+ l, err = net.Listen("tcp", "127.0.0.1:8080")
- l, err = net.Listen("tcp", ":0")
+ l, err = net.Listen("tcp", "127.0.0.1:0")Let's verify if there are other instances of this pattern in the codebase:
🏁 Script executed:
#!/bin/bash
# Check for other instances of binding to all interfaces
rg "net\.Listen\(\"tcp\", \":[0-9]*\"\)" --type goLength of output: 163
Restrict server binding to loopback interface
We’ve verified that the only occurrences of net.Listen("tcp", ":[port]") are in server/e2e/common.go. To prevent the test server from listening on all network interfaces, update both bindings to the loopback address:
- File: server/e2e/common.go (around lines 130–155)
- l, err = net.Listen("tcp", ":8080")
+ l, err = net.Listen("tcp", "127.0.0.1:8080")
- l, err = net.Listen("tcp", ":0")
+ l, err = net.Listen("tcp", "127.0.0.1:0")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var l net.Listener | |
| var err error | |
| if cfg.InternalApi.Active { | |
| l, err = net.Listen("tcp", ":8080") | |
| if err != nil { | |
| t.Fatalf("server failed to listen: %v", err) | |
| } | |
| close(ch) | |
| }() | |
| go func() { | |
| if err := srv.ServeGRPC(l); err != http.ErrServerClosed { | |
| ch <- err | |
| } | |
| close(ch) | |
| }() | |
| } else { | |
| l, err = net.Listen("tcp", ":0") | |
| if err != nil { | |
| t.Fatalf("server failed to listen: %v", err) | |
| } | |
| go func() { | |
| if err := srv.Serve(l); err != http.ErrServerClosed { | |
| ch <- err | |
| } | |
| close(ch) | |
| }() | |
| } | |
| var l net.Listener | |
| var err error | |
| if cfg.InternalApi.Active { | |
| - l, err = net.Listen("tcp", ":8080") | |
| + l, err = net.Listen("tcp", "127.0.0.1:8080") | |
| if err != nil { | |
| t.Fatalf("server failed to listen: %v", err) | |
| } | |
| go func() { | |
| if err := srv.ServeGRPC(l); err != http.ErrServerClosed { | |
| ch <- err | |
| } | |
| close(ch) | |
| }() | |
| } else { | |
| - l, err = net.Listen("tcp", ":0") | |
| + l, err = net.Listen("tcp", "127.0.0.1:0") | |
| if err != nil { | |
| t.Fatalf("server failed to listen: %v", err) | |
| } | |
| go func() { | |
| if err := srv.Serve(l); err != http.ErrServerClosed { | |
| ch <- err | |
| } | |
| close(ch) | |
| }() | |
| } |
🧰 Tools
🪛 ast-grep (0.38.1)
[warning] 133-133: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":8080")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
🤖 Prompt for AI Agents
In server/e2e/common.go around lines 130 to 155, the server is currently binding
to all network interfaces using ":8080" and ":0". To restrict the server to only
listen on the loopback interface for security in testing, change the bind
addresses to "127.0.0.1:8080" and "127.0.0.1:0" respectively in both net.Listen
calls.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
server/e2e/common.go (1)
133-155:⚠️ Potential issueCritical security issue persists: Fix network binding and use configured port.
The server is still binding to all network interfaces, which poses a security risk in testing environments. Additionally, the hardcoded port doesn't use the configuration value.
Apply this fix to address both the security issue and port configuration:
- if cfg.Visualizer.InternalApi.Active { - l, err = net.Listen("tcp", ":8080") - if err != nil { - t.Fatalf("server failed to listen: %v", err) - } - go func() { - if err := srv.ServeGRPC(l); err != http.ErrServerClosed { - ch <- err - } - close(ch) - }() - } else { - l, err = net.Listen("tcp", ":0") - if err != nil { - t.Fatalf("server failed to listen: %v", err) - } - go func() { - if err := srv.Serve(l); err != http.ErrServerClosed { - ch <- err - } - close(ch) - }() - } + if cfg.Visualizer.InternalApi.Active { + port := "8080" + if cfg.Visualizer.InternalApi.Port != "" { + port = cfg.Visualizer.InternalApi.Port + } + l, err = net.Listen("tcp", "127.0.0.1:"+port) + if err != nil { + t.Fatalf("server failed to listen: %v", err) + } + go func() { + if err := srv.ServeGRPC(l); err != http.ErrServerClosed { + ch <- err + } + close(ch) + }() + } else { + l, err = net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("server failed to listen: %v", err) + } + go func() { + if err := srv.Serve(l); err != http.ErrServerClosed { + ch <- err + } + close(ch) + }() + }🧰 Tools
🪛 ast-grep (0.38.1)
[warning] 133-133: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":8080")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
server/schemas/internalapi/v1/schema.proto (4)
3-3:⚠️ Potential issueFix package directory mismatch.
The package name doesn't match the file location, causing Buf linting errors.
Apply this fix to align the package with the directory structure:
-package reearth.visualizer.v1; +package server.schemas.internalapi.v1;🧰 Tools
🪛 Buf (1.54.0)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
7-7: 🛠️ Refactor suggestionUse fully-qualified go_package option.
The current
go_packagepath is too generic and could cause import conflicts.Apply this fix for proper Go module imports:
-option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/internal/adapter/internalapi/proto/v1;internalapiv1";
74-74: 🛠️ Refactor suggestionReplace string visibility with enum for type safety.
Using a string for visibility allows typos and inconsistent values. This should be an enum type.
Define a Visibility enum and update the field:
+enum Visibility { + VISIBILITY_UNSPECIFIED = 0; + VISIBILITY_PUBLIC = 1; + VISIBILITY_LIMITED = 2; + VISIBILITY_PRIVATE = 3; +} + message Project { // ... other fields ... - string visibility = 14; + Visibility visibility = 14;Also update the corresponding fields in
CreateProjectRequestandUpdateProjectVisibilityRequest:- optional string visibility = 6; + optional Visibility visibility = 6;- optional string visibility = 2; + optional Visibility visibility = 2;
93-95:⚠️ Potential issueRemove sensitive password field from response.
Returning
basic_auth_passwordin API responses creates unnecessary security risks and potential for credential leakage.Remove the password field from the response:
// publishmen value string basic_auth_username = 23; - // publishmen value - string basic_auth_password = 24; // publishmen value - bool enable_ga = 25; + bool enable_ga = 24; // publishmen value - string tracking_id = 26; + string tracking_id = 25;If password input is needed, create a separate write-only request message.
🧹 Nitpick comments (2)
server/schemas/internalapi/v1/schema.proto (2)
22-22: Fix grammatical error in comment.There's a grammatical error in the comment.
Apply this fix:
- // Update the visibility a project. + // Update the visibility of a project.
76-99: Improve field documentation consistency.Multiple fields have generic "publishmen value" comments that don't provide meaningful documentation.
Improve the field documentation:
- // publishmen value + // URL alias for the published project string alias = 15; - // publishmen value + // Current publication status PublishmentStatus publishment_status = 16; - // publishmen value + // Timestamp when the project was published optional google.protobuf.Timestamp published_at = 17; - // publishmen value + // Public display title for the published project string public_title = 18; - // publishmen value + // Public description for the published project string public_description = 19; - // publishmen value + // Public image URL for the published project string public_image = 20; - // publishmen value + // Whether to prevent search engine indexing bool public_no_index = 21; - // publishmen value + // Whether basic authentication is enabled bool is_basic_auth_active = 22; - // publishmen value + // Basic authentication username string basic_auth_username = 23; - // publishmen value + // Whether Google Analytics is enabled bool enable_ga = 25; - // publishmen value + // Google Analytics tracking ID string tracking_id = 26;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.goserver/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
server/.env.example(1 hunks)server/e2e/common.go(4 hunks)server/e2e/proto_project_test.go(1 hunks)server/internal/adapter/internalapi/internalapimodel/project.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/app/config/config.go(1 hunks)server/internal/app/server.go(3 hunks)server/internal/usecase/interactor/project.go(3 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- server/.env.example
- server/internal/app/config/config.go
- server/internal/usecase/interfaces/project.go
- server/internal/usecase/interactor/project.go
- server/internal/adapter/internalapi/internalapimodel/project.go
- server/internal/app/server.go
- server/internal/adapter/internalapi/server.go
- server/e2e/proto_project_test.go
🧰 Additional context used
🪛 Buf (1.54.0)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
🪛 ast-grep (0.38.1)
server/e2e/common.go
[warning] 133-133: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":8080")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-server / ci-server-test
🔇 Additional comments (3)
server/e2e/common.go (2)
45-55: New configuration looks good with minor formatting suggestion.The
internalApiConfigis properly structured with appropriate fields for testing the gRPC internal API.Consider adding a comment explaining the purpose of this configuration:
+ // Configuration for testing the internal gRPC API internalApiConfig = &config.Config{
186-188: Good helper function for gRPC testing.The
GRPCServerfunction provides a clean interface for starting the gRPC server in tests, properly reusing the existing infrastructure.server/schemas/internalapi/v1/schema.proto (1)
9-30: Well-designed service interface with comprehensive project management.The service provides a complete set of operations for project management with clear documentation about authentication requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/internal/infrastructure/memory/project.go (1)
131-138: Critical issues remain unaddressed from previous reviews.The
FindActiveByIdmethod still has the two critical issues identified in previous reviews:
- Race condition: Missing lock acquisition when accessing shared data
- Security vulnerability: Missing permission check for workspace access
Apply this diff to fix both issues:
func (r *Project) FindActiveById(ctx context.Context, id id.ProjectID) (*project.Project, error) { + r.lock.Lock() + defer r.lock.Unlock() + for _, p := range r.data { - if p.ID() == id && !p.IsDeleted() { + if p.ID() == id && !p.IsDeleted() && r.f.CanRead(p.Workspace()) { return p, nil } } return nil, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
server/internal/adapter/gql/loader_project.go(1 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/infrastructure/memory/project.go(1 hunks)server/internal/infrastructure/mongo/project.go(1 hunks)server/internal/usecase/interactor/project.go(3 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/internal/usecase/repo/project.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- server/internal/adapter/gql/resolver_query.go
- server/internal/adapter/gql/loader_project.go
- server/internal/usecase/repo/project.go
- server/internal/usecase/interfaces/project.go
- server/internal/infrastructure/mongo/project.go
- server/internal/usecase/interactor/project.go
- server/internal/adapter/internalapi/server.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/internal/infrastructure/memory/project.go (3)
server/internal/usecase/repo/project.go (1)
Project(19-35)server/pkg/project/project.go (1)
Project(13-41)server/pkg/id/id.go (5)
Project(10-10)Project(30-30)ProjectID(49-49)Workspace(16-16)Workspace(36-36)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-server / ci-server-test
| func (r *Project) FindVisibilityByWorkspace(ctx context.Context, authenticated bool, isWorkspaceOwner bool, id accountdomain.WorkspaceID) ([]*project.Project, error) { | ||
| r.lock.Lock() | ||
| defer r.lock.Unlock() | ||
|
|
||
| var result []*project.Project | ||
|
|
||
| if authenticated { | ||
| for _, p := range r.data { | ||
| if p.Workspace() == id { | ||
| result = append(result, p) | ||
| } | ||
| } | ||
| } else { | ||
| if isWorkspaceOwner { | ||
| for _, p := range r.data { | ||
| if p.Workspace() == id && !p.IsDeleted() { | ||
| result = append(result, p) | ||
| } | ||
| } | ||
| } else { | ||
| for _, p := range r.data { | ||
| if p.Workspace() == id && !p.IsDeleted() && p.Visibility() == "public" { | ||
| result = append(result, p) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| sort.Slice(result, func(i, j int) bool { | ||
| return result[i].UpdatedAt().After(result[j].UpdatedAt()) | ||
| }) | ||
|
|
||
| return result, nil | ||
| } |
There was a problem hiding this comment.
Fix logic issues and add missing permission check.
The FindVisibilityByWorkspace method has several issues:
- Missing workspace permission check: Unlike all other methods in this file, this method doesn't verify
r.f.CanRead(id)before accessing workspace data - Inconsistent filtering for authenticated users: Authenticated users receive ALL projects (including deleted ones), which contradicts the method purpose and other repository patterns
- Flawed conditional logic: The
authenticatedandisWorkspaceOwnerparameters should be independent, but the current if-else structure treats them as mutually exclusive
Apply this diff to fix the logic and add permission checks:
func (r *Project) FindVisibilityByWorkspace(ctx context.Context, authenticated bool, isWorkspaceOwner bool, id accountdomain.WorkspaceID) ([]*project.Project, error) {
r.lock.Lock()
defer r.lock.Unlock()
+ if !r.f.CanRead(id) {
+ return nil, nil
+ }
+
var result []*project.Project
- if authenticated {
+ if authenticated && isWorkspaceOwner {
+ // Workspace owner can see all non-deleted projects
for _, p := range r.data {
- if p.Workspace() == id {
+ if p.Workspace() == id && !p.IsDeleted() {
result = append(result, p)
}
}
- } else {
- if isWorkspaceOwner {
- for _, p := range r.data {
- if p.Workspace() == id && !p.IsDeleted() {
- result = append(result, p)
- }
- }
- } else {
- for _, p := range r.data {
- if p.Workspace() == id && !p.IsDeleted() && p.Visibility() == "public" {
- result = append(result, p)
- }
- }
- }
+ } else if authenticated {
+ // Authenticated non-owner can see all non-deleted projects they have access to
+ for _, p := range r.data {
+ if p.Workspace() == id && !p.IsDeleted() {
+ result = append(result, p)
+ }
+ }
+ } else {
+ // Unauthenticated users can only see public non-deleted projects
+ for _, p := range r.data {
+ if p.Workspace() == id && !p.IsDeleted() && p.Visibility() == "public" {
+ result = append(result, p)
+ }
+ }
}
sort.Slice(result, func(i, j int) bool {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *Project) FindVisibilityByWorkspace(ctx context.Context, authenticated bool, isWorkspaceOwner bool, id accountdomain.WorkspaceID) ([]*project.Project, error) { | |
| r.lock.Lock() | |
| defer r.lock.Unlock() | |
| var result []*project.Project | |
| if authenticated { | |
| for _, p := range r.data { | |
| if p.Workspace() == id { | |
| result = append(result, p) | |
| } | |
| } | |
| } else { | |
| if isWorkspaceOwner { | |
| for _, p := range r.data { | |
| if p.Workspace() == id && !p.IsDeleted() { | |
| result = append(result, p) | |
| } | |
| } | |
| } else { | |
| for _, p := range r.data { | |
| if p.Workspace() == id && !p.IsDeleted() && p.Visibility() == "public" { | |
| result = append(result, p) | |
| } | |
| } | |
| } | |
| } | |
| sort.Slice(result, func(i, j int) bool { | |
| return result[i].UpdatedAt().After(result[j].UpdatedAt()) | |
| }) | |
| return result, nil | |
| } | |
| func (r *Project) FindVisibilityByWorkspace(ctx context.Context, authenticated bool, isWorkspaceOwner bool, id accountdomain.WorkspaceID) ([]*project.Project, error) { | |
| r.lock.Lock() | |
| defer r.lock.Unlock() | |
| // Permission check: return no projects if caller can't read this workspace | |
| if !r.f.CanRead(id) { | |
| return nil, nil | |
| } | |
| var result []*project.Project | |
| if authenticated && isWorkspaceOwner { | |
| // Workspace owner can see all non-deleted projects | |
| for _, p := range r.data { | |
| if p.Workspace() == id && !p.IsDeleted() { | |
| result = append(result, p) | |
| } | |
| } | |
| } else if authenticated { | |
| // Authenticated non-owner can see all non-deleted projects | |
| for _, p := range r.data { | |
| if p.Workspace() == id && !p.IsDeleted() { | |
| result = append(result, p) | |
| } | |
| } | |
| } else { | |
| // Unauthenticated users can only see public, non-deleted projects | |
| for _, p := range r.data { | |
| if p.Workspace() == id && !p.IsDeleted() && p.Visibility() == "public" { | |
| result = append(result, p) | |
| } | |
| } | |
| } | |
| sort.Slice(result, func(i, j int) bool { | |
| return result[i].UpdatedAt().After(result[j].UpdatedAt()) | |
| }) | |
| return result, nil | |
| } |
🤖 Prompt for AI Agents
In server/internal/infrastructure/memory/project.go lines 140 to 173, the
FindVisibilityByWorkspace method lacks a workspace read permission check and has
flawed logic combining authenticated and isWorkspaceOwner flags. First, add a
check using r.f.CanRead(id) to verify read permission on the workspace before
proceeding. Then, refactor the filtering logic so that authenticated and
isWorkspaceOwner are treated independently: if the user is not authenticated,
return an empty result; if authenticated, filter projects by workspace ID and
exclude deleted projects unless the user is the workspace owner, in which case
include all projects except deleted ones. Ensure only non-deleted projects are
returned for authenticated users who are not owners, and always exclude deleted
projects for unauthenticated users. This will align the method with permission
checks and consistent filtering patterns used elsewhere.
98a7822 to
63bf03c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
server/e2e/proto_project_test.go (1)
277-277:⚠️ Potential issueFix gRPC client connection to use correct API.
The
grpc.NewClientfunction does not exist in the gRPC library. Usegrpc.DialContextinstead to establish the connection.Apply this diff to fix the compilation error:
-conn, err := grpc.NewClient("localhost:8080", grpc.WithTransportCredentials(insecure.NewCredentials())) +conn, err := grpc.DialContext(ctx, "localhost:8080", + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithBlock())server/schemas/internalapi/v1/schema.proto (4)
3-3:⚠️ Potential issueFix package directory mismatch.
The package
reearth.visualizer.v1does not match the file locationserver/schemas/internalapi/v1/. This violates Buf'sPACKAGE_DIRECTORY_MATCHrequirement.Either move the file to match the package path or update the package declaration:
-package reearth.visualizer.v1; +package server.schemas.internalapi.v1;🧰 Tools
🪛 Buf (1.54.0)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
7-7: 🛠️ Refactor suggestionProvide fully-qualified go_package option.
The current
go_packageoption can cause import collisions and violates Go module conventions.-option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/internal/adapter/internalapi/schemas/internalapi/v1;internalapiv1";
74-74: 🛠️ Refactor suggestionModel visibility as an enum for type safety.
Using
stringfor visibility allows typos and inconsistent usage. Define a proper enum type.- string visibility = 14; + Visibility visibility = 14;Add the enum definition:
+enum Visibility { + VISIBILITY_UNSPECIFIED = 0; + VISIBILITY_PUBLIC = 1; + VISIBILITY_LIMITED = 2; + VISIBILITY_PRIVATE = 3; +}Also update the request fields:
- optional string visibility = 6; + optional Visibility visibility = 6;- optional string visibility = 2; + optional Visibility visibility = 2;
98-98:⚠️ Potential issueRemove basic auth password from response to prevent credential leakage.
Exposing
basic_auth_passwordin theProjectmessage creates unnecessary security risks through logs, client dumps, etc.- // Publishment value - string basic_auth_password = 25; + // Note: Password is write-only and not exposed in responsesConsider providing password update through a separate secured endpoint if needed.
server/e2e/common.go (1)
133-155:⚠️ Potential issueFix security issues and use configured port instead of hardcoded values.
This code segment has multiple issues that have been previously identified:
- Security: The server binds to all network interfaces which could expose it publicly
- Configuration: Uses hardcoded port 8080 instead of the configured port from
cfg.InternalApi.PortApply this comprehensive fix to address both issues:
if cfg.Visualizer.InternalApi.Active { - l, err = net.Listen("tcp", ":8080") + port := "8080" + if cfg.InternalApi.Port != "" { + port = cfg.InternalApi.Port + } + l, err = net.Listen("tcp", "127.0.0.1:"+port) if err != nil { t.Fatalf("server failed to listen: %v", err) } go func() { if err := srv.ServeGRPC(l); err != http.ErrServerClosed { ch <- err } close(ch) }() } else { - l, err = net.Listen("tcp", ":0") + l, err = net.Listen("tcp", "127.0.0.1:0") if err != nil { t.Fatalf("server failed to listen: %v", err) }🧰 Tools
🪛 ast-grep (0.38.1)
[warning] 133-133: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":8080")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
server/internal/app/server.go (1)
72-75:⚠️ Potential issueFix incorrect port configuration and guard against empty values.
There are two issues with the internal server initialization:
- Wrong configuration field: Using
cfg.Config.Portinstead ofcfg.Config.InternalApi.Port- Missing guard: No protection against empty port values
Apply this fix to use the correct configuration and provide a default:
if cfg.Config.Visualizer.InternalApi.Active { - w.internalPort = ":" + cfg.Config.Port + port := cfg.Config.Visualizer.InternalApi.Port + if port == "" { + port = "50051" + } + w.internalPort = ":" + port w.internalServer = initGrpc(cfg) }
🧹 Nitpick comments (2)
server/e2e/seeder.go (1)
113-142: Consider simplifying the member copying logic.The function correctly implements workspace member management. However, the member copying logic could be simplified since
members.Users()already returns a map.Consider this minor simplification:
- newMembers := make(map[workspace.UserID]workspace.Member) - for k, v := range members.Users() { - newMembers[k] = v - } w2 := workspace.New(). ID(w.ID()). Name(w.Name()). Personal(w.IsPersonal()). - Members(newMembers). + Members(members.Users()). MustBuild()If the workspace domain requires a fresh map instance (for immutability), the current implementation is fine.
server/internal/app/server.go (1)
79-105: Enhanced Run method with context support and dual server modes.The updated Run method effectively handles both HTTP and gRPC server modes. The HTTP/2 cleartext handler setup is appropriate for gRPC over HTTP/2 without TLS in testing environments.
Minor suggestion: Consider adding error handling for the gRPC server startup to match the HTTP server error handling pattern.
Consider this enhancement for consistent error handling:
if w.internalServer != nil { go func() { handler := h2c.NewHandler(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { if r.ProtoMajor == 2 && r.Header.Get("Content-Type") == "application/grpc" { w.internalServer.ServeHTTP(rw, r) } }), &http2.Server{}) if err := http.ListenAndServe(w.internalPort, handler); err != nil { - log.Fatalf("ListenAndServe failed: %v", err) + log.Fatalc(ctx, "internal gRPC server failed: %v", err) } }() - log.Infof("server: started internal grpc server at %d", w.internalPort) + log.Infof("server: started internal grpc server at %s", w.internalPort) } else {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.goserver/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.gois excluded by!**/*.pb.goserver/schemas/internalapi/docs/schema.mdis excluded by!**/*.md
📒 Files selected for processing (16)
server/.env.example(1 hunks)server/e2e/common.go(4 hunks)server/e2e/proto_project_test.go(1 hunks)server/e2e/seeder.go(3 hunks)server/internal/adapter/gql/loader_project.go(1 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/internalapi/internalapimodel/project.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/app/config/config.go(1 hunks)server/internal/app/server.go(3 hunks)server/internal/infrastructure/memory/project.go(1 hunks)server/internal/infrastructure/mongo/project.go(1 hunks)server/internal/usecase/interactor/project.go(3 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/internal/usecase/repo/project.go(1 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- server/internal/adapter/gql/resolver_query.go
- server/internal/adapter/internalapi/internalapimodel/project.go
🚧 Files skipped from review as they are similar to previous changes (9)
- server/.env.example
- server/internal/app/config/config.go
- server/internal/adapter/gql/loader_project.go
- server/internal/infrastructure/memory/project.go
- server/internal/usecase/interfaces/project.go
- server/internal/usecase/repo/project.go
- server/internal/infrastructure/mongo/project.go
- server/internal/usecase/interactor/project.go
- server/internal/adapter/internalapi/server.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
server/e2e/seeder.go (4)
server/internal/infrastructure/mongo/container.go (1)
New(25-62)server/pkg/id/id.go (5)
Workspace(16-16)Workspace(36-36)User(17-17)User(37-37)UserID(53-53)server/internal/adapter/gql/gqlmodel/models_gen.go (4)
User(1407-1412)User(1414-1414)RoleReader(1878-1878)Role(1875-1875)server/internal/usecase/repo/container.go (1)
Container(21-40)
server/e2e/proto_project_test.go (6)
server/e2e/common.go (1)
GRPCServer(186-188)server/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.go (1)
ReEarthVisualizerClient(32-48)server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.go (19)
CreateProjectRequest(557-573)CreateProjectRequest(586-586)CreateProjectRequest(601-603)Visualizer(25-25)Visualizer(57-59)Visualizer(61-63)Visualizer(70-72)GetProjectListRequest(454-462)GetProjectListRequest(475-475)GetProjectListRequest(490-492)DeleteProjectRequest(703-709)DeleteProjectRequest(722-722)DeleteProjectRequest(737-739)Project(182-238)Project(251-251)Project(266-268)GetProjectRequest(510-516)GetProjectRequest(529-529)GetProjectRequest(544-546)server/internal/adapter/internalapi/client.go (1)
SafeClose(54-58)server/pkg/id/id.go (1)
ProjectIDFrom(100-100)server/internal/usecase/repo/project.go (1)
Project(19-35)
🪛 GitHub Actions: ci
server/e2e/proto_project_test.go
[error] 234-263: TestInternalAPIX failed: expected 3 but got 4; also expected 0 but got 1.
🪛 ast-grep (0.38.1)
server/e2e/common.go
[warning] 133-133: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":8080")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
🪛 Buf (1.54.0)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (11)
server/e2e/seeder.go (4)
37-55: LGTM! Clear organization of test data.The separation of user data into distinct sections with clear comments improves readability and makes it easy to identify which variables belong to which test user.
81-100: LGTM! Proper user creation with error handling.The code correctly creates and saves the additional test users with appropriate error handling for each save operation.
101-111: LGTM! Clear test setup flow.The code properly establishes the test environment by first setting up the base configuration and then adding user3 as a reader to user1's workspace, which aligns with the testing requirements for access control scenarios.
185-185: LGTM! Explicit visibility setting for test consistency.Setting the project visibility to "private" ensures predictable test behavior and aligns with the visibility testing requirements of the internal API.
server/e2e/common.go (2)
45-55: Internal API configuration looks good.The configuration structure for the internal API is well-defined and follows the existing pattern.
186-188: Helper function for gRPC testing is well-implemented.The
GRPCServerfunction provides a clean way to start a test server with internal API configuration enabled.server/internal/app/server.go (5)
18-19: New imports for gRPC integration are appropriate.The addition of h2c and grpc imports supports the new internal gRPC server functionality.
36-40: WebServer struct extension for gRPC support is well-designed.The addition of
internalPortandinternalServerfields properly extends the server to support both HTTP and gRPC modes.
107-109: Simplified signal handling is appropriate for the use case.The removal of
syscall.SIGTERMhandling and graceful shutdown timeout simplifies the signal handling while maintaining the essential interrupt handling.
116-118: New ServeGRPC method provides clean interface for gRPC serving.The method properly delegates to the internal gRPC server's Serve method.
121-123: Enhanced Shutdown method handles gRPC server gracefully.The addition of graceful stop for the internal gRPC server before shutting down the HTTP server ensures proper cleanup.
| func SafeClose(c io.Closer) { | ||
| if err := c.Close(); err != nil { | ||
| log.Printf("warning: failed to close: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate SafeClose function.
This function is already defined in server/internal/adapter/internalapi/client.go. Import and use the existing function instead of duplicating it.
Apply this diff to remove the duplicate:
-func SafeClose(c io.Closer) {
- if err := c.Close(); err != nil {
- log.Printf("warning: failed to close: %v", err)
- }
-}And update the import to use the existing function:
+ "github.com/reearth/reearth/server/internal/adapter/internalapi"Then use internalapi.SafeClose(conn) instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func SafeClose(c io.Closer) { | |
| if err := c.Close(); err != nil { | |
| log.Printf("warning: failed to close: %v", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In server/e2e/proto_project_test.go around lines 311 to 315, there is a
duplicate SafeClose function that already exists in
server/internal/adapter/internalapi/client.go. Remove this duplicate function
from proto_project_test.go, import the internalapi package, and replace calls to
SafeClose with internalapi.SafeClose to reuse the existing implementation.
server/e2e/proto_project_test.go
Outdated
| func TestInternalAPIX(t *testing.T) { | ||
| _, r, _ := GRPCServer(t, baseSeeder) | ||
|
|
||
| var publicProjectId string | ||
|
|
||
| // user1 call api | ||
| runTestWithUser(t, uID.String(), func(client pb.ReEarthVisualizerClient, ctx context.Context) { | ||
|
|
||
| // create public Project | ||
| CreateProjectInternal( | ||
| t, ctx, r, client, "public", | ||
| &pb.CreateProjectRequest{ | ||
| WorkspaceId: wID.String(), | ||
| Visualizer: pb.Visualizer_VISUALIZER_CESIUM, | ||
| Name: lo.ToPtr("Test Project1"), | ||
| Description: lo.ToPtr("Test Description1"), | ||
| CoreSupport: lo.ToPtr(true), | ||
| Visibility: lo.ToPtr("public"), | ||
| }) | ||
|
|
||
| // create private Project | ||
| CreateProjectInternal( | ||
| t, ctx, r, client, "private", | ||
| &pb.CreateProjectRequest{ | ||
| WorkspaceId: wID.String(), | ||
| Visualizer: pb.Visualizer_VISUALIZER_CESIUM, | ||
| Name: lo.ToPtr("Test Project1"), | ||
| Description: lo.ToPtr("Test Description1"), | ||
| CoreSupport: lo.ToPtr(true), | ||
| Visibility: lo.ToPtr("private"), | ||
| }) | ||
|
|
||
| // create public Project2 | ||
| CreateProjectInternal( | ||
| t, ctx, r, client, "public", | ||
| &pb.CreateProjectRequest{ | ||
| WorkspaceId: wID.String(), | ||
| Visualizer: pb.Visualizer_VISUALIZER_CESIUM, | ||
| Name: lo.ToPtr("Test Project1"), | ||
| Description: lo.ToPtr("Test Description1"), | ||
| CoreSupport: lo.ToPtr(true), | ||
| Visibility: lo.ToPtr("public"), | ||
| }) | ||
|
|
||
| // 0: creante seeder => private delete => false | ||
| // 1: creante public => public delete => true !! | ||
| // 2: creante private => private delete => false | ||
| // 3: creante public => public delete => false | ||
|
|
||
| // get list size 4 | ||
| res3, err := client.GetProjectList(ctx, &pb.GetProjectListRequest{ | ||
| WorkspaceId: wID.String(), | ||
| }) | ||
| assert.Nil(t, err) | ||
| assert.Equal(t, 3, len(res3.Projects)) | ||
| assert.Equal(t, "private", res3.Projects[0].Visibility) | ||
| assert.Equal(t, "public", res3.Projects[1].Visibility) | ||
| assert.Equal(t, "private", res3.Projects[2].Visibility) | ||
|
|
||
| publicProjectId = res3.Projects[1].Id | ||
|
|
||
| }) | ||
|
|
||
| // user1 call api | ||
| runTestWithUser(t, uID.String(), func(client pb.ReEarthVisualizerClient, ctx context.Context) { | ||
|
|
||
| // test DeleteProject | ||
| res6, err := client.DeleteProject(ctx, &pb.DeleteProjectRequest{ | ||
| ProjectId: publicProjectId, | ||
| }) | ||
| assert.Nil(t, err) | ||
| assert.NotNil(t, res6) | ||
|
|
||
| }) | ||
|
|
||
| // user2 call api | ||
| runTestWithUser(t, uID2.String(), func(client pb.ReEarthVisualizerClient, ctx context.Context) { | ||
|
|
||
| // get list size 0 | ||
| res7, err := client.GetProjectList(ctx, &pb.GetProjectListRequest{ | ||
| WorkspaceId: wID.String(), // not wID2 | ||
| }) | ||
| assert.Nil(t, err) | ||
| assert.Equal(t, 0, len(res7.Projects)) | ||
|
|
||
| }) | ||
|
|
||
| } |
There was a problem hiding this comment.
Fix test logic in TestInternalAPIX to match actual project creation flow.
The test has incorrect expectations about project counts. The test creates 4 projects total (1 seeder + 3 new) but expects only 3 in the list. Additionally, after deletion, it expects 0 projects but gets 1, suggesting the deletion logic or count expectations are wrong.
Looking at the comments and test flow:
- Seeder creates 1 project (private)
- Test creates 3 more projects (public, private, public)
- Total: 4 projects
- After deleting 1 public project: 3 remaining
Apply this diff to fix the test logic:
- assert.Equal(t, 3, len(res3.Projects))
+ assert.Equal(t, 4, len(res3.Projects))And update the expectation after deletion:
- assert.Equal(t, 0, len(res7.Projects))
+ assert.Equal(t, 1, len(res7.Projects))Also fix the typo in comments:
- // 0: creante seeder => private delete => false
- // 1: creante public => public delete => true !!
- // 2: creante private => private delete => false
- // 3: creante public => public delete => false
+ // 0: create seeder => private delete => false
+ // 1: create public => public delete => true !!
+ // 2: create private => private delete => false
+ // 3: create public => public delete => false🧰 Tools
🪛 GitHub Actions: ci
[error] 234-263: TestInternalAPIX failed: expected 3 but got 4; also expected 0 but got 1.
🤖 Prompt for AI Agents
In server/e2e/proto_project_test.go from lines 180 to 267, the test logic
incorrectly expects 3 projects after creating 4 (1 seeder + 3 new) and 0
projects after deleting one, which conflicts with actual behavior. Update the
assertions to expect 4 projects before deletion and 3 projects after deletion.
Also, correct the comment typos to accurately reflect the project creation and
deletion flow. Adjust the test to align with the actual project counts and
visibility states accordingly.
63bf03c to
e44df1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
server/e2e/common.go (2)
133-134:⚠️ Potential issueFix security issue: Server binding to all network interfaces
This is a duplicate of a previous security issue. The server is binding to all network interfaces (
:8080), which could expose it publicly in test environments.Apply this diff to fix the security issue and use configured port:
- l, err = net.Listen("tcp", ":8080") + port := "8080" + if cfg.Visualizer.InternalApi.Port != "" { + port = cfg.Visualizer.InternalApi.Port + } + l, err = net.Listen("tcp", "127.0.0.1:"+port)🧰 Tools
🪛 ast-grep (0.38.1)
[warning] 133-133: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":8080")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
144-144:⚠️ Potential issueFix security issue: Server binding to all network interfaces
This is a duplicate of a previous security issue. The server is binding to all network interfaces (
:0), which could expose it publicly.Apply this diff to restrict binding to loopback interface:
- l, err = net.Listen("tcp", ":0") + l, err = net.Listen("tcp", "127.0.0.1:0")🧰 Tools
🪛 ast-grep (0.38.1)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
server/e2e/proto_project_test.go (1)
380-384: 🛠️ Refactor suggestionRemove duplicate SafeClose function
This function is already defined in
server/internal/adapter/internalapi/client.go. Import and use the existing function instead of duplicating it.Apply this diff to remove the duplicate:
-func SafeClose(c io.Closer) { - if err := c.Close(); err != nil { - log.Printf("warning: failed to close: %v", err) - } -}And update the import to use the existing function:
+ "github.com/reearth/reearth/server/internal/adapter/internalapi"Then use
internalapi.SafeClose(conn)instead ofSafeClose(conn).server/schemas/internalapi/v1/schema.proto (4)
3-3:⚠️ Potential issueFix package directory mismatch causing Buf lint error
The package declaration
reearth.visualizer.v1doesn't match the file pathserver/schemas/internalapi/v1/, causing aPACKAGE_DIRECTORY_MATCHviolation.Apply this diff to fix the package declaration:
-package reearth.visualizer.v1; +package server.schemas.internalapi.v1;🧰 Tools
🪛 Buf (1.54.0)
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
7-7: 🛠️ Refactor suggestionProvide fully-qualified go_package option
The current
go_packageoption yields ambiguous import paths that can collide easily.Apply this diff to specify the complete module path:
-option go_package = "proto/v1"; +option go_package = "github.com/reearth/reearth/server/internal/adapter/internalapi/schemas/internalapi/v1;internalapiv1";
96-98:⚠️ Potential issueSecurity vulnerability: Remove basic_auth_password from responses
Exposing
basic_auth_passwordin everyProjectresponse creates a serious security risk, increasing the likelihood of credential leakage through logs, client dumps, etc.Apply this diff to remove the password field:
- // Publishment value - string basic_auth_username = 24; - // Publishment value - string basic_auth_password = 25; + // Publishment value + string basic_auth_username = 24; + // Publishment value - password removed for security + bool is_basic_auth_enabled = 25;If password input is needed, create a separate write-only request message.
74-74: 🛠️ Refactor suggestionModel visibility as enum for type safety
Using
string visibilityinvites typos and inconsistent usage. An enum provides better type safety and validation.Apply this diff to define a Visibility enum:
+enum Visibility { + VISIBILITY_UNSPECIFIED = 0; + VISIBILITY_PUBLIC = 1; + VISIBILITY_LIMITED = 2; + VISIBILITY_PRIVATE = 3; +} + message Project { // ... other fields ... - string visibility = 14; + Visibility visibility = 14;Also update the request message:
- optional string visibility = 6; + optional Visibility visibility = 6;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
server/internal/adapter/internalapi/schemas/internalapi/v1/schema.pb.gois excluded by!**/*.pb.goserver/internal/adapter/internalapi/schemas/internalapi/v1/schema_grpc.pb.gois excluded by!**/*.pb.goserver/schemas/internalapi/docs/schema.mdis excluded by!**/*.md
📒 Files selected for processing (16)
server/.env.example(1 hunks)server/e2e/common.go(4 hunks)server/e2e/proto_project_test.go(1 hunks)server/e2e/seeder.go(3 hunks)server/internal/adapter/gql/loader_project.go(1 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/internalapi/internalapimodel/project.go(1 hunks)server/internal/adapter/internalapi/server.go(1 hunks)server/internal/app/config/config.go(1 hunks)server/internal/app/server.go(3 hunks)server/internal/infrastructure/memory/project.go(1 hunks)server/internal/infrastructure/mongo/project.go(1 hunks)server/internal/usecase/interactor/project.go(3 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/internal/usecase/repo/project.go(1 hunks)server/schemas/internalapi/v1/schema.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- server/.env.example
- server/internal/app/config/config.go
- server/internal/adapter/gql/loader_project.go
- server/internal/usecase/repo/project.go
- server/internal/usecase/interfaces/project.go
- server/e2e/seeder.go
- server/internal/adapter/gql/resolver_query.go
- server/internal/infrastructure/mongo/project.go
- server/internal/usecase/interactor/project.go
- server/internal/adapter/internalapi/internalapimodel/project.go
- server/internal/infrastructure/memory/project.go
- server/internal/adapter/internalapi/server.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/e2e/common.go (3)
server/internal/usecase/gateway/file.go (1)
File(25-48)server/internal/app/config/config.go (3)
Config(24-77)VisualizerConfig(79-81)InternalApiConfig(83-85)server/internal/usecase/repo/container.go (1)
Container(21-40)
🪛 Buf (1.54.0)
server/schemas/internalapi/v1/schema.proto
3-3: Files with package "reearth.visualizer.v1" must be within a directory "reearth/visualizer/v1" relative to root but were in directory "server/schemas/internalapi/v1".
(PACKAGE_DIRECTORY_MATCH)
🪛 ast-grep (0.38.1)
server/e2e/common.go
[warning] 133-133: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":8080")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
[warning] 144-144: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
🔇 Additional comments (5)
server/e2e/common.go (1)
186-188: LGTM: Clean helper function for gRPC server setupThe new
GRPCServerhelper function provides a clean way to start the internal API server for testing purposes. The implementation correctly uses theinternalApiConfigto enable the internal API.server/internal/app/server.go (2)
79-110: LGTM: Well-structured conditional server startup logicThe updated
Runmethod properly handles both internal gRPC server and regular HTTP server startup based on configuration. The HTTP/2 cleartext (h2c) handler for gRPC routing is correctly implemented, and the simplified signal handling is appropriate.
116-118: LGTM: Clean gRPC server interfaceThe new
ServeGRPCmethod provides a clean interface for serving the internal gRPC server on a provided listener, maintaining consistency with the existingServemethod.server/e2e/proto_project_test.go (2)
25-84: LGTM: Comprehensive test coverage for private project visibilityThe test correctly validates that private projects are only accessible to authorized users and that unauthenticated users cannot see private projects from other workspaces.
337-337:✅ Verification successful
Verify gRPC client API compatibility
The
grpc.NewClientfunction may not exist in the current gRPC version. This could cause compilation failures.Run the following script to verify the gRPC client API:
If
grpc.NewClientdoesn't exist, usegrpc.DialContextinstead:- conn, err := grpc.NewClient("localhost:8080", grpc.WithTransportCredentials(insecure.NewCredentials())) + conn, err := grpc.DialContext(ctx, "localhost:8080", + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithBlock())
🏁 Script executed:
#!/bin/bash # Description: Check if grpc.NewClient exists in the current gRPC version # Expected: Find usage patterns and verify API availability echo "Checking gRPC version and NewClient availability..." cd server go list -m google.golang.org/grpc echo "Searching for NewClient usage in go.mod dependencies..." go mod graph | grep google.golang.org/grpc echo "Checking gRPC documentation for NewClient..." go doc google.golang.org/grpc | grep -i "newclient\|dial"Length of output: 11007
🏁 Script executed:
#!/bin/bash # Verify duplicate SafeClose definitions in proto_project_test.go grep -R "func SafeClose" -n server/e2e/proto_project_test.go || echo "No SafeClose definitions found"Length of output: 96
No gRPC API or duplicate SafeClose issues detected
Thegrpc.NewClientfunction is present in google.golang.org/grpc v1.70.0, and there is only oneSafeClosedefinition inserver/e2e/proto_project_test.go. Both compatibility and duplication concerns are unfounded—no changes are necessary.
| if cfg.Config.Visualizer.InternalApi.Active { | ||
| w.internalPort = ":" + cfg.Config.Port | ||
| w.internalServer = initGrpc(cfg) | ||
| } |
There was a problem hiding this comment.
Fix incorrect port configuration for internal API
The internal API is using the main server port (cfg.Config.Port) instead of a dedicated internal API port. This will cause port conflicts when both servers try to start.
Apply this diff to use the correct internal API port configuration:
- if cfg.Config.Visualizer.InternalApi.Active {
- w.internalPort = ":" + cfg.Config.Port
- w.internalServer = initGrpc(cfg)
- }
+ if cfg.Config.Visualizer.InternalApi.Active {
+ port := cfg.Config.Visualizer.InternalApi.Port
+ if port == "" {
+ port = "50051"
+ }
+ w.internalPort = ":" + port
+ w.internalServer = initGrpc(cfg)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if cfg.Config.Visualizer.InternalApi.Active { | |
| w.internalPort = ":" + cfg.Config.Port | |
| w.internalServer = initGrpc(cfg) | |
| } | |
| if cfg.Config.Visualizer.InternalApi.Active { | |
| port := cfg.Config.Visualizer.InternalApi.Port | |
| if port == "" { | |
| port = "50051" | |
| } | |
| w.internalPort = ":" + port | |
| w.internalServer = initGrpc(cfg) | |
| } |
🤖 Prompt for AI Agents
In server/internal/app/server.go around lines 72 to 75, the internal API is
incorrectly configured to use the main server port (cfg.Config.Port), causing
port conflicts. Update the code to use the dedicated internal API port
configuration (likely cfg.Config.Visualizer.InternalApi.Port) when setting
w.internalPort, ensuring the internal server listens on its own separate port.
Overview
Implement the internal API for the visualizer
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Documentation