Skip to content
Merged
18 changes: 18 additions & 0 deletions apps/skit/src/permissions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,13 @@ impl Permissions {
"containers::*".to_string(),
// Transport: allow MoQ, deny HTTP fetcher by default (SSRF risk)
"transport::moq::*".to_string(),
// Transport HTTP: allow the sink/IO nodes that serve or receive over the
// client's own request (no arbitrary-URL fetch), but keep
// `transport::http::fetcher` denied (SSRF risk). This lets least-privilege
// gateways serve live casts (mse) and run oneshots without admin.
"transport::http::mse".to_string(),
"streamkit::http_input".to_string(),
"streamkit::http_output".to_string(),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: http_input/http_output are implicitly allowed for oneshot regardless of allowlist

Adding streamkit::http_input and streamkit::http_output to the user role's allowed_nodes is harmless but redundant for the oneshot path: apps/skit/src/server/oneshot.rs:529-536 explicitly skips these marker kinds, treating them as implicitly allowed whenever oneshot execution is permitted. The allowlist entries only have an effect at the other is_node_allowed call sites (sessions/websocket/mcp/validation), where these oneshot-only marker nodes would not normally appear. Net: the entries are safe and the documented gateway example is still correct, just over-specified for oneshot use.

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks — agreed, info-only and correct. The oneshot path (server/oneshot.rs) skips these marker kinds so they're implicitly allowed there; the allowlist entries only matter at the session/websocket/mcp/validation is_node_allowed call sites. I'm keeping them for consistency with the documented gateway role example (and the new samples/skit.toml role) so the same allowlist works whether or not a node is gated — over-specified but not incorrect.

// Core: explicitly allow safe-ish nodes; deny core::file_writer by default (arbitrary write risk)
"core::passthrough".to_string(),
"core::file_reader".to_string(),
Expand Down Expand Up @@ -511,6 +518,17 @@ mod tests {
assert!(user.is_node_allowed("plugin::wasm::gain_filter_rust"));
}

#[test]
fn test_default_user_allows_http_sink_and_io_nodes() {
let user = Permissions::user();
// Sink/IO HTTP nodes are safe (serve/receive over the client's own request).
assert!(user.is_node_allowed("transport::http::mse"));
assert!(user.is_node_allowed("streamkit::http_input"));
assert!(user.is_node_allowed("streamkit::http_output"));
// The HTTP fetcher remains denied (arbitrary-URL fetch / SSRF risk).
assert!(!user.is_node_allowed("transport::http::fetcher"));
}

#[test]
fn test_global_session_limits() {
let config = PermissionsConfig { max_concurrent_sessions: Some(10), ..Default::default() };
Expand Down
29 changes: 29 additions & 0 deletions docs/src/content/docs/guides/authorization.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,35 @@ allowed_assets = ["*"]
> [!NOTE]
> Role permissions are deny-by-default. If you define a custom role in `skit.toml`, any permission you omit defaults to `false`.

> [!NOTE]
> The built-in `user` role allows the safe HTTP sink/IO nodes — `transport::http::mse` (live-cast playback), `streamkit::http_input` and `streamkit::http_output` (oneshot request body/response) — but **not** `transport::http::fetcher`, which can fetch arbitrary URLs (SSRF risk). A trusted gateway that only serves or receives over the caller's own request therefore does not need `admin`.

## Example: Least-privilege gateway role

Trusted intermediaries (e.g. the `web-capture` or `speech-gateway` examples) build a small set of fixed pipelines and should run with a scoped token instead of `admin`. This role can create/destroy sessions and use the HTTP-transport sink/IO nodes plus a specific plugin, but cannot load/delete plugins or touch other users' sessions:

```toml
[permissions.roles.gateway]
create_sessions = true
destroy_sessions = true
modify_sessions = true
tune_nodes = true
list_sessions = true
list_nodes = true
access_all_sessions = false # Only its own sessions
load_plugins = false
delete_plugins = false
upload_assets = false
delete_assets = false
allowed_nodes = [
"transport::http::mse", # serve live casts to the browser (MSE)
"streamkit::http_input", # oneshot request body
"streamkit::http_output", # oneshot response
"core::*",
]
allowed_plugins = ["plugin::native::servo"] # only what the gateway needs
```

## Permission reference

| Permission | Description |
Expand Down
40 changes: 38 additions & 2 deletions samples/skit.toml
Original file line number Diff line number Diff line change
Expand Up @@ -483,10 +483,15 @@ allowed_samples = [
"user/*.yaml",
]
Comment on lines 488 to 489

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: Sample user role allowed_samples diverges slightly from code default

The samples/skit.toml user role's allowed_samples includes demo/*.yml/demo/*.yaml (lines 480-481), while the code default Permissions::user() (apps/skit/src/permissions.rs:149-157) does not. This is a pre-existing divergence not introduced by this PR (the PR only touched allowed_nodes/allowed_plugins for the user role), so it is not a regression, but the code and sample are not a perfect mirror of each other despite the PR's stated goal of mirroring Permissions::user().

(Refers to lines 475-484)

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed: removed demo/* from the sample user role's allowed_samples so it now mirrors the built-in Permissions::user() (oneshot/dynamic/user only).


# Users can use most nodes except potentially dangerous ones
# Users can use most nodes except potentially dangerous ones.
# Note: transport::http::fetcher is intentionally excluded (it fetches arbitrary
# URLs - SSRF risk). The safe HTTP sink/IO nodes are allowed instead.
allowed_nodes = [
"audio::*",
"transport::*",
"transport::moq::*",
Comment thread
staging-devin-ai-integration[bot] marked this conversation as resolved.
"transport::http::mse", # serve live casts to the browser (MSE)
"streamkit::http_input", # oneshot request body
"streamkit::http_output", # oneshot response
"core::*",
Comment thread
staging-devin-ai-integration[bot] marked this conversation as resolved.
Outdated
"containers::*",
]
Comment on lines 487 to 514

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: User role node allowlist tightened from broad wildcards to explicit list

The sample user role previously allowed transport::* and core::*, which is broader than the code's Permissions::user(). This PR replaces those with an explicit list matching the code (e.g. transport::moq::*, transport::http::mse, and an enumerated set of core:: nodes), removing transport::rtmp, core::file_writer, core::object_store_writer, etc. from the user role. This is an intentional behavior change that could break existing user-created pipelines relying on the removed nodes; worth confirming there are no shipped user//oneshot/ sample pipelines that depend on them (none currently exist under samples/pipelines/user).

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch on the breakage surface. I checked the shipped pipelines: oneshot/tts_to_s3.yml, oneshot/transcode_to_s3.yml, dynamic/moq_s3_archive.yml use core::object_store_writer, and dynamic/moq_to_rtmp_composite.yml uses transport::rtmp::publish. A user can still read those samples but no longer run them.

That's intentional and not a regression relative to the secure default: the built-in Permissions::user() already denies object_store_writer (external write) and rtmp::publish (egress) — the old sample's core::*/transport::* wildcards were the outlier. Those pipelines remain runnable under admin (or a custom role that opts into the egress/write nodes). The point of this PR is to make the sample mirror that secure default.

Expand All @@ -500,6 +505,37 @@ allowed_assets = [
"samples/audio/user/*",
]

[permissions.roles.gateway]
# Least-privilege role for trusted intermediaries (e.g. the speech-gateway or
# web-capture examples) that build a small set of fixed pipelines. Avoids
# granting admin just to use HTTP-transport nodes.
create_sessions = true
destroy_sessions = true
list_sessions = true
modify_sessions = true
tune_nodes = true
load_plugins = false
delete_plugins = false
list_nodes = true
access_all_sessions = false # Only its own sessions
upload_assets = false
delete_assets = false

allowed_samples = []

# Only the HTTP sink/IO nodes the gateway needs - no fetcher (SSRF risk).
allowed_nodes = [
"transport::http::mse", # serve live casts to the browser (MSE)
"streamkit::http_input", # oneshot request body
"streamkit::http_output", # oneshot response
"core::*",
]
Comment on lines +554 to +562

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: Gateway role's core::* allows arbitrary-write file_writer despite least-privilege framing

The new gateway role in samples/skit.toml:538-543 and the docs example (docs/src/content/docs/guides/authorization.md:132-137) grant core::*, which includes core::file_writer. The built-in user role (apps/skit/src/permissions.rs:172-181) deliberately enumerates individual core nodes and excludes core::file_writer because of arbitrary-write risk. So the role labelled "least-privilege" is actually broader (for core nodes) than the user role with respect to file_writer. This is a config/security-policy choice rather than a code bug, and security findings are out of scope for this review, but the inconsistency with the stated least-privilege intent may be worth a deliberate decision.

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed: the gateway example (sample config + docs) now grants only the safe core plumbing nodes (passthrough, file_reader, pacer, sink) instead of core::*, so it no longer exposes core::file_writer — consistent with the least-privilege intent and the built-in user role.


# Only the specific plugin(s) the gateway needs (example: servo for web-capture).
allowed_plugins = ["plugin::native::servo"]

@staging-devin-ai-integration staging-devin-ai-integration Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🚩 Gateway example references plugin::native::servo

Both the docs (docs/src/content/docs/guides/authorization.md:138) and sample config (samples/skit.toml:535) use plugin::native::servo as the gateway's allowed plugin for the web-capture example. Reviewer may want to confirm a servo native plugin kind actually exists/is the right name used by the web-capture example, otherwise the example would be non-functional as written.

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed valid — plugin::native::servo is the correct kind. The servo plugin (plugins/native/servo/plugin.yml) declares kind: native, node_kind: servo, and the existing sample pipelines (samples/pipelines/oneshot/web_capture.yml, web_pip_compositor.yml) use kind: plugin::native::servo. So the gateway example is functional as written.

Comment on lines +554 to +565

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: Plugin enforcement ordering relied upon by gateway role

The gateway role lists plugin::native::servo in both allowed_nodes and allowed_plugins. This is required because enforcement checks is_node_allowed(kind) before the plugin allowlist (e.g. apps/skit/src/websocket_handlers.rs:55, apps/skit/src/server/sessions.rs:327). Verified the glob matching via glob::Pattern treats * ordinarily across :: (no / separators), so the literal plugin::native::servo entry and plugin::* entries resolve correctly. The new tests cover this ordering dependency.

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right — this ordering dependency (is_node_allowed runs before the plugin allowlist) was the root cause of the original gateway-role bug, where plugin::native::servo was in allowed_plugins but absent from allowed_nodes and so rejected first. The new sample_config_test assertion (gateway.is_node_allowed("plugin::native::servo")) plus the docs [!IMPORTANT] note now guard against regressing it.


allowed_assets = []

[permissions.roles.readonly]
# Read-only role - can only view, not modify
create_sessions = false
Expand Down
Loading