u
This commit is contained in:
67
TODO.md
67
TODO.md
@@ -133,3 +133,70 @@ Conversion order (bottom-up by dependency):
|
|||||||
(queries/schema stay mssql-specific, so likely keep it)
|
(queries/schema stay mssql-specific, so likely keep it)
|
||||||
- [ ] Decide export for `SCHEMA_SCRIPTS` (currently re-exported from `index.js`
|
- [ ] Decide export for `SCHEMA_SCRIPTS` (currently re-exported from `index.js`
|
||||||
from the moved schema file)
|
from the moved schema file)
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
I'll check how auth is applied across the routes.
|
||||||
|
|
||||||
|
Viewed index.js:1-305
|
||||||
|
Searched for "app\.(post|put|patch|delete|get|use|all)\("
|
||||||
|
Viewed auth.js:1-132
|
||||||
|
|
||||||
|
The middleware skips auth on several path prefixes. Next I'll check which of those can write.
|
||||||
|
|
||||||
|
Searched for "\.(post|put|patch|delete)\("
|
||||||
|
Searched for "'/(api/)?(public|mcp|oauth|\.well-known)"
|
||||||
|
|
||||||
|
Next: the telemetry public route, the `/oauth/register` handler, and the v1 router.
|
||||||
|
|
||||||
|
Viewed telemetry.js:1-118
|
||||||
|
Viewed mcp.js:360-460
|
||||||
|
Searched for "(router|app)\.(post|put|patch|delete|use|all)\(|auth|token"
|
||||||
|
Viewed index.js:1-27
|
||||||
|
Searched for "registerClient|export (async )?function"
|
||||||
|
Viewed mcp.js:455-540
|
||||||
|
Searched for "db\.\w+|verifyIdToken|allow|jwt\.sign"
|
||||||
|
|
||||||
|
**No. With no JWT, an attacker can still write to the database.** The checks I did are static code review only, and I haven't run anything.
|
||||||
|
|
||||||
|
`authMiddleware` ([auth.js](file:///home/seb/src/dashback/src/api/middleware/auth.js#L49-L68)) lets several path groups through without a token. Most of them can't write to the SQL database, but two can.
|
||||||
|
|
||||||
|
### 1. Open OAuth registration: `POST /oauth/register`
|
||||||
|
- This route is public, and it calls `mcpDb.registerClient` ([mcp.js:368](file:///home/seb/src/dashback/src/api/routes/mcp.js#L368)) to insert a client row.
|
||||||
|
- Anyone can create unlimited rows with no auth and no rate limit.
|
||||||
|
- `client_name`, `redirect_uris`, `grant_types` and `scope` all come straight from the request body.
|
||||||
|
|
||||||
|
### 2. Auth bypass: `POST /oauth/authorize`
|
||||||
|
- The middleware calls `next()` on this route even with no token ([auth.js:70-79](file:///home/seb/src/dashback/src/api/middleware/auth.js#L70-L79)).
|
||||||
|
- The handler ([mcp.js:455-503](file:///home/seb/src/dashback/src/api/routes/mcp.js#L455-L503)) takes an `email` from the body when there's no `req.user`.
|
||||||
|
- If that email is on the allowlist, it writes an auth code (`createAuthCode`) for that user and redirects with it. **No password, Google token or JWT is checked.**
|
||||||
|
- An attacker can chain this with the previous hole:
|
||||||
|
1. Register a client with `/oauth/register`.
|
||||||
|
2. Post to `/oauth/authorize` with the victim's allowlisted email and their own PKCE `code_challenge`.
|
||||||
|
3. Exchange the code at `/oauth/token`, which I haven't read.
|
||||||
|
4. Call `/mcp` as that user.
|
||||||
|
- That would be a full impersonation if the MCP tools can write. Please confirm this by testing against a dev instance.
|
||||||
|
- Also, `new URL(redirect_uri)` runs with no try/catch and no check that `redirect_uri` matches the registered URIs. A bad value throws, and a valid one is an open redirect that sends the code to the attacker.
|
||||||
|
|
||||||
|
### Not writing to the SQL database
|
||||||
|
- `/api/public/telemetry/crash` ([telemetry.js](file:///home/seb/src/dashback/src/api/routes/telemetry.js#L87-L104)) is public and unauthenticated. It writes to a file on disk rather than SQL, but there's no size or rate limit beyond `bodyLimit`, so it can fill the disk.
|
||||||
|
- `/v1` and `/api/v1` skip the middleware, but the routes I saw are all `GET`. The ones with data use `posAuthMiddleware`, and `/client`, `/pimage` and `/cimage` have no auth.
|
||||||
|
- `/api/auth/login` verifies a Google ID token and checks the allowlist. If `IMPLICIT_CREATE=true`, any valid Google account gets added to the allowlist ([login.js:107](file:///home/seb/src/dashback/src/api/routes/auth/login.js#L107)). Make sure that's off in production.
|
||||||
|
|
||||||
|
### Suggested fixes
|
||||||
|
- Make `/oauth/authorize` POST require `req.user`, and remove the `email` fallback.
|
||||||
|
- Validate `redirect_uri` against the registered URIs for the client.
|
||||||
|
- Rate-limit `/oauth/register`, or gate it.
|
||||||
|
|
||||||
|
Want me to implement these? I haven't changed any code.
|
||||||
Reference in New Issue
Block a user