Skip to content

feat(reborn): delegate serve to WebUI ingress - #3828

Closed
serrrfirat wants to merge 0 commit into
reborn-integrationfrom
feat/reborn-serve-webui-delegation
Closed

serrrfirat wants to merge 0 commit into
reborn-integrationfrom
feat/reborn-serve-webui-delegation

Conversation

@serrrfirat

@serrrfirat serrrfirat commented May 20, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Tests

  • cargo fmt --check
  • cargo test -p ironclaw_reborn_webui_ingress
  • cargo test -p ironclaw_reborn_cli
  • cargo test -p ironclaw_reborn_composition --features webui-v2-beta
  • cargo test -p ironclaw_webui_v2 --features webui-v2-beta
  • cargo test -p ironclaw_architecture
  • cargo clippy -p ironclaw_reborn_webui_ingress --all-targets -- -D warnings
  • cargo clippy -p ironclaw_reborn_cli --all-targets -- -D warnings

Stacked on #3815.

@github-actions github-actions Bot added scope: docs Documentation scope: dependencies Dependency updates size: L 200-499 changed lines risk: medium Business logic, config, or moderate-risk modules contributor: core 20+ merged PRs labels May 20, 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_reborn_host crate to manage the host-side ingress lifecycle for the standalone Reborn binary, including listener binding and graceful shutdown. The ironclaw-reborn serve command has been updated to delegate to this new crate and now requires environment variables for authentication. Feedback identifies a security vulnerability where authentication tokens are compared using non-constant-time operations, which could lead to timing attacks. Additionally, the reviewer noted an issue in the error handling logic where a failure during runtime shutdown could potentially swallow a primary server error.

Comment on lines +124 to +128
if token == self.token {
Some(self.user_id.clone())
} else {
None
}

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.

security-high high

Authentication tokens should be compared using constant-time operations to prevent timing attacks. Using a simple string comparison (==) allows an attacker to potentially guess the token byte-by-byte by measuring response time differences. Use the subtle crate's ConstantTimeEq for a secure comparison.

Comment on lines +18 to +24
anyhow = "1"
async-trait = "0.1"
axum = "0.8"
ironclaw_host_api = { path = "../ironclaw_host_api", version = "0.1.0" }
ironclaw_reborn_composition = { path = "../ironclaw_reborn_composition", version = "0.1.0", features = ["webui-v2-beta"] }
tokio = { version = "1", features = ["net", "signal"] }
tracing = "0.1"

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

Add the subtle crate as a dependency to enable constant-time comparison for authentication tokens, which helps prevent timing attacks.

Comment on lines +48 to +54
let result =
serve_runtime_webui(&runtime, listen_addr, auth_token, tenant_id, auth_user_id).await;
let shutdown = runtime.shutdown().await;

result?;
shutdown.context("failed to shut down Reborn runtime after WebUI service exits")?;
Ok(())

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

If both the WebUI service (serve_runtime_webui) and the runtime shutdown fail, the shutdown error will overwrite the primary server error. Per the general rules, the primary error should be returned to the caller, while the secondary failure (shutdown) should be logged as a warning.

References
  1. When a primary operation fails and a subsequent cleanup or secondary operation also fails, log the secondary failure as a warning and ensure the original primary error is returned to the caller to prevent it from being swallowed.

@serrrfirat
serrrfirat force-pushed the feat/reborn-serve-webui-delegation branch from 5b8745a to 0462b3a Compare May 20, 2026 21:50
@serrrfirat serrrfirat changed the title feat(reborn): delegate serve to WebUI host ingress feat(reborn): delegate serve to WebUI ingress May 20, 2026
Base automatically changed from feat/webui-v2-gateway-composition-3580 to reborn-integration May 22, 2026 10:50
@serrrfirat serrrfirat closed this May 22, 2026
@serrrfirat
serrrfirat force-pushed the feat/reborn-serve-webui-delegation branch from 0462b3a to 3c81d71 Compare May 22, 2026 14:55
@github-actions github-actions Bot added size: XS < 10 changed lines (excluding docs) risk: low Changes to docs, tests, or low-risk modules and removed size: L 200-499 changed lines risk: medium Business logic, config, or moderate-risk modules labels May 22, 2026
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: low Changes to docs, tests, or low-risk modules scope: dependencies Dependency updates scope: docs Documentation size: XS < 10 changed lines (excluding docs)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant