Skip to content

feat(gateway): extract gateway frontend into ironclaw_gateway crate with widget system - #1725

Merged
ilblackdragon merged 48 commits into
stagingfrom
feat/frontend-extension-system
Apr 10, 2026
Merged

ilblackdragon merged 48 commits into
stagingfrom
feat/frontend-extension-system

Conversation

@ilblackdragon

@ilblackdragon ilblackdragon commented Mar 28, 2026 •

Copy link
Copy Markdown
Member

Summary

Extracts the gateway frontend into a dedicated ironclaw_gateway crate and adds the widget extension system, enabling users and companies to customize the UI from the workspace.

  • Static assets moved to crates/ironclaw_gateway/static/ — app.js, style.css, index.html, i18n/*, theme-init.js, favicon.ico
  • Embedded via crate — ironclaw_gateway::assets::* constants replace include_str!() in gateway handlers
  • Layout configuration — LayoutConfig type with branding (title, colors), tab order, chat features, per-widget config. Stored as .system/gateway/layout.json in workspace.
  • Widget system — WidgetManifest + WidgetSlot enum (Tab, ChatHeader, Sidebar, etc., though only Tab is currently mounted by the browser runtime). Widgets stored in workspace at .system/gateway/widgets/{id}/.
  • CSS scoping — scope_css() auto-prefixes widget CSS with [data-widget="id"] for isolation; brace-depth-aware so nested @media / @supports / @container rules are scoped recursively while @keyframes bodies pass through opaque.
  • Bundle assembly — assemble_index() injects layout config, widget scripts, scoped widget CSS, and branding into the base HTML; XSS-hardened against </script> and </style> breakouts in case-insensitive UTF-8-safe form.
  • CSP nonce — injected <script> blocks carry a per-request CSP nonce via the NONCE_PLACEHOLDER sentinel; index_handler stamps a fresh nonce per response and emits a matching Content-Security-Policy: script-src 'nonce-…' header so the browser actually executes the inlined widget runtime.
  • HTML cache — GatewayState::frontend_html_cache keys on the updated_at of .system/gateway/layout.json and .system/gateway/widgets/ (one cheap list() call), so the warm path skips reading every widget manifest / JS / CSS file.
  • Frontend API — GET/PUT /api/frontend/layout, GET /api/frontend/widgets, GET /api/frontend/widget/{id}/{*file}. The {id} segment goes through component-based path-traversal validation (is_safe_segment / is_safe_relative_path).
  • Browser widget API — IronClaw.registerWidget({ slot: "tab", … }) and IronClaw.registerChatRenderer({ id, match, render }) for inline rendering of structured data. Widgets get an IronClaw.api object with authenticated fetch, SSE event subscription, theme info, i18n, and navigation.

Workspace layout

All gateway state lives under .system/gateway/ in the workspace, alongside other .system/* subsystems being introduced in parallel work:

.system/gateway/
  README.md           # seeded on first boot from src/workspace/seeds/FRONTEND.md
  layout.json         # branding, tabs, chat features, per-widget config
  custom.css          # appended to /style.css
  widgets/{id}/
    manifest.json
    index.js
    style.css         # optional, auto-scoped

Stacked on

New files

File Purpose
crates/ironclaw_gateway/ New crate (Cargo.toml + 4 source files)
crates/ironclaw_gateway/static/ Moved from src/channels/web/static/
src/channels/web/handlers/frontend.rs Layout + widget API handlers
src/workspace/seeds/FRONTEND.md Customization guide seeded into the workspace

Test plan

  • cargo fmt — clean
  • cargo clippy --all --benches --tests --examples --all-features — zero warnings
  • cargo test -p ironclaw_gateway — 46 unit tests + 1 doctest pass
  • cargo test --lib -p ironclaw — 4238 passed
  • Per-response CSP nonce verified by test_build_csp_with_nonce_includes_nonce_source and test_generate_csp_nonce_is_unique_and_hex
  • Manual: gateway serves unchanged HTML with no workspace customizations
  • Manual: write .system/gateway/layout.json to workspace → branding changes visible
  • Manual: create widget (manifest + index.js) → new tab appears, browser executes widget under script-src 'nonce-…'

🤖 Generated with Claude Code

@github-actions github-actions Bot added scope: channel/web Web gateway channel scope: dependencies Dependency updates size: XL 500+ changed lines risk: medium Business logic, config, or moderate-risk modules contributor: core 20+ merged PRs labels Mar 28, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the ironclaw_frontend crate, which manages embedded static assets, layout configurations, and a widget extension system for the IronClaw web gateway. It includes logic for assembling the final served HTML by injecting branding and widget-specific scripts/styles, alongside new Axum handlers for layout CRUD and widget discovery. Feedback suggests reverting the Rust edition and version to stable releases and improving error handling by logging warnings when widget manifests fail to parse.

Comment thread crates/ironclaw_frontend/Cargo.toml Outdated
Comment on lines +4 to +5
edition = "2024"
rust-version = "1.85"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The Rust edition and version are set to unreleased versions (edition = "2024", rust-version = "1.85"). This can cause build issues and unexpected behavior for developers using the stable Rust toolchain. It's recommended to use the latest stable edition (2021) and set rust-version to the project's actual Minimum Supported Rust Version (MSRV) that is a stable release.

Suggested change
edition = "2024"
rust-version = "1.85"
edition = "2021"
rust-version = "1.75"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The crate was renamed from ironclaw_frontend to ironclaw_gateway in a later commit, and the project standard is edition = "2024" / rust-version = "1.92" (every workspace member matches). Edition 2024 has been stable since Rust 1.85 (Feb 2025), so this is no longer an unreleased combination — it's the project baseline. Closing as a false positive.

Comment thread src/channels/web/handlers/frontend.rs Outdated
Comment on lines +89 to +91
if let Ok(manifest) = serde_json::from_str::<WidgetManifest>(&doc.content) {
manifests.push(manifest);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Errors from parsing a widget's manifest.json are silently ignored. While this prevents one broken widget from breaking the entire list, it makes debugging difficult for widget developers. Following repository patterns, when a resource cannot be parsed, it should be skipped with a warning log providing specific context about the failure to aid debugging.

            match serde_json::from_str::<WidgetManifest>(&doc.content) {
                Ok(manifest) => manifests.push(manifest),
                Err(e) => tracing::warn!("Skipping invalid widget manifest at '{}': {}", manifest_path, e),
            }
References
  1. When a resource cannot be processed due to parsing errors, log a warning and skip it. Provide specific error context to provide semantically correct and clear error messages.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Already addressed. Both load_widget_manifests and load_resolved_widgets now route through read_widget_manifest (src/channels/web/handlers/frontend.rs:161), which logs a tracing::warn! with the manifest path and parse error before returning None:

Err(e) => {
    tracing::warn!(
        path = %manifest_path,
        error = %e,
        "skipping widget with invalid manifest"
    );
    return None;
}

Widget developers get the specific error context you asked for, and one broken widget still doesn't break the rest of the list.

@ilblackdragon
ilblackdragon force-pushed the feat/frontend-extension-system branch from c298041 to f2114e3 Compare March 28, 2026 19:17
@ilblackdragon ilblackdragon added the skip-regression-check Bypass regression test CI gate (tests exist but not in tests/ dir) label Mar 28, 2026
@ilblackdragon
ilblackdragon force-pushed the feat/frontend-extension-system branch from f2114e3 to fc32a31 Compare March 28, 2026 19:28

@ilblackdragon ilblackdragon left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Review comments addressed in fc32a31:

  1. Edition/rust-version (high) — Fixed rust-version to 1.92 to match other crates. The edition = "2024" is correct and consistent with all other workspace crates.

  2. Silent widget manifest errors (medium) — Now logs a warning with the path and parse error via tracing::warn! when a manifest fails to parse, instead of silently skipping.

Also fixed: missing license field for cargo-deny, ran cargo fmt.

@github-actions github-actions Bot added scope: workspace Persistent memory / workspace scope: docs Documentation labels Mar 29, 2026
@jasperan

Copy link
Copy Markdown

CI Fix for Formatting + Clippy Failures

The 5 failing CI jobs all trace to 4 root-cause errors in ironclaw_frontend. Here are the fixes:

1. cargo fmt (server.rs)

Run cargo fmt --all to fix 3 formatting diffs in src/channels/web/server.rs (function signature wrapping and format!() call layout).

2. Collapse nested if-let (bundle.rs:93)

// Before:
if let Some(ref custom_css) = bundle.custom_css {
    if !custom_css.trim().is_empty() {
        body_injections.push(format!("<style data-custom-css>{}</style>", custom_css));
    }
}

// After:
if let Some(ref custom_css) = bundle.custom_css
    && !custom_css.trim().is_empty()
{
    body_injections.push(format!("<style data-custom-css>{}</style>", custom_css));
}

3. Collapse triple nested if-let (bundle.rs:112-114)

// Before:
if let Some(ref title) = bundle.layout.branding.title {
    if let Some(start) = result.find("<title>") {
        if let Some(end) = result[start..].find("</title>") {
            let end = start + end + "</title>".len();
            result.replace_range(start..end, &format!("<title>{}</title>", title));
        }
    }
}

// After:
if let Some(ref title) = bundle.layout.branding.title
    && let Some(start) = result.find("<title>")
    && let Some(end) = result[start..].find("</title>")
{
    let end = start + end + "</title>".len();
    result.replace_range(start..end, &format!("<title>{}</title>", title));
}

4. while-let → for loop (widget.rs:77)

// Before:
let mut chars = css.chars().peekable();
while let Some(ch) = chars.next() {

// After:
let chars = css.chars();
for ch in chars {

(peekable() is unused, mut becomes unnecessary)

Bonus: pre-existing clippy lint (libsql/users.rs:984)

// Before:
assert!(stats.iter().find(|s| s.user_id == "bob").is_none());
// After:
assert!(!stats.iter().any(|s| s.user_id == "bob"));

@ilblackdragon
ilblackdragon force-pushed the feat/frontend-extension-system branch from 9559050 to f7fc508 Compare March 29, 2026 21:12
Base automatically changed from feat/workspace-metadata-versioning-patch to staging April 2, 2026 06:58
ilblackdragon and others added 11 commits April 2, 2026 23:57
…dget extension system

Moves all frontend static assets (app.js, style.css, index.html, i18n/*,
theme-init.js, favicon.ico) from src/channels/web/static/ into a dedicated
ironclaw_frontend crate. The crate also adds:

- Layout configuration types (branding, tab order, chat features, per-widget config)
- Widget manifest types with named slot system (tab, chat_header, sidebar, etc.)
- CSS scoping utility (auto-prefixes selectors with [data-widget="id"])
- Bundle assembly (injects layout config, widgets, and custom CSS into HTML)
- Frontend API endpoints (GET/PUT layout, list widgets, serve widget files)
- Browser-side IronClaw.registerWidget() API with authenticated fetch,
  event subscription, theme access, and i18n

Widgets are stored in workspace at frontend/widgets/{id}/ and served via
the API. Layout config is stored at frontend/layout.json. The agent can
create/edit both using existing memory_write/memory_read tools.

Gateway handlers now reference ironclaw_frontend::assets constants instead
of include_str!() with local paths, completing the separation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t warnings

- Add license = "MIT OR Apache-2.0" to ironclaw_frontend Cargo.toml (cargo-deny)
- Fix rust-version to 1.92 to match other crates
- Log warning for invalid widget manifests instead of silent skip
- Run cargo fmt across all files

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ssage rendering

Agent responses containing JSON/structured data (like mission results,
status objects) now render as styled cards with labeled fields, status
badges, and monospaced IDs instead of raw text.

Built-in rendering:
- Detects inline JSON objects (including Python-style single quotes)
- Renders as data cards with key-value rows
- Status/state fields get colored badges (success/error/pending)
- UUIDs rendered in monospace

Extensible via widgets:
- IronClaw.registerChatRenderer({ id, match, render, priority })
- First matching renderer wins (priority ordering)
- Renderer gets the content element to mutate in place

Also adds ChatRenderer variant to WidgetSlot enum.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Navigation state is now encoded in window.location.hash so refreshing
the page (or sharing a URL) restores the current view:

  #/chat                   → chat tab, assistant thread
  #/chat/{threadId}        → specific conversation
  #/memory/{path/to/file}  → memory browser with file open
  #/jobs/{jobId}           → job detail view
  #/routines/{id}          → routine detail view
  #/settings/{subtab}      → settings sub-tab (extensions, etc.)
  #/logs                   → logs tab

Hooked into all navigation functions: switchTab, switchThread,
switchToAssistant, createNewThread, readMemoryFile, openJobDetail,
closeJobDetail, openRoutineDetail, closeRoutineDetail,
switchSettingsSubtab.

Thread restore is deferred until loadThreads() completes (async),
then the pending thread ID is matched against the loaded thread list.

Browser back/forward buttons work via hashchange listener.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two bugs caused the hash to reset on Cmd+R:
1. Auth URL cleanup (replaceState) stripped the hash fragment —
   now preserves it via cleaned.hash
2. restoreFromHash() called switchTab() which called updateHash()
   overwriting the full hash before the detail was restored —
   now suppresses hash updates during the entire restore sequence

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…agent

The agent didn't know it could customize the frontend via workspace writes.
Now seeds frontend/README.md on first boot with a guide covering:
- Layout config (branding, colors, tab order) via frontend/layout.json
- Custom CSS via frontend/custom.css with common variable names
- Widget creation (manifest + index.js + style.css)
- API endpoints

Also seeds frontend/.config with skip_indexing: true so frontend assets
aren't chunked/embedded for search.

When a user says "change the color scheme to red", the agent can now
discover frontend/README.md via memory_tree, read the guide, and write
the appropriate layout.json or custom.css.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
….css

The index_handler and css_handler now read from workspace to apply
frontend customizations on page load:

- index_handler: reads frontend/layout.json, discovers widgets in
  frontend/widgets/*, reads frontend/custom.css, then calls
  assemble_index() to inject branding colors, layout config,
  widget scripts, and custom CSS into the base HTML.
  Falls back to embedded HTML if no customizations exist.

- css_handler: appends frontend/custom.css from workspace after
  the embedded base stylesheet.

This completes the end-to-end flow:
  Agent writes frontend/layout.json → user refreshes → sees changes

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Audit-driven fixes for the widget extension system:

1. Widget tab panel ID: panels now get id="tab-{widgetId}" so
   switchTab() can find and activate them

2. Widget JS auth: inline widget JS in assembled HTML instead of
   <script src> to protected endpoint (browser script tags can't
   send Authorization headers)

3. Layout config: fully implement tab ordering, default_tab,
   chat.suggestions, chat.image_upload application

4. SSE event forwarding: wrap EventSource.addEventListener to
   intercept all named events and dispatch to widget subscribers
   via IronClaw.api._dispatch()

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ositives

Security (2 XSS fixes):
1. HTML-escape branding title in assemble_index() to prevent
   <script>alert(1)</script> injection via layout.json
2. Escape </script> in inlined widget JS to prevent script tag
   breakout — uses <\/script> replacement
3. Escape widget IDs in HTML attributes via escape_html_attr()

Correctness:
4. Drain _widgetInitQueue after DOM is ready — widgets registered
   before tab-bar exists now mount correctly instead of silently
   failing
5. Skip inline <code> elements in upgradeInlineJson to prevent
   false-positive JSON card rendering on code spans like
   <code>{key: value}</code>
6. Document scope_css limitation with nested @media rules

Tests (13 new):
- XSS: title injection escaped, widget JS </script> breakout escaped,
  widget ID attribute escaped
- Edge cases: escape_html basic, escape_html_attr quotes, missing
  head/body tags, empty widget JS, whitespace-only custom CSS skipped
- Widget: at-rule not prefixed, declarations preserved, special chars
  in widget ID, all slot variants round-trip, minimal manifest

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ilblackdragon
ilblackdragon force-pushed the feat/frontend-extension-system branch from 70e7091 to 7aac180 Compare April 3, 2026 07:07
@ilblackdragon
ilblackdragon marked this pull request as ready for review April 3, 2026 07:11
Copilot AI review requested due to automatic review settings April 3, 2026 07:11
ilblackdragon and others added 2 commits April 9, 2026 14:49
…(PR #1725)

Four findings from the latest review pass.

1. `read_widget_manifest` validated `directory_name` with `is_safe_segment`,
   but `manifest.id == directory_name` is enforced later AND `manifest.id`
   itself must pass `is_safe_widget_id`. Accepting a wider charset at the
   discovery step than the loader/runtime contract allows only surfaces
   widgets that can never resolve. Switched discovery to `is_safe_widget_id`
   so discovery, serving (`frontend_widget_file_handler`), and `manifest.id`
   validation all use the same canonical check. Removed the now-dead
   `is_safe_segment` helper and its tests; expanded
   `skips_widget_with_unsafe_directory_name` to also exercise the wider
   charset (`-flag`, `.hidden`, quoted/bracketed/whitespace names) that the
   previous validator wrongly permitted.

2. `src/workspace/seeds/FRONTEND.md` referenced `is_safe_segment` /
   `is_safe_relative_path` — both are gone now. Updated the security-model
   bullet to point to `is_safe_widget_id` (the single canonical validator,
   defined in `crates/ironclaw_gateway/src/layout.rs`).

3. `assemble_index` always emits `window.__IRONCLAW_LAYOUT__`, which is
   pinned by `test_assemble_index_no_customizations`, but the production
   call site (`build_frontend_html`) short-circuits via
   `layout_has_customizations()` so the default-bundle branch is only
   reachable from tests. Added a doc-comment block at the top of
   `assemble_index` explaining the production gate so future maintainers
   don't read the always-injected layout JSON as a contradiction.

4. `window.IronClaw = window.IronClaw || {};` honored any pre-existing
   value on `window.IronClaw`. The gateway HTML loads `app.js` before any
   deferred widget module and has no inline scripts that touch the
   namespace, so this isn't an exploitable bug today, but the `|| {}` form
   would silently honor a hostile pre-init via a future template change
   or a stray browser extension. Replaced with
   `Object.defineProperty(window, 'IronClaw', { value: {}, writable: false,
   configurable: false, enumerable: true })` so the binding is locked: a
   hostile widget can still mutate properties on the fixed object (same
   authority every other widget already has) but cannot replace the entire
   `IronClaw` namespace. Defense in depth, with a comment explaining why.

Addresses review comments r3057150364/415/449/466/487 (×5 dupes),
r3057572833, r3057573554, r3057574018.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nsion-system

# Conflicts:
#	src/channels/web/handlers/settings.rs
#	src/channels/web/mod.rs
#	src/channels/web/server.rs
#	src/channels/web/test_helpers.rs
#	src/channels/web/tests/multi_tenant.rs
#	src/channels/web/ws.rs
#	src/workspace/mod.rs
#	tests/multi_tenant_integration.rs
#	tests/oauth_greeting_integration.rs
#	tests/openai_compat_integration.rs
#	tests/support/gateway_workflow_harness.rs
#	tests/ws_gateway_integration.rs
Copilot AI review requested due to automatic review settings April 9, 2026 15:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 31 out of 40 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +55 to +58
}
},
Err(_) => LayoutConfig::default(),
}

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

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

read_layout_config treats any workspace.read() error as “missing file” and silently falls back to LayoutConfig::default(). That will hide real backend issues (e.g., WorkspaceError::IoError, SearchFailed) and make both /api/frontend/layout and the index assembly path ignore customizations without any signal. Consider matching on the error: return default only for DocumentNotFound, and warn! (or propagate as 500 for the API endpoint) for other error variants.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 8065f94. You're right — the blanket Err(_) => LayoutConfig::default() masked IoError/SearchFailed/backend connectivity problems and dropped customizations without any operator signal.

Split the match in read_layout_config (src/channels/web/handlers/frontend.rs:45): WorkspaceError::DocumentNotFound stays silent (it's the common case — every page load hits this path for a workspace with no customizations), and every other variant now logs at warn! before the default fallback:

Err(WorkspaceError::DocumentNotFound { .. }) => LayoutConfig::default(),
Err(e) => {
    tracing::warn!(
        error = %e,
        path = LAYOUT_PATH,
        "workspace read failed — falling back to default layout \
         (customizations may be silently skipped)"
    );
    LayoutConfig::default()
}

Kept the infallible signature because the cache assembly path can't crash on workspace errors — the trade-off is that the API endpoint still returns a 200 with the default layout on backend failures instead of a 500, but backend issues now surface in logs instead of being invisible.

Comment on lines +125 to +127
pub(crate) async fn load_widget_manifests(workspace: &Workspace) -> Vec<WidgetManifest> {
let entries = workspace.list(WIDGETS_DIR).await.unwrap_or_default();

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

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

load_widget_manifests swallows errors from workspace.list(WIDGETS_DIR) via unwrap_or_default(), so /api/frontend/widgets can return 200 [] even if listing fails due to an underlying workspace/backend error. Consider returning a Result (and letting the handler map to 500) or at least logging the error before falling back to an empty list.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 8065f94. Both load_widget_manifests AND load_resolved_widgets used the same workspace.list().await.unwrap_or_default() pattern, so a real listing failure came out as 200 [] from /api/frontend/widgets and as "no widgets" on the index page — hiding the outage behind an empty-list response.

Both functions now match explicitly and log at warn! before the empty-list fallback (src/channels/web/handlers/frontend.rs):

let entries = match workspace.list(WIDGETS_DIR).await {
    Ok(entries) => entries,
    Err(e) => {
        tracing::warn!(
            error = %e,
            path = WIDGETS_DIR,
            "workspace list failed — returning empty widget list \
             (installed widgets may be silently skipped)"
        );
        Vec::new()
    }
};

An empty directory is still the silent default (the workspace returns an empty Vec for that case, not an error). Only genuine listing failures log now.

Comment on lines +324 to +331
let doc = workspace.read(&path).await.map_err(|e| {
tracing::warn!(
workspace_path = %path,
error = %e,
"widget file not found"
);
(StatusCode::NOT_FOUND, "Widget file not found".to_string())
})?;

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

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

frontend_widget_file_handler maps all workspace.read() failures to 404 Not Found. This will incorrectly return 404 for backend errors (e.g., DB connectivity / IO issues) and make operational debugging harder. Consider matching on WorkspaceError::DocumentNotFound for 404, and returning 500 for other error variants (while still logging details server-side).

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 8065f94. frontend_widget_file_handler now distinguishes 404 from 500 based on the error variant (src/channels/web/handlers/frontend.rs):

let doc = workspace.read(&path).await.map_err(|e| {
    use crate::error::WorkspaceError;
    match e {
        WorkspaceError::DocumentNotFound { .. } => {
            tracing::warn!(workspace_path = %path, "widget file not found");
            (StatusCode::NOT_FOUND, "Widget file not found".to_string())
        }
        other => {
            tracing::warn!(
                workspace_path = %path,
                error = %other,
                "widget file read failed (backend error)"
            );
            (
                StatusCode::INTERNAL_SERVER_ERROR,
                "Failed to read widget file".to_string(),
            )
        }
    }
})?;

Backend failures (IoError, SearchFailed, connectivity) now show up as 500s so operational problems surface in status codes as well as logs. The client-facing bodies stay generic in both cases to preserve the path-enumeration hardening you noted in the surrounding block.

Comment on lines +333 to +346
// Determine MIME type from the file extension (case-insensitive — the
// browser doesn't care about `.JS` vs `.js`).
let ext = file
.rsplit('.')
.next()
.map(|s| s.to_ascii_lowercase())
.unwrap_or_default();
let content_type = match ext.as_str() {
"js" | "mjs" => "application/javascript",
"css" => "text/css",
"json" => "application/json",
"map" => "application/json",
_ => "text/plain",
};

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

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

The handler’s MIME type mapping falls back to text/plain for any non-(js/css/json/map) extension, but your docs/tests explicitly allow assets like assets/icon.svg. Serving SVG as text/plain can break rendering and may trigger content sniffing in some clients. Consider adding common types (e.g., image/svg+xml, image/png, image/jpeg, image/gif, image/webp, image/x-icon) or using a small MIME lookup (e.g., mime_guess) to keep widget assets usable.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 8065f94. You're right — assets/icon.svg is explicitly allowed by both the regression test (widget_file_path_components_use_strict_charset) and the docs, but the MIME map was serving it as text/plain, which broke SVG rendering and would have triggered content sniffing on Chromium for the webfont files too.

Extended the match with the common widget asset types (src/channels/web/handlers/frontend.rs):

"js" | "mjs" => "application/javascript",
"css" => "text/css",
"json" => "application/json",
"map" => "application/json",
"svg" => "image/svg+xml",
"png" => "image/png",
"jpg" | "jpeg" => "image/jpeg",
"gif" => "image/gif",
"webp" => "image/webp",
"ico" => "image/x-icon",
"woff" => "font/woff",
"woff2" => "font/woff2",
"ttf" => "font/ttf",
"otf" => "font/otf",
_ => "text/plain",

Went with an explicit match rather than pulling in mime_guess — the list is short enough to be self-documenting and keeps the gateway's asset surface narrow (a widget author can't rely on serving arbitrary media types this way). text/plain is still the last-resort fallback for unknown extensions.

Comment on lines +52 to +57
The session-scoped ``ironclaw_server`` fixture is shared across every
test in the run, so anything we write into the workspace must be wiped
before yielding back to the next test. ``memory_write`` accepts an empty
body for non-layer paths, and the gateway treats empty / unparseable
widget files as "skip silently", which is exactly the cleanup behavior
we want without needing a real DELETE endpoint.

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

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

The _wipe_customizations docstring says the gateway treats empty/unparseable widget files as “skip silently”, but read_widget_manifest() logs a warn! on JSON parse failure. Either adjust this comment (so it doesn’t promise silence) or update cleanup to remove/overwrite manifests in a way that avoids parse warnings if that’s important for test output clarity.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 8065f94. You're right — read_widget_manifest logs a warn! on parse failure, so the "skip silently" promise in the docstring was wrong.

Updated _wipe_customizations in tests/e2e/scenarios/test_widget_customization.py:

memory_write accepts an empty body for non-layer paths, and the gateway's widget loader (read_widget_manifest) treats empty / unparseable widget manifests as "skip with a warn! log and continue" — no 500s, no index-page breakage — which is exactly the cleanup behavior we want without needing a real DELETE endpoint. The parse-failure warn lines are expected noise in the server log for the duration of this suite.

Kept the same cleanup mechanism (writing empty bodies) because it's the cleanest path we have without adding a DELETE endpoint purely for test plumbing — the warn lines are acceptable suite noise, now documented as such.

ilblackdragon and others added 2 commits April 9, 2026 16:24
…MIME map (PR #1725)

Five findings from the latest Copilot review pass — all correct.

1. `read_layout_config` (`src/channels/web/handlers/frontend.rs`) treated
   every `workspace.read()` error as "missing file" and silently fell back
   to `LayoutConfig::default()`. That masks `IoError`/`SearchFailed`/
   backend connectivity problems and drops customizations without any
   operator signal. Split the match: `WorkspaceError::DocumentNotFound`
   stays silent (common case, hit on every page load), every other
   variant now logs at `warn!` before the default fallback so backend
   problems surface. Keeping the infallible signature because the cache
   assembly path can't crash on workspace errors.

2. `load_widget_manifests` (and `load_resolved_widgets`, which had the
   same bug) used `workspace.list().await.unwrap_or_default()`. An empty
   widgets directory is a normal empty `Vec`, but a real listing failure
   used to come out as `200 []` from `/api/frontend/widgets` — hiding
   the outage behind a "no widgets installed" response. Now logs at
   `warn!` before the empty-list fallback.

3. `frontend_widget_file_handler` used to map *every* `workspace.read()`
   failure to 404, turning every backend outage into a silent stream of
   "not found" responses. Match on `WorkspaceError::DocumentNotFound`
   for the real 404 path and route every other variant to 500 (with a
   distinct `warn!` log) so operational issues show up in status codes
   as well as logs. The client-facing body stays generic in both cases
   to preserve the path-enumeration hardening.

4. The MIME type fallback for non-(js/css/json/map) extensions was
   `text/plain`, which broke SVG rendering and triggered content
   sniffing for icon / webfont assets. Docs and tests both explicitly
   allow `assets/icon.svg`-shaped paths. Extended the match with
   `svg`/`png`/`jpg`/`jpeg`/`gif`/`webp`/`ico` for images and
   `woff`/`woff2`/`ttf`/`otf` for webfonts. `text/plain` remains the
   last-resort fallback.

5. `_wipe_customizations` in `tests/e2e/scenarios/test_widget_customization.py`
   claimed the gateway treats empty/unparseable widget files as "skip
   silently", but `read_widget_manifest` logs a `warn!` on parse
   failure. Updated the docstring to match reality ("skip with a
   `warn!` log and continue") and note that parse-failure warn lines
   are expected suite noise.

Addresses review comments r3058951720, r3058951819, r3058951855,
r3058951889, r3058951920.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… view (PR #1725)

`is_safe_url` called `.trim()` before the `len() > 2048` cap, so a 4 KB
value padded with leading/trailing whitespace could collapse to a short
URL after trim and slip past the byte-length guard. The cap is a guard
against exfil-shaped payloads (the doc comment is explicit: "longer
values are either pathological or an exfil vector"), so the right thing
to count is what the caller actually wrote.

Reordered: length check now runs against the raw input, then `.trim()`
runs for the empty/whitespace check and the rest of the validation.
Added a regression test (`padded`) that pins the new behavior — without
the raw-length check the trimmed value would be 24 chars and silently
pass.

Independent code review nit; no exploitable bug today (the character
allowlist is the real defense and trailing whitespace URLs are rejected
by every consumer), but the comment and the code now agree.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 9, 2026 17:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 31 out of 40 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

serrrfirat
serrrfirat previously approved these changes Apr 9, 2026

@serrrfirat serrrfirat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thorough paranoid architect review — this is an exceptionally well-defended PR. XSS escaping, CSP nonce rotation, multi-tenant leak guards, widget ID charset validation, path traversal protection, and same-origin enforcement are all solid. Approving.

A few non-blocking hardening suggestions were left as inline comments:

  • Medium: frontend_widget_file_handler maps binary MIME types (PNG, WOFF2, etc.) but Workspace::read() returns String — binary widget assets would be corrupted. Text assets (JS/CSS/JSON/SVG) work fine, so no current production impact.
  • Low: scope_css brace-depth parser doesn't handle CSS comments/string literals with {/} — worth a note in FRONTEND.md.
  • Low: No per-widget size cap on JS/CSS loaded into the inline HTML bundle.
  • Low: SSE event forwarding swallows JSON.parse errors silently — a console.warn would help debugging.
  • Nit: escape_html_attr doesn't escape single quotes (safe today, all sites use double-quoted attrs).

None of these are blocking. Ship it. 🚢

.filter(|c| !c.trim().is_empty());

// Respect the layout's `enabled` flag; default is `true` when the
// widget has no entry at all (see WidgetInstanceConfig::default).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Medium — Correctness: This handler maps binary MIME types (image/png, font/woff2, font/ttf, etc.) but serves doc.content: String from Workspace::read(). Binary widget assets (images, fonts) would be silently corrupted because String is UTF-8 text.

No current production impact since primary widget assets (JS/CSS/JSON/SVG) are all text, but the MIME mapping advertises capabilities the handler cannot deliver. Options:

  1. Remove binary MIME mappings and return 415 for binary extensions until a read_bytes() path exists
  2. Add a comment marking them as aspirational (// TODO: requires binary workspace read)

Non-blocking.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 3ca3e5a. You're right — Workspace::read() returns String (UTF-8 text), so binary payloads would be silently corrupted. Added a // TODO: requires read_bytes() comment on the binary MIME entries in the handler and documented the limitation in FRONTEND.md:

Binary assets: Widget files are served through the workspace text layer (Workspace::read()), which returns UTF-8 strings. Text-format assets (JS, CSS, JSON, SVG) work correctly. Binary assets (PNG, WOFF2, TTF, etc.) will be corrupted — host them externally or Base64-encode them into CSS/JS until a binary workspace read path is available.

Kept the MIME mappings rather than returning 415 so the browser at least sees the right Content-Type header — it surfaces the corruption instead of hiding it behind a text/plain content-sniffed interpretation.

/// ```
pub fn scope_css(css: &str, widget_id: &str) -> String {
let prefix = format!("[data-widget=\"{}\"]", widget_id);
let mut result = String::with_capacity(css.len() + css.len() / 4);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Low — Robustness: scope_css is brace-depth-based and does not handle CSS comments (/* } */) or string literals (content: "{") containing braces. Widget CSS with these patterns will produce malformed output.

The limitation is documented in the Rust doc comment, but the user-facing FRONTEND.md guide (which widget authors will actually read) does not mention it. Consider adding a note under the "Widgets" section:

Widget CSS must not contain { or } inside string literals or comments — the CSS scoping engine is a text transform, not a full parser.

Non-blocking.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 3ca3e5a. Added a "CSS scoping caveat" note to src/workspace/seeds/FRONTEND.md:

CSS scoping caveat: Widget CSS is scoped via a brace-counting text transform, not a full CSS parser. Braces inside CSS comments (/* } */) or string literals (content: "{") will confuse the scoper and produce malformed output. Avoid { / } in comments and string values — use Unicode escapes (\\7B / \\7D) if you need literal braces in content: properties.

The Rust doc comment on scope_css already documented this, but widget authors reading the FRONTEND.md guide (their primary reference) would have missed it.

}
if manifest.id != directory_name {
tracing::warn!(
path = %manifest_path,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Low — Robustness: No size guard on widget JS/CSS loaded into the HTML bundle. A single widget with a multi-MB index.js gets read into memory, scoped, escaped, and injected inline into the cached HTML — bloating every page response.

Consider a per-widget cap (e.g., 512KB JS / 256KB CSS) with a warn! log and skip for oversized files.

Non-blocking.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 3ca3e5a. Added per-widget size caps in load_resolved_widgets (src/channels/web/handlers/frontend.rs):

  • MAX_WIDGET_JS_BYTES = 512 KB — widget JS over this is skipped with a warn!
  • MAX_WIDGET_CSS_BYTES = 256 KB — widget CSS over this is dropped with a warn!

The caps are generous enough for real-world widget bundles (a full charting library is ~250 KB minified) but stop a multi-MB file from ending up in the cached HTML. The warn! log names the widget and reports both the actual byte count and the cap so an operator can adjust or minify.

_origAddEventListener(type, function(e) {
// Dispatch to widget handlers
if (IronClaw.api && e.data) {
try {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Low — Robustness: The SSE event forwarding wrapper swallows JSON.parse errors in an empty catch (_) {}. If the gateway ever emits non-JSON SSE data, widget dispatching silently fails with no diagnostic.

} catch (_) {
  // Consider: console.warn("[IronClaw] SSE parse error for event", type, _);
}

Non-blocking.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 3ca3e5a. Replaced the empty catch (_) {} with:

} catch (parseErr) {
  console.warn('[IronClaw] SSE parse error for event', type, parseErr);
}

Now surfaces non-JSON SSE data in the browser console so widget dispatch failures have a diagnostic. The gateway currently always emits JSON SSE data, but if that ever regresses this will be the first visible signal.

ilblackdragon and others added 2 commits April 10, 2026 07:25
…, SSE parse logging (PR #1725)

Four non-blocking findings from serrrfirat, all valid.

1. Binary MIME types (png, woff2, ttf, etc.) are mapped in the widget
   file handler but `Workspace::read()` returns `String` — binary
   payloads get UTF-8 corrupted. Added a `// TODO: requires read_bytes()`
   comment on the binary entries and documented the limitation in
   FRONTEND.md so widget authors know to host binary assets externally
   or Base64-encode them until a binary workspace read path exists.

2. `scope_css` is a brace-counting text transform that doesn't handle
   CSS comments (`/* } */`) or string literals (`content: "{"`).
   Limitation was documented in the Rust doc comment but not in the
   user-facing FRONTEND.md guide. Added a "CSS scoping caveat" note
   recommending Unicode escapes for literal braces in `content:`.

3. No per-widget size guard — a multi-MB `index.js` would get inlined
   into the cached HTML and bloat every page response. Added
   `MAX_WIDGET_JS_BYTES` (512 KB) and `MAX_WIDGET_CSS_BYTES` (256 KB)
   constants in `load_resolved_widgets`. Oversized files are skipped
   with a `warn!` log naming the widget and the byte count.

4. The SSE event forwarding wrapper silently swallowed `JSON.parse`
   errors in an empty `catch (_) {}`, making widget dispatching
   failures invisible. Replaced with
   `console.warn('[IronClaw] SSE parse error for event', type, parseErr)`.

Also fixed a missing `frontend_html_cache` field in a new
`GatewayState` construction site from the latest staging merge
(`src/channels/web/tests/multi_tenant.rs`).

Addresses review comments r3060175180, r3060175488, r3060175732,
r3060175998.

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

Labels

contributor: core 20+ merged PRs risk: medium Business logic, config, or moderate-risk modules scope: channel/cli TUI / CLI channel scope: channel/wasm WASM channel runtime scope: channel/web Web gateway channel scope: dependencies Dependency updates scope: docs Documentation scope: extensions Extension management scope: workspace Persistent memory / workspace size: XL 500+ changed lines skip-regression-check Bypass regression test CI gate (tests exist but not in tests/ dir)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants