Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
ensureAuthHandlerbypass for/authroutes usesstrings.Contains(r.RequestURI, "/auth"), which also matches query strings and other paths containingauth; consider checkingr.URL.Pathwith a stricter predicate (e.g.HasPrefix("/auth")or exact route matching) to avoid unintentionally skipping auth on unrelated endpoints. - In
RegisterSiteHandlersandRegisterSystemHandler,ensureAuthHandleris applied both to the/apirouter and again to theprotectedsubrouter, resulting in redundant middleware wrapping; simplifying so each request passes through the auth middleware only once will reduce complexity and potential for subtle behavior differences.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ensureAuthHandler` bypass for `/auth` routes uses `strings.Contains(r.RequestURI, "/auth")`, which also matches query strings and other paths containing `auth`; consider checking `r.URL.Path` with a stricter predicate (e.g. `HasPrefix("/auth")` or exact route matching) to avoid unintentionally skipping auth on unrelated endpoints.
- In `RegisterSiteHandlers` and `RegisterSystemHandler`, `ensureAuthHandler` is applied both to the `/api` router and again to the `protected` subrouter, resulting in redundant middleware wrapping; simplifying so each request passes through the auth middleware only once will reduce complexity and potential for subtle behavior differences.
## Individual Comments
### Comment 1
<location path="server/http_auth.go" line_range="168-169" />
<code_context>
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+
+ // allow if route is prefixed /auth
+ if strings.Contains(r.RequestURI, "/auth") {
+ next.ServeHTTP(w, r)
+ return
</code_context>
<issue_to_address>
**🚨 issue (security):** Using strings.Contains on RequestURI for the /auth bypass is overly broad and can unintentionally skip auth on unrelated routes.
Using `strings.Contains(r.RequestURI, "/auth")` will match any URL containing that substring, including query params or unrelated paths like `/api/oauth/callback`, and unintentionally skip auth. Instead, base this on `r.URL.Path` and use a stricter condition such as `strings.HasPrefix(r.URL.Path, "/auth")` or a more specific prefix like `"/api/auth"` so only the intended auth endpoints are exempted.
</issue_to_address>
### Comment 2
<location path="assets/js/views/App.vue" line_range="203-206" />
<code_context>
}
},
- connect() {
+ async connect() {
console.log("websocket connect");
const supportsWebSockets = "WebSocket" in window;
</code_context>
<issue_to_address>
**issue (bug_risk):** The async connect flow doesn’t guard against updateAuthStatus() failures, which can surface as unhandled promise rejections.
Because callers like `mounted()` and the reconnect logic don’t await or wrap `connect()`, any rejection from `updateAuthStatus()` (e.g. network/5xx errors) will surface as an unhandled promise rejection. Consider wrapping the `await updateAuthStatus()` call in a try/catch and either logging and continuing, or otherwise handling the error so it doesn’t break the reconnect loop.
```js
async connect() {
console.log("websocket connect");
const supportsWebSockets = "WebSocket" in window;
// ... feature checks using supportsWebSockets ...
try {
await updateAuthStatus();
} catch (e) {
console.error("auth status update failed", e);
// decide whether to abort or still attempt WS
return;
}
if (authState.loggedIn === false) {
openLoginModal();
return;
}
// proceed with WebSocket
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| async connect() { | ||
| console.log("websocket connect"); | ||
| const supportsWebSockets = "WebSocket" in window; | ||
| if (!supportsWebSockets) { |
There was a problem hiding this comment.
issue (bug_risk): The async connect flow doesn’t guard against updateAuthStatus() failures, which can surface as unhandled promise rejections.
Because callers like mounted() and the reconnect logic don’t await or wrap connect(), any rejection from updateAuthStatus() (e.g. network/5xx errors) will surface as an unhandled promise rejection. Consider wrapping the await updateAuthStatus() call in a try/catch and either logging and continuing, or otherwise handling the error so it doesn’t break the reconnect loop.
async connect() {
console.log("websocket connect");
const supportsWebSockets = "WebSocket" in window;
// ... feature checks using supportsWebSockets ...
try {
await updateAuthStatus();
} catch (e) {
console.error("auth status update failed", e);
// decide whether to abort or still attempt WS
return;
}
if (authState.loggedIn === false) {
openLoginModal();
return;
}
// proceed with WebSocket
}…st login before actually going to dashboard
…efore server connection
|
Hi @xBlaz3kx, thanks for the PR. The current implementation, having daily function without auth and critical functionality (secrets, configuration) protected by auth, is a deliberate tradeoff (convenience vs security, home usage). We do not recommend exposing evcc directly to the internet. For these cases a reverse proxy or VPN is recommended. That said, we are aware that the above tradeoffs might not be right for everyone. See also existing feature requests #17488. But we'll not switch to having everything auth-required be default. This would be a breaking change with implications to many automations and integrations. We also don't have an API key mechanism yet. We're not fundamentally against making the auth behavior configurable for special needs. I'd appreciate starting a discussions or participating in existing feature request first, before submitting an implementation. |
IMO, at least "destructive" and data-sensitive endpoints should be secured. But I would argue that even apps in private networks should be sufficiently secured. |
securing the API endpoints with auth middleware and adapted FE to request login before continuing.
Both websocket and http endpoints are now protected. This helps when the app is exposed to the public.