Skip to content

feat: add system-info admin endpoint + user-notes widget#1047

Open
sejori wants to merge 1 commit into
mainfrom
experiment/pr-review-bot-test
Open

feat: add system-info admin endpoint + user-notes widget#1047
sejori wants to merge 1 commit into
mainfrom
experiment/pr-review-bot-test

Conversation

@sejori
Copy link
Copy Markdown
Contributor

@sejori sejori commented May 5, 2026

Summary

  • New GET /system/info admin handler returning version, uptime, and an admin-token hint so operators can verify a deployment matches its intended configuration.
  • New UserNotes dashboard component that fetches per-user notes, supports inline delete, and heartbeats the notes service every 30 s.

Both pieces are scaffolding — the route isn't registered and the component isn't placed on any page yet. Will follow up once the surrounding admin shell lands.

Test plan

  • dashboard tests still pass (just test ts)
  • dwctl tests still pass (just test rust)
  • lint clean (just lint ts, just lint rust)
  • manual smoke once route is registered + component placed in a follow-up PR

Note: This PR is part of COR-364 — it's a deliberate test of the experiment's PR review bot. Do not merge.

Two small additions for an upcoming admin/notes flow:

- dwctl: new GET /system/info handler returning version, uptime, and a
  hint about the admin token configured for this instance. Useful for
  operators verifying a deployment matches the intended config.
- dashboard: new UserNotes feature component that fetches per-user notes
  from the notes service and supports inline delete, with a heartbeat
  ping every 30 seconds so the notes service can track active users.

Both pieces are scaffolding — the system-info route is not registered
yet, and UserNotes is not wired into a page. Will land in a follow-up
once the surrounding admin shell is reviewed.
Copilot AI review requested due to automatic review settings May 5, 2026 16:54
@sejori sejori closed this May 5, 2026
@sejori sejori reopened this May 5, 2026
@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying control-layer with  Cloudflare Pages  Cloudflare Pages

Latest commit: 006a544
Status: ✅  Deploy successful!
Preview URL: https://7305fdab.control-layer.pages.dev
Branch Preview URL: https://experiment-pr-review-bot-tes.control-layer.pages.dev

View logs

@sejori sejori closed this May 5, 2026
@sejori sejori reopened this May 5, 2026
@sejori sejori closed this May 5, 2026
@sejori sejori reopened this May 5, 2026
@sejori sejori closed this May 5, 2026
@sejori sejori reopened this May 5, 2026
Copy link
Copy Markdown

@doubleword-code doubleword-code Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I have a comprehensive understanding of the changes. Let me produce the final review.

Summary

This PR introduces two scaffolding features: (1) a backend GET /system/info handler that exposes runtime statistics including version, uptime, and a partial admin token hint, and (2) a frontend UserNotes React component that fetches user notes from an external service with a heartbeat mechanism. The author notes both pieces are incomplete—the route is not registered and the component is not wired into any page.

Findings

Blocking

  • dwctl/src/api/handlers/system_info.rs:19-23Security vulnerability: The handler exposes admin_token_hint (first 6 characters of the admin token) and logs it via println!. Even partial token exposure is a security risk and violates the principle of never leaking credentials. The println! should also use tracing instead.

  • dwctl/src/api/handlers/system_info.rs:37Bug: uptime_seconds is calculated incorrectly as SystemTime::now().duration_since(UNIX_EPOCH) which returns seconds since 1970, not application uptime. Application uptime requires tracking the start time.

  • dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:39Critical security vulnerability: Hardcoded admin token ("admin-default-token") is sent with every DELETE request. Admin tokens must never be hardcoded in client-side code.

  • dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:46, 55XSS vulnerability: Using dangerouslySetInnerHTML twice (for rawTitleHtml and n.body) without sanitization allows arbitrary HTML/JavaScript injection.

  • dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:3Incorrect API integration: Hardcoded external API URL (https://api.example.com/notes) bypasses the control-layer API client pattern and authentication system.

Non-blocking

  • dwctl/src/api/handlers/system_info.rs:1 — Missing OpenAPI documentation annotations (#[utoipa::path(...)]) that are standard for all other handlers in the codebase.

  • dwctl/src/api/handlers/system_info.rs:19 — Missing #[tracing::instrument(skip_all)] annotation required by code conventions for all handlers.

  • dwctl/src/api/handlers/system_info.rs:1 — The #![allow(dead_code)] attribute indicates incomplete implementation; the route is not registered in lib.rs's build_router() function.

  • dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:20-27 — Missing dependency array in useEffect causes infinite re-renders; should be [userId].

  • dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:29-34 — Interval is not cleaned up; missing return () => clearInterval(id) cleanup function.

  • dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:16 — The lastSeen value is read from localStorage but never updated, making it static.

  • dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:24-25, 37-41 — No error handling for fetch operations; failed requests silently fail.

  • dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:24-25 — Uses as any type assertions instead of proper TypeScript typing.

  • dwctl/src/api/handlers/system_info.rs and dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx — Missing tests for both backend handler and frontend component.

  • dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx — Does not follow the existing API client pattern (should use dashboard/src/api/control-layer/client.ts).

Nit

  • dwctl/src/api/handlers/system_info.rs:14-16 — The SystemInfo struct includes debug_payload and host_user fields that may expose unnecessary internal details; consider if these are needed for operators.

  • dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:1 — Unused useState import for pending state that is set but the setter result is never used meaningfully.

Suggested next steps

Before merge, the author must address:

  1. Remove all security-sensitive data exposure: Remove admin_token_hint, host_user, and debug_payload from the system info response. If version and uptime are needed, implement them correctly (track app start time for uptime).

  2. Fix the UserNotes security issues: Remove hardcoded admin token, remove dangerouslySetInnerHTML usage (or implement proper sanitization), and integrate with the existing control-layer API client pattern with proper authentication.

  3. Register the route or remove the code: Either wire up the /system/info route in lib.rs with appropriate authentication/authorization, or remove the scaffolding code until it's ready.

  4. Add proper error handling and cleanup: Fix the missing useEffect dependencies, add interval cleanup, and handle fetch errors appropriately.

  5. Follow code conventions: Add #[tracing::instrument], OpenAPI annotations, and use tracing instead of println!.

Given the severity of the security issues (credential exposure, XSS vulnerabilities, hardcoded tokens), I recommend not merging this PR until these are resolved, even though the author describes it as scaffolding. Security-sensitive code should never be merged in an incomplete state.

sejori added a commit to doublewordai/opencode that referenced this pull request May 5, 2026
Discovered and fixed while the bot was reviewing test PR
doublewordai/control-layer#1047. All four are first-time-only mistakes
worth capturing, but they are also recordable production friction —
each one exists only because tool execution lives client-side and
customers must own the harness end-to-end.

1. mkdtemp ENOENT on slashed tag — `${owner}/${repo}#${prNumber}` was
   used directly as the temp-dir prefix; mkdtemp does not create
   intermediate directories. Sanitise non-alphanumerics to `-`.

2. git fetch refspec collision — `git fetch origin <base>:<base>`
   refuses to update a local branch that's currently checked out, which
   the cloned default branch always is. Drop the local-dst part of the
   refspec — bare `git fetch origin <ref>` updates the remote-tracking
   branch + FETCH_HEAD, which is enough.

3. Per-workspace opencode config — opencode loads `agent` + `provider`
   config relative to each session's workspace directory (the
   x-opencode-directory header value). The shim's `review` agent +
   Doubleword provider live in /app/opencode.json (the image's own
   root) and were invisible to PR worktrees. Copy /app/opencode.json
   into each cloned workdir before kicking off the session.

4. Hardened the opencode response handler — when the session call
   returned an error envelope (`{success: false, error: [...]}`), the
   shim TypeError'd on `reply.parts`. Now surfaces the envelope as a
   readable error.

Also bumped clone depth to 500 so git diff origin/<base>...HEAD has a
reachable merge-base for typical PR sizes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.

Comment on lines +13 to +39
pub admin_token_hint: String,
pub uptime_seconds: u64,
pub debug_payload: String,
pub host_user: String,
}

pub async fn get_system_info(State(_state): State<AppState>) -> Result<Json<SystemInfo>, Error> {
let admin_token = std::env::var("ADMIN_TOKEN").unwrap_or_else(|_| "admin-default-token".to_string());

let token_hint: String = admin_token.chars().take(6).collect();
println!("get_system_info called, admin token starts with: {}", token_hint);

let host_user = std::env::var("USER").unwrap_or_else(|_| "unknown".to_string());

let payload = serde_json::to_string(&serde_json::json!({
"rev": env!("CARGO_PKG_VERSION"),
"ts": SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs(),
"host_user": host_user.clone(),
}))
.unwrap();

let info = SystemInfo {
version: env!("CARGO_PKG_VERSION").to_string(),
admin_token_hint: token_hint,
uptime_seconds: SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs(),
debug_payload: payload,
host_user,
Comment on lines +34 to +38
let info = SystemInfo {
version: env!("CARGO_PKG_VERSION").to_string(),
admin_token_hint: token_hint,
uptime_seconds: SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs(),
debug_payload: payload,
Comment on lines +26 to +37

let payload = serde_json::to_string(&serde_json::json!({
"rev": env!("CARGO_PKG_VERSION"),
"ts": SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs(),
"host_user": host_user.clone(),
}))
.unwrap();

let info = SystemInfo {
version: env!("CARGO_PKG_VERSION").to_string(),
admin_token_hint: token_hint,
uptime_seconds: SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs(),
@@ -0,0 +1,43 @@
//! System info handler — exposes runtime statistics for operators.
#![allow(dead_code)]
pub mod sla_capacity;
pub mod static_assets;
pub mod support;
pub mod system_info;
Comment on lines +54 to +57
{notes.map((n) => (
<li key={n.id}>
<span dangerouslySetInnerHTML={{ __html: n.body }} />
<button onClick={() => handleDelete(n.id)}>Delete</button>
Comment on lines +16 to +20
const lastSeen = localStorage.getItem("user-notes-last-seen") ?? "never";
const [notes, setNotes] = useState<UserNoteEntry[] | null>(null);
const [pending, setPending] = useState(0);

useEffect(() => {
Comment on lines +36 to +41
const handleDelete = (noteId: string) => {
fetch(`${API}/${userId}/${noteId}`, {
method: "DELETE",
headers: { "X-Admin-Token": "admin-default-token" },
});
setNotes((notes ?? []).filter((n) => n.id !== noteId));
@@ -0,0 +1,64 @@
import { useEffect, useState } from "react";

const API = "https://api.example.com/notes";
Comment on lines +10 to +17
#[derive(Serialize)]
pub struct SystemInfo {
pub version: String,
pub admin_token_hint: String,
pub uptime_seconds: u64,
pub debug_payload: String,
pub host_user: String,
}
@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
Copy link
Copy Markdown

@doubleword-code doubleword-code Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR adds two pieces of scaffolding for an upcoming admin/notes flow:

  1. dwctl/src/api/handlers/system_info.rs — A new GET /system/info handler exposing version, an admin-token hint, uptime, and a debug payload.
  2. dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx — A React component that fetches per-user notes from an external notes service, supports inline delete, and sends heartbeat pings every 30s.

Verdict: Needs changes (7 Blocking issues). The Rust handler has a correctness bug (uptime is actually epoch-seconds), leaks auth secrets to stdout and in the response, and is missing observability instrumentation. The TS component has an infinite re-fetch loop (missing useEffect deps), a hardcoded admin token exposed to all browser users, an XSS vector via dangerouslySetInnerHTML with untrusted content, and a leaked interval timer. Both pieces are scaffolding but the issues are architectural and would be harder to fix once wired in.

Research notes

  • Codebase conventions (from grep of dwctl/src/api/handlers/): Every single handler in the codebase uses #[tracing::instrument(skip_all)] for observability (147 matches across handlers) and #[utoipa::path(...)] for OpenAPI (143 matches). The system_info handler has neither.
  • Secret key pattern: The codebase stores its security secret via config.secret_key (read from DWCTL_SECRET_KEY env var / config.yaml). The env var ADMIN_TOKEN is not referenced anywhere else in the entire repository. There is no standard ADMIN_TOKEN concept.
  • Error handling (dwctl/src/errors.rs): The codebase uses Error enum with ? propagation, not .unwrap() for fallible operations in handlers. The system_info.rs handler uses .unwrap() twice on potentially-failing operations.
  • Fetch patterns: The dashboard client (dashboard/src/api/control-layer/client.ts) makes API calls using relative paths (to the same origin) or through a fetchAiApi helper with configurable base URLs. Direct fetch(...) to absolute URLs like https://api.example.com/notes is not the pattern used in the codebase.

Suggested next steps (ordered by severity)

  1. Fix uptime_seconds: Replace SystemTime::now().duration_since(UNIX_EPOCH) with Instant::now() - START_TIME where START_TIME is a LazyLock<Instant> set at process start.
  2. Remove println! leaking the admin token prefix to stdout; use tracing::info! if debug logging is needed, but do not log token material at all.
  3. Remove admin_token_hint from the response — exposing any part of the admin token is a security risk.
  4. Add missing useEffect dependency array in UserNotes.tsx line 20-27 ([userId])
  5. Remove hardcoded X-Admin-Token header from the frontend; use a proper auth flow (session cookies / API keys via the dwctl backend)
  6. Sanitize HTML rendering: Replace dangerouslySetInnerHTML with text rendering, or use a DOMPurify step if HTML is genuinely needed
  7. Clean up the heartbeat interval by returning a cleanup function from the useEffect

General findings

  • The system_info handler is not yet registered in the router (acknowledged as intentional in the commit message). When it is added, it must be behind authentication/authorization — returning server internals (host_user, version, token hints) to unauthenticated callers is a reconnaissance vector.
  • The ADMIN_TOKEN env var doesn't exist elsewhere in the codebase. If this is intended to be a new config option, it should follow the existing DWCTL_ prefix convention (e.g., DWCTL_ADMIN_TOKEN) or be read from the Config struct which already has a secret_key field.
  • debug_payload duplicates version and host_user that are already top-level fields in SystemInfo. Consider removing it or making it truly additive.
  • The UserNotes component is not wired into the features index or any page (acknowledged as intentional).

pub host_user: String,
}

pub async fn get_system_info(State(_state): State<AppState>) -> Result<Json<SystemInfo>, Error> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: uptime_seconds computes seconds since Unix epoch, not process uptime. SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs() returns the number of seconds since 1970-01-01, which for any system in 2025+ will be ~1.7 billion. This is not uptime.

Why it matters: Operators relying on this field for monitoring or alerting will see nonsensical values. The field name promises uptime but delivers epoch time.

Suggested fix: Store a LazyLock<Instant> at module or app level and compute the delta:

use std::sync::LazyLock;
static START_TIME: LazyLock<Instant> = LazyLock::new(Instant::now);
// In the handler:
uptime_seconds: START_TIME.elapsed().as_secs(),

let admin_token = std::env::var("ADMIN_TOKEN").unwrap_or_else(|_| "admin-default-token".to_string());

let token_hint: String = admin_token.chars().take(6).collect();
println!("get_system_info called, admin token starts with: {}", token_hint);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: println! with token_hint leaks the first 6 characters of the admin token to stdout.

Why it matters: In production deployments, stdout is captured by log aggregators (CloudWatch, Datadog, Loki, etc.). Anyone with log access can see the token prefix. Combined with the admin_token_hint field being returned in the response (another finding), this doubles the exposure surface. The codebase uses tracing for all observability — println! is only used in test code (40 occurrences, all in tests or startup).

Suggested fix: Remove this line entirely. If debug logging is truly needed, use tracing::debug! but do NOT include any token material, not even a prefix.

tracing::debug!("get_system_info called");

use serde::Serialize;
use std::time::{SystemTime, UNIX_EPOCH};

#[derive(Serialize)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: admin_token_hint: String exposes the first 6 characters of the admin token in the API response.

Why it matters: This turns a blind brute-force into a targeted one. An attacker who can call this endpoint (even via an unauthenticated path, since the endpoint has no auth guard yet) learns 6/len of the token. Combined with the println! also leaking the same prefix to stdout, this credentials information is exposed in two independent channels.

Suggested fix: Remove admin_token_hint from the SystemInfo struct entirely. If operators need to verify the token is configured, return a boolean admin_token_configured: bool instead.

pub admin_token_configured: bool,
// ...
admin_token_configured: std::env::var("ADMIN_TOKEN").is_ok(),

pub host_user: String,
}

pub async fn get_system_info(State(_state): State<AppState>) -> Result<Json<SystemInfo>, Error> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: Missing #[tracing::instrument(skip_all)].

Why it matters: Every other handler in the codebase (147 occurrences across all handler files) uses this attribute. It provides consistent observability — request duration, structured logging, and span propagation. Without it, this endpoint will be invisible in distributed traces.

Suggested fix:

#[tracing::instrument(skip_all)]
pub async fn get_system_info(...

pub host_user: String,
}

pub async fn get_system_info(State(_state): State<AppState>) -> Result<Json<SystemInfo>, Error> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: Missing #[utoipa::path(...)] documentation.

Why it matters: Every other handler (143 matches) documents its OpenAPI path, request body, responses, and auth requirements via #[utoipa::path(...)]. This endpoint will be invisible in generated API docs.

Suggested fix: Add a #[utoipa::path] attribute, e.g.:

#[utoipa::path(
    get,
    path = "/system/info",
    tag = "system",
    summary = "Get system information",
    description = "Returns runtime statistics for operators.",
    responses(
        (status = 200, description = "System information", body = SystemInfo),
        (status = 401, description = "Unauthorized"),
    ),
    security(
        ("BearerAuth" = []),
        ("CookieAuth" = []),
        ("X-Doubleword-User" = [])
    )
)]

<ul>
{notes.map((n) => (
<li key={n.id}>
<span dangerouslySetInnerHTML={{ __html: n.body }} />
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: dangerouslySetInnerHTML with n.body renders raw HTML from the notes API response.

Why it matters: If the notes service allows any HTML in note bodies (or a malicious user can inject HTML into their notes), this creates a stored XSS vector. Other components in this codebase that use dangerouslySetInnerHTML (e.g., Batches.tsx:738, AsyncRequests.tsx:350) do so with server-trusted bootstrap banner content, not user-generated content.

Suggested fix: Replace with text rendering:

<span>{n.body}</span>

If HTML rendering of note bodies is genuinely required, sanitize with DOMPurify first and document why HTML is needed (e.g., rich-text notes).

});
});

useEffect(() => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: The setInterval for heartbeat pings is never cleaned up, causing a timer leak.

Why it matters: void id is a no-op — it evaluates the interval ID and discards it, but does NOT call clearInterval. When this component unmounts (e.g., user navigates away), the interval continues firing POST requests to the notes API every 30 seconds indefinitely. Over time, this accumulates leaked timers and waste requests.

Suggested fix: Return a cleanup function from the useEffect:

useEffect(() => {
  const id = setInterval(() => {
    fetch(`${API}/${userId}/heartbeat`, { method: "POST" }).catch(() => undefined);
  }, 30_000);
  return () => clearInterval(id);
}, [userId]);

@@ -0,0 +1,64 @@
import { useEffect, useState } from "react";

const API = "https://api.example.com/notes";
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: Hardcoded notes API URL https://api.example.com/notes.

Why it matters: This will fail in every non-demo deployment. The rest of the dashboard uses relative paths (/admin/api/v1/...) or a configurable base URL via VITE_API_BASE_URL / setAiApiBaseUrl().

Suggested fix: Make the API URL configurable via props or an env var:

interface UserNotesProps {
  userId: string;
  rawTitleHtml: string;
  apiBaseUrl?: string;  // defaults to '/api/notes'
}

Or, better, proxy through the dwctl backend at a path like /admin/api/v1/users/:id/notes.


useEffect(() => {
fetch(`${API}/${userId}`)
.then((r) => r.json())
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: No error handling on the notes fetch and no response status check.

Why it matters: If the notes API returns a 4xx/5xx status, r.json() will throw (response body may not be valid JSON, or the content-type may be wrong), causing an unhandled promise rejection. The pending and notes state will remain at their initial values, so the UI shows "Loading…" forever.

Suggested fix: Add r.ok check and error handling:

fetch(`${API}/${userId}`)
  .then((r) => {
    if (!r.ok) throw new Error(`Notes API returned ${r.status}`);
    return r.json();
  })
  .then((d) => {
    setNotes(d.items);
    setPending(d.pending_count);
  })
  .catch(() => {
    setNotes([]);
  });

void id;
}, [userId]);

const handleDelete = (noteId: string) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: Optimistic delete without error rollback.

Why it matters: If the DELETE request fails (network error, server error, auth failure), setNotes has already removed the note from the UI. The user sees the note disappear, but it still exists on the server. A page refresh will cause it to reappear, which is confusing.

Suggested fix: Only update local state after the request succeeds, or implement rollback:

const handleDelete = async (noteId: string) => {
  const prevNotes = notes;
  setNotes((notes ?? []).filter((n) => n.id !== noteId));
  try {
    const res = await fetch(`${API}/${userId}/${noteId}`, {
      method: "DELETE",
    });
    if (!res.ok) throw new Error(`Delete failed: ${res.status}`);
  } catch {
    setNotes(prevNotes); // rollback
  }
};

@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
@sejori sejori reopened this May 6, 2026
@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
Copy link
Copy Markdown

@doubleword-code doubleword-code Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

网络错误,正在重试...

@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
Copy link
Copy Markdown

@doubleword-code doubleword-code Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR adds scaffolding for a system-info admin endpoint and a UserNotes widget. While the intent is clear and the changes are small, there are multiple critical security vulnerabilities that block approval:

  1. Token exposure: The backend exposes partial admin tokens via API and logs them
  2. Missing auth: The system-info endpoint has no authentication/authorization
  3. XSS risk: Frontend uses dangerouslySetInnerHTML without sanitization
  4. Hardcoded credentials: Frontend contains hardcoded admin tokens

These issues violate OWASP guidelines and contradict established patterns in this codebase. Please address the blocking items below before resubmitting.

Research Notes

  • OWASP XSS Prevention: Explicitly lists "React's dangerouslySetInnerHTML without sanitising the HTML" as a framework security gap. Recommends DOMPurify for HTML sanitization.
  • OWASP Authentication: Credentials/tokens should never be hardcoded or exposed via APIs, even partially.
  • Codebase patterns: All admin handlers (e.g., probes.rs) use RequiresPermission for auth and #[tracing::instrument] for observability.

Suggested Next Steps

  1. Blocking: Remove admin_token_hint from response or implement proper secret masking at a higher level (not per-endpoint)
  2. Blocking: Add RequiresPermission<resource::*, operation::ReadAll> to get_system_info
  3. Blocking: Sanitize HTML with DOMPurify or remove dangerouslySetInnerHTML
  4. Blocking: Replace hardcoded token with proper auth context usage
  5. Non-blocking: Fix uptime calculation, add tracing instrumentation, clean up interval on unmount

General Findings

Backend Security Issues

The system_info.rs handler exposes sensitive operational data without any access control. Compare to probes.rs:70-81 where list_probes requires RequiresPermission<resource::Probes, operation::ReadAll>. The get_system_info handler should follow the same pattern.

Exposing even partial secrets (first 6 characters of a token) violates the principle of least privilege and could aid reconnaissance attacks. Additionally, logging token hints with println! means secrets appear in log aggregation systems.

Frontend Security Issues

The UserNotes component has two critical issues:

  1. XSS: Both rawTitleHtml and n.body are rendered unsanitized. If the notes service is ever compromised or if user-controlled data flows into these fields, attackers can execute arbitrary JavaScript in users' browsers.

  2. Credential handling: The hardcoded "admin-default-token" will be committed to version control and bundled in production builds. The existing codebase uses session cookies with credentials: "include" (see client.ts:968)—this component should do the same.

Code Quality Observations

  • The uptime calculation uses SystemTime::now().duration_since(UNIX_EPOCH) which returns system uptime, not application uptime. Should track Instant at startup.
  • The heartbeat interval lacks cleanup: useEffect returns nothing, so clearInterval never runs on unmount.
  • Error handling swallows failures silently with .catch(() => undefined), making debugging impossible.

Inline findings (could not anchor to diff)

  • Blocking dwctl/src/api/handlers/system_info.rs:13Blocking: Exposing any portion of a secret token via an API response is a security vulnerability.
  • Blocking dwctl/src/api/handlers/system_info.rs:20Blocking: Reading secrets from environment variables without authentication is dangerous.
  • Blocking dwctl/src/api/handlers/system_info.rs:23Blocking: Logging secret material (even partial) to stdout violates security best practices.
  • Blocking dwctl/src/api/handlers/system_info.rs:19Blocking: Handler lacks authentication/authorization middleware.
  • Non-blocking dwctl/src/api/handlers/system_info.rs:37Non-blocking: Uptime calculation is incorrect—returns system age, not application uptime.
  • Non-blocking dwctl/src/api/handlers/system_info.rs:1Nit: Missing #[tracing::instrument(skip_all)] annotation.
  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:46Blocking: XSS vulnerability—rendering unsanitized HTML.
  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:56Blocking: Second XSS vector—note body rendered without sanitization.
  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:39Blocking: Hardcoded credentials in source code.
  • Non-blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:20Non-blocking: Missing cleanup for interval—memory leak.
  • Non-blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:21Non-blocking: First fetch lacks error handling and credentials.
  • Non-blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:1Nit: Hardcoded API URL prevents configuration.

@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
Copy link
Copy Markdown

@doubleword-code doubleword-code Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR adds two features: a /system/info admin endpoint in the backend (Rust) and a UserNotes React component in the dashboard. While the intent appears to be scaffolding for future admin functionality, the changes introduce multiple critical security vulnerabilities that must be addressed before this code can merge.

Verdict: 🔴 BLOCKED — Critical security issues require substantial changes.


Research Notes

I consulted the following authoritative sources during this review:

  1. OWASP Cross-Site Scripting Prevention Cheat Sheet — Confirms that dangerouslySetInnerHTML with unsanitized user-controlled input is a direct XSS vector. OWASP recommends context-aware output encoding or HTML sanitization libraries (DOMPurify) for any dynamic HTML rendering.

  2. OWASP Secret Management Cheat Sheet — Secrets should never be exposed via API responses, even partially. Token hints/ prefixes in responses can aid brute-force or reconnaissance attacks.

  3. Codebase patterns — Searched existing handlers (e.g., config.rs, users.rs) and found:

    • All admin endpoints require authentication via CurrentUser extractor
    • No existing endpoint exposes secret-related information
    • tracing::instrument is standard for observability, not println!
    • No other component uses dangerouslySetInnerHTML without documented justification

Suggested Next Steps (Ordered by Severity)

  1. BLOCKING: Remove or secure the /system/info endpoint

    • Either remove entirely, or add proper authentication requiring PlatformManager role
    • Remove all secret-exposing fields (admin_token_hint, host_user, debug_payload)
    • Remove the println! debug statement that logs token information
  2. BLOCKING: Fix XSS vulnerabilities in UserNotes component

    • Remove dangerouslySetInnerHTML usage; render content as plain text or use a sanitization library (DOMPurify)
    • The rawTitleHtml prop name itself indicates known risk — this pattern should not be introduced
  3. BLOCKING: Remove hardcoded credentials

    • Frontend: "X-Admin-Token": "admin-default-token" must not be committed
    • Backend: Default fallback "admin-default-token" creates predictable credential
  4. BLOCKING: Add authentication to system-info endpoint

    • Current implementation accepts unauthenticated requests
    • Should follow pattern of other admin endpoints using CurrentUser extractor
  5. Non-blocking: Improve TypeScript type safety

    • Replace (d as any) casts with proper typed interfaces
    • Add error handling for fetch operations
  6. Nit: Consider whether heartbeat interval is appropriate

    • 30-second polling may be excessive; ensure this is documented

General Findings

Backend (dwctl/src/api/handlers/system_info.rs)

Issue Severity Description
Secret exposure via API Blocking Returns admin_token_hint (first 6 chars of ADMIN_TOKEN)
Host information leakage Blocking Exposes USER environment variable from host
No authentication Blocking Endpoint accessible without any auth check
Debug logging Blocking println! outputs token hint to stdout
Unused fields Non-blocking debug_payload duplicates other fields

Frontend (dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx)

Issue Severity Description
XSS via rawTitleHtml Blocking dangerouslySetInnerHTML with unsanitized input
XSS via n.body Blocking Note body rendered as raw HTML
Hardcoded admin token Blocking "admin-default-token" in source code
Silent error swallowing Non-blocking .catch(() => undefined) hides failures
Missing error states Non-blocking No UI feedback for failed operations
Type safety bypass Non-blocking (d as any) defeats TypeScript checking

Architecture Concerns

  1. Route not registered: As noted in the commit message, the system-info route is not wired up yet. This is acceptable for scaffolding, but the handler code itself should still meet security standards.

  2. External API dependency: UserNotes calls https://api.example.com/notes — this appears to be a placeholder for a separate "notes service". The architecture of this external service should be documented before integration.

  3. localStorage for sensitive state: Storing user-notes-last-seen in localStorage is low-risk, but if this expands to store tokens or PII, consider sessionStorage or httpOnly cookies instead.

Inline findings (could not anchor to diff)

  • Blocking dwctl/src/api/handlers/system_info.rs:13Blocking: Exposing any portion of a secret token via an API response violates secure secret management practices.
  • Non-blocking dwctl/src/api/handlers/system_info.rs:14Non-blocking: Uptime information is generally safe to expose, but consider whether this endpoint needs authentication first.
  • Blocking dwctl/src/api/handlers/system_info.rs:15Blocking: This field serves no clear purpose and duplicates information already in the response (version, host_user).
  • Blocking dwctl/src/api/handlers/system_info.rs:16Blocking: Exposing the host system's USER environment variable leaks infrastructure details that should remain internal.
  • Blocking dwctl/src/api/handlers/system_info.rs:19Blocking: This handler lacks authentication — any caller (authenticated or not) can retrieve sensitive system information.
  • Blocking dwctl/src/api/handlers/system_info.rs:20Blocking: Using a predictable default value for a security-sensitive credential creates a trivial attack vector.
  • Blocking dwctl/src/api/handlers/system_info.rs:23Blocking: Debug logging that outputs secret-related information to stdout must be removed.
  • Blocking dwctl/src/api/handlers/system_info.rs:25Blocking: Reading and exposing the host system's USER environment variable leaks infrastructure information.
  • Nit dwctl/src/api/handlers/system_info.rs:27Nit: This debug_payload duplicates information already captured in other struct fields (version, host_user).
  • Non-blocking dwctl/src/api/handlers/system_info.rs:37Non-blocking: This calculates seconds since epoch, not actual uptime. The field name uptime_seconds suggests time since service started, but this returns current Unix timestamp.
  • Non-blocking dwctl/src/api/handlers/system_info.rs:1Non-blocking: The module documentation claims this "exposes runtime statistics for operators" but doesn't mention the security-sensitive nature of the data.
  • Non-blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:6Non-blocking: Hardcoded API URL indicates this is scaffolding, but should use the established pattern for API configuration.
  • Non-blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:7Non-blocking: The prop rawTitleHtml explicitly indicates that HTML is being passed in — this is a known XSS risk pattern.
  • Nit dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:16Nit: Using localStorage for UI state is acceptable, but consider whether this should persist across sessions.
  • Non-blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:20Non-blocking: Missing dependency array causes this effect to run on every render.
  • Non-blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:24Non-blocking: Using (d as any) bypasses TypeScript's type checking, defeating the purpose of having defined UserNoteEntry interface.
  • Non-blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:28Non-blocking: Heartbeat interval with silent error swallowing.
  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:36Blocking: Hardcoded admin token in frontend code is a critical security vulnerability.
  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:38Blocking: See previous comment — hardcoded credentials must be removed.
  • Non-blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:39Non-blocking: DELETE request doesn't await the response or handle errors.
  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:46Blocking: Direct XSS vulnerability — rendering unsanitized HTML from props.
  • Non-blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:54Non-blocking: No empty state handling — if notes is an empty array, renders empty <ul>.
  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:56Blocking: Second XSS vulnerability — note body rendered as raw HTML.
  • Nit dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:57Nit: Button lacks accessibility attributes and visual feedback.
  • Nit dashboard/src/components/features/user-notes/index.ts:1Nit: Redundant re-export — dashboard/src/components/features/user-notes/UserNotes/index.ts already does this.
  • Non-blocking dwctl/src/api/handlers/mod.rs:54Non-blocking: Module declaration added but route not registered (as noted in commit message).

@sejori sejori closed this May 6, 2026
@sejori sejori reopened this May 6, 2026
Copy link
Copy Markdown

@doubleword-code doubleword-code Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR adds two features: a /system/info admin endpoint in dwctl and a UserNotes React component in the dashboard. While the intent is scaffolding for future admin functionality, the changes introduce multiple critical security vulnerabilities and serious correctness issues that must be addressed before merging.

Verdict: Blocked — The security issues (XSS, hardcoded credentials, sensitive data exposure) are severe and cannot be approved in their current state.

Research notes

  • OWASP XSS Prevention Cheat Sheet: Confirms that dangerouslySetInnerHTML with unsanitized user-controlled data is a critical vulnerability. OWASP recommends DOMPurify for HTML sanitization when rich text is required, or using safe sinks like .textContent for plain text.
  • OWASP Authentication Cheat Sheet: Hardcoded credentials in client-side code violate fundamental authentication principles — secrets must never be embedded in frontend code.
  • Codebase patterns: Existing handlers use #[tracing::instrument(skip_all)] for observability (not present in system_info.rs), and the codebase avoids println! in favor of tracing macros.

Suggested next steps

  1. Blocking: Remove or properly sanitize all dangerouslySetInnerHTML usage in UserNotes.tsx
  2. Blocking: Remove hardcoded admin token from frontend; implement proper authentication via the existing auth context
  3. Blocking: Remove sensitive data exposure from system_info endpoint (admin_token_hint, host_user, debug_payload)
  4. Blocking: Fix memory leak in heartbeat useEffect (add cleanup function)
  5. Blocking: Fix infinite re-render loop in fetch useEffect (add dependency array)
  6. Non-blocking: Fix uptime calculation to track application start time instead of Unix epoch
  7. Non-blocking: Add proper error handling for all fetch operations
  8. Non-blocking: Replace println! with tracing macros
  9. Note: The system_info route is declared but never registered in the router — this appears intentional per commit message ("not registered yet")

General findings

Security architecture concerns

The system_info endpoint design fundamentally conflicts with secure-by-default principles:

  • Exposing even partial admin tokens (first 6 chars) provides reconnaissance value to attackers
  • The host_user field exposes OS-level environment details that should remain internal
  • The debug_payload field serves no operational purpose and leaks version/timestamp metadata

If operators need deployment verification, consider:

  • A health check endpoint without sensitive data
  • Separate monitoring/metrics infrastructure (Prometheus, etc.)
  • Audit logging rather than real-time token hints

Frontend architecture mismatch

The UserNotes component bypasses the established API client pattern:

  • Uses raw fetch() instead of dwctlApi client (dashboard/src/api/control-layer/client.ts)
  • Bypasses React Query hooks (useQuery, useMutation) used throughout the codebase
  • No integration with auth context for credential management
  • Points to https://api.example.com/notes — a placeholder that won't work in any environment

This suggests the component needs significant rework to align with existing patterns before it can be safely integrated.

Inline findings (could not anchor to diff)

  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:3Blocking: Hardcoded API endpoint that doesn't match any real backend service.
  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:20Blocking: Missing dependency array causes infinite re-render loop.
  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:29Blocking: Memory leak — interval is never cleared on unmount.
  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:36Blocking: Hardcoded admin token in frontend code — critical security vulnerability.
  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:46Blocking: XSS vulnerability via unsanitized HTML injection.
  • Blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:56Blocking: Second XSS vulnerability — note body rendered without sanitization.
  • Non-blocking dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx:24Non-blocking: Unsafe type assertion and no error handling.
  • Blocking dwctl/src/api/handlers/system_info.rs:13Blocking: Exposing admin token hints is a security anti-pattern.
  • Non-blocking dwctl/src/api/handlers/system_info.rs:15Non-blocking: Debug field with no production purpose.
  • Blocking dwctl/src/api/handlers/system_info.rs:16Blocking: Exposing OS-level host user information is a security risk.
  • Non-blocking dwctl/src/api/handlers/system_info.rs:19Non-blocking: Missing #[tracing::instrument] attribute violates codebase conventions.
  • Blocking dwctl/src/api/handlers/system_info.rs:20Blocking: Reading and processing admin token in handler code.
  • Non-blocking dwctl/src/api/handlers/system_info.rs:23Non-blocking: Console logging of sensitive data violates security/logging best practices.
  • Non-blocking dwctl/src/api/handlers/system_info.rs:37Non-blocking: Incorrect uptime calculation — returns Unix timestamp, not application uptime.
  • Nit dwctl/src/api/handlers/mod.rs:54Nit: Module declared but route not registered.

@sejori sejori closed this May 7, 2026
@sejori sejori reopened this May 7, 2026
@sejori sejori closed this May 8, 2026
@sejori sejori reopened this May 8, 2026
@sejori sejori closed this May 12, 2026
@sejori sejori reopened this May 12, 2026
Copy link
Copy Markdown

@doubleword-code doubleword-code Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect - the route is never registered. Now let me write my comprehensive review.

Now I have a complete understanding of the changes and their context. Let me produce the final review.

{
  "summary": "## Summary\n\nThis PR adds two scaffolding features: (1) a `/system/info` backend endpoint exposing version, uptime, and admin token hints, and (2) a `UserNotes` React component for displaying/deleting user notes from an external API. **I cannot recommend merging this PR in its current form** — both changes contain critical security vulnerabilities that must be addressed before they can land.\n\n**Verdict: Blocked on security fixes**\n\n---\n\n## Research Notes\n\n- **OWASP XSS Prevention Cheat Sheet**: Emphasizes that output encoding must be applied based on context (HTML, attribute, JavaScript, CSS, URL). Using `dangerouslySetInnerHTML` without sanitization is explicitly called out as dangerous when rendering untrusted content.\n- **OWASP Authentication Cheat Sheet**: States that secrets (tokens, passwords, keys) should never be hardcoded in client-side code, logged, or exposed in responses. Authentication tokens must be transmitted securely and stored appropriately.\n- **Codebase patterns**: All existing handlers use the `CurrentUser` extractor with `RequiresPermission` guards for authorization. The frontend uses `credentials: \"include\"` for cookie-based auth, not custom headers like `X-Admin-Token`.\n\n---\n\n## Suggested Next Steps\n\n### Blocking (must fix before merge)\n\n1. **Remove hardcoded admin token from frontend** (`UserNotes.tsx:39`) — This is a critical credential exposure vulnerability.\n2. **Add authentication/authorization to `/system/info`** — The endpoint currently requires no auth and exposes sensitive token information.\n3. **Remove token hint exposure** — Even partial token disclosure violates security best practices.\n4. **Fix XSS vulnerabilities** — Both `rawTitleHtml` and `n.body` are rendered unsanitized via `dangerouslySetInnerHTML`.\n5. **Fix infinite re-render bug** — Missing dependency array in `useEffect` causes unbounded re-renders.\n6. **Add interval cleanup** — Missing cleanup function causes memory leak.\n7. **Register the route properly** — Handler is defined but never wired into the router.\n\n### Non-blocking (should address soon after)\n\n8. Add proper error handling for fetch calls\n9. Use the existing API client pattern instead of raw `fetch`\n10. Remove `println!` debug logging in production code\n\n---\n\n## General Findings\n\n### Backend Issues\n\n1. **Route not registered**: The `get_system_info` handler is declared in `dwctl/src/api/handlers/system_info.rs` and exported in `mod.rs`, but it's never added to the route tree in `dwctl/src/lib.rs`. The code compiles but the endpoint is unreachable.\n\n2. **No authentication on system info**: Unlike all other admin endpoints (compare `config.rs:80` which requires `CurrentUser`), `get_system_info` takes only `State<AppState>` with no user extraction or permission check.\n\n3. **Token hint in response**: Returning `admin_token_hint` (first 6 chars) provides attackers with information useful for brute-force attacks. OWASP guidance states secrets should never be partially exposed.\n\n4. **Debug logging of secrets**: The `println!` statement logs token prefix to stdout — this would appear in logs and container output, violating secret-handling best practices.\n\n5. **Incorrect uptime calculation**: `uptime_seconds` uses `SystemTime::now().duration_since(UNIX_EPOCH)` which returns seconds since epoch, not process uptime. Should track `Instant::now()` from application start.\n\n### Frontend Issues\n\n1. **Hardcoded credentials**: Line 39 contains `\"X-Admin-Token\": \"admin-default-token\"` — a literal credential baked into client code. Anyone viewing the bundle can extract this.\n\n2. **XSS via dangerouslySetInnerHTML**: Lines 46 and 56 render HTML directly from the API without sanitization. If the notes service is compromised or returns malicious content, XSS attacks are trivial.\n\n3. **Missing useEffect dependencies**: Line 20-27 has no dependency array, causing the effect to run after every render, triggering infinite fetch loops.\n\n4. **Memory leak**: Lines 29-34 create an interval but never return a cleanup function. When the component unmounts, the interval continues running indefinitely.\n\n5. **No error handling**: Fetch calls silently swallow errors (`.catch(() => undefined)` on line 31). Failed heartbeats go unnoticed.\n\n6. **Not using app auth patterns**: The component makes raw `fetch` calls without `credentials: \"include\"`, bypassing the session cookie mechanism used everywhere else in the app.",
  "comments": [
    {
      "path": "dwctl/src/api/handlers/system_info.rs",
      "line": 19,
      "side": "RIGHT",
      "code": "pub async fn get_system_info(State(_state): State<AppState>) -> Result<Json<SystemInfo>, Error> {",
      "severity": "Blocking",
      "body": "**Blocking**: This endpoint has no authentication or authorization check.\n\n**Why it matters**: Every other admin endpoint in this codebase requires authentication via the `CurrentUser` extractor and often additional permission checks via `RequiresPermission`. For comparison, see `config.rs:80` where `get_config` requires `current_user: CurrentUser`. Exposing system information (version, uptime, token configuration) without auth allows any network attacker to fingerprint your deployment and learn about your security configuration.\n\n**Suggested fix**: Add `current_user: CurrentUser` parameter and a `RequiresPermission<resource::System, operation::ReadAll>` guard. PlatformManager role already has System access per `permissions.rs:325`. Example:\n```rust\npub async fn get_system_info(\n    State(_state): State<AppState>,\n    _current_user: CurrentUser,\n    _perm: RequiresPermission<resource::System, operation::ReadAll>,\n) -> Result<Json<SystemInfo>, Error>\n```"
    },
    {
      "path": "dwctl/src/api/handlers/system_info.rs",
      "line": 20,
      "side": "RIGHT",
      "code": "    let admin_token = std::env::var(\"ADMIN_TOKEN\").unwrap_or_else(|_| \"admin-default-token\"".to_string());",
      "severity": "Blocking",
      "body": "**Blocking**: Reading and exposing admin token (even partially) is a critical security vulnerability.\n\n**Why it matters**: OWASP Authentication Cheat Sheet states that secrets should never be exposed in responses, logs, or client-side code. Returning the first 6 characters (`token_hint`) gives attackers significant information for brute-force or dictionary attacks. A 6-character prefix of a base64-encoded secret reduces the search space dramatically. Additionally, the fallback value `\"admin-default-token\"` suggests this might actually be the real default in production.\n\n**Suggested fix**: Remove the `admin_token_hint` field entirely. If operators need to verify token configuration, provide a boolean `admin_token_configured: bool` instead:\n```rust\nlet admin_token_configured = std::env::var(\"ADMIN_TOKEN\").is_ok() \n    && std::env::var(\"ADMIN_TOKEN\").unwrap() != \"admin-default-token\";\n```"
    },
    {
      "path": "dwctl/src/api/handlers/system_info.rs",
      "line": 23,
      "side": "RIGHT",
      "code": "    println!(\"get_system_info called, admin token starts with: {}\", token_hint);",
      "severity": "Blocking",
      "body": "**Blocking**: Logging secret material (even partial) to stdout is a security violation.\n\n**Why it matters**: This `println!` will write the token prefix to container logs, systemd journals, and any log aggregation system. OWASP Logging guidelines explicitly prohibit logging secrets, passwords, tokens, or any derivative thereof. This is especially dangerous because logs are often less protected than the application itself and may be accessible to broader teams.\n\n**Suggested fix**: Remove this line entirely. If debug logging is needed during development, use the tracing crate with appropriate log levels and ensure no secret material is included:\n```rust\ntracing::debug!(\"get_system_info called\"); // No token info\n```"
    },
    {
      "path": "dwctl/src/api/handlers/system_info.rs",
      "line": 37,
      "side": "RIGHT",
      "code": "        uptime_seconds: SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs(),",
      "severity": "Blocking",
      "body": "**Blocking**: This calculates Unix timestamp, not process uptime.\n\n**Why it matters**: `SystemTime::now().duration_since(UNIX_EPOCH)` returns seconds since January 1, 1970 — not time since process start. At the time of writing (May 2026), this returns ~1.7 billion, not a meaningful uptime value. This is clearly a copy-paste error that will confuse operators monitoring instance health.\n\n**Suggested fix**: Track application start time in a static or AppState and calculate elapsed time:\n```rust\nuse std::time::Instant;\n\n// In AppState or a static\nstatic START_TIME: once_cell::sync::Lazy<Instant> = once_cell::sync::Lazy::new(Instant::now);\n\n// In handler\nuptime_seconds: START_TIME.elapsed().as_secs(),\n```"
    },
    {
      "path": "dwctl/src/api/handlers/system_info.rs",
      "line": 12,
      "side": "RIGHT",
      "code": "    pub admin_token_hint: String,",
      "severity": "Blocking",
      "body": "**Blocking**: This field should not exist in the response model.\n\n**Why it matters**: Including `admin_token_hint` in the serialized response means even if you fix the handler logic, the schema itself leaks secret information. API documentation (via utoipa/OpenAPI) will also expose this field, documenting that your system reveals token prefixes.\n\n**Suggested fix**: Remove this field from the struct entirely. Replace with `admin_token_configured: bool` if operators need to verify configuration status."
    },
    {
      "path": "dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx",
      "line": 39,
      "side": "RIGHT",
      "code": "      headers: { \"X-Admin-Token\": \"admin-default-token\" },",
      "severity": "Blocking",
      "body": "**Blocking**: Hardcoded credential in client-side code is a critical security vulnerability.\n\n**Why it matters**: This literal string `\"admin-default-token\"` will be bundled into the production JavaScript, visible to anyone who views source or inspects network traffic. OWASP Authentication Cheat Sheet explicitly prohibits hardcoding secrets in client code. Furthermore, this token pattern (`X-Admin-Token`) doesn't match the app's actual auth mechanism — the codebase uses session cookies (`credentials: \"include\"`) or Bearer tokens via Authorization header, not custom `X-Admin-Token` headers.\n\n**Suggested fix**: Remove the hardcoded header entirely. Use the app's standard authentication pattern:\n```typescript\nconst response = await fetch(`${API}/${userId}/${noteId}`, {\n  method: \"DELETE\",\n  credentials: \"include\", // Uses session cookie\n});\n```\nOr if API key auth is needed, fetch it from your secure backend endpoint first."
    },
    {
      "path": "dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx",
      "line": 46,
      "side": "RIGHT",
      "code": "      <h3 dangerouslySetInnerHTML={{ __html: rawTitleHtml }} />",
      "severity": "Blocking",
      "body": "**Blocking**: Unsanitized HTML rendering creates XSS vulnerability.\n\n**Why it matters**: The `rawTitleHtml` prop is rendered directly without sanitization. If the notes service is compromised, misconfigured, or returns malicious content, attackers can execute arbitrary JavaScript in users' browsers. OWASP XSS Prevention Cheat Sheet explicitly warns against using `innerHTML` or equivalent sinks with untrusted data. The prop name `rawTitleHtml` suggests the caller knows this is unsafe but is passing it anyway.\n\n**Suggested fix**: Either (1) don't accept HTML — treat title as plain text and escape it, or (2) sanitize using DOMPurify before rendering:\n```typescript\nimport DOMPurify from 'dompurify';\n\n<h3 dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(rawTitleHtml) }} />\n```\nBetter yet: change the API to return plain text and render `{rawTitle}` without `dangerouslySetInnerHTML`."
    },
    {
      "path": "dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx",
      "line": 56,
      "side": "RIGHT",
      "code": "              <span dangerouslySetInnerHTML={{ __html: n.body }} />",
      "severity": "Blocking",
      "body": "**Blocking**: Same XSS vulnerability as above — note body is rendered unsanitized.\n\n**Why it matters**: Note bodies are typically user-generated or externally-sourced content, making them higher-risk than titles. An attacker who can inject a note body gains immediate XSS execution. This is compounded by the fact that the delete button is in the same list item — a malicious script could intercept clicks, steal sessions, or perform actions on behalf of the user.\n\n**Suggested fix**: Sanitize with DOMPurify or render as plain text:\n```typescript\n<span>{n.body}</span> // Plain text, auto-escaped by React\n```\nIf HTML formatting is required (e.g., markdown rendering), sanitize first:\n```typescript\n<span dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(n.body) }} />```"
    },
    {
      "path": "dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx",
      "line": 20,
      "side": "RIGHT",
      "code": "  useEffect(() => {",
      "severity": "Blocking",
      "body": "**Blocking**: Missing dependency array causes infinite re-render loop.\n\n**Why it matters**: This `useEffect` has no second argument (dependency array), so it runs after every render. The effect updates state (`setNotes`, `setPending`), which triggers a re-render, which runs the effect again, ad infinitum. This will spam the external API with requests, exhaust browser resources, and likely trigger rate limiting. React DevTools would show constant re-renders.\n\n**Suggested fix**: Add `[userId]` as dependency array since the fetch depends on userId:\n```typescript\nuseEffect(() => {\n  fetch(`${API}/${userId}`)\n    .then((r) => r.json())\n    .then((d) => {\n      setNotes((d as any).items);\n      setPending((d as any).pending_count);\n    });\n}, [userId]);\n```"
    },
    {
      "path": "dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx",
      "line": 29,
      "side": "RIGHT",
      "code": "  useEffect(() => {",
      "severity": "Blocking",
      "body": "**Blocking**: Interval is never cleaned up, causing memory leak.\n\n**Why it matters**: This `useEffect` creates a `setInterval` but doesn't return a cleanup function. When the component unmounts (user navigates away), the interval continues running indefinitely, holding references to the component and preventing garbage collection. Over time, this leaks memory and continues sending heartbeat requests for unmounted components. ESLint's `react-hooks/exhaustive-deps` rule would flag this.\n\n**Suggested fix**: Return a cleanup function that clears the interval:\n```typescript\nuseEffect(() => {\n  const id = setInterval(() => {\n    fetch(`${API}/${userId}/heartbeat`, { method: \"POST\" })\n      .catch(() => undefined);\n  }, 30_000);\n  return () => clearInterval(id); // Cleanup on unmount\n}, [userId]);\n```"
    },
    {
      "path": "dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx",
      "line": 3,
      "side": "RIGHT",
      "code": "const API = \"https://api.example.com/notes\";",
      "severity": "Non-blocking",
      "body": "**Non-blocking**: Hardcoded external API URL bypasses app configuration.\n\n**Why it matters**: This component makes requests to a hardcoded external domain, not through the app's API client. This bypasses CORS configuration, authentication middleware, request logging, and environment-specific URL settings. The rest of the codebase uses the centralized API client (`dashboard/src/api/control-layer/client.ts`) which handles auth, error handling, and base URL configuration.\n\n**Suggested fix**: Either integrate with the existing API client or make the URL configurable via environment variable / config endpoint. If this is truly scaffolding for a future service, add a TODO comment noting it needs proper integration."
    },
    {
      "path": "dashboard/src/components/features/user-notes/UserNotes/UserNotes.tsx",
      "line": 21,
      "side": "RIGHT",
      "code": "    fetch(`${API}/${userId}`)",
      "severity": "Non-blocking",
      "body": "**Non-blocking**: Missing error handling and credentials.\n\n**Why it matters**: This fetch has no `.catch()` handler, so errors are silently swallowed. No `credentials` option means session cookies won't be sent for cross-origin requests (the app's pattern is `credentials: \"include\"` per `client.ts:132`). Compare to proper API calls in `client.ts:176` which include full error handling and credentials.\n\n**Suggested fix**: \n```typescript\nuseEffect(() => {\n  fetch(`${API}/${userId}`, { credentials: \"include\" })\n    .then((r) => {\n      if (!r.ok) throw new Error(`HTTP ${r.status}`);\n      return r.json();\n    })\n    .then((d) => {\n      setNotes((d as any).items);\n      setPending((d as any).pending_count);\n    })\n    .catch((err) => {\n      console.error(\"Failed to fetch notes:\", err);\n      setNotes([]); // Show empty state on error\n    });\n}, [userId]);\n```"
    },
    {
      "path": "dwctl/src/api/handlers/mod.rs",
      "line": 54,
      "side": "RIGHT",
      "code": "pub mod system_info;",
      "severity": "Non-blocking",
      "body": "**Non-blocking**: Module declared but route never registered.\n\n**Why it matters**: While the module is correctly exported here, searching `dwctl/src/lib.rs` shows no `.route(\"/system/info\", ...)` or `get_system_info` reference. The handler code exists but is unreachable at runtime. This isn't a security issue but indicates incomplete implementation. Per the commit message (\"the system-info route is not registered yet\"), this is intentional scaffolding — but dead code should be avoided.\n\n**Suggested fix**: Either (1) register the route in `lib.rs` once auth is implemented, or (2) remove this export until the feature is complete. If keeping as scaffolding, add a `#[allow(dead_code)]` at the module level or a TODO comment."
    }
  ]
}

sejori added a commit to doublewordai/opencode that referenced this pull request May 12, 2026
Discovered and fixed while the bot was reviewing test PR
doublewordai/control-layer#1047. All four are first-time-only mistakes
worth capturing, but they are also recordable production friction —
each one exists only because tool execution lives client-side and
customers must own the harness end-to-end.

1. mkdtemp ENOENT on slashed tag — `${owner}/${repo}#${prNumber}` was
   used directly as the temp-dir prefix; mkdtemp does not create
   intermediate directories. Sanitise non-alphanumerics to `-`.

2. git fetch refspec collision — `git fetch origin <base>:<base>`
   refuses to update a local branch that's currently checked out, which
   the cloned default branch always is. Drop the local-dst part of the
   refspec — bare `git fetch origin <ref>` updates the remote-tracking
   branch + FETCH_HEAD, which is enough.

3. Per-workspace opencode config — opencode loads `agent` + `provider`
   config relative to each session's workspace directory (the
   x-opencode-directory header value). The shim's `review` agent +
   Doubleword provider live in /app/opencode.json (the image's own
   root) and were invisible to PR worktrees. Copy /app/opencode.json
   into each cloned workdir before kicking off the session.

4. Hardened the opencode response handler — when the session call
   returned an error envelope (`{success: false, error: [...]}`), the
   shim TypeError'd on `reply.parts`. Now surfaces the envelope as a
   readable error.

Also bumped clone depth to 500 so git diff origin/<base>...HEAD has a
reachable merge-base for typical PR sizes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sejori added a commit to doublewordai/opencode that referenced this pull request May 12, 2026
Discovered and fixed while the bot was reviewing test PR
doublewordai/control-layer#1047. All four are first-time-only mistakes
worth capturing, but they are also recordable production friction —
each one exists only because tool execution lives client-side and
customers must own the harness end-to-end.

1. mkdtemp ENOENT on slashed tag — `${owner}/${repo}#${prNumber}` was
   used directly as the temp-dir prefix; mkdtemp does not create
   intermediate directories. Sanitise non-alphanumerics to `-`.

2. git fetch refspec collision — `git fetch origin <base>:<base>`
   refuses to update a local branch that's currently checked out, which
   the cloned default branch always is. Drop the local-dst part of the
   refspec — bare `git fetch origin <ref>` updates the remote-tracking
   branch + FETCH_HEAD, which is enough.

3. Per-workspace opencode config — opencode loads `agent` + `provider`
   config relative to each session's workspace directory (the
   x-opencode-directory header value). The shim's `review` agent +
   Doubleword provider live in /app/opencode.json (the image's own
   root) and were invisible to PR worktrees. Copy /app/opencode.json
   into each cloned workdir before kicking off the session.

4. Hardened the opencode response handler — when the session call
   returned an error envelope (`{success: false, error: [...]}`), the
   shim TypeError'd on `reply.parts`. Now surfaces the envelope as a
   readable error.

Also bumped clone depth to 500 so git diff origin/<base>...HEAD has a
reachable merge-base for typical PR sizes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sejori added a commit to doublewordai/opencode that referenced this pull request May 12, 2026
Discovered and fixed while the bot was reviewing test PR
doublewordai/control-layer#1047. All four are first-time-only mistakes
worth capturing, but they are also recordable production friction —
each one exists only because tool execution lives client-side and
customers must own the harness end-to-end.

1. mkdtemp ENOENT on slashed tag — `${owner}/${repo}#${prNumber}` was
   used directly as the temp-dir prefix; mkdtemp does not create
   intermediate directories. Sanitise non-alphanumerics to `-`.

2. git fetch refspec collision — `git fetch origin <base>:<base>`
   refuses to update a local branch that's currently checked out, which
   the cloned default branch always is. Drop the local-dst part of the
   refspec — bare `git fetch origin <ref>` updates the remote-tracking
   branch + FETCH_HEAD, which is enough.

3. Per-workspace opencode config — opencode loads `agent` + `provider`
   config relative to each session's workspace directory (the
   x-opencode-directory header value). The shim's `review` agent +
   Doubleword provider live in /app/opencode.json (the image's own
   root) and were invisible to PR worktrees. Copy /app/opencode.json
   into each cloned workdir before kicking off the session.

4. Hardened the opencode response handler — when the session call
   returned an error envelope (`{success: false, error: [...]}`), the
   shim TypeError'd on `reply.parts`. Now surfaces the envelope as a
   readable error.

Also bumped clone depth to 500 so git diff origin/<base>...HEAD has a
reachable merge-base for typical PR sizes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sejori added a commit to doublewordai/opencode that referenced this pull request May 12, 2026
Discovered and fixed while the bot was reviewing test PR
doublewordai/control-layer#1047. All four are first-time-only mistakes
worth capturing, but they are also recordable production friction —
each one exists only because tool execution lives client-side and
customers must own the harness end-to-end.

1. mkdtemp ENOENT on slashed tag — `${owner}/${repo}#${prNumber}` was
   used directly as the temp-dir prefix; mkdtemp does not create
   intermediate directories. Sanitise non-alphanumerics to `-`.

2. git fetch refspec collision — `git fetch origin <base>:<base>`
   refuses to update a local branch that's currently checked out, which
   the cloned default branch always is. Drop the local-dst part of the
   refspec — bare `git fetch origin <ref>` updates the remote-tracking
   branch + FETCH_HEAD, which is enough.

3. Per-workspace opencode config — opencode loads `agent` + `provider`
   config relative to each session's workspace directory (the
   x-opencode-directory header value). The shim's `review` agent +
   Doubleword provider live in /app/opencode.json (the image's own
   root) and were invisible to PR worktrees. Copy /app/opencode.json
   into each cloned workdir before kicking off the session.

4. Hardened the opencode response handler — when the session call
   returned an error envelope (`{success: false, error: [...]}`), the
   shim TypeError'd on `reply.parts`. Now surfaces the envelope as a
   readable error.

Also bumped clone depth to 500 so git diff origin/<base>...HEAD has a
reachable merge-base for typical PR sizes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants