Conversation
Signed-off-by: E99p1ant <i@github.red>
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a dynamic reverse-proxy feature: new Service configuration (backend list), configuration parsing to populate backends, a /api/v1/service/{**} route, and a Proxy handler that routes requests to configured backends with header injection and tracing propagation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Router as "API Router\n(/api/v1)"
participant Proxy as "Service.Proxy\n(handler)"
participant Backend as "Backend Service"
participant Tracer as "OpenTelemetry"
Client->>Router: HTTP request to /service/{...}
Router->>Proxy: call Proxy(ctx)
Proxy->>Proxy: extract path, find backend by prefix
alt backend matched
Proxy->>Tracer: propagate tracing headers
Note right of Proxy `#DFF2E1`: inject headers\nX-NekoBox-From, X-NekoBox-User-ID
Proxy->>Backend: forward request (rewritten URL/path)
Backend-->>Proxy: response
Proxy-->>Client: proxied response
else no match
Proxy-->>Client: 404 JSON error
end
alt forward error
Proxy-->>Client: 500 JSON error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR introduces a service gateway feature that enables proxying HTTP requests to configurable backend services. The gateway acts as a reverse proxy with OpenTelemetry tracing support and user context propagation.
- Adds a new reverse proxy handler that routes requests based on URL prefixes to configured backend services
- Implements configuration support for multiple backend services with prefix-based routing
- Integrates the service proxy endpoint into the API routing structure
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| route/service/proxy.go | New reverse proxy handler with tracing, user context propagation, and configurable backend routing |
| internal/route/route.go | Registers the new service proxy endpoint at /api/v1/service/{**} |
| internal/conf/static.go | Adds Service configuration struct to hold backend service definitions |
| internal/conf/conf.go | Implements configuration parsing for service backends from INI file child sections |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: E99p1ant <i@github.red>
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @wuhan005. * #70 (comment) The following files were modified: * `internal/conf/conf.go` * `internal/route/route.go` * `route/service/proxy.go`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
route/service/proxy.go (2)
39-48: Consider optimizing backend lookup for better performance.The current linear search through
conf.Service.Backendsis acceptable for a small number of backends, but could become a bottleneck if many services are configured. Consider using a map for O(1) lookup.🔎 Alternative implementation with map-based lookup
You could create a map during initialization in
internal/conf/conf.go:Service struct { Backends []struct { Prefix string `ini:"prefix"` ForwardURL string `ini:"forward_url"` } BackendMap map[string]string // Add this field }Then populate it after loading:
// After line 85 in conf.go Service.BackendMap = make(map[string]string) for _, backend := range Service.Backends { Service.BackendMap[backend.Prefix] = backend.ForwardURL }Then in the proxy:
-var forwardURLStr string -for _, backend := range conf.Service.Backends { - if backend.Prefix == basePath { - forwardURLStr = backend.ForwardURL - break - } -} -if len(forwardURLStr) == 0 { +forwardURLStr, ok := conf.Service.BackendMap[basePath] +if !ok { return ctx.JSONError(http.StatusNotFound, "页面不存在") }
57-60: Review path construction logic for correctness.The URL path construction on line 59 may produce incorrect results in some cases:
- If
forwardPathdoesn't start with/, it will be concatenated directly:"/api" + "users"→"/apiusers"- If
forwardURLcontains a path (e.g.,http://backend/api), that base path is used, which may or may not be intendedConsider using
path.Joinor ensuringforwardPathalways starts with/for more predictable behavior.🔎 Suggested improvement
+import "path" Director: func(req *http.Request) { req.URL = forwardURL - req.URL.Path = strings.TrimRight(req.URL.Path, "/") + forwardPath + req.URL.Path = path.Join(req.URL.Path, forwardPath) req.Host = forwardURL.HostOr ensure
forwardPathalways starts with/:Director: func(req *http.Request) { req.URL = forwardURL + if !strings.HasPrefix(forwardPath, "/") && forwardPath != "" { + forwardPath = "/" + forwardPath + } req.URL.Path = strings.TrimRight(req.URL.Path, "/") + forwardPath req.Host = forwardURL.Hostinternal/route/route.go (1)
145-145: Clarify that the service gateway intentionally allows unauthenticated access.The service proxy route does not require authentication middleware, unlike the pixel proxy on line 144. However, examining the implementation in
route/service/proxy.goreveals this is intentional: the proxy accepts both authenticated and unauthenticated requests, passing the user ID (defaulting to 0 when not logged in) to backends via theX-NekoBox-User-IDheader. This design delegates authentication responsibility to the backend services. If this is not the intended behavior, addreqUserSignInmiddleware to line 145.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/conf/conf.gointernal/conf/static.gointernal/route/route.goroute/service/proxy.go
🧰 Additional context used
🧬 Code graph analysis (3)
internal/conf/conf.go (1)
internal/conf/static.go (1)
Service(87-92)
route/service/proxy.go (3)
internal/context/context.go (1)
Context(48-57)internal/db/users.go (1)
User(43-56)internal/conf/static.go (1)
Service(87-92)
internal/route/route.go (2)
route/service/proxy.go (1)
Proxy(21-84)route/pixel/pixel.go (1)
Proxy(23-62)
🪛 ast-grep (0.40.3)
route/service/proxy.go
[warning] 71-73: 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{
InsecureSkipVerify: true,
}
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)
🪛 GitHub Check: CodeQL
route/service/proxy.go
[failure] 73-73: Disabled TLS certificate check
InsecureSkipVerify should not be used in production code.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Test MySQL
🔇 Additional comments (4)
internal/conf/conf.go (1)
74-85: LGTM! Configuration parsing is correct.The service configuration parsing follows the established pattern in this file. The loop correctly iterates through child sections and populates the Service.Backends slice.
internal/conf/static.go (1)
87-92: LGTM!The Service configuration struct is well-defined and follows the established patterns in this file. The field names and INI tags are clear and appropriate for the gateway configuration.
route/service/proxy.go (2)
21-33: LGTM! Proper tracing and user context handling.The implementation correctly handles optional authentication and integrates OpenTelemetry tracing with appropriate defensive checks.
35-37: The path parsing logic is correct and does not have the edge cases described.In Echo framework, the
{**}wildcard parameter captures the remaining path segment without the leading slash. For example, a request to/service/foo/barresults inuri = "foo/bar"(not/foo/bar). Therefore:
strings.Split("foo/bar", "/")[0]correctly returns"foo"as the basePathstrings.TrimPrefix("foo/bar", "foo")correctly returns"/bar"as the forwardPathThe code functions as intended and properly handles backend prefix matching.
| Transport: &http.Transport{ | ||
| TLSClientConfig: &tls.Config{ | ||
| InsecureSkipVerify: true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
CRITICAL: TLS certificate verification is disabled.
The InsecureSkipVerify: true setting completely disables TLS certificate verification, making the proxy vulnerable to man-in-the-middle attacks. This should never be used in production code.
Additionally, the TLS configuration is missing MinVersion, which should be set to at least tls.VersionTLS12 (or preferably tls.VersionTLS13).
🔎 Required security fix
Remove InsecureSkipVerify and add proper TLS configuration:
Transport: &http.Transport{
TLSClientConfig: &tls.Config{
- InsecureSkipVerify: true,
+ MinVersion: tls.VersionTLS12,
},
},If you need to support self-signed certificates in development environments, use a proper certificate pool or make this configurable per-backend with clear warnings, but never use InsecureSkipVerify in production.
Based on static analysis hints: CWE-327, OWASP A02:2021 - Cryptographic Failures.
📝 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.
| Transport: &http.Transport{ | |
| TLSClientConfig: &tls.Config{ | |
| InsecureSkipVerify: true, | |
| }, | |
| }, | |
| Transport: &http.Transport{ | |
| TLSClientConfig: &tls.Config{ | |
| MinVersion: tls.VersionTLS12, | |
| }, | |
| }, |
🧰 Tools
🪛 ast-grep (0.40.3)
[warning] 71-73: 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{
InsecureSkipVerify: true,
}
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)
🪛 GitHub Check: CodeQL
[failure] 73-73: Disabled TLS certificate check
InsecureSkipVerify should not be used in production code.
🤖 Prompt for AI Agents
In route/service/proxy.go around lines 71 to 75, the TLS config currently sets
InsecureSkipVerify: true which disables certificate validation; remove that
setting, set TLSConfig.MinVersion to at least tls.VersionTLS12 (prefer
tls.VersionTLS13), and populate RootCAs with a proper x509.CertPool if you need
to trust self-signed certs for dev/testing or make an explicit per-backend
“allow insecure” flag (off by default) that uses a separate, clearly documented
config path for non-production use; do not leave InsecureSkipVerify enabled in
production.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.