<review>
  <summary>
    The router has critical authentication and authorization flaws, leaks
    credentials, and violates several REST conventions. Issues are ordered
    below from most to least severe.
  </summary>

  <issues>

    <issue id="1" severity="critical" category="auth">
      <title>Authentication tokens are unsigned and unverified</title>
      <description>
        login() builds the token with base64.b64encode of "uid:username".
        Base64 is encoding, not signing, so any client can forge a token for
        any user. current_user() compounds this by calling jwt.decode with
        options={"verify_signature": False}, accepting any token without
        checking the signature. The declared SECRET is never used. There is
        effectively no authentication.
      </description>
      <fix>
        Issue real signed JWTs and verify them. Encode with
        jwt.encode({"sub": uid, "exp": ...}, SECRET, algorithm="HS256");
        decode with jwt.decode(token, SECRET, algorithms=["HS256"]) and map
        InvalidTokenError / ExpiredSignatureError to a 401 response. Load
        SECRET from environment/secret store, never hardcode it.
      </fix>
    </issue>

    <issue id="2" severity="critical" category="auth">
      <title>Passwords stored and returned in plaintext</title>
      <description>
        users_db holds raw passwords, login() compares them directly, and
        GET /users returns each user's password in the response body. A
        single read of the user list exposes every credential. GET
        /users/{user_id} also returns the full record including the password.
      </description>
      <fix>
        Store only salted password hashes (bcrypt/argon2 via passlib).
        Compare with the library's verify function. Never include password
        (or hash) fields in any serialized response; use an explicit
        response schema that whitelists id/username/email/is_admin.
      </fix>
    </issue>

    <issue id="3" severity="critical" category="authorization">
      <title>Sensitive endpoints have no authorization checks</title>
      <description>
        list_users, get_user, and reset_password accept no credentials at
        all. reset_password lets anyone set any user's password via a query
        parameter (account takeover). update_email decodes the caller but
        then ignores it, writing to users_db[user_id] regardless of who the
        caller is, so any authenticated user can edit any other user's email
        (IDOR/privilege escalation).
      </description>
      <fix>
        Require authentication on every protected route via a dependency
        (Depends(current_user)). Enforce ownership/role: a user may only
        modify their own record unless they are an admin; admin-only routes
        must check the verified is_admin claim. Return 403 when the
        authenticated caller lacks permission.
      </fix>
    </issue>

    <issue id="4" severity="high" category="authorization">
      <title>Admin gate relies on a spoofable client header</title>
      <description>
        delete_user authorizes deletion solely on the request header
        X-Admin == "true". Any caller can send that header, so the admin
        check provides no protection.
      </description>
      <fix>
        Derive admin status from the verified token claims (is_admin), not
        from a client-supplied header. Reject with 403 if the authenticated
        user is not an admin.
      </fix>
    </issue>

    <issue id="5" severity="high" category="http-method">
      <title>State-changing actions exposed over GET</title>
      <description>
        DELETE is performed via GET /users/{user_id}/delete. GET must be
        safe and idempotent; using it for deletion means the action can be
        triggered by prefetching, caching, crawlers, or a simple link, and
        is trivially exploitable via CSRF.
      </description>
      <fix>
        Use the DELETE method on the resource: DELETE /users/{user_id}.
        Remove the /delete verb-in-path. Reserve GET for safe reads only.
      </fix>
    </issue>

    <issue id="6" severity="medium" category="rest-convention">
      <title>Credentials passed as URL query parameters</title>
      <description>
        login() and reset_password() take username, password, and
        new_password as Query parameters. Secrets in the URL are logged by
        servers/proxies, stored in browser history, and leak via the
        Referer header.
      </description>
      <fix>
        Accept credentials in the request body using a Pydantic model
        (e.g. a LoginRequest with username/password fields) so they travel
        in the POST/PUT body, not the URL.
      </fix>
    </issue>

    <issue id="7" severity="medium" category="status-codes">
      <title>Errors return HTTP 200 with an error body</title>
      <description>
        Bad credentials, "not found", and "admin only" all return with a
        200 OK status and a {"status": "error"} payload. Clients, caches,
        and monitoring cannot distinguish success from failure by status
        code, which is a REST violation.
      </description>
      <fix>
        Use proper status codes: 401 for failed login / missing-invalid
        token, 403 for authorization failures, 404 for not found, 400/422
        for bad input. Raise HTTPException(status_code=..., detail=...)
        instead of returning ad-hoc error dicts with 200.
      </fix>
    </issue>

    <issue id="8" severity="medium" category="error-handling">
      <title>Missing-key writes raise unhandled 500s</title>
      <description>
        reset_password and update_email index users_db[user_id] directly. If
        the user_id does not exist this raises KeyError, surfacing as an
        unhandled 500 instead of a clean 404. get_user handles the missing
        case but the write paths do not.
      </description>
      <fix>
        Look the user up first and raise HTTPException(404) when absent
        before mutating. Apply the same not-found guard consistently across
        all routes that take a user_id.
      </fix>
    </issue>

    <issue id="9" severity="low" category="status-codes">
      <title>Successful creation/mutation uses default 200</title>
      <description>
        Mutating endpoints return the implicit 200 and a hand-rolled
        {"status": "ok"} envelope. A successful DELETE conventionally
        returns 204, and responses should represent the affected resource
        rather than a status string.
      </description>
      <fix>
        Set explicit status_code on the route decorator (e.g. 204 for
        delete) and return the resource representation or an empty body as
        appropriate. Drop the redundant status:"ok" wrapper.
      </fix>
    </issue>

    <issue id="10" severity="low" category="rest-convention">
      <title>Resource paths use verbs</title>
      <description>
        /users/{id}/delete and /users/{id}/reset-password put actions in the
        path. REST favors resource nouns with HTTP methods conveying the
        action.
      </description>
      <fix>
        Model deletion as DELETE /users/{id}. Model a password change as a
        write to a sub-resource, e.g. PUT /users/{id}/password, keeping the
        path noun-based.
      </fix>
    </issue>

  </issues>
</review>
